RFC: rework GPIO interrupt configuration

[pabigot]

The existing GPIO API has a somewhat baroque approach to configuring interrupts and callbacks, the motivation for which is unclear:

• The gpio_callback structure provides the handler to use in the callback, and specifies the pins for which the callback applies;
• The gpio_{add,remove}_callback() API adds or removes gpio_callback objects from registry maintained in the driver but does not automatically enable what's been added;
• The gpio_{en,dis}able_callback API operates on pins in isolation of whether any callback handlers happen to be registered for them.

The combination seems to allow for multiple drivers/application functions to request notification of interrupts occurring on a pin, but if somebody takes advantage of that any one of them can enable or disable the callback for all them.

This comment on work to clean up the GPIO API on proposes a change that moves the pin's interrupt-related configuration (viz. edge/level, high/low) from being performed through GPIO configuration into being provided in the callback structure.

So if the API may be changed, two questions:

• Is there any motivation for allowing multiple consumers of interrupts on a single pin-in-driver? Yes, a soft-reset feature
• Taking into account the answer, does it make sense to change the API so that configuring a pin for interrupt and enabling it are not two separate operations?

Tentatively my answers would be "no", and "yes", which could be accomplished by using something like gpio_enable_interrupt to handle the registration, configuration, and enable operations and gpio_disable_interrupt to disable, return the GPIO to its non-interrupt configuration, and remove the callback registration.

It would be nice to have this along with the other improvements of #16648, although it's not directly related to device tree flags so probably belongs in a coordinated PR.

I would like to have consensus on the value and approach before anybody's asked to spend time on this.

[pabigot]

@b0661 @anangl FYI.

If we can agree on value and details I'd be willing to help in the implementation, unless it falls naturally within other work in progress.

[pabigot]

Reworded description to remove my temporary confusion that a GPIO driver might control multiple ports. It'd be nice if that was true, but it's not.

[tbursztyka]

if somebody takes advantage of that any one of them can enable or disable the callback for all them.

And with your proposal, if somebody takes advantage of enabling interrupt on all pins for itself, no other system could register a callback anymore. Same problem then. This issue of "taking advantage to is a non-issue as there is no way you can really fix it.

Tentatively my answers would be "no", and "yes"

You are assuming wrong.

Let's say a soft reset for peripherals that would be triggered via gpio pin, meaning each peripheral drivers would trigger a soft reset based on that pin event. That's a valid use case. You would have multiple listeners for the same pin. And it is just an example.

And for the second question, configuring and enabling are 2 distinct things. You could have them in one go, or separate as it is right now: in the end it is just a matter of taste whether you prefer a function doing 1 thing or 2. It's been chosen to have functions doing one thing only, fine.

In general, as long as there is no real life issues with an API, so anything besides assumptions, let's not change it. It is a costly move for zephyr and its users. Better be sure a change is worthy to be made before actually doing it.

[pabigot]

I'm not assuming anything: I'm raising a question to try to understand why things are the way they are. You've presented a reasonable use case for multiple listeners. That needs to be traded against the risks of having those listeners apply inconsistent configurations.

Enabling and disabling each have multiple steps, one of which is registration. Whether and how to fragment the steps is a design decision. There is a cost to leaving a callback registered when it's disabled.

My overall point is that we've discovered reasons to separate a pin's GPIO functionality from its interrupt functionality, so the API may be reworked anyway. Now is the time to revisit past decisions, see if they're still relevant, and ensure that any changes handle both existing and new use cases.

[b0661]

In general, as long as there is no real life issues with an API, so anything besides assumptions, let's not change it.

@tbursztyka there is a real life issue for me. Currently the combined interrupt/ pin configuration muxes the pin to the GPIO function. In my case the GPIO pin is used as a HW CS (muxed to the SPI) for my board acting as a SPI slave. I still need an interrupt when the SPI chip select is asserted but I can not do a software based chip select handling for the SPI function. With the current API I can either have HW CS or the interrupt. This forces me to create an OOT GPIO driver with a changed API.

[tbursztyka]

