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