[TOPIC-GPIO] Handle logical interrupt setting in common layer

[galak]

Move handling of inverting of flags for gpio_pin_interrupt_configure to common code so drivers don't have to do it.

[zephyrbot]

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

[mnkp]

Handling of interrupt logical levels directly in the include/drivers/gpio.h does simplify the driver code, makes it more readable and robust. I would do it a bit differently though. Rather than replacing the bit fields in the flags parameter, which has a potential to confuse some users, I would translate the bit fields to a new, clean API which is internal to the driver. I.e. we could define the pin_interrupt_configure function as:

enum gpio_int_mode {
	GPIO_INT_MODE_DISABLED,
	GPIO_INT_MODE_LEVEL,
	GPIO_INT_MODE_EDGE,
};

enum gpio_int_trigger {
	/* Trigger detection when input state is (or transitions to) physical low. */
	GPIO_INT_TRIGGER_LOW = 1,
	/* Trigger detection when input state is (or transitions to) physical high. */
	GPIO_INT_TRIGGER_HIGH,
	/* Trigger detection on pin rising or falling edge. */
	GPIO_INT_TRIGGER_BOTH,
};

int (*pin_interrupt_configure)(struct device *port, unsigned int pin,
			       enum gpio_int_mode mode, enum gpio_int_trigger trigger);

This part would not be visible in the public API, i.e. it wouldn't have public doxygen documentation.

This approach has also potential of making #19251 redundant.

One - temporary - drawback of the proposal is that the int (*config)() function to be backwards compatible needs to call int (*pin_interrupt_configure)(). Rather then calling an internal version it would need to call the public one. That's not going to look pretty and neither is efficient but at some point in time we will need to remove those. I'm actually not sure how to go about deprecating this feature (being able to configure interrupts also via gpio_pin_configure() function). Alternatively we could call int (*pin_interrupt_configure)() directly in static inline int gpio_pin_configure() in include/drivers/gpio.h.

[galak]

enum gpio_int_mode {
	GPIO_INT_MODE_DISABLED,
	GPIO_INT_MODE_LEVEL,
	GPIO_INT_MODE_EDGE,
};

enum gpio_int_trigger {
	/* Trigger detection when input state is (or transitions to) physical low. */
	GPIO_INT_TRIGGER_LOW = 1,
	/* Trigger detection when input state is (or transitions to) physical high. */
	GPIO_INT_TRIGGER_HIGH,
	/* Trigger detection on pin rising or falling edge. */
	GPIO_INT_TRIGGER_BOTH,
};

int (*pin_interrupt_configure)(struct device *port, unsigned int pin,
			       enum gpio_int_mode mode, enum gpio_int_trigger trigger);

If we go done this path I would suggest a single enum rather than having to deal with invalid combinations of gpio_int_mode & gpio_int_trigger.

Unfortunately this would mean a conversion between the existing flags and gpio_int_trigger_mode

[mnkp]

If we go done this path I would suggest a single enum rather than having to deal with invalid combinations of gpio_int_mode & gpio_int_trigger.

I'm not sure about this one. We would need then to decode the enum in the driver. That's going to be a lot of if statements (we shouldn't treat enum as a flag value anymore). The driver code has only a few decision it needs to take: should I enable/disable interrupts. If I should enable interrupts should it be level or edge. If it's edge or level, should it be falling/low, rising/high or both. In the hardware information about falling/low and rising/high trigger is often configured using the same register bits. The two argument approach will result in a smaller code size. Also, the only invalid combination is level interrupt and trigger on both edges. If we are concern about scenario mode == GPIO_INT_MODE_DISABLED and trigger == GPIO_INT_TRIGGER_HIGH I would rather cover this case via an extra ASSERT in the gpio_pin_interrupt_configure() function.

[galak]

I didn't update gpio_mcux_igpio.c as @MaureenHelm already has a PR outstanding for that.