Re: [PATCH] input: Change timer function to workqueue for gpio_keys driver

Re: [PATCH] input: Change timer function to workqueue for gpio_keys driver

[Trilok Soni]

Hi Alek,

Adding linux-input ML, so not deleting any code.

On Mon, Jun 8, 2009 at 11:24 AM, Alek Du <alek.du@intel.com> wrote:
> From 35101ecc80cfc638ac80a2cea590ab86135be395 Mon Sep 17 00:00:00 2001
> From: Alek Du <alek.du@intel.com>
> Date: Fri, 8 May 2009 12:25:34 +0800
> Subject: [PATCH] input: Change timer function to workqueue for gpio_keys driver
>
> The gpio_get_value function of I2C/SPI GPIO expander may sleep thus this
> function call can not be called in a timer function.
>
> Signed-off-by: Alek Du <alek.du@intel.com>
> ---
>  drivers/input/keyboard/gpio_keys.c |   32 ++++++++++++++------------------
>  1 files changed, 14 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> index ad67d76..9205ac6 100644
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c
> @@ -22,13 +22,17 @@
>  #include <linux/platform_device.h>
>  #include <linux/input.h>
>  #include <linux/gpio_keys.h>
> +#include <linux/workqueue.h>
>
>  #include <asm/gpio.h>
>
>  struct gpio_button_data {
>        struct gpio_keys_button *button;
>        struct input_dev *input;
> -       struct timer_list timer;
> +/* Change timer func to workqueue func due to the fact that gpio_get_value
> + * may sleep for some i2c and spi GPIO expander
> + */
> +       struct delayed_work work;
>  };
>
>  struct gpio_keys_drvdata {
> @@ -36,8 +40,10 @@ struct gpio_keys_drvdata {
>        struct gpio_button_data data[0];
>  };
>
> -static void gpio_keys_report_event(struct gpio_button_data *bdata)
> +static void gpio_keys_report_event(struct work_struct *work)
>  {
> +       struct gpio_button_data *bdata = container_of(work,
> +                       struct gpio_button_data, work.work);
>        struct gpio_keys_button *button = bdata->button;
>        struct input_dev *input = bdata->input;
>        unsigned int type = button->type ?: EV_KEY;
> @@ -47,13 +53,6 @@ static void gpio_keys_report_event(struct gpio_button_data *bdata)
>        input_sync(input);
>  }
>
> -static void gpio_check_button(unsigned long _data)
> -{
> -       struct gpio_button_data *data = (struct gpio_button_data *)_data;
> -
> -       gpio_keys_report_event(data);
> -}
> -
>  static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
>  {
>        struct gpio_button_data *bdata = dev_id;
> @@ -62,10 +61,10 @@ static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
>        BUG_ON(irq != gpio_to_irq(button->gpio));
>
>        if (button->debounce_interval)
> -               mod_timer(&bdata->timer,
> -                       jiffies + msecs_to_jiffies(button->debounce_interval));
> +               schedule_delayed_work(&bdata->work,
> +                       msecs_to_jiffies(button->debounce_interval));
>        else
> -               gpio_keys_report_event(bdata);
> +               schedule_work(&bdata->work.work);
>
>        return IRQ_HANDLED;
>  }
> @@ -112,8 +111,7 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
>
>                bdata->input = input;
>                bdata->button = button;
> -               setup_timer(&bdata->timer,
> -                           gpio_check_button, (unsigned long)bdata);
> +               INIT_DELAYED_WORK(&bdata->work, gpio_keys_report_event);
>
>                error = gpio_request(button->gpio, button->desc ?: "gpio_keys");
>                if (error < 0) {
> @@ -173,8 +171,7 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
>  fail2:
>        while (--i >= 0) {
>                free_irq(gpio_to_irq(pdata->buttons[i].gpio), &ddata->data[i]);
> -               if (pdata->buttons[i].debounce_interval)
> -                       del_timer_sync(&ddata->data[i].timer);
> +               cancel_delayed_work_sync(&ddata->data[i].work);
>                gpio_free(pdata->buttons[i].gpio);
>        }
>
> @@ -198,8 +195,7 @@ static int __devexit gpio_keys_remove(struct platform_device *pdev)
>        for (i = 0; i < pdata->nbuttons; i++) {
>                int irq = gpio_to_irq(pdata->buttons[i].gpio);
>                free_irq(irq, &ddata->data[i]);
> -               if (pdata->buttons[i].debounce_interval)
> -                       del_timer_sync(&ddata->data[i].timer);
> +               cancel_delayed_work_sync(&ddata->data[i].work);
>                gpio_free(pdata->buttons[i].gpio);
>        }
>
> --
> 1.6.0.4
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>



-- 
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH]input: Change timer function to workqueue for gpio_keys driver

[Alek Du]

Thanks Trilok, just resending this patch to the right mail list...



>From 35101ecc80cfc638ac80a2cea590ab86135be395 Mon Sep 17 00:00:00 2001
From: Alek Du <alek.du@intel.com>
Date: Fri, 8 May 2009 12:25:34 +0800
Subject: [PATCH] input: Change timer function to workqueue for gpio_keys driver

The gpio_get_value function of I2C/SPI GPIO expander may sleep thus this
function call can not be called in a timer function.

Signed-off-by: Alek Du <alek.du@intel.com>
---
 drivers/input/keyboard/gpio_keys.c |   32 ++++++++++++++------------------
 1 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index ad67d76..9205ac6 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -22,13 +22,17 @@
 #include <linux/platform_device.h>
 #include <linux/input.h>
 #include <linux/gpio_keys.h>
+#include <linux/workqueue.h>
 
 #include <asm/gpio.h>
 
 struct gpio_button_data {
 	struct gpio_keys_button *button;
 	struct input_dev *input;
-	struct timer_list timer;
+/* Change timer func to workqueue func due to the fact that gpio_get_value
+ * may sleep for some i2c and spi GPIO expander
+ */
+	struct delayed_work work;
 };
 
 struct gpio_keys_drvdata {
@@ -36,8 +40,10 @@ struct gpio_keys_drvdata {
 	struct gpio_button_data data[0];
 };
 
-static void gpio_keys_report_event(struct gpio_button_data *bdata)
+static void gpio_keys_report_event(struct work_struct *work)
 {
+	struct gpio_button_data *bdata = container_of(work,
+			struct gpio_button_data, work.work);
 	struct gpio_keys_button *button = bdata->button;
 	struct input_dev *input = bdata->input;
 	unsigned int type = button->type ?: EV_KEY;
@@ -47,13 +53,6 @@ static void gpio_keys_report_event(struct gpio_button_data *bdata)
 	input_sync(input);
 }
 
-static void gpio_check_button(unsigned long _data)
-{
-	struct gpio_button_data *data = (struct gpio_button_data *)_data;
-
-	gpio_keys_report_event(data);
-}
-
 static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
 {
 	struct gpio_button_data *bdata = dev_id;
@@ -62,10 +61,10 @@ static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
 	BUG_ON(irq != gpio_to_irq(button->gpio));
 
 	if (button->debounce_interval)
-		mod_timer(&bdata->timer,
-			jiffies + msecs_to_jiffies(button->debounce_interval));
+		schedule_delayed_work(&bdata->work,
+			msecs_to_jiffies(button->debounce_interval));
 	else
-		gpio_keys_report_event(bdata);
+		schedule_work(&bdata->work.work);
 
 	return IRQ_HANDLED;
 }
@@ -112,8 +111,7 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 
 		bdata->input = input;
 		bdata->button = button;
-		setup_timer(&bdata->timer,
-			    gpio_check_button, (unsigned long)bdata);
+		INIT_DELAYED_WORK(&bdata->work, gpio_keys_report_event);
 
 		error = gpio_request(button->gpio, button->desc ?: "gpio_keys");
 		if (error < 0) {
@@ -173,8 +171,7 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
  fail2:
 	while (--i >= 0) {
 		free_irq(gpio_to_irq(pdata->buttons[i].gpio), &ddata->data[i]);
-		if (pdata->buttons[i].debounce_interval)
-			del_timer_sync(&ddata->data[i].timer);
+		cancel_delayed_work_sync(&ddata->data[i].work);
 		gpio_free(pdata->buttons[i].gpio);
 	}
 
@@ -198,8 +195,7 @@ static int __devexit gpio_keys_remove(struct platform_device *pdev)
 	for (i = 0; i < pdata->nbuttons; i++) {
 		int irq = gpio_to_irq(pdata->buttons[i].gpio);
 		free_irq(irq, &ddata->data[i]);
-		if (pdata->buttons[i].debounce_interval)
-			del_timer_sync(&ddata->data[i].timer);
+		cancel_delayed_work_sync(&ddata->data[i].work);
 		gpio_free(pdata->buttons[i].gpio);
 	}
 
-- 
1.6.0.4

Re: [PATCH]input: Change timer function to workqueue for gpio_keys driver

[Trilok Soni]

Hi Alek,

>
> From 35101ecc80cfc638ac80a2cea590ab86135be395 Mon Sep 17 00:00:00 2001
> From: Alek Du <alek.du@intel.com>
> Date: Fri, 8 May 2009 12:25:34 +0800
> Subject: [PATCH] input: Change timer function to workqueue for gpio_keys driver
>
> The gpio_get_value function of I2C/SPI GPIO expander may sleep thus this
> function call can not be called in a timer function.
>
> Signed-off-by: Alek Du <alek.du@intel.com>
> ---
>  drivers/input/keyboard/gpio_keys.c |   32 ++++++++++++++------------------
>  1 files changed, 14 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> index ad67d76..9205ac6 100644
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c
> @@ -22,13 +22,17 @@
>  #include <linux/platform_device.h>
>  #include <linux/input.h>
>  #include <linux/gpio_keys.h>
> +#include <linux/workqueue.h>
>
>  #include <asm/gpio.h>
>
>  struct gpio_button_data {
>        struct gpio_keys_button *button;
>        struct input_dev *input;
> -       struct timer_list timer;
> +/* Change timer func to workqueue func due to the fact that gpio_get_value
> + * may sleep for some i2c and spi GPIO expander
> + */

This is anyway goes into commit text and git history, so I don't see
any additional value in keeping this comment here. Patch looks good
otherwise, but it is better if someone can verify this driver having
direct gpio keys.

> +       struct delayed_work work;
>  };
>
>  struct gpio_keys_drvdata {
> @@ -36,8 +40,10 @@ struct gpio_keys_drvdata {
>        struct gpio_button_data data[0];
>  };
>
> -static void gpio_keys_report_event(struct gpio_button_data *bdata)
> +static void gpio_keys_report_event(struct work_struct *work)
>  {
> +       struct gpio_button_data *bdata = container_of(work,
> +                       struct gpio_button_data, work.work);
>        struct gpio_keys_button *button = bdata->button;
>        struct input_dev *input = bdata->input;
>        unsigned int type = button->type ?: EV_KEY;
> @@ -47,13 +53,6 @@ static void gpio_keys_report_event(struct gpio_button_data *bdata)
>        input_sync(input);
>  }
>
> -static void gpio_check_button(unsigned long _data)
> -{
> -       struct gpio_button_data *data = (struct gpio_button_data *)_data;
> -
> -       gpio_keys_report_event(data);
> -}
> -
>  static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
>  {
>        struct gpio_button_data *bdata = dev_id;
> @@ -62,10 +61,10 @@ static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
>        BUG_ON(irq != gpio_to_irq(button->gpio));
>
>        if (button->debounce_interval)
> -               mod_timer(&bdata->timer,
> -                       jiffies + msecs_to_jiffies(button->debounce_interval));
> +               schedule_delayed_work(&bdata->work,
> +                       msecs_to_jiffies(button->debounce_interval));
>        else
> -               gpio_keys_report_event(bdata);
> +               schedule_work(&bdata->work.work);
>
>        return IRQ_HANDLED;
>  }
> @@ -112,8 +111,7 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
>
>                bdata->input = input;
>                bdata->button = button;
> -               setup_timer(&bdata->timer,
> -                           gpio_check_button, (unsigned long)bdata);
> +               INIT_DELAYED_WORK(&bdata->work, gpio_keys_report_event);
>
>                error = gpio_request(button->gpio, button->desc ?: "gpio_keys");
>                if (error < 0) {
> @@ -173,8 +171,7 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
>  fail2:
>        while (--i >= 0) {
>                free_irq(gpio_to_irq(pdata->buttons[i].gpio), &ddata->data[i]);
> -               if (pdata->buttons[i].debounce_interval)
> -                       del_timer_sync(&ddata->data[i].timer);
> +               cancel_delayed_work_sync(&ddata->data[i].work);
>                gpio_free(pdata->buttons[i].gpio);
>        }
>
> @@ -198,8 +195,7 @@ static int __devexit gpio_keys_remove(struct platform_device *pdev)
>        for (i = 0; i < pdata->nbuttons; i++) {
>                int irq = gpio_to_irq(pdata->buttons[i].gpio);
>                free_irq(irq, &ddata->data[i]);
> -               if (pdata->buttons[i].debounce_interval)
> -                       del_timer_sync(&ddata->data[i].timer);
> +               cancel_delayed_work_sync(&ddata->data[i].work);
>                gpio_free(pdata->buttons[i].gpio);
>        }
>
> --
> 1.6.0.4
>


-- 
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni

Re: [PATCH]input: Change timer function to workqueue for gpio_keys driver

[Jani Nikula]

On Fri, Jun 12, 2009 at 8:40 PM, Trilok Soni<soni.trilok@gmail.com> wrote:
>>  static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
>>  {
>>        struct gpio_button_data *bdata = dev_id;
>> @@ -62,10 +61,10 @@ static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
>>        BUG_ON(irq != gpio_to_irq(button->gpio));
>>
>>        if (button->debounce_interval)
>> -               mod_timer(&bdata->timer,
>> -                       jiffies + msecs_to_jiffies(button->debounce_interval));
>> +               schedule_delayed_work(&bdata->work,
>> +                       msecs_to_jiffies(button->debounce_interval));
>>        else
>> -               gpio_keys_report_event(bdata);
>> +               schedule_work(&bdata->work.work);
>>
>>        return IRQ_HANDLED;
>>  }

Correct me if I'm wrong, but as far as I can tell,
schedule_delayed_work doesn't modify the timer if the work was already
pending. The result is not the same as with the timer. This breaks the
debouncing.

It looks like a slightly modified version of this patch has already
been committed [1], but it has the same problem.

[1] 0b346838c5862bfe911432956a106d602535d030 Input: gpio-keys - change
timer to workqueue


BR,
Jani.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH]input: Change timer function to workqueue for gpio_keys driver

[Alek Du]

On Thu, 25 Jun 2009 18:29:25 +0800
Jani Nikula <ext-jani.1.nikula@nokia.com> wrote:

> On Fri, Jun 12, 2009 at 8:40 PM, Trilok Soni<soni.trilok@gmail.com> wrote:
> >>  static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
> >>  {
> >>        struct gpio_button_data *bdata = dev_id;
> >> @@ -62,10 +61,10 @@ static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
> >>        BUG_ON(irq != gpio_to_irq(button->gpio));
> >>
> >>        if (button->debounce_interval)
> >> -               mod_timer(&bdata->timer,
> >> -                       jiffies + msecs_to_jiffies(button->debounce_interval));
> >> +               schedule_delayed_work(&bdata->work,
> >> +                       msecs_to_jiffies(button->debounce_interval));
> >>        else
> >> -               gpio_keys_report_event(bdata);
> >> +               schedule_work(&bdata->work.work);
> >>
> >>        return IRQ_HANDLED;
> >>  }
> 
> Correct me if I'm wrong, but as far as I can tell,
> schedule_delayed_work doesn't modify the timer if the work was already
> pending. The result is not the same as with the timer. This breaks the
> debouncing.

No. The workqueue is per button, if the work is already pending, then last
key press is not handled yet. That keeps the debouncing. Why you want the second
key press to break the first one? The second key press should be ignored, that's
the meaning of debouncing right?

> 
> It looks like a slightly modified version of this patch has already
> been committed [1], but it has the same problem.
> 
> [1] 0b346838c5862bfe911432956a106d602535d030 Input: gpio-keys - change
> timer to workqueue

Yes, the patch is already in Linus tree.


Thanks,
Alek
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH]input: Change timer function to workqueue for gpio_keys driver

[Jani Nikula]

On Thu, 2009-06-25 at 15:06 +0200, ext Alek Du wrote:
> On Thu, 25 Jun 2009 18:29:25 +0800
> Jani Nikula <ext-jani.1.nikula@nokia.com> wrote:
> 
> > On Fri, Jun 12, 2009 at 8:40 PM, Trilok Soni<soni.trilok@gmail.com> wrote:
> > >>  static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
> > >>  {
> > >>        struct gpio_button_data *bdata = dev_id;
> > >> @@ -62,10 +61,10 @@ static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
> > >>        BUG_ON(irq != gpio_to_irq(button->gpio));
> > >>
> > >>        if (button->debounce_interval)
> > >> -               mod_timer(&bdata->timer,
> > >> -                       jiffies + msecs_to_jiffies(button->debounce_interval));
> > >> +               schedule_delayed_work(&bdata->work,
> > >> +                       msecs_to_jiffies(button->debounce_interval));
> > >>        else
> > >> -               gpio_keys_report_event(bdata);
> > >> +               schedule_work(&bdata->work.work);
> > >>
> > >>        return IRQ_HANDLED;
> > >>  }
> > 
> > Correct me if I'm wrong, but as far as I can tell,
> > schedule_delayed_work doesn't modify the timer if the work was already
> > pending. The result is not the same as with the timer. This breaks the
> > debouncing.
> 
> No. The workqueue is per button, if the work is already pending, then last
> key press is not handled yet. That keeps the debouncing. Why you want the second
> key press to break the first one? The second key press should be ignored, that's
> the meaning of debouncing right?

No, debouncing is supposed to let the gpio line stabilize to either
state before doing *anything*. You only want to schedule the work (and
send the input event) once the line has been in the same state for
debounce_interval ms. This is what the original code did, by kicking the
timer further at each state change.

> > It looks like a slightly modified version of this patch has already
> > been committed [1], but it has the same problem.
> > 
> > [1] 0b346838c5862bfe911432956a106d602535d030 Input: gpio-keys - change
> > timer to workqueue
> 
> Yes, the patch is already in Linus tree.

IMHO it should be either fixed or reverted.


BR,
Jani.

Re: [PATCH]input: Change timer function to workqueue for gpio_keys driver

[Alek Du]

On Thu, 25 Jun 2009 21:31:33 +0800
Jani Nikula <ext-jani.1.nikula@nokia.com> wrote:

> On Thu, 2009-06-25 at 15:06 +0200, ext Alek Du wrote:
> > On Thu, 25 Jun 2009 18:29:25 +0800
> > Jani Nikula <ext-jani.1.nikula@nokia.com> wrote:
> > 
> > > On Fri, Jun 12, 2009 at 8:40 PM, Trilok Soni<soni.trilok@gmail.com> wrote:
> > > >>  static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
> > > >>  {
> > > >>        struct gpio_button_data *bdata = dev_id;
> > > >> @@ -62,10 +61,10 @@ static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
> > > >>        BUG_ON(irq != gpio_to_irq(button->gpio));
> > > >>
> > > >>        if (button->debounce_interval)
> > > >> -               mod_timer(&bdata->timer,
> > > >> -                       jiffies + msecs_to_jiffies(button->debounce_interval));
> > > >> +               schedule_delayed_work(&bdata->work,
> > > >> +                       msecs_to_jiffies(button->debounce_interval));
> > > >>        else
> > > >> -               gpio_keys_report_event(bdata);
> > > >> +               schedule_work(&bdata->work.work);
> > > >>
> > > >>        return IRQ_HANDLED;
> > > >>  }
> > > 
> > > Correct me if I'm wrong, but as far as I can tell,
> > > schedule_delayed_work doesn't modify the timer if the work was already
> > > pending. The result is not the same as with the timer. This breaks the
> > > debouncing.
> > 
> > No. The workqueue is per button, if the work is already pending, then last
> > key press is not handled yet. That keeps the debouncing. Why you want the second
> > key press to break the first one? The second key press should be ignored, that's
> > the meaning of debouncing right?
> 
> No, debouncing is supposed to let the gpio line stabilize to either
> state before doing *anything*. You only want to schedule the work (and
> send the input event) once the line has been in the same state for
> debounce_interval ms. This is what the original code did, by kicking the
> timer further at each state change.
> 
If you schedule the timer when you decide it "stabilized", the final gpio_get_value()
could still return 0 in the timer handler, if the key released at that time. So your previous
"stabilized" state is useless.
Isn't the delay work itself the mechanism to decide the "stabilized" ?

The work will finally call gpio_get_value to determine the state to be sent
to input layer. I don't think there is any defect here. 
 
> IMHO it should be either fixed or reverted.
> 

No, the original timer handler will crash kernel if you are using a I2C GPIO or SPI GPIO expander
Since it try to call sleep-able gpio_get_value in atomic context.

Re: [PATCH]input: Change timer function to workqueue for gpio_keys driver

[Jani Nikula]

On Thu, 2009-06-25 at 16:08 +0200, ext Alek Du wrote:
> On Thu, 25 Jun 2009 21:31:33 +0800
> Jani Nikula <ext-jani.1.nikula@nokia.com> wrote:
> 
> > On Thu, 2009-06-25 at 15:06 +0200, ext Alek Du wrote:
> > > On Thu, 25 Jun 2009 18:29:25 +0800
> > > Jani Nikula <ext-jani.1.nikula@nokia.com> wrote:
> > > > Correct me if I'm wrong, but as far as I can tell,
> > > > schedule_delayed_work doesn't modify the timer if the work was already
> > > > pending. The result is not the same as with the timer. This breaks the
> > > > debouncing.
> > > 
> > > No. The workqueue is per button, if the work is already pending, then last
> > > key press is not handled yet. That keeps the debouncing. Why you want the second
> > > key press to break the first one? The second key press should be ignored, that's
> > > the meaning of debouncing right?
> > 
> > No, debouncing is supposed to let the gpio line stabilize to either
> > state before doing *anything*. You only want to schedule the work (and
> > send the input event) once the line has been in the same state for
> > debounce_interval ms. This is what the original code did, by kicking the
> > timer further at each state change.
> > 
> If you schedule the timer when you decide it "stabilized", the final gpio_get_value()
> could still return 0 in the timer handler, if the key released at that time. So your previous
> "stabilized" state is useless.

True, gpio_keys_report_event should also compare the value to the
previous state and bail out if it's unchanged. Something along the lines
of:

@@ -46,6 +46,10 @@ static void gpio_keys_report_event(struct work_struct *work)
 	unsigned int type = button->type ?: EV_KEY;
 	int state = (gpio_get_value(button->gpio) ? 1 : 0) ^ button->active_low;
 
+	if (state == bdata->state)
+		return;
+	bdata->state = state;
+
 	input_event(input, type, button->code, !!state);
 	input_sync(input);
 }

Bailing out would mean the gpio line wasn't quite long enough in the
same state after all.

> Isn't the delay work itself the mechanism to decide the "stabilized" ?

Delay is like "something happenened now, take a random sample later".

> The work will finally call gpio_get_value to determine the state to be sent
> to input layer. I don't think there is any defect here. 

Please try to consider the difference in functionality before and after
your patch. What if the line keeps going high and low faster than
debounce_interval?

Debouncing should also completely ignore a single spike shorter than
debounce_interval. Admittedly gpio-keys was flawed, but please consider
a change like above which should fix that.

> No, the original timer handler will crash kernel if you are using a I2C GPIO or SPI GPIO expander
> Since it try to call sleep-able gpio_get_value in atomic context.

It should be fixed then.


BR,
Jani.

Re: [PATCH]input: Change timer function to workqueue for gpio_keys driver

[Jani Nikula]

On Thu, 2009-06-25 at 16:52 +0200, Jani Nikula wrote:
> On Thu, 2009-06-25 at 16:08 +0200, ext Alek Du wrote:
> > If you schedule the timer when you decide it "stabilized", the final gpio_get_value()
> > could still return 0 in the timer handler, if the key released at that time. So your previous
> > "stabilized" state is useless.
> 
> True, gpio_keys_report_event should also compare the value to the
> previous state and bail out if it's unchanged. Something along the lines
> of:
> 
> @@ -46,6 +46,10 @@ static void gpio_keys_report_event(struct work_struct *work)
>  	unsigned int type = button->type ?: EV_KEY;
>  	int state = (gpio_get_value(button->gpio) ? 1 : 0) ^ button->active_low;
>  
> +	if (state == bdata->state)
> +		return;
> +	bdata->state = state; 

Actually scrap that, the input layer already ignores events with no
state changes, right?

> Debouncing should also completely ignore a single spike shorter than
> debounce_interval. Admittedly gpio-keys was flawed, but please consider
> a change like above which should fix that.

Same here, gpio-keys did ignore spikes shorter than debounce_interval.


BR,
Jani.

Re: [PATCH]input: Change timer function to workqueue for gpio_keys driver

[Alek Du]

On Thu, 25 Jun 2009 23:05:55 +0800
Jani Nikula <ext-jani.1.nikula@nokia.com> wrote:

> On Thu, 2009-06-25 at 16:52 +0200, Jani Nikula wrote:
> > On Thu, 2009-06-25 at 16:08 +0200, ext Alek Du wrote:
> > > If you schedule the timer when you decide it "stabilized", the final gpio_get_value()
> > > could still return 0 in the timer handler, if the key released at that time. So your previous
> > > "stabilized" state is useless.
> > 
> > True, gpio_keys_report_event should also compare the value to the
> > previous state and bail out if it's unchanged. Something along the lines
> > of:
> > 
> > @@ -46,6 +46,10 @@ static void gpio_keys_report_event(struct work_struct *work)
> >  	unsigned int type = button->type ?: EV_KEY;
> >  	int state = (gpio_get_value(button->gpio) ? 1 : 0) ^ button->active_low;
> >  
> > +	if (state == bdata->state)
> > +		return;
> > +	bdata->state = state; 
> 
> Actually scrap that, the input layer already ignores events with no
> state changes, right?
> 
Yes, correct. I just want to reply your previous mail, but seems you find that. :-)
> > Debouncing should also completely ignore a single spike shorter than
> > debounce_interval. Admittedly gpio-keys was flawed, but please consider
> > a change like above which should fix that.
> 
> Same here, gpio-keys did ignore spikes shorter than debounce_interval.
> 
Yes, sending first state 0 to input layer does nothing wrong. 
> 
> BR,
> Jani.
> 
> 

Re: [PATCH]input: Change timer function to workqueue for gpio_keys driver

[Jani Nikula]

On Thu, 2009-06-25 at 17:09 +0200, ext Alek Du wrote:
> On Thu, 25 Jun 2009 23:05:55 +0800
> Jani Nikula <ext-jani.1.nikula@nokia.com> wrote:
> > Actually scrap that, the input layer already ignores events with no
> > state changes, right?
> > 
> Yes, correct. I just want to reply your previous mail, but seems you find that. :-)

