api: gpio: Clean up GPIO dt-bindings flags

[mnkp]

This PR attempts to resolve the concerns and discussions of issue #10339 and is based on work in #11880 by @pabigot.

The main differences to #11880 are as follows:

• multi-bit flags are defined using following standard pattern

#define GPIO_MULTI_BIT_FIELD             (N << GPIO_MULTI_BIT_FIELD_SHIFT)

where 'N' is the value/state we want to encode.

• no _SHIFT and _MASK macros for single bit flags.
• GPIO_POL_ flags are deprecated as their function overlaps with the new GPIO_ACTIVE_LOW, GPIO_ACTIVE_HIGH flags.
• the documentation of gpio_pin_read(), gpio_pin_write() functions was updated to clarify usage of the new GPIO_ACTIVE_ flags.

The following design goals were applied:

• only issues discussed in gpio: Cleanup flags #10339 are addressed. Otherwise the changes are kept to minimum. Especially this PR does not attempt to align the API with the future pinctrl API (e.g. drivers: add pinctrl device interface #6760).
• to be able to keep DTS of GPIO nodes close to the Linux's dt-bindings gpio.h
  Linux implementation is followed.

One extra point which we likely should revisit are issues raised by @dwagenk. Current implementation of pin read/write and interrupt API is not consistent.

I have not yet verified that generated documentation is formatted correctly.

Fixes #10339

[mnkp]

In the next few days I'll try to prepare another PR with more involved changes to GPIO flags where I will try to align the design to the future pinctrl API as proposed in #6760 by @b0661. In general the pinctrl API gives an impression of a better thought of than the GPIO one.

[codecov-io]

Codecov Report

Merging #12651 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #12651   +/-   ##
=======================================
  Coverage   54.06%   54.06%           
=======================================
  Files         239      239           
  Lines       26922    26922           
  Branches     6505     6505           
=======================================
  Hits        14556    14556           
  Misses       9701     9701           
  Partials     2665     2665

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5bd5e4e...c19c884. Read the comment docs.

[pabigot]

There are a couple capabilities in #11880 that don't seem to be supported by this version:

• The ability to configure direction as simultaneously input and output, a feature supported by multiple drivers including SX1509B and Nordic. By using separate bits we get better control of power consumption by explicitly disabling input and output buffers.
• The ability to explicitly configure drive strength as low or high, where the defaults differ between SX1509B and Nordic (noted as lacking)
• The ability to specify the initial level for output GPIOs.

There may be others. It also lacks an example of how to implement the extensions it provides, and it doesn't build.

All that can of course be addressed, but fundamentally I don't see anything here that justifies spending more time on a solution that is not demonstrably superior to the one that was proposed in early December and already has four approvals plus a LGTM.

[mnkp]

• The ability to configure direction as simultaneously input and output, a feature supported by multiple drivers including SX1509B and Nordic. By using separate bits we get better control of power consumption by explicitly disabling input and output buffers.

The functionality was not changed. Pin configured as GPIO_DIR_OUT works simultaneously as input. Current proposal doesn't have ability to disable pin. This could be addressed by adding GPIO_DIR_DISABLED flag. But a better approach would be to use pinctrl like interface which provides a more sophisticated approach to configuring pin drive capabilities.

• The ability to explicitly configure drive strength as low or high, where the defaults differ between SX1509B and Nordic (noted as lacking)

It's mentioned on the To Do list, will address shortly.

• The ability to specify the initial level for output GPIOs.

Supported by GPIO_DIR_OUT_LOW, GPIO_DIR_OUT_HIGH flags

[pabigot]

If part of your motivation for rejecting #11880 was objection to defining offset macros for single-bit fields, you really ought to remove this one.

[mnkp]

All GPIO_POL_ flags were deprecated and are going to be removed in the future. GPIO_POL_SHIFT is required to define GPIO_POL_MASK which is used in the code base.

[pabigot]

So there is existing precedent in Zephyr for providing mask and shift defines for single-bit fields. Good to know.

[pabigot]

• The ability to configure direction as simultaneously input and output, a feature supported by multiple drivers including SX1509B and Nordic.

The functionality was not changed.

Yes, which is my point. Ability to control input enable explicitly was added in #11880, and is relevant to at least two existing GPIO driver implementations.

But a better approach would be to use pinctrl like interface which provides a more sophisticated approach to configuring pin drive capabilities.

