rework GPIO configuration flags #11880
Conversation
|
All checks are passing now. Review history of this comment for details about previous failed status. |
|
Fixed spelling in first commit, and added following: dt-bindings: gpio: revise drive strength flags for generalityThe documentation and design for drive strength was based on Nordic GPIO The Semtech SX15088/SX1509B GPIO extender went the other way, with Rework the flags so that low and high are distinct from default. Update Deprecate the ALT flag but define it to implement the documented :100644 100644 ddb097b3d4 7654f7e763 M drivers/gpio/gpio_cc2650.c |
Codecov Report
@@ Coverage Diff @@
## master #11880 +/- ##
==========================================
+ Coverage 48.63% 54.2% +5.56%
==========================================
Files 313 235 -78
Lines 46436 26221 -20215
Branches 10710 6262 -4448
==========================================
- Hits 22585 14212 -8373
+ Misses 19395 9412 -9983
+ Partials 4456 2597 -1859
Continue to review full report at Codecov.
|
pabigot
force-pushed
the
issue/10339
branch
2 times, most recently
from
042f44d
to
a53da19
Compare
|
Added example of use of the API. I've tested this on nrf52_pca20020 on an internal branch that depends on material not yet merged to master. drivers: gpio: sx1509b: remove ability to configure by portIt seems highly unlikely that anybody would want to configure all pins Retain the ability to read and write all pins simultaneously as that has Signed-off-by: Peter A. Bigot pab@pabigot.com :100644 100644 fb0270364d a2d848928e M drivers/gpio/gpio_sx1509b.c drivers: gpio: sx1509b: transition to new GPIO configurationDemonstrates an implementation that takes advantage of the new Signed-off-by: Peter A. Bigot pab@pabigot.com :100644 100644 a2d848928e 8ca90d4485 M drivers/gpio/gpio_sx1509b.c |
|
Sorry for the blitz, but this has been open for four weeks with no reviews. I'd appreciate it if people would look at it, so we can discuss any remaining concerns on the API call next week. In particular, how might these changes affect your GPIO driver implementations? @galak @psidhu @mnkp @b0661 @jfischer-phytec-iot @anangl @tbursztyka @MaureenHelm @pfalcon @carlescufi |
|
lgtm |
There was a problem hiding this comment.
There was a PR #6760 prepared by @b0661 which introduced a reworked GPIO flags along the pinctrl API. It was closed in a meantime without any comment. As I argued in #2288 having only GPIO driver would make design more straightforward. However, if we want to have DTS compatible with / similar to Linux DTS then it would probably make sense to have also pinctrl driver. If we decide in the future to go the direction #6760 pointed to it would make sense to use GPIO flags Linux DTS is using.
In any case DTS properties used by the Linux and flag names proposed by @b0661 in #6760 seem to be more consistent (they are not complete since they cover only pin control part).
As an example, to encode pin driving mode we have
PINCTRL_CONFIG_DRIVE_PUSH_PULL
PINCTRL_CONFIG_DRIVE_OPEN_DRAIN
PINCTRL_CONFIG_DRIVE_OPEN_SOURCE
rather than
GPIO_PUSH_PULL
GPIO_SINGLE_ENDED
GPIO_LINE_SOURCE
GPIO_LINE_DRAIN
GPIO_OPEN_DRAIN
GPIO_OPEN_SOURCE
In case of PINCTRL_* defines we could get rid of '_CONFIG' part and would need to replace 'PINCTRL' with 'GPIO'. 'PINCTRL_CONFIG_DRIVE_PUSH_PULL' would become 'GPIO_DRIVE_PUSH_PULL'.
Some names used by PINCTRL_ flags are however questionable, e.g. 'PINCTRL_CONFIG_OUTPUT_LOW' means "A '1' in the output buffer drives the output to low level" which is a bit confusing. In this case our ACTIVE_LOW flag is better.
I have two general remarks:
- It's a long standing industry standard to use _SHIFT prefix to indicate bit field position. This convention is used by other parts of Zephyr API, Linux as well as various vendor provided header files. The GPIO API used _POS instead. Since we are reworking the flag names that's a good opportunity to align the naming.
- This rule is not written in stone but most of the register header files provided by vendors (heavy users of #define flags) do not provide _SHIFT and _MASK defines for single bit flags. _SHIFT and _MASK are mainly provided for bit fields only. As such meaning of a single bit flag is enforced to be 'feature enabled' if flag is present or 'default behavior' if flag is absent. Also some Zephyr APIs follow this rule. It makes it easier to test for the flag e.g. we have
if (flags & GPIO_ACTIVE_HIGH) {
}
rather than
if ((flags & GPIO_ACTIVE_MASK) == GPIO_ACTIVE_HIGH) {
}
the latter typically results in one more compare assembly instruction (depending on the flag values).
|
As @mnkp already mentioned, I'm in favour of using the Pinctrl definitions of Linux. This would ease the transition to a pinctl driver that acts as a backend to the GPIO driver and would allow to directly use the DTS Pinctrl definitions. I wrote a shim driver that translates from the current GPIO interface to my pinctrl driver. It showed Linux Pinctrl is feature complete with respect to the GPIO pin control interface. The benefit of a third interface for pin control (not current Zephyr, not Linux, and not DTS compatible) seems to me less than to use the Linux Pinctrl definitions. BTW: The Pinctrl driver is currently maintained OOT. The PR has to wait for the code generation issue to be decided/ solved. |
|
@mnkp I don't object to switching to The CMSIS version of @b0661 The names like I appreciate the feedback. |
In Linux the pin control naming in the GPIO interface diverts from the naming in the Pinctrl interface !-). I'm in favour of the Pinctrl pin control naming as IMHO it is more versatile, complete and supported by DTS. Would be nice if we could avoid having different pin control defines to learn for DTS pinctrl and GPIO drivers.
My main argument is not related to pinctrl support but to not re-invent the wheel if there is a proven complete set of pin control definitions supported by DTS already available. |
include/dt-bindings/gpio/gpio.h
Outdated
|
|
||
| /** Enable GPIO pin debounce. */ | ||
| #define GPIO_DEBOUNCE (1U << GPIO_DEBOUNCE_POS) |
There was a problem hiding this comment.
Just a detail. I think it would be good to use unsigned values in all the shifts here. To avoid problems with undefined behavior in case the bit positions are altered in the future, or some definitions are modified after copying to some other module etc.
include/dt-bindings/gpio/gpio.h
Outdated
| /** GPIO pin polarity is normal. */ | ||
| #define GPIO_POL_NORMAL (0 << GPIO_POL_POS) | ||
| /** Mask to isolate the `GPIO_INT_LOW` and `GPIO_INT_HIGH` flags. */ | ||
| #define GPIO_INT_DIR_MASK (0x03 << (GPIO_INT_POS + 2)) |
There was a problem hiding this comment.
INT_DIR looks a bit strange to me (is it "interrupt direction"?). But will this mask be actually useful, where you see the need to isolate this two flags?
There was a problem hiding this comment.
This would be used instead of GPIO_INT_DOUBLE_EDGE in conditions like this:
switch(flags & GPIO_INT_DIR_MASK) {
default: // invalid interrupt specification
case GPIO_INT_LOW: // detect low
case GPIO_INT_HIGH: // detect high
case GPIO_INT_LOW | GPIO_INT_HIGH: // detect both
}
GPIO_INT_DOUBLE_EDGE is currently equivalent to the last case, but it also allows some enhancements (e.g. its absence can indicate use of a low-precision level-based double edge detection approach on Nordic, useful when monitoring a large number of dry contacts).
There was a problem hiding this comment.
But in the above code you could do (flags & (GPIO_INT_LOW | GPIO_INT_HIGH)), what would be equally readable (at least to me).
There was a problem hiding this comment.
I don't understand. All flags & (GPIO_INT_LOW | GPIO_INT_HIGH) tells you is that interrupts are supposed to be enabled. It doesn't let know how which direction to configure the hardware. (In the switch statement I didn't provide bodies for each case, but if they were there they'd all be different.)
There was a problem hiding this comment.
OK, I see; yes, you could do flags & (GPIO_INT_LOW | GPIO_INT_HIGH) instead of flags & GPIO_INT_DIR_MASK since they have the same effect. I think the flags & FOO_MASK idiom is more clear, as it's obvious you're interested in the FOO field without requiring the code extracting it to know all the bits that might be present in it, or how they combine to define distinct values.
|
|
||
| /** Enable GPIO pin pull-up. */ | ||
| #define GPIO_PUD_PULL_UP (1 << GPIO_PUD_POS) | ||
| /** GPIO pin polarity is inverted. */ |
There was a problem hiding this comment.
Maybe it would be good to clarify on the occasion what the polarity inversion means? I recall I found it a bit confusing when reviewing the updated nRF GPIO driver, and I'm still not sure if I got it right. I assume that it's that bit of value 1 indicates the low state and bit of value 0 the high one, and this concerns reading inputs and writing outputs. But what about the interrupt triggers, are they also supposed to be applied inversely?
There was a problem hiding this comment.
I'm just preserving the original comment (though that's not clear from the way the diff gets extracted). I don't believe the impact of using the flag can or should be generically described. The SX1509B has this feature, and it may not have the same behavior as Nordic or other devices (as I found with the drive strength flags, where the described behavior was wrong for SX1509B).
I will update the description with a note that the impact of the flag is driver-specific, including that it may be ignored.
To me the set of definitions suggested by @mnkp, i.e.: |
pabigot
force-pushed
the
issue/10339
branch
2 times, most recently
from
cecc5c1
to
f60f9e3
Compare
|
The last two pushes rebased the initial PR onto current master, then applied the changes. The effective changes can be viewed from the last Below I've listed the changes I've made, and those I have not made. My hope is to discuss this in the API call tomorrow and get consensus so this can be merged. I have held work that requires a resolution to this issue. @mnkp please take a look. I was uncertain from your comments exactly what changes resulted in a NACK, versus what you would have done differently and wanted reconsidered. fixup: use _SHIFT instead of _POS for field offset definesNote that the NXP HAL uses this suffix in combination with the Previously defined
None of those were used outside this file so the old macros have been eliminated rather than deprecated. fixup: consistently use unsuffixed integer literalsThis addresses @anangl's suggestion of using unsigned values in all the shifts here, except that I've changed my mind to use the unsuffixed values instead. The vast majority of existing code in Zephyr does not use the suffix, and I'm not going to add the suffix to hex constants: even though it's technically necessary, because it just looks...bad. fixup: remove misplaced doxygen grouping directiveCorrected, but I note that AFAICT the contents in this file are not processed by the doxygen infrastructure so I can't verify that they look right. Adding documentation generation is not in scope for this PR. fixup: document that polarity configuration is driver-specificnochange: remove
|
|
The following for |
|
As discussed in dev-review I've implemented the proposed compromise described in
I've updated the SX1509B driver. I've also updated the Nordic driver for the purposes of testing the updated blinky. This driver still needs to be revisited: the existing code applies the configured interrupt level to read and write operations, which was probably never right, and the polarity inversion affects interrupts, which was unspecified but is now incorrect if that is intended to be the solution for active low support. All other GPIO drivers need to support for Differences shown in the force push. Options to consider:
In-order commits:
|
How this approach is going to scale for the functions operating on a port? Are we going to have
We shouldn't add extra Remaining issues with this PR:
|
We'll have to figure out what makes sense when the new API for port operations is determined.
Rearchitecting the core GPIO driver API is not in scope.
There was nothing in the header that specified how the flag was meant to be used; existing use does not match the current semantics of That repeated, if the consensus is to mark them deprecated that's fine.
Their semantics is different as the drain capability is configured by setting the output signal (since the API was designed to support Nordic devices). This is probably something that should be in PINCTRL anyway.
We'll have to address that when we have something to integrate. For now, this is the only place we can specify direction, and we need at least three, possibly four, combinations to fully support functionality and control power consumption. A reasonable plan is to replace GPIO with an implementation compatible with PINCTRL whenever the latter gets attention, especially given the desires to remove things like drain/pull from the GPIO API and to change the driver structure away from access operators. |
Introducing a second set of functions to have the first set read/write logical pin value and the second set to read/write physical pin state makes sense. While making the driver code honor
To add support for the new functions we don't need to re-architect GPIO core. It's enough to add the new functionality so it does not depend on the old implementation. There are a lot of gpio drivers to update and test. It doesn't make sense to do all this work twice.
And that's the primary reason why it should be deprecated. The main goal of this PR is to clean up GPIO flags. If there is a flag which is not well specified we either need to clarify its meaning or deprecate it. It really doesn't make sense to spend all this work only to have the API as ambiguous as it was before. Is there a use case which is not going to be covered by the remaining flags?
How exactly is the semantics of the flag different? What will be the functional difference in the implementation of
Once again, that's a very shortsighted view. We know enough about the requirements of pinctrl driver to come up with matching implementation of GPIO driver.
We've decided to honor Linux DTS and that puts some limits on how we design Zephyr drivers. E.g open drain functionality can be specified by both gpio and pinctrl nodes. As such it needs to be supported by gpio and pinctrl driver. |
|
@mnkp When I created this PR two months ago I described the constraints on my solution. They're right there in the introductory comment. In response to your concerns I've added an API that implements support for negative logic via I understand that you wish I had done something much more significant and in line with how you would have solved the problem. I even agree in principal with much of what you've proposed. But that was not what I agreed to do. I've donated as much time to this PR as I can afford. It stands or falls either as-is or---since you don't like them---with the commits for the new API dropped. Personally, I'd prefer dropped. |
I appreciate your effort however, as I've stated multiple times already, I believe that this PR should not be accepted without resolving all the problems. As it stands, it introduces just as many issues as it tries to fix. |
GPIO configuration flags will move and some that used to be in the low 8 bits are now higher, resulting in implicit constant conversion overflows. Use a boolean data type to hold boolean values. Signed-off-by: Peter A. Bigot <pab@pabigot.com>
Updated GPIO constants to provide more meaningful names and more complete functionality. The intent is that the presence and interpretation of existing flags remains unchanged, although their bitwise representations has. Added signal type and direction flags matching Linux device tree bindings, allowing open drain and open source to be distinguished from push-pull signals. Extended GPIO_DIR to support bidirectional configuration and setting an initial value for outputs. Extended GPIO_INT to more clearly identify level and edge configurations. Some redundancy is required to continue supporting the existing single-bit GPIO_INT_DOUBLE_EDGE flag. GPIO_INT_ACTIVE_foo renamed to GPIO_ACTIVE_foo since active level is meaningful for non-interrupt inputs and output signals (e.g. RESETn is an active low output). GPIO_INT_DEBOUNCE similarly renamed to GPIO_DEBOUNCE as debounce applies to non-interrupt inputs. The legacy macros are retained with their original values to avoid the need for a global substitution change. Other flags have changed bit representations but not interpretation. Closes zephyrproject-rtos#10339 Signed-off-by: Peter A. Bigot <pab@pabigot.com>
The documentation and design for drive strength was based on Nordic GPIO capabilities, and presumed that the default state was standard while the alternate state was high. The Semtech SX15088/SX1509B GPIO extender went the other way, with default being high and alternate being low. Rework the flags so that low and high are distinct from default. Update the two in-tree GPIO implementations that currently support drive strength to make use of these flags. Deprecate the ALT flag but define it to implement the documented selection (high drive) in case it's being used by out-of-tree systems. Signed-off-by: Peter A. Bigot <pab@pabigot.com>
It seems highly unlikely that anybody would want to configure all pins to the same configuration. Remove this feature to simplify the code prior to rework. Retain the ability to read and write all pins simultaneously as that has use cases. Signed-off-by: Peter A. Bigot <pab@pabigot.com>
Demonstrates an implementation that takes advantage of the new flexibility in GPIO pin configuration. New features include support for bi-directional GPIO and drive strength. Signed-off-by: Peter A. Bigot <pab@pabigot.com>
Interrupts default to trigger on level for historical reasons, so use of GPIO_INT_LEVEL` as a mask results in a zero value. Use a mask macro to isolate the trigger configuration. Signed-off-by: Peter A. Bigot <pab@pabigot.com>
Interrupts default to trigger on level for historical reasons, so use of GPIO_INT_LEVEL` as a mask results in a zero value. Use a mask macro to isolate the trigger configuration. Signed-off-by: Peter A. Bigot <pab@pabigot.com>
|
I've rebased this, and removed the commits that added new API that would require driver support. For overall goals and intent behind this PR please see the introductory comment. In summary, this attempts to clean up the flags to provide more extensibility to driver implementations that wish to support them, without impacting any in-tree use or changing the interpretation of flags like
|
|
Since the behavior of
Remaining issues, mainly poorly specified and overlapping functionality, were listed here. |
To address this, we could provide simple wrappers that take the and call it from the application like this: Would it be that cumbersome and error prone? Similarly, we could provide macros which would help in configuring the interrupts to be triggered on active/inactive level or "to active/inactive" edge, to address @dwagenk's #10339 (comment). |
|
my general comments to that PR or maybe more to flags are following:
|
(*) In Linux As for the proposal from @anangl to use this could work. But also in this case we require application to keep around flags value every time we want to write/read from the pin, even if this is required only during the pin configuration phase. The approach does not allow inverting pin value in hardware (when supported). Also, since we introduced |
|
TSC decision 2019-02-06
This PR was not intended to address the whole issue, so it is no longer relevant. |
This PR attempts to resolve the concerns and discussions of issue #10339. The following design goals were applied:
The semantics and behavior of existing macros must be retained wherever possible. The specific bit values of existing macros were not preserved. In some cases a feature that was indicated by an unset bit is now given its own bit.
It is not the scope of this PR to change existing code. As a result several legacy identifiers like
GPIO_INT_ACTIVE_LOWremain present throughout the code. I have confirmed that replacing them with the recommended alternative results in a system that passes the sanity check. It will also conflict with any in-progress work that touches device trees. So: removing use of legacy identifiers is the responsibility of the board and driver maintainers, as is taking advantage of new features likeGPIO_DIR_OUT_LOW.Capabilities were designed to meet the requests in the issue, informed by existing use within Zephyr and the capabilities of Linux for gpio, pinctrl, and interrupt configuration options as well as highly configurable GPIO devices like the Semtech SX1509x.
The flag values remain placed in the dt-bindings include file because most existing DTS bindings set more than just active level and signal type, and there's no motivation to complicate things by attempting to separate out what we might think is relevant only to an application. Best practice, however, is probably to reduce the set of flags indicated in the bindings.
At this point I've run the sanity checks and on applications with Nordic hardware. I won't have non-Nordic platforms available to me until later this week.
I have not yet verified that generated documentation is formatted correctly.
drivers: gpio: fix misuse of u8_t where bool is intended
GPIO configuration flags will move and some that used to be in the low
8 bits are now higher, resulting in implicit constant conversion
overflows. Use a boolean data type to hold boolean values.
Signed-off-by: Peter A. Bigot pab@pabigot.com
:100644 100644 021f8d23a3 9f16f893de M drivers/gpio/gpio_dw.c
:100644 100644 46822c52c4 0e335fe40f M drivers/gpio/gpio_qmsi.c
:100644 100644 0ce5c5bab1 68fc8adb28 M drivers/gpio/gpio_qmsi_ss.c
dts-bindings: gpio: revise flags to support more complete control
Updated GPIO constants to provide more meaningful names and more
complete functionality. The intent is that the presence and
interpretation of existing flags remains unchanged, although their
bitwise representations has.
Added signal type and direction flags matching Linux device tree
bindings, allowing open drain and open source to be distinguished from
push-pull signals.
Extended GPIO_DIR to support bidirectional configuration and setting an
initial value for outputs.
Extended GPIO_INT to more clearly identify level and edge
configurations. Some redundancy is required to continue supporting the
existing single-bit GPIO_INT_DOUBLE_EDGE flag.
GPIO_INT_ACTIVE_foo renamed to GPIO_ACTIVE_foo since active level is
meaningful for non-interrupt inputs and output signals (e.g. RESETn is
an active low output). GPIO_INT_DEBOUNCE similarly renamed to
GPIO_DEBOUNCE as debounce applies to non-interrupt inputs. The legacy
macros are retained with their original values to avoid the need for
a global substitution change.
Other flags have changed bit representations but not interpretation.
Closes #10339
Signed-off-by: Peter A. Bigot pab@pabigot.com
:100644 100644 c365d449e3 97c2d3b110 M include/dt-bindings/gpio/gpio.h