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_RAISING
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_ACTIVE
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.