That's an opinion that can be defended or challenged. But we don't have pinctrl. We won't have pinctrl for 1.14-LTS. I would like to have the functionality now.

[mnkp]

• The ability to configure direction as simultaneously input and output, a feature supported by multiple drivers including SX1509B and Nordic.

The functionality was not changed.

Yes, which is my point. Ability to control input enable explicitly was added in #11880, and is relevant to at least two existing GPIO driver implementations.

It was always possible "to configure direction as simultaneously input and output". I've added also GPIO_DIR_DISABLED flag to disable / disconnect the pin. What kind of use case are you concerned about?

[b0661]

LGTM

[carlescufi]

@Mierunski could you please take a look at this PR in the context of #11880?

[pabigot]

• GPIO_POL_ flags are deprecated as their function overlaps with the new GPIO_ACTIVE_LOW, GPIO_ACTIVE_HIGH flags.

Wait, what? No they don't. At least I don't think so. I thought they were meant to say "configure the device to invert the signal polarity". (Whatever that means.)

At least that's how they're used in most drivers now.

[pabigot]

• the documentation of gpio_pin_read(), gpio_pin_write() functions was updated to clarify usage of the new GPIO_ACTIVE_ flags.

That's not right either. The way I've always seen the GPIO_ACTIVE_ flags used is as an indicator of whether the active state of the signal is high or low. E.g. a pressed button may result in a connection to ground (active low) or to a current source (active high). Similarly for LEDs where you write a zero to illuminate an active low LED and a one to illuminate an active high LED.

They certainly don't affect the value written by gpio_pin_write, and they had better not affect the value returned from gpio_pin_read.

[pabigot]

Some more issues resulting from a closer look.

[pabigot]

This isn't what those flags are intended to convey, nor does this describe how this function actually behaves (unless I am very confused...).

[pabigot]

The function does not adjust its return value based on GPIO_ACTIVE_LOW.

[pabigot]

These two are not equivalent. In the intended use as documented in #11880 GPIO_ACTIVE_foo is informational, indicating the active level of the signal. It's specifically needed in the DTS bindings to indicate whether an LED (for example) needs zero or one written to turn it on.

Polarity inversion is a configuration that affects how the driver sets up the GPIO peripheral.

[pabigot]

Same: inverted polarity does not mean active low.

[dleach02]

For all deprecated defines I would suggest compressing the defines and having a single comment block around them that states they will be deprecated. Don't attempt to document them any further because only existing implementation will have them in use and new implementations you want to discourage usage.

[galak]

Is there any particular reason to have how bit positions 0, 1, and 2 are defined differ from the linux dt-binding gpio.h:

/* Bit 0 express polarity */
#define GPIO_ACTIVE_HIGH 0
#define GPIO_ACTIVE_LOW 1

/* Bit 1 express single-endedness */
#define GPIO_PUSH_PULL 0
#define GPIO_SINGLE_ENDED 2

/* Bit 2 express Open drain or open source */
#define GPIO_LINE_OPEN_SOURCE 0
#define GPIO_LINE_OPEN_DRAIN 4

/*
 * Open Drain/Collector is the combination of single-ended open drain interface.
 * Open Source/Emitter is the combination of single-ended open source interface.
 */
#define GPIO_OPEN_DRAIN (GPIO_SINGLE_ENDED | GPIO_LINE_OPEN_DRAIN)
#define GPIO_OPEN_SOURCE (GPIO_SINGLE_ENDED | GPIO_LINE_OPEN_SOURCE)

[mnkp]

• the documentation of gpio_pin_read(), gpio_pin_write() functions was updated to clarify usage of the new GPIO_ACTIVE_ flags.

That's not right either. The way I've always seen the GPIO_ACTIVE_ flags used is as an indicator of whether the active state of the signal is high or low. E.g. a pressed button may result in a connection to ground (active low) or to a current source (active high). Similarly for LEDs where you write a zero to illuminate an active low LED and a one to illuminate an active high LED.

In the intended use as documented in #11880 GPIO_ACTIVE_foo is informational, indicating the active level of the signal. It's specifically needed in the DTS bindings to indicate whether an LED (for example) needs zero or one written to turn it on.

That was my impression when reviewing #11880 that the usage of GPIO_ACTIVE_ flags was not understood and clarified well enough. As per comment from @galak the GPIO_POL_ flags should be removed and replaced by GPIO_ACTIVE_.

GPIO_ACTIVE_LOW, GPIO_ACTIVE_HIGH are used by the Linux's dt-bindings gpio.h. And we want to keep Zephyr's usage of GPIO DTS node close to the Linux equivalent. GPIO_ACTIVE_ flags are not merely informational. Their meaning is exactly as documented in the description of gpio_pin_read(), gpio_pin_write() functions.

Linux documents its usage of Active-High and Active-Low flags. This is also mentioned in the description of gpio get_value, set_value functions.

[pabigot]

I don't read that comment from @galak quite the same way, though I can see that interpretation.

The usage of active high/low flags in Linux is exactly as I've described; the difference is that Linux apparently uses them to control the sense of read/write operations in their API. Zephyr's read/write GPIO functions affect the signal value directly, not the logical value as Linux does. Maybe that was not the intent, but AFAICT all existing use of GPIO_INT_ACTIVE_foo is restricted to detecting the signal level when the interrupt is asserted. That's not something we can change now.

The GPIO_POL_ flags also have an existing behavior that we can't suddenly change.

[galak]

I don't read that comment from @galak quite the same way, though I can see that interpretation.

The usage of active high/low flags in Linux is exactly as I've described; the difference is that Linux apparently uses them to control the sense of read/write operations in their API. Zephyr's read/write GPIO functions affect the signal value directly, not the logical value as Linux does. Maybe that was not the intent, but AFAICT all existing use of GPIO_INT_ACTIVE_foo is restricted to detecting the signal level when the interrupt is asserted. That's not something we can change now.

The GPIO_POL_ flags also have an existing behavior that we can't suddenly change.

My comment was about the bit position and names. There's a subtle point here about Linux compatibility and Device Tree. We should be aiming to utilize and conform to the Device Tree specs that are utilized in Linux, that's the canonical device tree definition repository. If there are some area's that the DTS binding aren't clear or need clarity we should aim to get that clarity.

[mnkp]

My comment was about the bit position and names.

We were actually arguing about this comment. It's about intended use of GPIO_ACTIVE_ flags.

As for the

Is there any particular reason to have how bit positions 0, 1, and 2 are defined differ from the linux dt-binding gpio.h:

This shouldn't be required as we are not aiming for binary compatibility. However, I can change encoding to be the same as in linux dt-binding gpio.h. This is not a problem.

[pabigot]

Discussion in dev review telecon 2019-01-24 leaves this open until next week and more review.

[mnkp]

For all deprecated defines I would suggest compressing the defines and having a single comment block around them that states they will be deprecated.

I've tried to keep comments to minimum. Only in case of GPIO_INT_ACTIVE_LOW, GPIO_INT_ACTIVE_HIGH defines I left a longer text since their usage was ambiguous.

[mnkp]

I have reworked the way we configure input / output mode. Rather than using GPIO_DIR_* flags which are now deprecated I'm using a new GPIO_INPUT_*, GPIO_OUTPUT_* flags. This approach is consistent with pinctrl-bindings.txt as well as bindings gpio.txt used by Linux. What it means is that if we want to be compliant with Linux DTS for gpio and pinctlr nodes this API will support it.

I've added also usage examples of GPIO_ACTIVE_LOW, GPIO_ACTIVE_HIGH flags. As well as modified two drivers: gpio_sam.c, gpio_gecko.c to showcase the new GPIO API.

As per request from @galak the encoding of flags used in DTS is now identical as in linux dt-binding gpio.h.

[mnkp]

The usage of active high/low flags in Linux is exactly as I've described [in #11880]; the difference is that Linux apparently uses them to control the sense of read/write operations in their API.

This statement doesn't quite make sense. Either the implementation of GPIO_ACTIVE_LOW flag in #11880 is consistent with the way it's used by Linux driver API and in Linux DTS or it isn't. That's an important difference you're trying to overlook.

The GPIO_POL_ flags also have an existing behavior that we can't suddenly change.

The very purpose of this PR is to rework usage of GPIO flags. So the statement once again doesn't quite make sense. Double so, taking into account that the usage of GPIO_POL_ flags wasn't very well defined from the very beginning and #11880 changed their definition in a way that makes them even more opaque.

[pabigot]

The usage of active high/low flags in Linux is exactly as I've described [in #11880]; the difference is that Linux apparently uses them to control the sense of read/write operations in their API.

This statement doesn't quite make sense. Either the implementation of GPIO_ACTIVE_LOW flag in #11880 is consistent with the way it's used by Linux driver API and in Linux DTS or it isn't. That's an important difference you're trying to overlook.

