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,

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

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

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

[PATCH 0/2] fix gpio-keys debouncing and timer sleep issues

[Jani Nikula]

Alek, someone might depend on how debouncing worked before, so let's not 
break it. I, for one, am looking at using this in the future, and would 
want this to work as it was before.

Why don't we fix this instead of debating? For simplicity, here's first 
a revert of the original patch and then a fix to the sleep issue. 
Unfortunately, I do not have the equipment to properly test this right 
now.

Could you please review and test this if you have the equipment? Add 
your signed-off-by and comments, of course. Would that be okay?

BR,
Jani.


Jani Nikula (2):
  Revert "Input: gpio-keys - change timer to workqueue"
  input: gpio-keys: avoid possibility of sleeping in timer function

 drivers/input/keyboard/gpio_keys.c |   33 ++++++++++++++++++++++++---------
 1 files changed, 24 insertions(+), 9 deletions(-)

[PATCH 1/2] Revert "Input: gpio-keys - change timer to workqueue"

[Jani Nikula]

This reverts commit 0b346838c5862bfe911432956a106d602535d030.

This commit breaks GPIO debouncing by replacing the original mod_timer
with schedule_delayed_work in the interrupt handler. The latter does not
kick the timer further on GPIO line changes as it should to perform
debouncing.

Signed-off-by: Jani Nikula <ext-jani.1.nikula@nokia.com>
---
 drivers/input/keyboard/gpio_keys.c |   32 ++++++++++++++++++++------------
 1 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 2157cd7..9767213 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -22,14 +22,13 @@
 #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 delayed_work work;
+	struct timer_list timer;
 };
 
 struct gpio_keys_drvdata {
@@ -37,10 +36,8 @@ struct gpio_keys_drvdata {
 	struct gpio_button_data data[0];
 };
 
-static void gpio_keys_report_event(struct work_struct *work)
+static void gpio_keys_report_event(struct gpio_button_data *bdata)
 {
-	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;
@@ -50,17 +47,25 @@ static void gpio_keys_report_event(struct work_struct *work)
 	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;
 	struct gpio_keys_button *button = bdata->button;
-	unsigned long delay;
 
 	BUG_ON(irq != gpio_to_irq(button->gpio));
 
-	delay = button->debounce_interval ?
-			msecs_to_jiffies(button->debounce_interval) : 0;
-	schedule_delayed_work(&bdata->work, delay);
+	if (button->debounce_interval)
+		mod_timer(&bdata->timer,
+			jiffies + msecs_to_jiffies(button->debounce_interval));
+	else
+		gpio_keys_report_event(bdata);
 
 	return IRQ_HANDLED;
 }
@@ -107,7 +112,8 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 
 		bdata->input = input;
 		bdata->button = button;
-		INIT_DELAYED_WORK(&bdata->work, gpio_keys_report_event);
+		setup_timer(&bdata->timer,
+			    gpio_check_button, (unsigned long)bdata);
 
 		error = gpio_request(button->gpio, button->desc ?: "gpio_keys");
 		if (error < 0) {
@@ -166,7 +172,8 @@ 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]);
-		cancel_delayed_work_sync(&ddata->data[i].work);
+		if (pdata->buttons[i].debounce_interval)
+			del_timer_sync(&ddata->data[i].timer);
 		gpio_free(pdata->buttons[i].gpio);
 	}
 
@@ -190,7 +197,8 @@ 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]);
-		cancel_delayed_work_sync(&ddata->data[i].work);
+		if (pdata->buttons[i].debounce_interval)
+			del_timer_sync(&ddata->data[i].timer);
 		gpio_free(pdata->buttons[i].gpio);
 	}
 
-- 
1.6.3.2

[PATCH 2/2] input: gpio-keys: avoid possibility of sleeping in timer function

[Jani Nikula]

The gpio_get_value function may sleep, so it should not be called in a
timer function. Move gpio_get_value calls to workqueue.

Signed-off-by: Jani Nikula <ext-jani.1.nikula@nokia.com>
---
 drivers/input/keyboard/gpio_keys.c |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 9767213..efed0c9 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -22,6 +22,7 @@
 #include <linux/platform_device.h>
 #include <linux/input.h>
 #include <linux/gpio_keys.h>
