Re: [PATCH 3/3] input: add STMPExxxx keypad driver

[Rabin VINCENT]

Hi Dmitry,

On Wed, Jun 02, 2010 at 00:16:11 +0200, Dmitry Torokhov wrote:
> On Mon, May 31, 2010 at 05:47:16PM +0530, Rabin Vincent wrote:
> > +static struct stmpe_keypad_variant stmpe_keypad_variants[] = {
> > +     [STMPE1601] = {
> > +             .auto_increment         = true,
> > +             .num_data               = 5,
> > +             .num_normal_data        = 3,
> > +             .max_cols               = 8,
> > +             .max_rows               = 8,
> > +             .col_gpios              = 0x000ff,      /* GPIO 0 - 7 */
> > +             .row_gpios              = 0x0ff00,      /* GPIO 8 - 15 */
> > +     },
> > +     [STMPE2401] = {
> > +             .auto_increment         = false,
> > +             .num_data               = 3,
> > +             .num_normal_data        = 2,
> > +             .max_cols               = 8,
> > +             .max_rows               = 12,
> > +             .col_gpios              = 0x0000ff,     /* GPIO 0 - 7*/
> > +             .row_gpios              = 0x1fef00,     /* GPIO 8-14, 16-20 */
> > +     },
> > +     [STMPE2403] = {
> > +             .auto_increment         = true,
> > +             .num_data               = 5,
> > +             .num_normal_data        = 3,
> > +             .max_cols               = 8,
> > +             .max_rows               = 12,
> > +             .col_gpios              = 0x0000ff,     /* GPIO 0 - 7*/
> > +             .row_gpios              = 0x1fef00,     /* GPIO 8-14, 16-20 */
> > +     },
> > +};
> 
> I think it would be better if you moved stmpe_keypad_variant into a
> separate header had have board code provide variant instead of needing
> to add new variants to the driver itself. Or it is not defined by the
> board?

The data in this table is not really dependent on the board, but only on
the model number of the STMPExxxx used (this is dynamically detected in
the MFD driver).

For example, col_gpios lists the pins that can possibly be used for
columns on that STMPE model, not the ones which are actually used on the
board.  The ones that are actually used on the board are determined from
the platform data.

> 
> > +
> > +struct stmpe_keypad {
> > +     struct stmpe *stmpe;
> > +     struct input_dev *input;
> > +     struct stmpe_keypad_variant *variant;
> 
> const?
> 
> > +     struct stmpe_keypad_platform_data *plat;
> 
> const?

I've changed these.

[...]
> > +static int __devinit stmpe_keypad_probe(struct platform_device *pdev)
> > +{
[...]
> > +     ret = request_threaded_irq(irq, NULL, stmpe_keypad_irq, IRQF_ONESHOT,
> > +                                "stmpe-keypad", keypad);
> > +     if (ret) {
> > +             dev_err(&pdev->dev, "unable to get irq: %d\n", ret);
> > +             goto out_freeinput;
> > +     }
> > +
> > +     ret = input_register_device(input);
> > +     if (ret) {
> > +             dev_err(&pdev->dev,
> > +                     "unable to register input device: %d\n", ret);
> > +             goto out_freeirq;
> > +     }
> > +
> > +     platform_set_drvdata(pdev, keypad);
> > +
> > +     return 0;
> > +
> > +out_freeirq:
> > +     free_irq(irq, keypad);
> > +out_freeinput:
> > +     input_free_device(input);
> > +out_freekeypad:
> > +     kfree(keypad);
> > +     return ret;
> > +}
> > +
> > +static int __devexit stmpe_keypad_remove(struct platform_device *pdev)
> > +{
> > +     struct stmpe_keypad *keypad = platform_get_drvdata(pdev);
> > +     int irq = platform_get_irq(pdev, 0);
> > +
> > +     input_unregister_device(keypad->input);
> > +     free_irq(irq, keypad);
> 
> You want to free IRQ first, before unregisterin the device. Also, is

Done.  I've also changed the probe sequence to register the device
before requesting the IRQ.

> there a way to power down keypad parts?

I've added code to do this.

> 
> > +     platform_set_drvdata(pdev, NULL);
> > +     kfree(keypad);
> > +
> > +     return 0;
> > +}

Updated patch below.

Thanks,
Rabin