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

[Christian Lamparter]

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
> @@ -241,6 +241,7 @@ static int button_get_index(unsigned int code)
>  static void button_hotplug_event(struct gpio_keys_button_data *data,
>  			   unsigned int type, int value)
>  {
> +	int last_state = data->last_state;
>  	struct bh_priv *priv = &data->bh;
>  	unsigned long seen = jiffies;
>  	int btn;
> @@ -250,6 +251,14 @@ static void button_hotplug_event(struct gpio_keys_button_data *data,
>  	if ((type != EV_KEY) && (type != EV_SW))
>  		return;
>  
> +	if (value == last_state)
> +		return;
> +
> +	data->last_state = value;
> +
> +	if (last_state == -1 && type != EV_SW)
> +		return;
> +
>  	btn = button_get_index(data->b->code);
>  	if (btn < 0)
>  		return;
> @@ -285,22 +294,14 @@ static int gpio_button_get_value(struct gpio_keys_button_data *bdata)
>  
>  static void gpio_keys_polled_check_state(struct gpio_keys_button_data *bdata)
>  {
> -	int state = gpio_button_get_value(bdata);
> -
> -	if (state != bdata->last_state) {
> -		unsigned int type = bdata->b->type ?: EV_KEY;
> -
> -		if (bdata->count < bdata->threshold) {
> -			bdata->count++;
> -			return;
> -		}
> -
> -		if (bdata->last_state != -1 || type == EV_SW)
> -			button_hotplug_event(bdata, type, state);
> -
> -		bdata->last_state = state;
> +	if (bdata->count < bdata->threshold) {
> +		bdata->count++;
> +		return;
>  	}
>  
> +	button_hotplug_event(bdata, bdata->b->type ?: EV_KEY,
> +				gpio_button_get_value(bdata));
> +
>  	bdata->count = 0;
>  }
Doesn't this change the logic of the gpio-key-polled software-debounce
a bit too aggressivly?

Previously, for the button event to happen the button new state had to
be stable for bdata->threshold counts.

Whereas now, bdata->count is counted upwards on every "tick" and once
bdata->count == bdata->threshold matches the "current state" gets passed
on. This seems that it would interfere with the debounce since a signal
doesn't have to be asserted stable for the whole duration now, instead
it now just has to show up "just before" the
bdata->count == bdata->threshold tick in order to be noticed. 

> @@ -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... 

>  		ret = devm_request_threaded_irq(&pdev->dev,
>  			bdata->irq, NULL, button_handle_irq,
>  			irqflags, dev_name(&pdev->dev), bdata);
> @@ -621,9 +625,6 @@ static int gpio_keys_probe(struct platform_device *pdev)
>  			dev_dbg(&pdev->dev, "gpio:%d has irq:%d\n",
>  				button->gpio, bdata->irq);
>  		}
> -
> -		if (bdata->b->type == EV_SW)
> -			button_hotplug_event(bdata, EV_SW, gpio_button_get_value(bdata));
>  	}
>  
>  	return 0;
> @@ -648,9 +649,6 @@ static int gpio_keys_polled_probe(struct platform_device *pdev)
>  	if (pdata->enable)
>  		pdata->enable(bdev->dev);
>  
> -	for (i = 0; i < pdata->nbuttons; i++)
> -		gpio_keys_polled_check_state(&bdev->data[i]);
> -

...and leave this as is.
>  	gpio_keys_polled_queue_work(bdev);
>  
>  	return ret;
> 





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