The point about your patch breaking debouncing is still valid, though.


BR,
Jani.

Re: [PATCH]input: Change timer function to workqueue for gpio_keys driver

[Alek Du]

On Thu, 25 Jun 2009 23:42:08 +0800
Jani Nikula <ext-jani.1.nikula@nokia.com> wrote:

> On Thu, 2009-06-25 at 17:09 +0200, ext Alek Du wrote:
> > On Thu, 25 Jun 2009 23:05:55 +0800
> > Jani Nikula <ext-jani.1.nikula@nokia.com> wrote:
> > > Actually scrap that, the input layer already ignores events with no
> > > state changes, right?
> > > 
> > Yes, correct. I just want to reply your previous mail, but seems you find that. :-)
> 
> The point about your patch breaking debouncing is still valid, though.
> 
>
How? If IRQ triggered then the delay work scheduled, after debouncing time, in work, it checks GPIO pin state again,
if pin is active, send "1" to input layer -- key pressed, if de-active, send "0" -- no event. 

I really did test on my board, could you also try it out?

Thanks,
Alek
> BR,
> Jani.
> 
> 

Re: [PATCH]input: Change timer function to workqueue for gpio_keys driver

[Phil Carmody]

On Thu, 2009-06-25 at 17:48 +0200, ext Alek Du wrote:
> On Thu, 25 Jun 2009 23:42:08 +0800
> Jani Nikula <ext-jani.1.nikula@nokia.com> wrote:
> 
> > On Thu, 2009-06-25 at 17:09 +0200, ext Alek Du wrote:
> > > On Thu, 25 Jun 2009 23:05:55 +0800
> > > Jani Nikula <ext-jani.1.nikula@nokia.com> wrote:
> > > > Actually scrap that, the input layer already ignores events with no
> > > > state changes, right?
> > > > 
> > > Yes, correct. I just want to reply your previous mail, but seems you find that. :-)
> > 
> > The point about your patch breaking debouncing is still valid, though.
> > 
> >
> How? If IRQ triggered then the delay work scheduled, after debouncing time, in work, it checks GPIO pin state again,
> if pin is active, send "1" to input layer -- key pressed, if de-active, send "0" -- no event. 
> 
> I really did test on my board, could you also try it out?

