gpio/pinctrl: GPIO and introduce PINCTRL API to support gpio, pinctrl DTS nodes

[mnkp]

Introduction

As a general requirement we want Zephyr DTS to adhere to the specification as well as to use bindings compatible with those of the Linux kernel. To support gpio, pinctrl DTS node bindings the current GPIO driver needs to be reworked. We also need to introduce PINCTRL driver.

A GPIO driver is used to configure and control the pins directly by the application. A PINCTRL driver is used to configure the pins and route the signals to the internal hardware modules on the SoC.

Requirement to support gpio, pinctrl DTS node bindings used by the Linux kernel doesn't imply that we have to provide the same GPIO, PINCTRL API like Linux does. In fact, the APIs can be quite different. We only need to process configuration data provided by the gpio, pinctrl DTS nodes in an easy and convenient manner.

Problem description

Unlike virtually any other driver e.g. I2C, RTC, CAN, UART which functionality is confined to a single hardware module the pin configuration and control functions can be spread among multiple SoC components. The implementation can vary significantly between vendors or even SoC families.

As an example both Intel Quark and NXP/Freescale K64 have separate GPIO and pinctrl modules however functionality assigned to each module by the respective SoC family is quite different. I.e. in one case debouncing is controlled by GPIO in the other by pinctrl module.

Pin functions controlled by

Intel Quark GPIO module: value, direction, interrupts, debouncing, source (pio/ext)
Intel Quark pinctrl module: muxing, pullup, slew rate

K64 GPIO module: value, direction
K64 pinctrl module: muxing, pullup/pulldown, slew rate, open drain, drive strength, interrupts, debouncing, source (pio/ext)

Proposed change

The driver API should hide implementation details and make it possible to write portable code. I.e. it would be impractical to expect user writing application code to know if they need to use PINCTRL or GPIO driver to configure an output as an open drain. As such both GPIO and PINCTRL API should provide means to fully configure pin properties such as open drain, drive strength, input debounce, pull up or schmitt-trigger mode.

Update GPIO and introduce PINCTRL API to let Zephyr drivers and application code configure pins using data provided by gpio, pinctrl DTS nodes. The API should use naming convention / configuration options which directly relate to the gpio, (gpio.h) and pinctrl bindings defined by Linux DTS. Zephyr implementation should preserve the meaning of Linux defined configuration options. E.g. relation between DTS pinctrl node property drive-open-drain and corresponding feature of the Zephyr API should be straightforward to identify and have the same effect.

Detailed RFC

GPIO/PINCTRL driver should use the following flags as a parameter to *_configure function:

• GPIO_ACTIVE_LOW, GPIO_ACTIVE_HIGH: indicate pin active state (logical value '1' in low state or logical value '1' in high state). gpio_pin_write/gpio_pin_read functions should write/read the logical pin value.
• GPIO_OPEN_DRAIN, GPIO_OPEN_SOURCE: configure single ended pin driving mode
• GPIO_PULL_UP, GPIO_PULL_DOWN: enable pin pull-up, pull-down
• GPIO_INPUT: configure pin as input
• GPIO_OUTPUT: configure pin as output. By default output is configured in push/pull mode. This behavior can be modified via GPIO_OPEN_DRAIN, GPIO_OPEN_SOURCE flags.
• GPIO_OUTPUT_INIT_LOW/GPIO_OUTPUT_INIT_HIGH: initialize the output in a low/high state
• GPIO_INPUT_DEBOUNCE: enable pin debounce circuitry

Linux DTS does not specify interrupt configuration within GPIO/PINCTRL node but rather a dedicated IRQ chip node. Zephyr GPIO API shall provide means to directly configure interrupts. Following base flags are proposed:

• GPIO_INT_ENABLE
• GPIO_INT_LEVEL
• GPIO_INT_EDGE
• GPIO_INT_LOW
• GPIO_INT_HIGH

To make interrupt configuration easier for the end user the following convenience flags are proposed:

• GPIO_INT_EDGE_RISING
• GPIO_INT_EDGE_FALLING
• GPIO_INT_EDGE_BOTH
• GPIO_INT_LEVEL_LOW
• GPIO_INT_LEVEL_HIGH

Where each of the convenience flags is defined as a combination of a base flag. E.g.

/** Trigger GPIO pin interrupt on level high. */
#define GPIO_INT_LEVEL_HIGH     (GPIO_INT_ENABLE  \
				 | GPIO_INT_LEVEL \
				 | GPIO_INT_HIGH)

New API functions

Following two functions are added to let the user directly control the pin level ignoring GPIO_ACTIVE_LOW flag. Writing value 1 to a pin will always set it to high output state, writing value 0 will always set it to low output state.

int gpio_pin_write_raw(struct device *port, u32_t pin, u32_t value);
int gpio_pin_read_raw(struct device *port, u32_t pin, u32_t *value);

Pin Controller API: TBD

Dependencies

When a new PINCTRL API is introduced the current PINMUX API can be deprecated.

Concerns and Unresolved Questions

The GPIO and PINCTRL drivers will significantly overlap their functionality since many driver features (e.g. pin drive strength, pull-up, pull-down, ability to configure output as on open source) need to be provided by both drivers. A better choice may be merging the two APIs into one.

[pabigot]

Pull-up and pull-down input configuration flags appear to be missing.

GPIO_DIR_OUT_LOW/HIGH and GPIO_OUTPUT_INIT_LOW/HIGH appear to be redundant.

Some devices (Nordic GPIO, Semtex SX1509B) support drive strength configuration with very different capabilities and defaults. Slew rate was also mentioned. Unless every potential hardware capability can be anticipated with a generic API, there should be some mechanism to pass driver-implementation-specific flags along with the standard ones so vendor extensions can be controlled by applications that are aware of the underlying hardware.

It's unclear whether the flags used in the device-tree gpios composite's flags field are to be the same as the corresponding argument to a GPIO driver configuration function, or if some of the flags may only relevant for either device-tree or dynamic configuration.

The rework should clearly define at what point the flags specified in the device tree will be applied: when the GPIO driver is initialized (e.g. to come up in a low-power mode) or when whatever uses the corresponding GPIO is initialized (which would likely be a very different configuration). It may be useful to provide multiple sets of configuration flags for a specific GPIO, perhaps by leveraging the PINMUX concept.

Functions in the new GPIO and PINCTRL driver API should clearly document exactly what is meant by each flag, and exactly what behavior is expected from drivers that are given flags that represent a configuration that is not supported by the hardware (ignore, error, make up something, ...). The existing solution failed to do this, and the results have been unsatisfactory.

[b0661]

GPIO_ACTIVE_LOW, GPIO_ACTIVE_HIGH: indicate pin active state (logical value '1' in low state or logical value '1' in high state)

If this is the output configuration it should be renamed to GPIO_OUTPUT_ACTIVE_LOW/HIGH to avoid any misinterpretation.

GPIO_OPEN_DRAIN, GPIO_OPEN_SOURCE: configure single ended pin driving mode

How is push/pull operation configured?

GPIO_INPUT_DEBOUNCE: enable pin debounce circuitry (the debounce parameters, if any, should be set by a SoC specific DTS)

Why not have a set of generic debounce times that may fit a lot of SoCs (e.g. DEBOUNCE_NONE, DEBOUNCE_SHORT, DEBOUNCE_MEDIUM, DEBOUNCE_LONG). What short/ medium/ long means is dependent on the SoC and may even be specified in the DTS.

Linux DTS does not specify interrupt configuration within GPIO/PINCTRL node but rather a dedicated IRQ chip node. Zephyr GPIO API shall provide means to directly configure interrupts. Following flags are proposed:

GPIO_INT_EDGE_RISING
GPIO_INT_EDGE_FALLING
GPIO_INT_EDGE_BOTH
GPIO_INT_LEVEL_LOW
GPIO_INT_LEVEL_HIGH

GPIO_INT_EDGE_BOTH is just a waste of configuration space, it should be the combination of the GPIO_INT_EDGE_RISING and GPIO_INT_EDGE_FALLING flags.

[mnkp]

Pull-up and pull-down input configuration flags appear to be missing.

Added to the issue description.

GPIO_DIR_OUT_LOW/HIGH and GPIO_OUTPUT_INIT_LOW/HIGH appear to be redundant.

I'm not sure how they made it there. Removed.

Some devices (Nordic GPIO, Semtex SX1509B) support drive strength configuration with very different capabilities and defaults. Slew rate was also mentioned. Unless every potential hardware capability can be anticipated with a generic API, there should be some mechanism to pass driver-implementation-specific flags along with the standard ones so vendor extensions can be controlled by applications that are aware of the underlying hardware.

I think we need to discuss drive strength, slew rate and debouncing configuration a bit more. In Linux DTS these are configured with a parameter.

• drive-strength takes as argument the target strength in mA.
• input-debounce takes the debounce time in usec as argument or 0 to disable debouncing
• slew-rate takes as argument an integer value (unit seems to be unspecified)

If we want to stay true to the promise that Zephyr DTS description is compatible with Linux DTS then we would need to configure these parameters via a dedicated functions taking respected parameter as an argument. The GPIO_INPUT_DEBOUNCE flag would need to be removed. Setting drive strength in mA doesn't seem to be very practical, on the other hand that's probably the only way to do it in a generic way. We could provide some SoC specific convenience macros that would encode available drive strengths in mA as an easy to understand name.

It's unclear whether the flags used in the device-tree gpios composite's flags field are to be the same as the corresponding argument to a GPIO driver configuration function, or if some of the flags may only relevant for either device-tree or dynamic configuration.

The DTS configuration has to follow the rules specified in GPIO bindings. These are documented in Linux DTS GPIO bindings.

The rework should clearly define at what point the flags specified in the device tree will be applied: when the GPIO driver is initialized (e.g. to come up in a low-power mode) or when whatever uses the corresponding GPIO is initialized (which would likely be a very different configuration). It may be useful to provide multiple sets of configuration flags for a specific GPIO, perhaps by leveraging the PINMUX concept.

According to the Linux DTS automatic configuration of GPIO pins is achieved by means of gpio-hog property.

The GPIO chip may contain GPIO hog definitions. GPIO hogging is a mechanism
providing automatic GPIO request and configuration as part of the
gpio-controller's driver probe function.

That's independent from GPIO API but surly needs to be clarified.

Functions in the new GPIO and PINCTRL driver API should clearly document exactly what is meant by each flag, and exactly what behavior is expected from drivers that are given flags that represent a configuration that is not supported by the hardware (ignore, error, make up something, ...). The existing solution failed to do this, and the results have been unsatisfactory.

Yes, fully agree. This should be taken care of in the PR.

[mnkp]

GPIO_ACTIVE_LOW, GPIO_ACTIVE_HIGH: indicate pin active state (logical value '1' in low state or logical value '1' in high state)

If this is the output configuration it should be renamed to GPIO_OUTPUT_ACTIVE_LOW/HIGH to avoid any misinterpretation.

The flag is honored by gpio_pin_write and gpio_pin_read functions. I've clarified the description.

GPIO_OPEN_DRAIN, GPIO_OPEN_SOURCE: configure single ended pin driving mode

How is push/pull operation configured?

By default (no flags) output is configured in push/pull mode. I've updated the description.

Why not have a set of generic debounce times that may fit a lot of SoCs (e.g. DEBOUNCE_NONE, DEBOUNCE_SHORT, DEBOUNCE_MEDIUM, DEBOUNCE_LONG). What short/ medium/ long means is dependent on the SoC and may even be specified in the DTS.

I discuss the configuration of debounce time in the previous comment.

Linux DTS does not specify interrupt configuration within GPIO/PINCTRL node but rather a dedicated IRQ chip node. Zephyr GPIO API shall provide means to directly configure interrupts. Following flags are proposed:

GPIO_INT_EDGE_RISING
GPIO_INT_EDGE_FALLING
GPIO_INT_EDGE_BOTH
GPIO_INT_LEVEL_LOW
GPIO_INT_LEVEL_HIGH

GPIO_INT_EDGE_BOTH is just a waste of configuration space, it should be the combination of the GPIO_INT_EDGE_RISING and GPIO_INT_EDGE_FALLING flags.

Indeed, that was not well explained. I propose to have to have two sets of flags. The base set used by the drivers, fully sufficient to configure an interrupt. Since their usage may not be intuitive to the end user / may involve too much typing the second set are convenience flags. This is a single identifier with a straightforward name which combines and may be used in place of a set of base flags. I've updated the description.

[anangl]

@mnkp
I am still concerned about the flags GPIO_ACTIVE_* and their influence on gpio_pin_write and gpio_pin_read functions. As I already mentioned in #12651 (comment), I think it would be better to keep the current behavior of gpio_pin_write and gpio_pin_read, and to handle these flags outside of GPIO drivers. My main concern is the inconsistency between reading/writing the state of pins and configuring interrupts for them (I assume from your previous comment in this topic that interrupt triggers should still operate only on physical input levels). I think this will be just confusing for people - one will configure an interrupt to be generated on the raising edge on a pin, and the state reported for this pin in the interrupt handler will be either 1 or 0, depending on the GPIO_ACTIVE_* flag used in this pin configuration. And to properly configure the interrupt to occur when the input changes its state to active, one would still need to use the same GPIO_ACTIVE_* flag that was used when the pin was configured (and a piece of code that will handle it, so probably it would be convenient to provide a wrapper for this in GPIO API).

You replied to my comment:

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.

I am not convinced that GPIO_ACTIVE_LOW in its current form is just a replacement for GPIO_POL_INV. As I understand it, the inversion of polarity should affect a given pin in all aspects, including the configuration of interrupts, just as if there was a hardware inverter connected to the pin. I tried to look at how the polarity inversion is handled in hardware currently supported by Zephyr's GPIO drivers:

• for Semtech SX1509B, polarity affects the relation between IO[x] and RegData[x], and the interrupt configuration is based on RegData[x], so it is influenced by the used polarity
• for TI CC13x0 and CC26x0, the documentation says only that the input/output can be configured as inverted, and interrupts can be generated for positive and/or negative edges on the IO, so I cannot tell what will be the actual behavior, and I don't have a piece of such hardware to check it (later on, for PWMs it is mentioned that output inversion affects the edge detection interrupts, but I'm not sure if any assumptions regarding GPIOs can be based on this)
• for ESP32 again, I cannot tell from the documentation whether GPIO interrupts are generated for pin levels before or after the inversion is applied
• for PCAL9535A, only the input pins can be inverted and the simplified schematic of the I/Os shows that the polarity inversion only affects the input port register data, it is not taken into account in the interrupt generation path
• other drivers either do not support the GPIO_POL_INV flag or support it in software

And now I'm really puzzled regarding what would be the best way to handle GPIOs polarity inversion. 🙂
Could anyone provide other examples of hardware supporting this feature, or provide any other hints?

Anyway, I still think that the influence of the GPIO_ACTIVE_LOW flag on the behavior of gpio_pin_write and gpio_pin_read functions should be reconsidered.

[mbolivar]

Hi @mnkp, I wanted to clarify my remarks about fast access functions during today's meeting.

I think that the _raw variants are good and should be included. The issue I am considering is a separate one that would be an enhancement to your existing work.

My main point is that it would also be useful for other device drivers that need fast access to GPIOs built into the SoC to have a generic API for accessing the output registers for these devices.

In other words, something like this:

struct device *gpio_port = device_get_binding(...);
u32_t *output_register;
u32_t my_bit = 3U;

gpio_get_output_register(gpio_port, &output_register);
*output_register |= (1U << my_bit);

To be equivalent to:

struct device *gpio_port = device_get_binding(...);
gpio_pin_write_raw(gpio_port, 3U, 1U);

except without the overhead, so that the bit manipulation on output_register could be done in a tight loop.

The inspiration is from the FastLED library's pin abstraction (https://github.com/FastLED/FastLED/blob/master/fastpin.h; also see PORTING.md in the same repository), which works on a variety of Arm SoCs, ESPs, and AVRs at the very least.

Of course, there are problems to consider:

• not all GPIO devices support this: SoCs may not have a GPIO port with this type of register, I2C-based GPIO expanders don't have this type of access, etc. But it is a common enough pattern that I think it would be useful to allow GPIO drivers to implement it, and gpio_get_output_register() can always return an error when it's not available. This would make it easier to write efficient drivers for timing sensitive bit-banging protocols when possible, without re-writing SoC-specific versions that duplicate GPIO driver code as we do today in e.g. https://github.com/zephyrproject-rtos/zephyr/blob/master/drivers/led_strip/ws2812b_sw.c.
• concurrent access to output_register and the GPIO APIs may be an issue as well. I think this is the responsibility of the driver author or application to avoid. And anyway, in designs where only a single core is accessing the GPIO port (the common case), accesses to output_register are likely to be done with interrupts disabled to ensure timing determinism.

Your thoughts are welcome. Thanks.

[mnkp]

@mbolivar thanks for the proposal.

I was hoping that removing the access_op from GPIO API would improve things significantly. It does improve things but there is still a fair share of pointer dereferencing going on. To test it I've implemented gpio_pin_write_raw on SAM E70 (ARM Cortex-M7) SoC which is a very friendly chip to work with.

static int gpio_sam_pin_write_raw(struct device *dev, u32_t pin, u32_t value)
{
	const struct gpio_sam_config * const cfg = DEV_CFG(dev);
	Pio *const pio = cfg->regs;

	if (value) {
		/* Set the pin. */
		pio->PIO_SODR = 1 << pin;
	} else {
		/* Clear the pin. */
		pio->PIO_CODR = 1 << pin;
	}
	return 0;
}

