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

[David Bauer]

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

You are right, i will rework this part.

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

Best wishes
David

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