This is not a matter of testing, this error can be seen simply by
algorithm analysis - that's how Jani and I discovered the problem in the
first place.

If you stopped calling the delay after the first transition "debouncing
time" and simply called it a "delay" you might more easily see that it
does *no* debouncing at all. Imagine putting noise on the line
constantly - the original code's timer would never expire. Your timer
will expire after a delay, and while the line is still toggling
frantically - you've not debounced.

Please investigate the meaning and implications of "debouncing" before
claiming your code does it.

Phil

Re: [PATCH]input: Change timer function to workqueue for gpio_keys driver

[Alek Du]

On Fri, 26 Jun 2009 00:09:14 +0800
Phil Carmody <ext-phil.2.carmody@nokia.com> wrote:

> If you stopped calling the delay after the first transition "debouncing
> time" and simply called it a "delay" you might more easily see that it
> does *no* debouncing at all. Imagine putting noise on the line
> constantly - the original code's timer would never expire. Your timer
> will expire after a delay, and while the line is still toggling
> frantically - you've not debounced.

I don't know if it is really meaningful if you want to handle such pool signal... 
Ok, if you want to handle this ultimate case, will this patch work?

        BUG_ON(irq != gpio_to_irq(button->gpio));

+       cancel_delayed_work_sync(&bdata->work);
        delay = button->debounce_interval ?
                        msecs_to_jiffies(button->debounce_interval) : 0;
        schedule_delayed_work(&bdata->work, delay);