@b0661 I don't see you issue being fixed by what's being proposed here (thus why these changes are not needed: they don't fix anything). Aren't you working on a complete rehearsal of the gpio/pin sub-sybstem in zephyr (pinctrl etc...)?

[pabigot]

@tbursztyka

This comment on work to clean up the GPIO API on proposes a change that moves the pin's interrupt-related configuration (viz. edge/level, high/low) from being performed through GPIO configuration into being provided in the callback structure.

So if the API may be changed, two questions:

Any changes related to @b0661's need may or may not be part of that PR, which primarily deals with GPIO device tree bindings. So I opened this to discuss what other interrupt configuration features might be worth considering.

I understand your perspective and will take it into consideration if I'm involved in any related concrete proposals.

[b0661]

@tbursztyka

Aren't you working on a complete rehearsal of the gpio/pin sub-sybstem in zephyr (pinctrl etc...)?

I´m working on a pinctrl subsystem that relates to pin configuration. Interrupt configuration is not part of it and the pinctrl subsystem is blocked anyway by the missing of appropriate code generation (or the decision on that). So it is not in scope of this discussion.

[anangl]

I somehow didn't realize earlier that it was possible to bind more than one callback structure to a given GPIO pin. And I don't think it would be reasonable to change this - @tbursztyka already provided a use case for such possibility. I can also imagine a need of having a common callback for a group of pins and separate specialized callbacks for individual pins from this group, and these callbacks added and removed from different modules (of course this can be also achieved in other ways but here we've got an API that already provides means for it).
Taking the above into account, moving the interrupt configuration to the callback structure doesn't seem to make sense. But I still think that it would be good to separate the configuration of the pin and configuration of the interrupt it can trigger, and as @b0661 argues there's an actual need for this. Maybe we could then move the interrupt related configuration flags to the parameter passed to gpio_enable_callback?
And actually, the function enabling a pin interrupt should be rather named gpio_enable_interrupt - this is what it actually does, and for some cases (like waking up a device) there's no need to have a callback called, the interrupt itself is enough. If we decide to modify the API, we could also consider such renaming.

[pabigot]

I'm still concerned about multiple devices changing the configuration of a pin without coordination and no way to detect or diagnose this situation.

What if we do put the interrupt configuration and some coordination flags in the structure that holds the callback, keep the distinct register and enable steps, and pass a reference to the (registered) callback structure in the enable operation?

Then, based on other flags in the callback-specific structure, we could validate the enable and configure operation. For example, we might have a "control" vs "observe" flag in the callback structure, where "observe" never changes the configuration but just ensures that the callback is invoked whenever the interrupt is generated according to the configuration set by the device that "controls" the pin. This handles the multiple consumer case without making them all responsible for configuration. (We could also have a "shared control" which allows them all to control with some defined semantics about compatible pin configuration and how to handle mixed enabled/disabled states, in case the multiple consumers can't rely on a single controller).

I too have a case where a single pin has two roles, one an active-low signal where I want to detect on falling edge, and one where it's a square wave and I want to detect on rising and falling edge. Changing the mode involves an I2C command to a peripheral. I could handle that with two registered callbacks, with only one of them enabled (and so in control) at any time.

[b0661]

What if we do put the interrupt configuration and some coordination flags in the structure that holds the callback, keep the distinct register and enable steps, and pass a reference to the (registered) callback structure in the enable operation?

LGTM. On every enable and disable operation the total (all currently enabled callbacks) interrupt configuration can be evaluated. Any enable request that would require an interrupt configuration that would jeopardize the current interrupt configuration can be rejected.

First idea of a possible interface. The callback configuration is split from the callback management to allow for constant configuration data to save ram memory.

/**
 * @brief GPIO callback configuration structure
 *
 * Used to configure a callback.
 */
struct gpio_callback_configuration {

	/** Actual callback function being called when relevant. */
	gpio_callback_handler_t handler;

	/**  A mask of trigger modes of pins the callback is interested in.
	 * If pin bit is set then edge mode is requested otherwise level mode.
	 */
	u32_t pin_trigger_mask;

	/** A mask of pins the callback is interested in.
	 * Callback is called if the pin rises to high or is on high level
	 * depending on the value of the pin_trigger_mask.
	 */
	u32_t pin_high_rising_mask;

	/** A mask of pins the callback is interested in.
	 * Callback is called if the pin falls to low or is on low level
	 * depending on the value of the pin_trigger_mask.
	 */
	u32_t pin_low_falling_mask;
}

/**
 * @brief GPIO callback structure
 *
 * Used to register a callback in the driver instance callback list.
 * As many callbacks as needed can be added as long as each of them
 * are unique pointers of struct gpio_callback.
 * Beware such structure should not be allocated on stack.
 *
 * Note: To help setting it, see gpio_init_callback() below
 */
struct gpio_callback {
	/** This is meant to be used in the driver and the user should not
	 * mess with it (see drivers/gpio/gpio_utils.h)
	 */
	sys_snode_t node;

	/** This is meant to be used in the driver and the user should not
	 * mess with it.
	 */
	bool enabled;

	/** The callback and interrupt configuration.
	 */
	const struct gpio_callback_configuration *config;
};

/**
 * @cond INTERNAL_HIDDEN
 *
 * GPIO driver API definition and system call entry points
 *
 * (Internal use only.)
 */
typedef int (*gpio_config_t)(struct device *port, int access_op,
			     u32_t pin, int flags);
typedef int (*gpio_write_t)(struct device *port, int access_op,
			    u32_t pin, u32_t value);
typedef int (*gpio_read_t)(struct device *port, int access_op,
			   u32_t pin, u32_t *value);
typedef int (*gpio_manage_callback_t)(struct device *port,
				      struct gpio_callback *callback,
				      bool set);
typedef int (*gpio_enable_callback_t)(struct device *port,
				      struct gpio_callback *callback);
typedef int (*gpio_disable_callback_t)(struct device *port,
				       struct gpio_callback *callback);
typedef u32_t (*gpio_api_get_pending_int)(struct device *dev);

struct gpio_driver_api {
	gpio_config_t config;
	gpio_write_t write;
	gpio_read_t read;
	gpio_manage_callback_t manage_callback;
	gpio_enable_callback_t enable_callback;
	gpio_disable_callback_t disable_callback;
	gpio_api_get_pending_int get_pending_int;
};