gpio_callback ambiguity in union pin vs pin_mask

[pabigot]

I notice that the gpio_callback structure has a union that's documented as identifying the pins that trigger a callback, expressed as either a single ordinal (pin) or a mask of pins (pin_mask). The union documentation in the header explicitly states that the application may change the pins associated with the callback at any time. (Oddly, documentation for members of the struct does not appear to be generated.)

There's nothing in the structure that clearly indicates which union member is valid.

As best I can tell, the gpio_utils.h assumes that the pin_mask field is the one used; this is also the one initialized by gpio_init_callback().

However, gpio_intel_apl references the pin element.

Something's hinky here. Probably the union should be replaced by the pin_mask field, any resulting compilation errors fixed, and the suggestion that callbacks can be controlled by pin ordinals removed from the documentation.

[tbursztyka]

Did not notice that change but it all goes down to this commit: eb6ea28

And there, the commit message it clearer than the actual documentation.

[pabigot]

Thanks; that explains it. Issue was introduced in PR #10085. Looping in @dcpleung.

Nonetheless, a field that's public and documented as under application control (since the application owns the structure) now has a driver-specific interpretation. That's a problem that should be resolved before it bites somebody. Somebody new might write an application that sets the callback pin by using an ordinal, which will fail on every platform except up_squared.

[tbursztyka]

yes, that might create an issue in application indeed, mostly a portability one since setting up the cb will not be the same on that controller than other ones.

[dcpleung]

Sorry for the late reply. I was on vacation. :)

I have given some thoughts about this. I think expanding the APIs to support both pin bitmasks and ordinal pin numbers would lessen the confusion. The GPIO drivers can decide which one to support/implement (or both). As for callback, it is always possible to tag each callback to specify whether the value is a pin mask or ordinal number. However, it would make the ISRs more complicated. I think the drivers should only support one value type in callback.

[pabigot]

Subordinate devices (I2C, SPI) that have auxiliary GPIOs like reset signals must be able to manipulate those GPIOs in driver code without knowing whether a particular SoC GPIO driver uses mask or ordinal pin specifiers. How would you support that?

Supporting two incompatible GPIO APIs seems like a far more complex and impactful approach than simply having up-squared split the 40 GPIO pins across two devices.

[dcpleung]

The Apollo Lake SoC has 245 exposed GPIO pins through the controllers. Given how those pins are arranged in hardware, we will need 10 devices in Zephyr to cover all of these.

I agree that maintaining 2 sets of GPIO APIs are problematic, especially with the need for other drivers to manipulate the GPIOs.