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

[David Bauer]

Hello Christian,

thanks for your reworked patch. I think i found two bugs around the debounce
interval for both flavors. I'll have to admit, they are both edgecases ;)

On 06.07.19 23:57, Christian Lamparter wrote
> -static void gpio_keys_polled_check_state(struct gpio_keys_button_data *bdata)
> +static void gpio_keys_handle_button(struct gpio_keys_button_data *bdata)
>   {
> +	unsigned int type = bdata->b->type ?: EV_KEY;
>   	int state = gpio_button_get_value(bdata);
> +	unsigned long seen = jiffies;
>   
> -	if (state != bdata->last_state) {
> -		unsigned int type = bdata->b->type ?: EV_KEY;
> +	pr_debug(PFX "event type=%u, code=%u, pressed=%d\n",
> +		 type, bdata->b->code, state);
> +
> +	/* is this the initialization state? */
> +	if (bdata->last_state == -1) {
> +		/*
> +		 * Don't advertise unpressed buttons on initialization.
> +		 * Just save their state and continue otherwise this
> +		 * can cause OpenWrt to enter failsafe.
> +		 */
> +		if (type == EV_KEY && state == 0)
> +			goto set_state;

As we are calling gpio_keys_handle_button every poll-interval, we need 
to make sure we save the initial state AFTER the button has been stable 
for the debounce interval. For irq-based keys we get this "for free" as 
we modify the execution timer for the irq handling function.

However, we need to track the button state for the polled GPIO keys, so 
we cannot piggy-back on the last_state for deciding if the initial state 
is already set. We could use negative numbers for that, but i think this 
is more hacky than using a dedicated variable for keeping track.

This was the reason for my 'has_initial_state' variable. :)

> +		/*
> +		 * But we are very interested in pressed buttons and
> +		 * initial switch state. These will be reported to
> +		 * userland.
> +		 */
> +	} else if (bdata->last_state == state) {
> +		/* reset asserted counter (only relevant for polled keys) */

This is also needed for irq-driven keys. If the state changes two times 
within the debounce interval, gpio_keys_handle_button is still executed 
and we need to terminate here. Otherwise, we would skip a "release" or
"push" action.

I did rework those two parts a bit and tested it with the irq and
polled flavors without a problem. See below: