Zephyr: gpio: Cleanup flags

[galak]

This issue is to discuss reworking the current GPIO flags, today we have:

/* direction */
#define GPIO_DIR_IN             (0 << 0)
#define GPIO_DIR_OUT            (1 << 0)

/* interrupt */
#define GPIO_INT                (1 << 1)
#define GPIO_INT_ACTIVE_LOW     (0 << 2)
#define GPIO_INT_ACTIVE_HIGH    (1 << 2)
#define GPIO_INT_CLOCK_SYNC     (1 << 3)
#define GPIO_INT_DEBOUNCE       (1 << 4)
#define GPIO_INT_LEVEL          (0 << 5)
#define GPIO_INT_EDGE           (1 << 5)
#define GPIO_INT_DOUBLE_EDGE    (1 << 6)

/* polarity */
#define GPIO_POL_NORMAL         (0 << GPIO_POL_POS)
#define GPIO_POL_INV            (1 << GPIO_POL_POS)

/* PULL up/down */
#define GPIO_PUD_NORMAL         (0 << GPIO_PUD_POS)
#define GPIO_PUD_PULL_UP        (1 << GPIO_PUD_POS)
#define GPIO_PUD_MASK           (3 << GPIO_PUD_POS)

#define GPIO_DS_LOW_MASK (0x3 << GPIO_DS_LOW_POS)
#define GPIO_DS_DFLT_LOW (0x0 << GPIO_DS_LOW_POS)
#define GPIO_DS_ALT_LOW (0x1 << GPIO_DS_LOW_POS)
#define GPIO_DS_DISCONNECT_LOW (0x3 << GPIO_DS_LOW_POS)
#define GPIO_DS_DFLT_HIGH (0x0 << GPIO_DS_HIGH_POS)
#define GPIO_DS_ALT_HIGH (0x1 << GPIO_DS_HIGH_POS)
#define GPIO_DS_DISCONNECT_HIGH (0x3 << GPIO_DS_HIGH_POS)

[galak]

Removing GPIO_INT_CLOCK_SYNC with PR #10340

[galak]

So my first though is that at a basic level that we should describe a GPIO pin with GPIO_DIR_{IN,OUT}, and pull out from the GPIO_INT_* the ACTIVE_HIGH/LOW flags, and remove GPIO_POL_NORMAL/GPIO_POL_INV.

The INT flags we'd cleanup to something like:

GPIO_INT_EDGE_FALLING
GPIO_INT_EDGE_BOTH
GPIO_INT_LEVEL_HIGH
GPIO_INT_LEVEL_LOW

Not sure what to do with GPIO_INT_DEBOUNCE, what exactly do people think this means?

[Mani-Sadhasivam]

So my first though is that at a basic level that we should describe a GPIO pin with GPIO_DIR_{IN,OUT}, and pull out from the GPIO_INT_* the ACTIVE_HIGH/LOW flags, and remove GPIO_POL_NORMAL/GPIO_POL_INV.

The INT flags we'd cleanup to something like:

GPIO_INT_EDGE_RAISING
GPIO_INT_EDGE_FALLING
GPIO_INT_EDGE_BOTH
GPIO_INT_LEVEL_HIGH
GPIO_INT_LEVEL_LOW

+1

[pfalcon]

Not sure what to do with GPIO_INT_DEBOUNCE, what exactly do people think this means?

My wild guess is: some hardware has hardware debounce settings, that might be for that. But I never saw that in general-purpose MCUs (which we kinda target in the mainline Zephyr), and then the config of that debounce functionality was more complex than just one "debounce on" bit (e.g. there was debounce time, maybe something else).

[kubiajir]

Hi,
Im missing flag for pin Open Drain settings.

[DWagenk]

@kubiajir I think the GPIO_DS_*_* flags are meant for your use case although I haven't seen them implemented in the gpio drivers yet.

@galak I'm still unsure about the proposed GPIO_INT_* flags. They contain parts that I deem board-specific and parts that I deem application specific.

e.g. for the button sample:
the boards .dts files need to set GPIO flags to select if a low level or a high level at the gpio pin corresponds to button pressed. If needed the pull up should also be set here.
On the other hand the application needs flags to specify

I'm interested in the edge from not pressed to pressed

So for me the proposed flags

GPIO_INT_EDGE_RAISING
GPIO_INT_EDGE_FALLING
GPIO_INT_EDGE_BOTH
GPIO_INT_LEVEL_HIGH
GPIO_INT_LEVEL_LOW