+#include <linux/workqueue.h>
 
 #include <asm/gpio.h>
 
@@ -29,6 +30,7 @@ struct gpio_button_data {
 	struct gpio_keys_button *button;
 	struct input_dev *input;
 	struct timer_list timer;
+	struct work_struct work;
 };
 
 struct gpio_keys_drvdata {
@@ -36,8 +38,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);
 	struct gpio_keys_button *button = bdata->button;
 	struct input_dev *input = bdata->input;
 	unsigned int type = button->type ?: EV_KEY;
@@ -47,11 +51,11 @@ static void gpio_keys_report_event(struct gpio_button_data *bdata)
 	input_sync(input);
 }
 
-static void gpio_check_button(unsigned long _data)
+static void gpio_keys_timer(unsigned long _data)
 {
 	struct gpio_button_data *data = (struct gpio_button_data *)_data;
 
-	gpio_keys_report_event(data);
+	schedule_work(&data->work);
 }
 
 static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
@@ -65,7 +69,7 @@ static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
 		mod_timer(&bdata->timer,
 			jiffies + msecs_to_jiffies(button->debounce_interval));
 	else
-		gpio_keys_report_event(bdata);
+		schedule_work(&bdata->work);
 
 	return IRQ_HANDLED;
 }
@@ -113,7 +117,8 @@ 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);
+			    gpio_keys_timer, (unsigned long)bdata);
+		INIT_WORK(&bdata->work, gpio_keys_report_event);
 
 		error = gpio_request(button->gpio, button->desc ?: "gpio_keys");
 		if (error < 0) {
@@ -174,6 +179,7 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 		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_work_sync(&ddata->data[i].work);
 		gpio_free(pdata->buttons[i].gpio);
 	}
 
@@ -199,6 +205,7 @@ static int __devexit gpio_keys_remove(struct platform_device *pdev)
 		free_irq(irq, &ddata->data[i]);
 		if (pdata->buttons[i].debounce_interval)
 			del_timer_sync(&ddata->data[i].timer);
+		cancel_work_sync(&ddata->data[i].work);
 		gpio_free(pdata->buttons[i].gpio);
 	}
 
-- 
1.6.3.2

Re: [PATCH 2/2] input: gpio-keys: avoid possibility of sleeping in timer function

[Alek Du]

On Fri, 26 Jun 2009 20:15:18 +0800
Jani Nikula <ext-jani.1.nikula@nokia.com> wrote:

> The gpio_get_value function may sleep, so it should not be called in a
> timer function. Move gpio_get_value calls to workqueue.
> 
> Signed-off-by: Jani Nikula <ext-jani.1.nikula@nokia.com>
> ---
>  drivers/input/keyboard/gpio_keys.c |   17 ++++++++++++-----
>  1 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/input/keyboard/gpio_keys.c
> b/drivers/input/keyboard/gpio_keys.c index 9767213..efed0c9 100644
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c
> @@ -22,6 +22,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/input.h>
>  #include <linux/gpio_keys.h>
> +#include <linux/workqueue.h>
>  
>  #include <asm/gpio.h>
>  
> @@ -29,6 +30,7 @@ struct gpio_button_data {
>  	struct gpio_keys_button *button;
>  	struct input_dev *input;
>  	struct timer_list timer;
> +	struct work_struct work;
>  };
>  
>  struct gpio_keys_drvdata {
> @@ -36,8 +38,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);
>  	struct gpio_keys_button *button = bdata->button;
>  	struct input_dev *input = bdata->input;
>  	unsigned int type = button->type ?: EV_KEY;
> @@ -47,11 +51,11 @@ static void gpio_keys_report_event(struct
> gpio_button_data *bdata) input_sync(input);
>  }
>  
> -static void gpio_check_button(unsigned long _data)
> +static void gpio_keys_timer(unsigned long _data)
>  {
>  	struct gpio_button_data *data = (struct gpio_button_data
> *)_data; 
> -	gpio_keys_report_event(data);
> +	schedule_work(&data->work);
>  }
>  
>  static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
> @@ -65,7 +69,7 @@ static irqreturn_t gpio_keys_isr(int irq, void
> *dev_id) mod_timer(&bdata->timer,
>  			jiffies +
> msecs_to_jiffies(button->debounce_interval)); else
> -		gpio_keys_report_event(bdata);
> +		schedule_work(&bdata->work);
>  
>  	return IRQ_HANDLED;
>  }
> @@ -113,7 +117,8 @@ 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);
> +			    gpio_keys_timer, (unsigned long)bdata);
> +		INIT_WORK(&bdata->work, gpio_keys_report_event);
>  
>  		error = gpio_request(button->gpio, button->desc ?:
> "gpio_keys"); if (error < 0) {
> @@ -174,6 +179,7 @@ static int __devinit gpio_keys_probe(struct
> platform_device *pdev) 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_work_sync(&ddata->data[i].work);
>  		gpio_free(pdata->buttons[i].gpio);
>  	}
>  
> @@ -199,6 +205,7 @@ static int __devexit gpio_keys_remove(struct
> platform_device *pdev) free_irq(irq, &ddata->data[i]);
>  		if (pdata->buttons[i].debounce_interval)
>  			del_timer_sync(&ddata->data[i].timer);
> +		cancel_work_sync(&ddata->data[i].work);
>  		gpio_free(pdata->buttons[i].gpio);
>  	}
>  

