[PATCH] input: keypad: lm8323: convert to threaded IRQ

[PATCH] input: keypad: lm8323: convert to threaded IRQ

[Felipe Balbi]

there's no need for that workqueue anymore.
Get rid of it and move to threaded IRQs
instead.

Signed-off-by: Felipe Balbi <balbi@ti.com>
---

compile tested only. I need someone to reply with
a Tested-by tag.

 drivers/input/keyboard/lm8323.c |   23 ++++-------------------
 1 files changed, 4 insertions(+), 19 deletions(-)

diff --git a/drivers/input/keyboard/lm8323.c b/drivers/input/keyboard/lm8323.c
index 71f744a8..3b21f42 100644
--- a/drivers/input/keyboard/lm8323.c
+++ b/drivers/input/keyboard/lm8323.c
@@ -146,7 +146,6 @@ struct lm8323_chip {
 	/* device lock */
 	struct mutex		lock;
 	struct i2c_client	*client;
-	struct work_struct	work;
 	struct input_dev	*idev;
 	bool			kp_enabled;
 	bool			pm_suspend;
@@ -162,7 +161,6 @@ struct lm8323_chip {
 
 #define client_to_lm8323(c)	container_of(c, struct lm8323_chip, client)
 #define dev_to_lm8323(d)	container_of(d, struct lm8323_chip, client->dev)
-#define work_to_lm8323(w)	container_of(w, struct lm8323_chip, work)
 #define cdev_to_pwm(c)		container_of(c, struct lm8323_pwm, cdev)
 #define work_to_pwm(w)		container_of(w, struct lm8323_pwm, work)
 
@@ -375,9 +373,9 @@ static void pwm_done(struct lm8323_pwm *pwm)
  * Bottom half: handle the interrupt by posting key events, or dealing with
  * errors appropriately.
  */
-static void lm8323_work(struct work_struct *work)
+static irqreturn_t lm8323_irq(int irq, void *_lm)
 {
-	struct lm8323_chip *lm = work_to_lm8323(work);
+	struct lm8323_chip *lm = _lm;
 	u8 ints;
 	int i;
 
@@ -409,16 +407,6 @@ static void lm8323_work(struct work_struct *work)
 	}
 
 	mutex_unlock(&lm->lock);
-}
-
-/*
- * We cannot use I2C in interrupt context, so we just schedule work.
- */
-static irqreturn_t lm8323_irq(int irq, void *data)
-{
-	struct lm8323_chip *lm = data;
-
-	schedule_work(&lm->work);
 
 	return IRQ_HANDLED;
 }
@@ -675,7 +663,6 @@ static int __devinit lm8323_probe(struct i2c_client *client,
 	lm->client = client;
 	lm->idev = idev;
 	mutex_init(&lm->lock);
-	INIT_WORK(&lm->work, lm8323_work);
 
 	lm->size_x = pdata->size_x;
 	lm->size_y = pdata->size_y;
@@ -746,9 +733,8 @@ static int __devinit lm8323_probe(struct i2c_client *client,
 		goto fail3;
 	}
 
-	err = request_irq(client->irq, lm8323_irq,
-			  IRQF_TRIGGER_FALLING | IRQF_DISABLED,
-			  "lm8323", lm);
+	err = request_threaded_irq(client->irq, NULL, lm8323_irq,
+			  IRQF_TRIGGER_FALLING, "lm8323", lm);
 	if (err) {
 		dev_err(&client->dev, "could not get IRQ %d\n", client->irq);
 		goto fail4;
@@ -783,7 +769,6 @@ static int __devexit lm8323_remove(struct i2c_client *client)
 
 	disable_irq_wake(client->irq);
 	free_irq(client->irq, lm);
-	cancel_work_sync(&lm->work);
 
 	input_unregister_device(lm->idev);
 
-- 
1.7.6.rc1

Re: [PATCH] input: keypad: lm8323: convert to threaded IRQ

[Leigh Brown]

Hi Felipe,

Many thanks for trying to assist me with my problem. To recap,
I'm seeing the keypad lock-up on my N810 using this driver,
when I type quickly.

I tried your patch, and it doesn't fix the issue.  However, I've
read the datasheet for the lm8323 and it looks to me like the
interrupt should be level rather than edge triggered.

The following additional patch makes things work for me.  I
couldn't tell you if it's the correct thing to do but I can no
longer cause the keypad to lock-up by typing too fast.

Regards,

Leigh.

-- 

diff --git a/drivers/input/keyboard/lm8323.c 
b/drivers/input/keyboard/lm8323.c
index 3b21f42..ab0acaf 100644
--- a/drivers/input/keyboard/lm8323.c
+++ b/drivers/input/keyboard/lm8323.c
@@ -734,7 +734,7 @@ static int __devinit lm8323_probe(struct i2c_client 
*client,
  	}

  	err = request_threaded_irq(client->irq, NULL, lm8323_irq,
-			  IRQF_TRIGGER_FALLING, "lm8323", lm);
+			  IRQF_TRIGGER_LOW|IRQF_ONESHOT, "lm8323", lm);
  	if (err) {
  		dev_err(&client->dev, "could not get IRQ %d\n", client->irq);
  		goto fail4;

Re: [PATCH] input: keypad: lm8323: convert to threaded IRQ

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 760 bytes --]

Hi,

On Sun, Jun 19, 2011 at 07:00:13PM +0000, Leigh Brown wrote:
> Many thanks for trying to assist me with my problem. To recap,

yeah, no problem ;-)

> I'm seeing the keypad lock-up on my N810 using this driver,
> when I type quickly.
> 
> I tried your patch, and it doesn't fix the issue.  However, I've
> read the datasheet for the lm8323 and it looks to me like the
> interrupt should be level rather than edge triggered.
> 
> The following additional patch makes things work for me.  I
> couldn't tell you if it's the correct thing to do but I can no
> longer cause the keypad to lock-up by typing too fast.

good good. Looks like IRQF_ONESHOT is what did the trick.

Did you check if IRQF_ONESHOT alone was enough ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH] input: keypad: lm8323: convert to threaded IRQ