don't really fit at the application level, because they are directly associated with the voltage level at the gpio pin, which is board-specific.

Maybe naming them

GPIO_INT_EDGE_TO_INACTIVE
GPIO_INT_EDGE_BOTH
GPIO_INT_LEVEL_ACTIVE
GPIO_INT_LEVEL_INACTIVE

communicates, that there's also an abstraction at the board level, that maps pin-voltage level to active/inactive state. In combination with GPIO_ACTIVE_[LOW/HIGH] in the boards .dts files this could work.

[DWagenk]

I'd also propose to be more verbose in the boards .dts files, to give more guidance for newly ported boards. Namely set all flags, that are deemed board-specific in the .dts file, even if they evaluate to the respective bit beeing set to 0.

e.g. for an LED:
(GPIO_DIR_OUT | GPIO_ACTIVE_LOW | GPIO_PUD_NORMAL)

and for a button:
(GPIO_DIR_IN | GPIO_ACTIVE_LOW | GPIO_PUD_PULL_UP)

Although GPIO_DIR_IN, GPIO_ACTIVE_LOW and GPIO_PUD_NORMAL don't have any effect on the code, because they are the defaults, it's more clear to me as a human reading it.

[mnkp]

So my first though is that at a basic level that we should describe a GPIO pin with GPIO_DIR_{IN,OUT}

When initializing pin as an output we need to define its initial state to either high or low. There is no way around it or we are going to drive the pin with some random value. Currently this requires two steps: setting the required voltage level on a pin and then configuring the pin as an output. This is clumsy and error prone.

So I propose to have two flags GPIO_DIR_OUT_HIGH, GPIO_DIR_OUT_LOW.

[mnkp]

Not sure what to do with GPIO_INT_DEBOUNCE, what exactly do people think this means?

My wild guess is: some hardware has hardware debounce settings, that might be for that. But I never saw that in general-purpose MCUs

The debounce circuitry is actually very common and present on many MCUs supported by Zephyr. Configuring its properties is a bit tricky though as it is MCU / vendor specific. Maybe this part could be done by DTS. The flag would only turn the circuitry on or off.

The debouncing is usually not interrupt specific. Also when reading / polling the pin directly the effect of debouncing circuitry is visible so the _INT part should probably be removed from the flag.

[galak]

Im missing flag for pin Open Drain settings.

What SoC / board are you needing/wanting open drain settings for?

[galak]

I'd also propose to be more verbose in the boards .dts files, to give more guidance for newly ported boards. Namely set all flags, that are deemed board-specific in the .dts file, even if they evaluate to the respective bit beeing set to 0.

e.g. for an LED:
(GPIO_DIR_OUT | GPIO_ACTIVE_LOW | GPIO_PUD_NORMAL)

and for a button:
(GPIO_DIR_IN | GPIO_ACTIVE_LOW | GPIO_PUD_PULL_UP)

Although GPIO_DIR_IN, GPIO_ACTIVE_LOW and GPIO_PUD_NORMAL don't have any effect on the code, because they are the defaults, it's more clear to me as a human reading it.

I'm in agreement with you here, I do wondering if DIR_IN/OUT is needed in the dts, it should be implied by the usage.

[tbursztyka]

@galak

Not sure what to do with GPIO_INT_DEBOUNCE, what exactly do people think this means?

That is meant to mitigate spurious interrupts due to crappy wiring or components (cheap buttons, etc...)

I guess we could remove that flag. Imo, if the controller supports debouncing internally, the driver should always enable it. If not, up to the electronic part to do it anyway.

[anangl]

I'd also propose to be more verbose in the boards .dts files, to give more guidance for newly ported boards. Namely set all flags, that are deemed board-specific in the .dts file, even if they evaluate to the respective bit beeing set to 0.
e.g. for an LED:
(GPIO_DIR_OUT | GPIO_ACTIVE_LOW | GPIO_PUD_NORMAL)
and for a button:
(GPIO_DIR_IN | GPIO_ACTIVE_LOW | GPIO_PUD_PULL_UP)
Although GPIO_DIR_IN, GPIO_ACTIVE_LOW and GPIO_PUD_NORMAL don't have any effect on the code, because they are the defaults, it's more clear to me as a human reading it.

I'm in agreement with you here, I do wondering if DIR_IN/OUT is needed in the dts, it should be implied by the usage.

What about the GPIO_DS_* flags? Should these also be specified in dts then?

[anangl]

So for me the proposed flags

GPIO_INT_EDGE_RAISING
GPIO_INT_EDGE_FALLING
GPIO_INT_EDGE_BOTH
GPIO_INT_LEVEL_HIGH
GPIO_INT_LEVEL_LOW

don't really fit at the application level, because they are directly associated with the voltage level at the gpio pin, which is board-specific.

Maybe naming them

GPIO_INT_EDGE_TO_INACTIVE
GPIO_INT_EDGE_BOTH
GPIO_INT_LEVEL_ACTIVE
GPIO_INT_LEVEL_INACTIVE

communicates, that there's also an abstraction at the board level, that maps pin-voltage level to active/inactive state. In combination with GPIO_ACTIVE_[LOW/HIGH] in the boards .dts files this could work.

But to be consistent we would then need to take this abstraction into account also in calls to gpio_pin_read and gpio_pin_write. I am afraid that this way we'd end up with a similar confusion that the one introduced by the GPIO_POL_* flags.
I'd prefer to stay with the naming proposed by @galak. At the application level one can use the flag from dts to check which edge, raising or falling, should be configured for the interrupt. This value is known at compile time so it is possible to provide macros to select the proper "to active" or "to inactive" edge if that is more convenient.

[jfischer-no]

@galak

Not sure what to do with GPIO_INT_DEBOUNCE, what exactly do people think this means?

That is meant to mitigate spurious interrupts due to crappy wiring or components (cheap buttons, etc...)

I guess we could remove that flag. Imo, if the controller supports debouncing internally, the driver should always enable it. If not, up to the electronic part to do it anyway.

Electromechanical components such as buttons bounce, no matter expensive or cheap. Debouncing causes signal delays, so it only makes sense to use it for the buttons and not for interrupt signals from sensors or transceivers. Please keep the GPIO_INT_DEBOUNCE flag or rename it to GPIO_DEBOUNCE as proposed by @mnkp.

[galak]

Electromechanical components such as buttons bounce, no matter expensive or cheap. Debouncing causes signal delays, so it only makes sense to use it for the buttons and not for interrupt signals from sensors or transceivers. Please keep the GPIO_INT_DEBOUNCE flag or rename it to GPIO_DEBOUNCE as proposed by @mnkp.

Do you think we should drop GPIO_DEBOUNCE from the sensor trigger code?

[galak]

What about the GPIO_DS_* flags? Should these also be specified in dts then?

Not sure if the semantics of GPIO_DS_* make sense across the board, but thinking we should pull DT flags from what Linux is doing as a starting point:

/* 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)

[jfischer-no]

Electromechanical components such as buttons bounce, no matter expensive or cheap. Debouncing causes signal delays, so it only makes sense to use it for the buttons and not for interrupt signals from sensors or transceivers. Please keep the GPIO_INT_DEBOUNCE flag or rename it to GPIO_DEBOUNCE as proposed by @mnkp.

Do you think we should drop GPIO_DEBOUNCE from the sensor trigger code?

I think yes.

[tbursztyka]

@jfischer-phytec-iot

Electromechanical components such as buttons bounce, no matter expensive or cheap. Debouncing causes signal delays, so it only makes sense to use it for the buttons and not for interrupt signals from sensors or transceivers. Please keep the GPIO_INT_DEBOUNCE flag or rename it to GPIO_DEBOUNCE as proposed by @mnkp.

The issue with providing a flag for DEBOUNCE is that not all hw support it. Actually, only a very few does if I look at our drivers, unlike what @mnkp said. Unless the feature was not handled in most of the drivers, then that's another issue.
Which makes a code using that flag not portable (aka: the gpio line behaviour will not be guaranteed depending on gpio controller below). Are we fine with that?

Or maybe we should avoid playing with it altogether: let the user does the right thing (having proper circuit between his button and the gpio line) or the crappy stuff (a soft debounce).

[b0661]

In Linux drivers (also GPIO drivers) may use the PINCTRL driver to configure the pins. ???This seems the way to go???. There is a standard set of pin characteristics to configure which are described in https://www.kernel.org/doc/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt.

If changing the GPIO flags why not use these well documented and seemingly sufficient flags for pin configuration?

I think the pinctrl flag names are easier to understand and the names correspond with what has to be put in the DTS for the pinctrl-x directive.

Here is the set of flags I used in the pinctrl driver #6760 but that can also be used by a GPIO driver configuring a pin. A GPIO driver usually only implements a subset depending on the HW capabilities.

• PINCTRL_CONFIG_BIAS_DISABLE
  Disable any pin bias.
• PINCTRL_CONFIG_BIAS_HIGH_IMPEDANCE
  High impedance mode ("third-state", "floating").
• PINCTRL_CONFIG_BIAS_BUS_HOLD
  Latch weakly.
• PINCTRL_CONFIG_BIAS_PULL_UP
  Pull up the pin.
• PINCTRL_CONFIG_BIAS_PULL_DOWN
  Pull down the pin.
• PINCTRL_CONFIG_BIAS_PULL_PIN_DEFAULT
  Use pin default pull state.
• PINCTRL_CONFIG_DRIVE_PUSH_PULL
  Drive actively high and low.
• PINCTRL_CONFIG_DRIVE_OPEN_DRAIN
  Drive with open drain (open collector).
  Output can only sink current.
• PINCTRL_CONFIG_DRIVE_OPEN_SOURCE
  Drive with open source (open emitter).
  Output can only source current.
• PINCTRL_CONFIG_DRIVE_STRENGTH_DEFAULT
  Drive with default drive strength.
• PINCTRL_CONFIG_DRIVE_STRENGTH_1
  Drive with minimum drive strength.
• PINCTRL_CONFIG_DRIVE_STRENGTH_2
  Drive with minimum to medium drive strength.
• PINCTRL_CONFIG_DRIVE_STRENGTH_3
  Drive with minimum to medium drive strength.
• PINCTRL_CONFIG_DRIVE_STRENGTH_4
  Drive with medium drive strength.
• PINCTRL_CONFIG_DRIVE_STRENGTH_5
  Drive with medium to maximum drive strength.
• PINCTRL_CONFIG_DRIVE_STRENGTH_6
  Drive with medium to maximum drive strength.
• PINCTRL_CONFIG_DRIVE_STRENGTH_7
  Drive with maximum drive strength.
• PINCTRL_CONFIG_INPUT_ENABLE
  Enable the pins input.
  Does not affect the pin's ability to drive output.
• PINCTRL_CONFIG_INPUT_DISABLE
  Disable the pins input.
  Does not affect the pin's ability to drive output.
• PINCTRL_CONFIG_INPUT_SCHMITT_ENABLE
  Enable schmitt trigger mode for input.
• PINCTRL_CONFIG_INPUT_SCHMITT_DISABLE
  Disable schmitt trigger mode for input.
• PINCTRL_CONFIG_INPUT_DEBOUNCE_NONE
  Do not debounce input.
• PINCTRL_CONFIG_INPUT_DEBOUNCE_SHORT
  Debounce input with short debounce time.
• PINCTRL_CONFIG_INPUT_DEBOUNCE_MEDIUM
  Debounce input with medium debounce time.
• PINCTRL_CONFIG_INPUT_DEBOUNCE_LONG
  Debounce input with long debounce time.
• PINCTRL_CONFIG_POWER_SOURCE_DEFAULT
  Select default power source for pin.
• PINCTRL_CONFIG_POWER_SOURCE_1
  Select power source #1 for pin.
• PINCTRL_CONFIG_POWER_SOURCE_2
  Select power source #2 for pin.
• PINCTRL_CONFIG_POWER_SOURCE_3
  Select power source #3 for pin.
• PINCTRL_CONFIG_LOW_POWER_ENABLE
  Enable low power mode.
• PINCTRL_CONFIG_LOW_POWER_DISABLE
  Disable low power mode.
• PINCTRL_CONFIG_OUTPUT_ENABLE
  Enable output on pin.
  Such as connecting the output buffer to the drive stage.
  Does not set the output drive.
• PINCTRL_CONFIG_OUTPUT_DISABLE
  Disable output on pin.
  Such as disconnecting the output buffer from the drive stage.
  Does not reset the output drive.
• PINCTRL_CONFIG_OUTPUT_LOW
  Set output to active low.
  A "1" in the output buffer drives the output to low level.
• PINCTRL_CONFIG_OUTPUT_HIGH
  Set output to active high.
  A "1" in the output buffer drives the output to high level.
• PINCTRL_CONFIG_SLEW_RATE_SLOW
  Select slow slew rate.
• PINCTRL_CONFIG_SLEW_RATE_MEDIUM
  Select medium slew rate.
• PINCTRL_CONFIG_SLEW_RATE_FAST
  Select fast slew rate.
• PINCTRL_CONFIG_SLEW_RATE_HIGH
  Select high slew rate.
• PINCTRL_CONFIG_SPEED_SLOW
  Select low toggle speed.
• PINCTRL_CONFIG_SPEED_MEDIUM
  Select medium toggle speed.
• PINCTRL_CONFIG_SPEED_FAST
  Select fast toggle speed.
• PINCTRL_CONFIG_SPEED_HIGH
  Select high toggle speed.

The current GPIO flags can be mapped to these pinctrl flags:

GPIO_DIR_IN
GPIO pin to be input.

PINCTRL pin configuration property:

- :c:macro:`PINCTRL_CONFIG_INPUT_ENABLE`,
- :c:macro:`PINCTRL_CONFIG_OUTPUT_DISABLE`

GPIO_DIR_OUT
GPIO pin to be output.

PINCTRL pin configuration property:

- :c:macro:`PINCTRL_CONFIG_INPUT_ENABLE`,
- :c:macro:`PINCTRL_CONFIG_OUTPUT_ENABLE`

GPIO_POL_NORMAL
GPIO pin polarity is normal.

PINCTRL pin configuration property:

- :c:macro:`PINCTRL_CONFIG_OUTPUT_HIGH`

GPIO_POL_INV
GPIO pin polarity is inverted.

PINCTRL pin configuration property:

- :c:macro:`PINCTRL_CONFIG_OUTPUT_LOW`

GPIO_PUD_NORMAL
GPIO pin to have no pull-up or pull-down.

PINCTRL pin configuration property:

- :c:macro:`PINCTRL_CONFIG_BIAS_DISABLE`

GPIO_PUD_PULL_UP
Pull-up GPIO pin.

PINCTRL pin configuration property:

- :c:macro:`PINCTRL_CONFIG_BIAS_PULL_UP`

GPIO_PUD_PULL_DOWN
Pull-down GPIO pin.

PINCTRL pin configuration property:

- :c:macro:`PINCTRL_CONFIG_BIAS_PULL_DOWN`

GPIO_DS_DFLT_LOW
Default drive strength when GPIO pin output is low.

PINCTRL pin configuration property:

- :c:macro:`PINCTRL_CONFIG_DRIVE_STRENGTH_DEFAULT`

GPIO_DS_DFLT_HIGH
Default drive strength when GPIO pin output is high.

PINCTRL pin configuration property:

- :c:macro:`PINCTRL_CONFIG_DRIVE_STRENGTH_DEFAULT`

GPIO_DS_ALT_LOW
Alternative drive strength when GPIO pin output is low.

PINCTRL pin configuration property:

- :c:macro:`PINCTRL_CONFIG_DRIVE_STRENGTH_7`

GPIO_DS_ALT_HIGH
Alternative drive strength when GPIO pin output is high.

PINCTRL pin configuration property:

- :c:macro:`PINCTRL_CONFIG_DRIVE_STRENGTH_7`

GPIO_DS_DISCONNECT_LOW
Disconnect pin when GPIO pin output is low.

PINCTRL pin configuration property:

- :c:macro:`PINCTRL_CONFIG_DRIVE_OPEN_SOURCE`

GPIO_DS_DISCONNECT_HIGH
Disconnect pin when GPIO pin output is high.

PINCTRL pin configuration property:

- :c:macro:`PINCTRL_CONFIG_DRIVE_OPEN_DRAIN`

[psidhu]

Im missing flag for pin Open Drain settings.

What SoC / board are you needing/wanting open drain settings for?

@galak bitbanged i2c. There's a few other cases I've needed open-drain setting configured. In fact, I've recently opened a PR that adds this for arch/stm32 (still a WIP) https://github.com/zephyrproject-rtos/zephyr/pull/9834

[pabigot]

IMO resolving this will be a good improvement to Zephyr: having to describe output signals like LEDs as GPIO_INT_ACTIVE_LOW in DT is weird. I also like the concept of leaving the choice of edge and level triggering to the application while active level is specified in the device-tree.

One point: Nordic hardware (GPIOTE) has resource-limited ability to detect edge transitions ("high accuracy"), and a general ability to detect level. The latter can be used to detect edges with some careful sense reconfiguration in the interrupt handler ("low accuracy). For signals like DATA_READY I want to detect the active state, but don't need high-accuracy; for others like triggers I do need high-accuracy.

Could this be supported with application-controlled flags like:

GPIO_INT_LOW_PRECISION

with the understanding that only some SOCs can take advantage of this distinction? This would avoid the need for SOC-specific code in generic drivers.

[pabigot]

@psidhu sorry I missed you on the original reviewer list; please review #11880 with respect to satisfying the needs of your #9834.

[carlescufi]

Addressed in the GPIO rework.