I don't think I'm overlooking it. I see it as two things: what do the flags indicate, and how do the driver methods work.

The flag semantics I described in #11880 is generally consistent with the first half of the reference you gave: they describe what line level indicates "active" for the signal.

I agree that the last half (how the flags affect interaction with the signal) is different, but that's not my fault. In #11880 I chose to leave the behavior of gpio_pin_write and gpio_pin_read the way it has always been in Zephyr (reads and writes the electrical signal level) rather than the way it's done in Linux (reads and writes the logical signal level).

I would in principal be open to changing the driver functions to work with the logical levels, but we'd have to change a few things:

• The default (zero) value must be allocated to GPIO_ACTIVE_HIGH so that writing a 1 has the same meaning it does now. This is what you've done, and now that I think about it it makes sense, so I'm going to update rework GPIO configuration flags #11880 to match.
• Every driver would have to be updated to store the configured active level and use it to mutate the write operand and read result. (Unless they happen to have an invert capability that provides those semantics.)
• There should probably be a new gpio_pin_configuration() method that can be used to query the pin configuration so somebody who wants the electrical rather than signal level can reconstruct it.

That seems a bit much to change, but if there's consensus that it's the right way forward and every GPIO driver maintainer commits to updating their code within the next few weeks (so it's available for several weeks of pre-release testing) I'd be willing to change #11880.

The GPIO_POL_ flags also have an existing behavior that we can't suddenly change.

The very purpose of this PR is to rework usage of GPIO flags. So the statement once again doesn't quite make sense. Double so, taking into account that the usage of GPIO_POL_ flags wasn't very well defined from the very beginning and #11880 changed their definition in a way that makes them even more opaque.

I'm not aware of doing anything to change the GPIO_POL_ flags. Drivers that respect them have exactly the same effect as they always did. I don't see a motivation for deprecating them, and somebody might well be depending on the existing behavior so I really don't think we can change it. It's certainly driver-dependent, but again---somebody put it into the API, other people used it to control configurations, and we need to be careful about taking things away once that's happened.

[mnkp]

Since during the API call there seem to be misunderstanding about the purpose of GPIO_ACTIVE_LOW, GPIO_ACTIVE_HIGH flags I would like to further clarify their usage. In principle they allow application to configure pin as working with positive or negative logic. Here is a short, random, on-line article that clarifies the concept: positive vs. negative logic. Linux documentation of gpio driver also provides abundant information.

If an application prefers to control CS (Chip Select) pin which is typically active low by writing 0 as a pin value it means that the pin should be configured as GPIO_ACTIVE_HIGH since we don't want to invert the logic. However, forcing a GPIO driver to work exclusively with positive logic is impractical. There are many use cases in embedded world where the only way to write simple, portable code is to have this functionality implemented in the driver. The typical examples are controlling the LEDs or power switches. They can be activated by either low or high output state depending on the board design or even the board version. To control a power switch application should not care about the specifics of the hardware but simply write 1 to enable the switch and 0 to disable it.

[pabigot]

Please see #11880 (comment) for a proposed compromise that supports existing usage while clarifying existing API and adding API to support positive/negative logic.

[anangl]

Wasn't the GPIO_ACTIVE_LOW flag supposed to be used also to specify active state of inputs? One may not want to use interrupts to handle buttons. Sometimes just reading the current state of the button could be desired.

[mnkp]

Indeed. I will change this line to

	gpios = <&gpiof 6 (GPIO_ACTIVE_LOW | GPIO_INT_LOW)>;

Significant unreviewed changes since this review was filed.

[anangl]

I am against changing the definition of how gpio_pin_write and gpio_pin_read are expected to behave. This would require alteration of all existing implementations of GPIO drivers (as the GPIO_ACTIVE_LOW flag cannot be just not supported).
In my opinion the GPIO_ACTIVE_* flags should be handled outside of particular GPIO drivers, for example in wrappers as I proposed in #11880 (comment).

[mnkp]

I provided a bit more elaborate answer in my comment in #11880. In short GPIO_ACTIVE_LOW is just a different name for GPIO_POL_INV flag which controls the value returned by gpio_pin_write and gpio_pin_read functions. Unfortunately, the documentation of these functions did not state it explicitly. The only reason we rename the GPIO_POL_INV flag to GPIO_ACTIVE_LOW is to be compatible with the definition of DTS gpio node in Linux.

[mnkp]

Superseded by #16648.