[TOPIC-GPIO] drivers: ht16k33: update to use new GPIO API #19641
Conversation
There was a problem hiding this comment.
Fix needed for validation of the supported I/O modes for the GPIO.
Also it's very confusing whether the interrupt is being configured for a logic level or a physical level. The devicetree uses a pull-up, and the interrupt seems to detect transition to low, but the devicetree defaults to active high. I would normally expect that pull configuration to indicate an active-low trigger on edge-to-active.
| @@ -12,7 +12,7 @@ | |||
| reg = <0x70>; | |||
| label = "HT16K33"; | |||
| /* Uncomment to use IRQ instead of polling: */ | |||
| /* irq-gpios = <&gpio1 8 (GPIO_PUD_PULL_UP | GPIO_INT_ACTIVE_LOW)>; */ | |||
| /* irq-gpios = <&gpio1 8 GPIO_PUD_PULL_UP>; */ | |||
There was a problem hiding this comment.
The (undocumented) consensus seems to be that active level should be made explicit, so this should add GPIO_ACTIVE_HIGH. Though that's not consistent with normal use of pull-up, so maybe it should be GPIO_ACTIVE_LOW. Except in the driver the interrupt is configured for edge-to-inactive.
There was a problem hiding this comment.
@pabigot My intention was to to use an IRQ which triggers on falling edges. I have updated it to explicitly specify the active level.
|
HT16K33 is not a General Purpose Input Output driver. I think it would be cleaner to move all the interrupt handling code directly into |
I agree with this in principle, especially as it appears that there's additional programming necessary to configure and access the keypresses. Since the LED driver doesn't support a keypress callback API taking that approach would require either a driver-specific API for the ht16k33 or an extension to the LED API to optionally support key presses (if this is not a unique feature of this controller). For now, since it's already in-tree, I don't think its unreasonable to just make it work the way it has been. |
This is already the case. The GPIO API is used exclusively for its interrupt subsystem. We really should get rid of |
|
@henrikbrixandersen |
henrikbrixandersen
force-pushed
the
topic-gpio-ht16k33
branch
from
84850ba
to
998a5e7
Compare
Implement the new GPIO driver APIs for the HT16K33 and update the driver to use the new GPIO flags. Signed-off-by: Henrik Brix Andersen <[email protected]>
henrikbrixandersen
force-pushed
the
topic-gpio-ht16k33
branch
from
998a5e7
to
0e5ca02
Compare
Update the IRQ GPIO handling code of the HT16K33 LED driver to use the new GPIO API. Signed-off-by: Henrik Brix Andersen <[email protected]>
Update the HT16K33 driver sample application to use the new GPIO flags. Signed-off-by: Henrik Brix Andersen <[email protected]>
henrikbrixandersen
force-pushed
the
topic-gpio-ht16k33
branch
from
0e5ca02
to
beb4e6a
Compare
I have been meaning to introduce a simple key event input subsystem in Zephyr for a while. For now, the closest we have seems to be the GPIO interrupt + callback functionality, which is why the HT16K33 driver is implementing that API. |
@carlescufi Rebased and fixed (I hope) the issues brought up in the review so far. |
Update the HT16K33 LED driver, key-scan driver, and sample application to use the new GPIO API.