with default Zephyr compiler optimization options the implementation takes 11 assembly instructions (for comparison the gpio_sam_pin_write function takes 32 assembly instructions. Since there is also some overhead for calling the function to have truly fast GPIO access we definitely need to considered the type of API you mentioned.

I believe though that letting an application directly access the output_register will not be possible. There are SoCs that provide only output_set, output_clear registers. There are SoCs that provide only output_write register. There are SoCs that provide both. The internal layout of the registers is also not always consistent. We would probably need a set of inline functions which would present a consistent API but be implemented individually by the SoC. Similar to your proposal but writing to the register would happen via an inline function. We would need to rely on CMake to include the right header (and in this case also implementation) file.

[mbolivar]

I believe though that letting an application directly access the output_register will not be possible. There are SoCs that provide only output_set, output_clear registers. There are SoCs that provide only output_write register. There are SoCs that provide both. The internal layout of the registers is also not always consistent.

Yes, I understand this is not a universally supported register type. However, I believe that the existence of the fastled library shows that there is a small set of "types" of GPIO output registers, which we can support through a common API. E.g. if gpio_get_output_register errors out, the driver could fall back on gpio_get_output_set_register(), gpio_get_output_clear_register(), etc., with coverage level depending on the driver. We could consider a query routine where the GPIO driver returns a bitmask of supported types.

We would probably need a set of inline functions which would present a consistent API but be implemented individually by the SoC. Similar to your proposal but writing to the register would happen via an inline function. We would need to rely on CMake to include the right header (and in this case also implementation) file

I'm not sure I can see clearly how some of these details would work in practice, but I look forward to seeing more details if you have them.

Thanks for your response!

[anangl]

@mnkp Regarding the question I was trying to signal yesterday on slack. It's about configuring interrupts for handling a button, for instance. An application will get the flags defined for the button in DTS through a a generated macro, like SW0_GPIO_FLAGS. Ideally, it would just pass the value to the gpio_pin_configure function to get the pin properly configured, just like in the case of the LED that you presented in your PR. But the flags controlling the interrupts generation are application specific - some applications will need to react on the input becoming active (button pressed), others on the input becoming inactive (button released), and in some cases, perhaps not buttons, there may be a need to react on the signal level, not an edge. Thus, I think that the flags related to interrupts should not be specified in DTS (like in your PR), but instead the GPIO API should provide some means to get the proper interrupt configuration based on the GPIO_ACTIVE_* flag used in DTS. Hence, I thought that we would need to add some macros or inline functions that would handle the translation. Or do you have some other vision of achieving this?

[anangl]

After taking another look, I've got one more doubt. Why do we change the required behavior of gpio_pin_{read|write} functions so that they now are to operate on the logic level and then we introduce gpio_pin_{read|write}_raw functions that are to behave just like gpio_pin_{read|write} were supposed to so far?
Wouldn't it be clearer/safer to only introduce new functions that take into account the GPIO_ACTIVE_LOW flag? Possibly with names clearly indicating this, for instance gpio_pin_{activate|deactivate|is_active} or gpio_pin_{assert|deassert|is_asserted} as @pabigot proposed earlier in his PR.

[mnkp]

I think that the flags related to interrupts should not be specified in DTS (like in your PR), but instead the GPIO API should provide some means to get the proper interrupt configuration based on the GPIO_ACTIVE_* flag used in DTS.

The current interrupt configuration done in the Zephyr version of the DTS is not standard and indeed it may be best to remove it. We can always add it back in the future, possibly in a standard form, if the need arises. The easiest way to have the application configure interrupts in a portable way is to define them in terms of pin active, inactive state as in: LEVEL_INACTIVE, LEVEL_ACTIVE, EDGE_TO_ACTIVE, EDGE_TO_INACTIVE.

A general remark regarding DTS:
According to the documentation DTS is meant to provide information about hardware the OS runs on as well as its initial configuration for the boot process. Thus uart DTS node contains register addresses of the UART module as well as the initial baud rate. The application is free to change the configuration later. It's actually OK to add application specific bits to the DTS.

[mnkp]

After taking another look, I've got one more doubt. Why do we change the required behavior of gpio_pin_{read|write} functions so that they now are to operate on the logic level and then we introduce gpio_pin_{read|write}_raw functions that are to behave just like gpio_pin_{read|write} were supposed to so far?

I'll copy paste my answer from #12651.

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.

In short we don't change the behavior of gpio_pin_{read|write} functions. We rename the GPIO_POL_INV flag to match Linux DTS and clarify its usage.

[anangl]

In short we don't change the behavior of gpio_pin_{read|write} functions. We rename the GPIO_POL_INV flag to match Linux DTS and clarify its usage.

I cannot entirely agree with that. As I recall, one of the reasons for making this GPIO flags cleanup was the need to replace GPIO_INT_ACTIVE_{HIGH|LOW} flags used in DTS files to define the active state of buttons and LEDs with something more appropriate (see #10339 and its predecessor #10300). In this context, when we replace GPIO_INT_ACTIVE_LOW with GPIO_ACTIVE_LOW, we change the behavior of gpio_pin_{read|write} functions.
See for instance the basic button sample. It takes the flags defined in DTS for the button with sw0 alias and passes them directly to gpio_pin_configure. After GPIO_INT_ACTIVE_LOW is replaced with GPIO_ACTIVE_LOW, this call to gpio_pin_read will give the opposite result than it does currently. The read value is only printed here, so it is not a big deal, but I'm afraid that we may encounter cases with more serious consequences of such change (related to incorrect driving of CS pins, for example). Do you think it is possible to catch them all while applying the proposed changes in GPIO flags?

[carlescufi]

@mnkp @anangl
My suggestion here would be to:

• Deprecate GPIO_POL_INV so that we get rid of the pseud-logical behavior of the read/write functions
• Leave gpio_pin_read/write as they are, physical and using the *value
• Add 4 new functions, all using __ASSERT():
  • bool gpio_pin_get(*port, pin) which returns the logical value as a return param
  • void gpio_pin_set(*port, pin, value)
  • u32_t gpio_port_get(*port)
  • void gpio_port_set(*port, mask, value)

[anangl]

@carlescufi
I was thinking about similar names for these new functions, but I wasn't sure if _get and _set would not be too general, so I thought that maybe _state_{get|set} could work, but actually these didn't convince me fully either. And so far I couldn't come up with anything better.
Anyway, your proposal looks good to me. With one correction, gpio_port_set should take 3 parameters: (*port, pin_mask, value), so that one could set states of pins from the group specified by pin_mask to desired states (active or inactive) as defined by the bits in value.

[pabigot]

@anangl @carlescufi gpio_pin_{assert,deassert} as with #11880 (comment)? I believe it's understood that "assert" means logic true and "deassert" logic false.

In any case, I think you're running into the same problems that killed #11880: we can be backward compatible, or we can match Linux/DTS API. Because GPIO_POL_INV wasn't clearly defined and implemented consistently across implementations we can't do both.

Or so I believe. I'm not sure everybody believes they're incompatible, and if they are I don't know that there's agreement on which requirement should take priority. If not, that should be clearly settled first.

[carlescufi]

@pabigot @anangl @mnkp
Now we're getting into personal preference territory, but I honestly think that assert,deassert is less clear than get,set. Looking at other APIs I can also propose:

• bool gpio_pin_get_level(*port, pin)
• void gpio_pin_set_level(*port, pin, value)
• u32_t gpio_port_get_levels(*port)
• void gpio_port_set_levels(*port, mask, value)

The advantage with get_level is that we could actually reuse it for physical or logical level depending on what you pass to gpio_configure(), which would allow users to use the new signatures (returning bool and u32_t and no pointer:

• GPIO_ACTIVE_LOW: logical levels
• no additional flag: physical (and logical) levels

that way we match the classical "active level low/high` way of describing the logical levels.
EDIT: A couple more choices below

• bool gpio_get_pin_level(*port, pin)
• void gpio_set_pin_level(*port, pin, value)
• u32_t gpio_get_port_levels(*port)
• void gpio_set_port_levels(*port, mask, value)

---

• bool gpio_get_level(*port, pin)
• void gpio_set_level(*port, pin, value)
• u32_t gpio_get_levels(*port)
• void gpio_set_levels(*port, mask, value)

[mnkp]

Thanks @carlescufi for the proposal. Nice selection!

Since we are getting into personal preference territory regarding naming I'll make my statement. I like very much the first proposal: gpio_pin_get, gpio_pin_set, gpio_port_get, gpio_port_set. The function names are short and to the point. They should become the most used GPIO API functions so it's better to keep them short. IMHO the level variants all read well but there is no added value, we only make the name longer.

[anangl]

While I agree with @pabigot that gpio_pin_{assert,deassert} would indicate the purpose of these functions in perhaps the clearest way, I see a disadvantage of such approach. We'd have separate functions for setting active and inactive pin levels, while set handles both cases. I think it matters especially for ports, where it will be convenient to have the possibility of setting a selected group of pins to different levels in one call.
And I agree with @mnkp that the get,set proposal seems to be the best one. But still, shouldn't the gpio_port_set function take the third parameter with values to set for the pins specified by the mask (as I already tried to signal above)? Or do I misunderstand something?

[pabigot]

And in this case maybe it will be better to use the _level variants of pin_get, pin_set functions (to easier distinguish the two) as proposed by @carlescufi i.e. use gpio_pin_get_level, gpio_pin_set_level functions.

I would prefer to avoid using the term "level" in the API identifiers. AFAICT the API in #16648 has the traditional gpio_pin_read which operates on the line (electrical/physical) level and new gpio_pin_get which operates on the logical level. Both of them are levels, so adding _level does not help the user to understand what the function does.

If this is for the "fast" version of the API I'd rather see that as a separate, parallel API with a different prefix, e.g. gpiof_pin_read(), just as Linux gpiod_get_value(&gpio_desc) is for a descriptor based API that replaces the legacy gpio_get_value(unsigned).

[MaureenHelm]

If you do:

• u32_t gpio_port_get(*port)
• void gpio_port_set(*port, set_bits, clear_bits)

you don't need need the mask on the read, and you get a slightly more powerful operation that I personally think is more clear than "set to this value, oh, but ignore everything but these bits". (I use this in my APIs, with the semantics that a bit set in both set_bits and clear_bits is inverted from whatever its previous value was. Very useful for flashing LEDs.)

How is the masked write not clear? It corresponds directly to hardware implementations with explicit set and clear port registers, which allow you to change the state of a pin without a read-modify-write cycle.

I don't see how a bit set in both set_bits and clear_bits should mean to invert it.

I think I'd rather see something like:

void gpio_port_set(*port, set_bits)
void gpio_port_clear(*port, clear_bits)

And if you really want an invert, you can add:

void gpio_port_invert(*port, invert_bits)

[pabigot]

So that'd be:

u32_t gpio_port_read(port); // line level
u32_t gpio_port_get(port);  // logic level
void gpio_port_write(port, value); // line level, all bits
void gpio_port_set1(port, value);  // logic level, all bits
void gpio_port_set(port, set_bits); // logic level, only specified bits
void gpio_port_clear(port, clear_bits); // logic level, only specified bits
void gpio_port_invert(port, invert_bits); // logic level, only specified bits

Other than the naming conflict when both line and logic level operations are supported I don't have a big problem with this. The original motivation for the API I proposed was to resolve the conflict when having a mask parameter to the set operation suggested there should be a corresponding mask in the get operation.

[carlescufi]

Here is my proposal based on everything that has been said so far, and taking into account the following:

• Simplicity and consistency
• Anything returning an int can return:
  • A negative value (error)
  • 0 (logical false, physical low)
  • A positive value (logical true, physical high)

Standard APIs:

/** Pin access **/
/* Logical */
int gpio_pin_get(*port, pin);
int gpio_pin_set(*port, pin);
/* Physical */
int gpio_pin_get_raw(*port, pin);
int gpio_pin_set_raw(*port, pin);

/** Port access **/
/* Logical */
int gpio_port_get(*port, *value);
int gpio_port_get_masked(*port, mask *value);
int gpio_port_set(*port, value); // writes the value to the port
int gpio_port_op(*port, set_bits, clear_bits, invert_bits);

/* Physical */
int gpio_port_get_raw(*port, *value);
int gpio_port_get_masked_raw(*port, mask *value);
int gpio_port_set_raw(*port, value); // writes the value to the port
int gpio_port_op_raw(*port, set_bits, clear_bits, invert_bits);

Fast/descriptor-based APIs:

/* Get a descriptor */
struct gpiod_t* gpiod_get(*port);

The rest of APIs should be exactly the same as the standard APIs but:
- Replacing gpio_ with gpiod_ as a prefix
- Replacing the first port parameter with struct gpiod_t*

[pabigot]

I don't believe we have consensus on the port set API. There have been three proposals:

// API 1
int gpio_port_set_masked(*port, value, mask); // writes the value to the port, only non-masked bits
// API 2
int gpio_port_set_sct(*port, set_bits, clear_bits); // sets some bits, clears some bits, toggles if both requested
// API 3
int gpio_port_set_bits(*port, set_bits); // sets specified bits
int gpio_port_clear_bits(*port, clear_bits); // clears specified bits
int gpio_port_invert_bits(*port, toggle_bits); // TBD: changes level of specified bits

(1) is existing practice, so should be the fallback if we can't agree on a change. (2) seemed to get some approval and interest, but one rejection. (3) hasn't really been discussed.

I prefer (2), because the set-clear-toggle bit mutation operation has general applicability for flags and register values outside of GPIO, and I am going to use it in other situations where it's more clear. I could live with the others.

So how about we vote? I'm going to post separate comments for each choice; sorry for the blitz. Click thumbs-up to prefer this API. Click thumbs-down if you think it's an absolutely horrible idea and would want to block a PR that includes it. Vote up on one choice, and down on any of them you want. We'll figure out how to tally the votes when we have some.

[pabigot]

Click thumbs-up to prefer this API. Click thumbs-down if you think it's an absolutely horrible idea.

// Multi-pin mutate operation API 1
int gpio_port_set_masked(*port, value, mask); // writes the value to the port, only non-masked bits

[pabigot]

Click thumbs-up to prefer this API. Click thumbs-down if you think it's an absolutely horrible idea.

// Multi-pin mutate operation API 2
int gpio_port_set_sct(*port, set_bits, clear_bits); // sets some bits, clears some bits, toggles if both requested

[pabigot]

Click thumbs-up to prefer this API. Click thumbs-down if you think it's an absolutely horrible idea.

// Multi-pin mutate operation API 3
int gpio_port_set_bits(*port, set_bits); // sets specified bits
int gpio_port_clear_bits(*port, clear_bits); // clears specified bits
int gpio_port_invert_bits(*port, toggle_bits); // TBD: changes level of specified bits

[pabigot]

Click thumbs-up to prefer an API that combines AP1 and API3. Click thumbs-down if you think it's an absolutely horrible idea.

// Multi-pin mutate operation API 1
int gpio_port_set_masked(*port, value, mask); // writes the value to the port, only non-masked bits
// Multi-pin mutate operation API 3
int gpio_port_set_bits(*port, set_bits); // sets specified bits
int gpio_port_clear_bits(*port, clear_bits); // clears specified bits
int gpio_port_invert_bits(*port, toggle_bits); // TBD: changes level of specified bits

[pabigot]

Reviewing some of the material above there's another consensus decision that needs to be summarized: handling of line vs logic level taking into account GPIO and interrupt configuration and the impact of adding line/logic level API on the use of existing GPIO_POL_* flags. Every bulleted item below is intended to be either a definition or a requirement that together determine whether an implementation of the API is conforming.

A clarified terminology summary from the pin/port access API summary above:

• Line levels are defined in a peripheral-specific way by comparing voltages against thresholds, and are provided by the peripheral as a binary value.
• Valid line levels are high (positive) and low (zero).
• Valid logic levels are true/1/asserted (positive) and false/0/deasserted (zero).
• Unknown/error state for both levels are represented by negative integers.

Logic level derives from line level based on a configured active level:

• A pin is configured ACTIVE_HIGH if and only if it is not configured ACTIVE_LOW.

I.e. active level is a binary state. Informally, if a pin is configured active high the value representations of logic level and line level are equivalent; otherwise the logic level is the inverse of the line level. In support of formalizing this, define these two functions for any pin p within the device:

• Let ACTLOW(p) return:
  • true when p is pin configured ACTIVE_LOW;
  • false when p is pin configured ACTIVE_HIGH;
• Let LEVCONV(p, lev) return:
  • (lev == 0) XOR ACTLOW(p) if lev is a valid level (i.e. non-negative);
  • lev if lev is an unknown/error state (negative)

where standard C conversion between integer and boolean value spaces is assumed and XOR is boolean exclusive or.

We have also introduced a distinction between GPIO mode and interrupt mode for a pin. Using the proposed API of #16648:

• A pin that was last configured by gpio_pin_configure() is a GPIO-controlled pin.
• A pin that was last configured by gpio_pin_interrupt_configure() is an interrupt-controlled pin.
• GPIO API functions other than gpio_pin_configure() are disallowed and shall return -ENOTSUP if invoked on interrupt-controlled pins.
• Interrupt API functions other than gpio_pin_interrupt_configure() are disallowed and shall return -ENOTSUP if invoked on GPIO-controlled pins.
• On driver startup every pin must be configured as either GPIO-controlled or interrupt-controlled.

This summarizes my understanding of what we've decided; again, this needs to be validated.

The effect of active level on pin mode is specified by this rule:

• The active level configuration of a pin affects API behavior for both GPIO-controlled and interrupt-controlled pins.

My recollection is that in early discussions this was not true (possibly because it was mixed with GPIO_POL_INV), so we need to confirm or reject this interpretation.

Abstracting away from public API names we have an inspect operation get and a mutate operation set, where the unadorned name operations on logic levels and a suffix _raw operates on line levels. The fundamental get_raw function defines the behavior of an internal inspection of pin state within the target, regardless of whether the pin is GPIO or interrupt controlled. For GPIO-controlled it is equivalent to the exposed GPIO function; for interrupt-controlled its defined behavior is used to configure the interrupt conditions.

Proposed detailed semantics for get_raw(p) for any pin p are:

• If a pin is configured to enable input, get_raw(p) returns the line input level for p sampled/provided by the GPIO peripheral; otherwise
• If a pin is configured to enable output, get_raw(p) returns the line output level configured for p in the GPIO peripheral; otherwise
• get_raw returns (TBD) -EINVAL

NOTE: The requirements above are carefully worded to specify the behavior when a pin is configured for both input and output. There are devices (e.g. SX1509B) where bidirectional configuration is supported and the input signal may be different from the configured output signal. This proposed behavior must be validated, because it fails to provide a way to read the configured output of a signal that is bidirectional. Also note that this requires that a driver be able to provide the last configured output value; it's possible some peripherals don't provide a way to read that back.

With the above, we then have:

• get(p) is LEVCONV(p, get_raw(p)) in all situations.

Where "all situations" covers GPIO and interrupt configuration, direction, and active level.

For mutation operations:

• set_raw(p, v) shall:
  • set the output signal to high and return TBD if v is line level high;
  • set the output signal to low and return TBD if v is line level low;
  • return -EINVAL if v is an unknown/error level
• set(p, v) shall be equivalent to set_raw(p, LEVCONV(p, v)) in all situations.

Finally we need the following to deal with the legacy parts of the API:

• GPIO_POL_NORMAL shall have the same effect as GPIO_ACTIVE_HIGH
• GPIO_POL_INV shall have the same effect as GPIO_ACTIVE_LOW
• These flags shall not be used independently to control an underlying hardware peripheral pin inversion capability. A hardware capability may be used to implement ACTIVE_LOW as long as the API requirements are satisfied.

Without this people may continue to use these flags to enable peripheral-specific inversion capabilities, resulting in cross-platform inconsistencies. (I would rather retain these flags and specify that their use has a target-specific impact on the behavior of getraw() and setraw(), but I don't think that's the consensus position.)

[anangl]

@pabigot
Two doubts from my side.

1. Regarding get_raw(p). Is this bullet:

• If a pin is configured to enable output, get_raw(p) returns the line output level configured for p in the GPIO peripheral; otherwise

really necessary, will such functionality be useful? Maybe we could remove it and hence simplify the API by allowing using get_raw(p) only for pins with enabled input?

2. Regarding GPIO- vs interrupt-controlled pins.

• A pin that was last configured by gpio_pin_configure() is a GPIO-controlled pin.
• A pin that was last configured by gpio_pin_interrupt_configure() is an interrupt-controlled pin.

I am not convinced that we should distinct such two modes for pins, and basing on them disallow parts of API. I'd rather instead set a requirement for set_raw(p) to return an error code (-EINVAL perhaps) if it is called for a pin that was not configured to enable output, and a similar one for get_raw(p).
I understand the introduced gpio_pin_interrupt_configure as an additional to gpio_pin_configure way of configuring GPIO interrupts, to be used in cases when a pin is not to be controlled via GPIO driver (like the one signaled by @b0661). And I thought this function could be used also for changing only the interrupt configuration for a pin configured previously by gpio_pin_configure.

[pabigot]

@anangl Thanks for the careful reading.

For the semantics of interrupt versus GPIO configuration I represented my understanding based on the goal of moving towards PINMUX functionality where interrupt functions are separated (to some degree) from GPIO functions. From earlier discussions I understood that on some hardware a pin configured to be in interrupt mode may not support GPIO operations. I haven't encountered that myself, but I defer to @mnkp to precisely define the two roles and the functions used to control them.

For reading the configured output state of a pin: if it is to be possible to toggle a pin state, as with flashing an LED, you need to be able to read the current setting so you know what value to write to change that state. That could be done by:

• (A) providing a facility to read the configured output state in the GPIO API;
• (B) providing a facility to toggle the configured output state in the GPIO API; or
• (C) requiring anything that wants to toggle to maintain the last configured state itself.

In my APIs I usually go with (B), but (C) is OK too and simplifies GPIO.

Thumbs up here to go with (C), removing explicit toggle support from the GPIO API.

[anangl]

@pabigot Both (B) and (C) are OK to me, my point was just to not go with (A). (B) seems to be a bit better option, since there are pieces of hardware providing dedicated registers for toggling (no need to read the output state then). It shouldn't be that hard to agree on how to support toggling in the GPIO API.

[pabigot]

It shouldn't be that hard to agree on how to support toggling in the GPIO API.

You are an optimist.

A minimal impact way to do it is to define toggle as the behavior specified when a bit is set in both the set_bits and clear_bits parameters to the fundamental set-clear operation. As there were reasonable objections to that based on it being obscure, and I think adding an explicit toggle operation is over-engineering. I still prefer (C)

Edit: Actually, if we specify the behavior of set_bits to be "Pins selected by this mask are changed to high/active if they were not already at that state", and clear_bits similarly, then the toggle behavior comes out naturally. So I'm totally fine with (B), and would take (C) as an option. I'm not supportive of a separate API function just to toggle.

Edit2: In fact this specification also defines the behavior when the driver knows the operation will not change the current configuration (viz., it must not appear to occur). This is relevant to drivers like SX1509B where an I2C transaction can be eliminated. That tips my vote to (B) provided though this mechanism.

[anangl]

A minimal impact way to do it is to define toggle as the behavior specified when a bit is set in both the set_bits and clear_bits parameters to the fundamental set-clear operation.

I agree and I think this is a good way to go. Such semantics may seem a bit "exotic" at first glance, but after you think of it a bit, you realize that it is a good way of utilizing a parameter combination that has to be checked anyway, but instead of returning an error, providing a nice feature for it. To me this is mainly a question of describing it properly in the documentation, so that it is clear to users. What you propose in your first edit seems reasonable.

Edit2: In fact this specification also defines the behavior when the driver knows the operation will not change the current configuration (viz., it must not appear to occur). This is relevant to drivers like SX1509B where an I2C transaction can be eliminated. That tips my vote to (B) provided though this mechanism.

Yes. That convinces me even more that this option is a better choice.

[carlescufi]

API meeting:

Standard APIs:

/** Pin access **/
/* Logical level */
int gpio_pin_get(*port, pin);
int gpio_pin_set(*port, pin, value);
/* Line level */
int gpio_pin_get_raw(*port, pin);
int gpio_pin_set_raw(*port, pin);

/** Port access **/
/* Logical level */
int gpio_port_get(*port, u32_t *value);
int gpio_port_set(*port, u32_t value); // writes the value to the port
int gpio_port_set_masked(*port, u32_t mask, u32_t value); // writes the value to the port
int gpio_port_set_clr_bits(*port, u32_t set_bits, u32_t clear_bits);
int gpio_port_set_bits(*port, u32_t bits);
int gpio_port_clear_bits(*port, u32_t bits);
int gpio_port_toggle_bits(*port, u32_t bits);

/* Line level */
int gpio_port_get_raw(*port, u32_t *value);
int gpio_port_set_raw(*port, u32_t value); // writes the value to the port
int gpio_port_set_masked_raw(*port, u32_t mask, u32_t value); // writes the value to the port
int gpio_port_set_clr_bits_raw(*port, u32_t set_bits, u32_t clear_bits);
int gpio_port_set_bits_raw(*port, u32_t bits);
int gpio_port_clear_bits_raw(*port, u32_t bits);
int gpio_port_toggle_bits_raw(*port, u32_t bits);

Fast/descriptor-based APIs:

/* Get a descriptor */
gpiod_t gpiod_get(*port);

The rest of APIs should be exactly the same as the standard APIs but:
- Replacing gpio_ with gpiod_ as a prefix
- Replacing the first port parameter with gpiod_t
- Only _raw versions of the APIs would be available
- Implemented as static inline functions

@mnkp the above is from the API meeting, are you happy with that?

[galak]

int gpio_port_sc_bits(*port, u32_t set_bits, u32_t clear_bits);

I'd suggest:

int gpio_port_set_clr_bits(*port, u32_t set_bits, u32_t clear_bits);

[lawrencek52]

All of the APIs above have a baseline assumption that you are working on a 32-bit machine, and can only set or clear up to 32 bits at a time. what happens when you move to a 64-bit machine? What happens if you are working on a 16-bit native machine, and it has to extend all of the parameters up to 32-bits? You are passing in 32-bit ints, but getting back native machine ints (which may or may not be 32-bits).

[pabigot]

The baseline assumption is that no GPIO device has more than 32 pins. There's one existing device that has more, and it has to be split into separate devices.

Yes, if there's a 16-bit target this would cause overhead, but Zephyr pretty strongly expects a minimum 32-bit system. I generally prefer native int types but for a cross-platform system they introduce complexity.

Plain int returns are (or should be) always zero, a boolean, or a negative error code. Pin sets should always be returned via out parameters. If that isn't happening, it needs to be fixed.

[mnkp]

The int that the gpio_port_* functions return contains an error status, as defined by errno.h. If there is a functional value that we want to return, as it is in case of gpio_port_get function, we do it via a function parameter.

That said, I agree that we should discuss / reconsider the need to enforce a fixed 32-bit types in Zephyr's API. This issue has been neglected for a while.

[pabigot]

In the API meeting last week I explicitly asked whether anybody had an issue with requiring a 32-bit value to represent the set of pins supported by a single driver and people were OK with it.

For future-proofing and clarity of argument purpose I'd be happy to see typedef u32_t gpio_value_t and use gpio_value_t in the API. It could be typedef'd on a per-platform basis, but that might cause problems with external GPIO devices that are not platform-specific and might be larger/smaller than the platform default.

[pabigot]

tl;dr: The facts have been collected; I've changed my mind; and I now propose revising the specification so set_clr does not support toggle and instead clears pins that are present in both sets.

So I've been spending some time with #16648 and based on experience would like to propose a change.

I originally promoted the set_clr interface because it's a basis operation: set, clear, and set_masked can all be implemented in terms of it, and toggle can too with careful definition of the operation. I've used this technique in several APIs and found it to reduce complexity (one function instead of four).

I've now implemented that on top of #16648. The effect is that it reduces code size slightly (102 By on nRF52) and slightly reduces the size of the driver structure.

It also incurs a noticeable overhead. Below are times, in microseconds, for each step of a series of operations. 16648 is the original implementation using four basis functions, Basis is one that replaces them with one core implementation, NoTgl is Basis with the branch test that supports toggling in set_clr removed, and New is explained later:

Operation	16648	Basis	NoTgl	New
gpio_port_clear_bits	0.56	1.1	1.0	0.57
gpio_port_set_bits	0.70	1.0	0.92	0.70
gpio_port_set_bits	0.68	1.1	1.0	0.68
gpio_port_set_clr_bits	1.12	1.15	0.95	0.49
gpio_pin_set	0.56	0.92	0.84	0.58
gpio_port_clear_bits_raw	0.59	0.94	0.87	0.55
Per Iteration	7.02	9.6	8.9	6.34

When measured at this level the time for each operation in Basis ranges from 43% to 96% more than for 16648, ignoring the toggle operation which is slightly faster in Basis. When measured at a higher level (the average time for several thousand iterations of a function call that wraps entire sequence) the Basis approach takes 36% longer.

If the original set_clr_bits call is replaced with toggle_bits the corresponding call reduces from 1.12 us to 0.49 us.

Analysis suggests that most of the increased cost in the basis function approach is due to branch conditions that deal with toggle support. The implementation also handled inversion in the basis function, which slightly confounds the results: the 16648 approach of doing it inline allows optimization using the integer constant parameters that aren't visible in the driver. Nonetheless, the need to check for toggle in each basis function invocation adds enough cost that reconsideration is appropriate.

Given this evidence I withdraw the proposal that driver implementations should use the basis function for simplicity: it is, indeed, too expensive. As a result there is no motivation for defining set_clr(bits, bits) to be a toggle operation as the primitive toggle can be used. The only cost of this is an increased delay between pin state changes in the presumably rare situation where "simultaneously" some bits are set or cleared and others are toggled.

Instead the semantics of set_clr(bits, bits) is defined to cause the bits to be cleared.

The New column above is the result of this change, replacing the set_clr_bits operator with toggle_bits. The effect of this is that the duration of the iteration reduces from 7.02 us to 6.34 us.

My branch #18689 has been updated to make this change.

BTW: I will be adding the function that does performance testing to the GPIO test in #18689, but right now it depends on devicetree capability not yet merged to master.

[erwango]

Now that new GPIO API has been implemented and merged, I think we can open the floor to pinctrl API.
Since this issue has been exploited in length for GPIO, I suggest to close this one a open a new one dedicated to pinctrl
@mnkp are you fine with this?