-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GPIO driver refactor - polling, multiple instance fixes, remove default Kconfigs #640
GPIO driver refactor - polling, multiple instance fixes, remove default Kconfigs #640
Conversation
* Actually allow defaulting yes in other places.
* Add easier macros for conditional polling/interrupt code. * Properly continue polling on intervals, without extra enable/disable code for pins that is superfluous when not trying to deal with interupts firing. * Fix to allow multiple GPIO drivers when doing splits w/ IO expanders
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I fully understand how the kscan driver works, but everything I see changing here looks reasonable to me.
(int err = kscan_gpio_enable_interrupts_##n(dev); \ | ||
if (err) { return err; } return kscan_gpio_read_##n(dev);)) \ | ||
COND_POLL_OR_INTERRUPTS((struct kscan_gpio_data_##n *data = dev->data; \ | ||
k_timer_start(&data->poll_timer, K_MSEC(10), K_MSEC(10)); \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I correctly interpreting this that when polling is enabled, we poll at a constant 100 Hz? It might be useful to make that configurable, for example if someone wanted to make a wired keyboard that polls as fast as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I hadn't remembered hardcoding.this, but it does seem so, yeah. Good item to move into Kconfig soon, yeah.
COND_POLL_OR_INTERRUPTS( \ | ||
(k_timer_init(&data->poll_timer, kscan_gpio_timer_handler_##n, NULL); \ | ||
kscan_gpio_set_output_state_##n(dev, 0);), \ | ||
(kscan_gpio_set_output_state_##n(dev, 1);)) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would I be correct in guessing that the interrupt behavior is different here because it sets all the outputs active, waits for an interrupt to happen on any input, and then does a regular scan?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. We want to drive all outputs to active when "idle", so that any key press will trigger an interrupt. Then we keep polling/scanning until all keys are released again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self approving, been using this extensively for several days on Ferris and on other interrupt based boards like Corne.
This refactor is needed for my upcoming Ferris board work. In particular, the Ferris:
While there, I took the opportunity to add some more useful macros to make the conditional code a little bit easier to read.
There's also a small commit in here as well that removes some
default n
settings from some Kconfig settings to allow them to be defaulted yes elsewhere as needed.