Alek
> Please investigate the meaning and implications of "debouncing" before
> claiming your code does it.
> 
> Phil
> 

Re: [PATCH]input: Change timer function to workqueue for gpio_keys driver

[Dmitry Torokhov]

On Thu, Jun 25, 2009 at 9:23 AM, Alek Du<alek.du@intel.com> wrote:
> On Fri, 26 Jun 2009 00:09:14 +0800
> Phil Carmody <ext-phil.2.carmody@nokia.com> wrote:
>
>> If you stopped calling the delay after the first transition "debouncing
>> time" and simply called it a "delay" you might more easily see that it
>> does *no* debouncing at all. Imagine putting noise on the line
>> constantly - the original code's timer would never expire. Your timer
>> will expire after a delay, and while the line is still toggling
>> frantically - you've not debounced.
>
> I don't know if it is really meaningful if you want to handle such pool signal...
> Ok, if you want to handle this ultimate case, will this patch work?
>
>        BUG_ON(irq != gpio_to_irq(button->gpio));
>
> +       cancel_delayed_work_sync(&bdata->work);

Can't do *_sync in interrupt context.

>        delay = button->debounce_interval ?
>                        msecs_to_jiffies(button->debounce_interval) : 0;
>        schedule_delayed_work(&bdata->work, delay);
>

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html