[Leigh Brown]

On Sun, 19 Jun 2011 22:56:06 +0300, Felipe Balbi wrote:
> Hi,
>
> On Sun, Jun 19, 2011 at 07:00:13PM +0000, Leigh Brown wrote:
>> Many thanks for trying to assist me with my problem. To recap,
>
> yeah, no problem ;-)
>
>> I'm seeing the keypad lock-up on my N810 using this driver,
>> when I type quickly.
>>
>> I tried your patch, and it doesn't fix the issue.  However, I've
>> read the datasheet for the lm8323 and it looks to me like the
>> interrupt should be level rather than edge triggered.
>>
>> The following additional patch makes things work for me.  I
>> couldn't tell you if it's the correct thing to do but I can no
>> longer cause the keypad to lock-up by typing too fast.
>
> good good. Looks like IRQF_ONESHOT is what did the trick.
>
> Did you check if IRQF_ONESHOT alone was enough ?

I tried that first, but it didn't fix it, although it makes it a
little bit harder to trigger.  I've just tried again to be sure,
but I got a lock-up quite easily.

Regards,

Leigh.

Re: [PATCH] input: keypad: lm8323: convert to threaded IRQ

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1335 bytes --]

Hi,

On Sun, Jun 19, 2011 at 11:01:43PM +0100, Leigh Brown wrote:
> On Sun, 19 Jun 2011 22:56:06 +0300, Felipe Balbi wrote:
> >Hi,
> >
> >On Sun, Jun 19, 2011 at 07:00:13PM +0000, Leigh Brown wrote:
> >>Many thanks for trying to assist me with my problem. To recap,
> >
> >yeah, no problem ;-)
> >
> >>I'm seeing the keypad lock-up on my N810 using this driver,
> >>when I type quickly.
> >>
> >>I tried your patch, and it doesn't fix the issue.  However, I've
> >>read the datasheet for the lm8323 and it looks to me like the
> >>interrupt should be level rather than edge triggered.
> >>
> >>The following additional patch makes things work for me.  I
> >>couldn't tell you if it's the correct thing to do but I can no
> >>longer cause the keypad to lock-up by typing too fast.
> >
> >good good. Looks like IRQF_ONESHOT is what did the trick.
> >
> >Did you check if IRQF_ONESHOT alone was enough ?
> 
> I tried that first, but it didn't fix it, although it makes it a
> little bit harder to trigger.  I've just tried again to be sure,
> but I got a lock-up quite easily.

Ok, cool thanks... Can you send your patch as a proper patch (with your
Signed-off-by line and everything) saying that it fixes the lock up ?

My patch converts to threaded IRQ and yours fixes the triggering ;-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[PATCH] input: keypad: lm8323: use level triggered interrupts

[Leigh Brown]

This patch, which should be applied on top of Felipe's
"input: keypad: lm8323: convert to threaded IRQ" patch, fixes the issue
of the Nokia N810 keypad stopping responding if multiple key events 
occur
in quick succession.

Signed-off-by: Leigh Brown <leigh@solinno.co.uk>
---
  drivers/input/keyboard/lm8323.c |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/input/keyboard/lm8323.c 
b/drivers/input/keyboard/lm8323.c
index 3b21f42..ab0acaf 100644
--- a/drivers/input/keyboard/lm8323.c
+++ b/drivers/input/keyboard/lm8323.c
@@ -734,7 +734,7 @@ static int __devinit lm8323_probe(struct i2c_client 
*client,
  	}

  	err = request_threaded_irq(client->irq, NULL, lm8323_irq,
-			  IRQF_TRIGGER_FALLING, "lm8323", lm);
+			  IRQF_TRIGGER_LOW|IRQF_ONESHOT, "lm8323", lm);
  	if (err) {
  		dev_err(&client->dev, "could not get IRQ %d\n", client->irq);
  		goto fail4;

Re: [PATCH] input: keypad: lm8323: convert to threaded IRQ

[Dmitry Torokhov]

On Tue, Jun 14, 2011 at 12:16:52AM +0300, Felipe Balbi wrote:
> there's no need for that workqueue anymore.
> Get rid of it and move to threaded IRQs
> instead.
> 
> Signed-off-by: Felipe Balbi <balbi@ti.com>

Applied, thanks Felipe.

-- 
Dmitry

Re: [PATCH] input: keypad: lm8323: use level triggered interrupts

[Dmitry Torokhov]

On Mon, Jun 20, 2011 at 08:02:27PM +0100, Leigh Brown wrote:
> This patch, which should be applied on top of Felipe's
> "input: keypad: lm8323: convert to threaded IRQ" patch, fixes the issue
> of the Nokia N810 keypad stopping responding if multiple key events
> occur
> in quick succession.
> 
> Signed-off-by: Leigh Brown <leigh@solinno.co.uk>

Applied, thanks Leigh.

-- 
Dmitry