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

[Christian Lamparter]

On Thursday, June 20, 2019 8:10:18 PM CEST David Bauer wrote:
> Hello Christian,
> On 20.06.19 17:21, Christian Lamparter wrote:
> > On Tuesday, June 18, 2019 1:06:12 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>
> >> ---
> >>  .../src/gpio-button-hotplug.c                 | 42 +++++++++----------
> >>  1 file changed, 20 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/package/kernel/gpio-button-hotplug/src/gpio-button-hotplug.c b/package/kernel/gpio-button-hotplug/src/gpio-button-hotplug.c
> >> index e63d414284..25150344e0 100644
> >> --- a/package/kernel/gpio-button-hotplug/src/gpio-button-hotplug.c
> >> +++ b/package/kernel/gpio-button-hotplug/src/gpio-button-hotplug.c
> >> @@ -351,8 +352,8 @@ static irqreturn_t button_handle_irq(int irq, void *_bdata)
> >>  	struct gpio_keys_button_data *bdata =
> >>  		(struct gpio_keys_button_data *) _bdata;
> >>  
> >> -	schedule_delayed_work(&bdata->work,
> >> -			      msecs_to_jiffies(bdata->software_debounce));
> >> +	mod_delayed_work(system_wq, &bdata->work,
> >> +			 msecs_to_jiffies(bdata->software_debounce));
> >>  
> >>  	return IRQ_HANDLED;
> >>  }
> >> @@ -608,6 +609,9 @@ static int gpio_keys_probe(struct platform_device *pdev)
> >>  
> >>  		INIT_DELAYED_WORK(&bdata->work, gpio_keys_irq_work_func);
> >>  
> >> +		schedule_delayed_work(&bdata->work,
> >> +				      msecs_to_jiffies(bdata->software_debounce));
> >> +
> > Hm, well since the first state is -1 we could just as well schedule the work
> > immediately here... 
> 
> Hmm, i have a bit trouble grasping your intention here.
> 
> Do you mean we can unify the scheduling for polled and
> interrupt-based keys?

I was gunning for making the polled and interrupt behave in the
same way. Currently the gpio_polled variant does

|	for (i = 0; i < pdata->nbuttons; i++)
|		gpio_keys_polled_check_state(&bdev->data[i]);

as part of it's probe function which immediately initializes the
button's state from -1 to either 0 or 1.

But with the 

| + schedule_delayed_work(&bdata->work,
| +				      msecs_to_jiffies(bdata->software_debounce));

the irq-path would wait software_debounce (5ms by default) time
until the -1 is cleared.

This is not really a problem, other than a symmetry mismatch when
targets switch from gpio-keys to gpio-keys-polled or vice versa
and expect the implementation to behave in the same manner.

Best Regards,
Christian



_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel