Re: [patch/rfc 2.6.28-rc2] input: twl4030_keypad driver

[David Brownell <david-b@pacbell.net>]

Hi,

On Wednesday 21 January 2009, Trilok Soni wrote:
> > +
> > +static void twl4030_kp_scan(struct twl4030_keypad *kp, int release_all)
> > +{
> > +       u16 new_state[MAX_ROWS];
> > +       int col, row;
> > +
> > +       ...
> > +
> > +                       key = twl4030_find_key(kp, col, row);
> > +                       if (key < 0)
> > +                               dev_warn(kp->dbg_dev,
> > +                                       "Spurious key event %d-%d\n",
> > +                                        col, row);
> > +                       else if (key & KEY_PERSISTENT)
> > +                               continue;
> > +                       else
> > +                               input_report_key(kp->input, key,
> > +                                                new_state[row] & (1 << col));
> > +               }
> > +               kp->kp_state[row] = new_state[row];
> > +       }
> 
> where do I find input_sync(...) being called?

Yeah, I was wondering about that too.  The code
obviously works without it ... I'll add that call
anyway.


> > +
> > +       dev_set_drvdata(&pdev->dev, kp);
> 
> How about platform_set_drvdata ??

Those calls are exactly equivalent, although you're
right that platform_* versions are preferred.  Either
is correct.


> > +       /* setup input device */
> > +       set_bit(EV_KEY, kp->input->evbit);
> 
> __set_bit please.
> 
> > +
> > +       /* Enable auto repeat feature of Linux input subsystem */
> > +       if (pdata->rep)
> > +               set_bit(EV_REP, kp->input->evbit);
> > +
> > +       for (i = 0; i < kp->keymapsize; i++)
> > +               set_bit(kp->keymap[i] & KEYNUM_MASK,
> > +                               kp->input->keybit);
> 
> Ditto.

The practical difference there being that __set_bit() is
a bit less costly (space and time), being non-atomic; there
is no semantic difference.  Either is correct.


> > +       /*
> > +        * This ISR will always execute in kernel thread context because of
> > +        * the need to access the TWL4030 over the I2C bus.
> > +        *
> > +        * NOTE:  we assume this host is wired to TWL4040 INT1, not INT2 ...
> > +        */
> > +       ret = request_irq(kp->irq, do_kp_irq, 0, pdev->name, kp);
> 
> How about adding IRQF_SAMPLE_RANDOME here ??

Input system does that automagically; it should not be
sampled twice.


> > +err5:
> > +       /* mask all events - we don't care about the result */
> > +       (void) twl4030_kpwrite_u8(kp, 0xff, KEYP_IMR1);
> > +       free_irq(kp->irq, NULL);
> > +err3:
> > +       input_unregister_device(kp->input);
> 
> No free_device after input_unregister_device. Add kp->input = NULL above.
> 
> > +err2:
> > +       input_free_device(kp->input);

and kfree(kp) was missing too ...

I'll merge the following with any other feedback.

- Dave

---
 drivers/input/keyboard/twl4030_keypad.c |   14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

--- a/drivers/input/keyboard/twl4030_keypad.c
+++ b/drivers/input/keyboard/twl4030_keypad.c
@@ -260,6 +260,7 @@ static void twl4030_kp_scan(struct twl40
 		}
 		kp->kp_state[row] = new_state[row];
 	}
+	input_sync(kp->input);
 }
 
 /*
@@ -316,7 +317,7 @@ static int __devinit twl4030_kp_probe(st
 
 	/* ASSERT:  cols <= 8, rows <= 8 */
 
-	dev_set_drvdata(&pdev->dev, kp);
+	platform_set_drvdata(pdev, kp);
 
 	/* Get the debug Device */
 	kp->dbg_dev = &pdev->dev;
@@ -334,14 +335,14 @@ static int __devinit twl4030_kp_probe(st
 	kp->irq = platform_get_irq(pdev, 0);
 
 	/* setup input device */
-	set_bit(EV_KEY, kp->input->evbit);
+	__set_bit(EV_KEY, kp->input->evbit);
 
 	/* Enable auto repeat feature of Linux input subsystem */
 	if (pdata->rep)
-		set_bit(EV_REP, kp->input->evbit);
+		__set_bit(EV_REP, kp->input->evbit);
 
 	for (i = 0; i < kp->keymapsize; i++)
-		set_bit(kp->keymap[i] & KEYNUM_MASK,
+		__set_bit(kp->keymap[i] & KEYNUM_MASK,
 				kp->input->keybit);
 
 	kp->input->name	= "TWL4030 Keypad";
@@ -441,15 +442,16 @@ err5:
 	free_irq(kp->irq, NULL);
 err3:
 	input_unregister_device(kp->input);
+	kp->input = NULL;
 err2:
 	input_free_device(kp->input);
-
+	kfree(kp);
 	return -ENODEV;
 }
 
 static int __devexit twl4030_kp_remove(struct platform_device *pdev)
 {
-	struct twl4030_keypad *kp = dev_get_drvdata(&pdev->dev);
+	struct twl4030_keypad *kp = platform_get_drvdata(pdev);
 
 	free_irq(kp->irq, kp);
 	input_unregister_device(kp->input);