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 <m...@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@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