Tested with my board, it works well. Dmitry, could you please consider
to apply it?

Tested-by: Alek Du <alek.du@intel.com>

RE: [PATCH 0/2] fix gpio-keys debouncing and timer sleep issues

[Du, Alek]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 1176 bytes --]

>From: Jani Nikula [mailto:ext-jani.1.nikula@nokia.com]
>Sent: 2009Äê6ÔÂ26ÈÕ 20:15
>To: Du, Alek
>Cc: dmitry.torokhov@gmail.com; linux-kernel@vger.kernel.org;
>linux-input@vger.kernel.org; soni.trilok@gmail.com; ben-linux@fluff.org;
>ext-phil.2.carmody@nokia.com; ext-jani.1.nikula@nokia.com
>Subject: [PATCH 0/2] fix gpio-keys debouncing and timer sleep issues
>
>Alek, someone might depend on how debouncing worked before, so let's not
>break it. I, for one, am looking at using this in the future, and would
>want this to work as it was before.
>
>Why don't we fix this instead of debating? For simplicity, here's first
>a revert of the original patch and then a fix to the sleep issue.
>Unfortunately, I do not have the equipment to properly test this right
>now.

Agree, and thanks for the patch.

>Could you please review and test this if you have the equipment? Add
>your signed-off-by and comments, of course. Would that be okay?

Ok, will test it later. And of course, we need Dmitry ACK it finally.

Alek

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 0/2] fix gpio-keys debouncing and timer sleep issues

[Dmitry Torokhov]

Hi Jani,

On Fri, Jun 26, 2009 at 03:15:16PM +0300, Jani Nikula wrote:
> Alek, someone might depend on how debouncing worked before, so let's not 
> break it. I, for one, am looking at using this in the future, and would 
> want this to work as it was before.
> 
> Why don't we fix this instead of debating? For simplicity, here's first 
> a revert of the original patch and then a fix to the sleep issue. 
> Unfortunately, I do not have the equipment to properly test this right 
> now.
> 

I intend to apply the both patches. It is unfortunate that we need to
use both a timer and a workqueue, I wonder what would it take to
implement something like reschedule_delayed_work() that would adjust
the delay in the same fashion that mod_timer() does?

-- 
Dmitry

Re: [PATCH 0/2] fix gpio-keys debouncing and timer sleep issues

[Jani Nikula]

On Mon, 2009-06-29 at 07:59 +0200, ext Dmitry Torokhov wrote:
> I intend to apply the both patches. It is unfortunate that we need to
> use both a timer and a workqueue, I wonder what would it take to
> implement something like reschedule_delayed_work() that would adjust
> the delay in the same fashion that mod_timer() does?

Hi Dmitry -

Since a delayed_work is simply a work_struct and a timer_list, the only
overhead here is the one line timer function. A work_struct per button
is of course an addition to the original in both Alek's patch and mine,
but I guess there are no alternatives if gpio_get_value may sleep.

I did think about something like reschedule_delayed_work myself, but
tackling the locking issues seemed like too much work compared to this
simple solution.


BR,
Jani.