[OpenWrt-Devel] [PATCH v2] gpio-button-hotplug: mind debounce interval consistently

[Christian Lamparter]

On Tuesday, July 2, 2019 11:57:18 PM CEST David Bauer wrote:
> This patch implements consistent handling of the debounce interval set
> for the GPIO buttons. Hotplug events will only be fired if
> 
> 1. It's the initial stable state (no state-change for duration of the
> debounce interval) for a switch. Buttons will not trigger an event for
> the initial stable state.
> 
> 2. The input changes it's state and remains stable for the debounce
> interval.
> 
> Prior to this patch, this was handled inconsistently for interrupt-based
> an polled gpio-keys. We unify the shared logic in button_hotplug_event
> and modify both implementations to read the initial state.
> 
> Run-tested for 'gpio-keys' and 'gpio-keys-polled' on
> 
>  - devolo WiFi pro 1200e
>  - devolo WiFi pro 1750c
>  - devolo WiFi pro 1750x
> 
> Signed-off-by: David Bauer <mail at david-bauer.net>
> ---

Thank you, I had to play much more around with as the logic around the
has_initial_state variable got me really consfused. Sadly, I had to dump
it down to a notch, so that I can follow. So, here's my version of your version ;)
sadly with more changes as I got carried away :(

  - removed a now unused variable "int i;" in gpio_keys_polled_probe.
    (gcc found it)

  - renamed button_hotplug_event to gpio_keys_handle_button
    (From what I know the name "hotplug"(2) came from the udev replacement
    daemon from back in the day. Nowadays, this is part of procd. maybe the
    whole driver should be renamed?) 

  - "button->active_low = flags & OF_GPIO_ACTIVE_LOW" is more robust(for now).
    This only works because  OF_GPIO_ACTIVE_LOW is 0x1.
    If it was bigger than 0x1, then this would have never worked.

  - moved the "check if EV_KEY or EV_SW" to gpio_keys_button_probe() +
  - moved the "button map" lookup performed by button_get_index()
    to gpio_keys_button_probe
  
    I did this to save a few cycles here and there (and the added
    warnings might help a poor debugger one day). In my opinion, it
    doesn't make sense to always check the type and perform the
    lookup for (pretty much at that point) constant values.
    (upstream gpio_keys has a sysfs to add new keys. I guess this
    is why the dev thought of that).

  - moved the "seen" from the bh_priv to gpio_keys_button_data.
    (again, it looks like the developer wanted to make it modular.
    but upstream and openwrt have diverted too much)

There's still stuff todo, like converting the gpio accessors to
just use gpiod_get_value_cansleep. But that's for another time...

The remaining question is: Does this still work as expected?

(I think some of the problems related to the "failsafe issues"
could be caused by the wrong polarity, I'm guilty of doing this
as well. I had copied these settings from netgears GPL code and
did not check. Netgear had their own hacked GPIO driver and then
I foolishly applied the same polarity setting for pretty much
all the apm821xx targets). 

Cheers,
Christian
---