Re: [RESEND/PATCHv5 1/2] drivers: input: keypad: Add device tree support

[Poddar, Sourav]

Hi Dmitry,

On Tue, Jul 10, 2012 at 11:43 AM, Dmitry Torokhov
<dmitry.torokhov@xxxxxxxxx> wrote:
> Hi Sourav,
>
> On Fri, Jun 08, 2012 at 04:22:59PM +0530, Sourav Poddar wrote:
>> Update the Documentation with omap4 keypad device tree
>> binding information.
>> Add device tree support for omap4 keypad driver.
>>
>> Tested on omap4430 sdp.
>>
>
> Sorry for the delay, I have a few comments:
>
>>
>>       /* platform data */
>>       pdata = pdev->dev.platform_data;
>> -     if (!pdata) {
>> +     if (np) {
>> +             of_property_read_u32(np, "keypad,num-rows", &num_rows);
>> +             of_property_read_u32(np, "keypad,num-columns", &num_cols);
>> +             if (!num_rows || !num_cols) {
>> +                     dev_err(&pdev->dev, "number of keypad rows/columns not specified\n");
>> +                     return -EINVAL;
>> +             }
>> +     } else if (pdata) {
>> +             num_rows = pdata->rows;
>> +             num_cols = pdata->cols;
>> +     } else {
>>               dev_err(&pdev->dev, "no platform data defined\n");
>>               return -EINVAL;
>>       }
>>
>
> I believe drivers should use platform data if it is supplied and use DT
> data if platform data is omitted. This way one can override firmware
> data if needed.
>
> Does the patch below (if applied on top of your) work for you?
>
Yes, the above patch works fine for me.
Acked-by: Sourav Poddar <sourav.poddar@xxxxxx>

Note, I did some mux setting changes in the bootloader for DT case.

Thanks,
Sourav
> Thanks.
>
> --
> Dmitry
>
> Input: omap4-keypad - misc fixes
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> ---
>
>  drivers/input/keyboard/omap4-keypad.c |  152 ++++++++++++++++-----------------
>  1 file changed, 74 insertions(+), 78 deletions(-)
>
>
> diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
> index d5a2d1a..033168e 100644
> --- a/drivers/input/keyboard/omap4-keypad.c
> +++ b/drivers/input/keyboard/omap4-keypad.c
> @@ -76,7 +76,6 @@ enum {
>
>  struct omap4_keypad {
>         struct input_dev *input;
> -       struct matrix_keymap_data *keymap_data;
>
>         void __iomem *base;
>         unsigned int irq;
> @@ -88,7 +87,7 @@ struct omap4_keypad {
>         unsigned int row_shift;
>         bool no_autorepeat;
>         unsigned char key_state[8];
> -       unsigned short keymap[];
> +       unsigned short *keymap;
>  };
>
>  static int kbd_readl(struct omap4_keypad *keypad_data, u32 offset)
> @@ -211,74 +210,52 @@ static void omap4_keypad_close(struct input_dev *input)
>         pm_runtime_put_sync(input->dev.parent);
>  }
>
> -static struct omap4_keypad *omap_keypad_parse_dt(struct device *dev,
> -                               uint32_t rows, uint32_t cols,
> -                               struct input_dev *input_dev)
> +#ifdef CONFIG_OF
> +static int __devinit omap4_keypad_parse_dt(struct device *dev,
> +                                          struct omap4_keypad *keypad_data)
>  {
>         struct device_node *np = dev->of_node;
> -       struct platform_device *pdev = to_platform_device(dev);
> -       struct omap4_keypad *keypad_data = platform_get_drvdata(pdev);
>         int error;
>
> -       error = matrix_keypad_build_keymap(NULL, "linux,keymap",
> -                               rows, cols, keypad_data->keymap, input_dev);
> -       if (error) {
> -               dev_err(&pdev->dev, "failed to build keymap\n");
> -               input_free_device(input_dev);
> +       if (!np) {
> +               dev_err(dev, "missing DT data");
> +               return -EINVAL;
> +       }
> +
> +       of_property_read_u32(np, "keypad,num-rows", &keypad_data->rows);
> +       of_property_read_u32(np, "keypad,num-columns", &keypad_data->cols);
> +       if (!keypad_data->rows || !keypad_data->cols) {
> +               dev_err(dev, "number of keypad rows/columns not specified\n");
> +               return -EINVAL;
>         }
>
>         if (of_get_property(np, "linux,input-no-autorepeat", NULL))
>                 keypad_data->no_autorepeat = true;
>
> -       return keypad_data;
> +       return 0;
> +}
> +#else
> +static inline int omap4_keypad_parse_dt(struct device *dev,
> +                                       struct omap4_keypad *keypad_data)
> +{
> +       return -ENOSYS;
>  }
> +#endif
>
>  static int __devinit omap4_keypad_probe(struct platform_device *pdev)
>  {
> -       struct device *dev = &pdev->dev;
> -       struct device_node *np = dev->of_node;
> -       const struct omap4_keypad_platform_data *pdata;
> +       const struct omap4_keypad_platform_data *pdata =
> +                               dev_get_platdata(&pdev->dev);
> +       const struct matrix_keymap_data *keymap_data =
> +                               pdata ? pdata->keymap_data : NULL;
>         struct omap4_keypad *keypad_data;
>         struct input_dev *input_dev;
>         struct resource *res;
> -       resource_size_t size;
> -       unsigned int row_shift = 0, max_keys = 0;
> -       uint32_t num_rows = 0, num_cols = 0;
> +       unsigned int max_keys;
>         int rev;
>         int irq;
>         int error;
>
> -       /* platform data */
> -       pdata = pdev->dev.platform_data;
> -       if (np) {
> -               of_property_read_u32(np, "keypad,num-rows", &num_rows);
> -               of_property_read_u32(np, "keypad,num-columns", &num_cols);
> -               if (!num_rows || !num_cols) {
> -                       dev_err(&pdev->dev, "number of keypad rows/columns not specified\n");
> -                       return -EINVAL;
> -               }
> -       } else if (pdata) {
> -               num_rows = pdata->rows;
> -               num_cols = pdata->cols;
> -       } else {
> -               dev_err(&pdev->dev, "no platform data defined\n");
> -               return -EINVAL;
> -       }
> -
> -       row_shift = get_count_order(num_cols);
> -       max_keys = num_rows << row_shift;
> -
> -       keypad_data = devm_kzalloc(dev, sizeof(struct omap4_keypad) +
> -                       max_keys * sizeof(keypad_data->keymap[0]),
> -                               GFP_KERNEL);
> -
> -       if (!keypad_data) {
> -               dev_err(&pdev->dev, "keypad_data memory allocation failed\n");
> -               return -ENOMEM;
> -       }
> -
> -       platform_set_drvdata(pdev, keypad_data);
> -
>         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>         if (!res) {
>                 dev_err(&pdev->dev, "no base address specified\n");
> @@ -291,9 +268,24 @@ static int __devinit omap4_keypad_probe(struct platform_device *pdev)
>                 return -EINVAL;
>         }
>
> -       size = resource_size(res);
> +       keypad_data = kzalloc(sizeof(struct omap4_keypad), GFP_KERNEL);
> +       if (!keypad_data) {
> +               dev_err(&pdev->dev, "keypad_data memory allocation failed\n");
> +               return -ENOMEM;
> +       }
> +
> +       keypad_data->irq = irq;
> +
> +       if (pdata) {
> +               keypad_data->rows = pdata->rows;
> +               keypad_data->cols = pdata->cols;
> +       } else {
> +               error = omap4_keypad_parse_dt(&pdev->dev, keypad_data);
> +               if (error)
> +                       return error;
> +       }
>
> -       res = request_mem_region(res->start, size, pdev->name);
> +       res = request_mem_region(res->start, resource_size(res), pdev->name);
>         if (!res) {
>                 dev_err(&pdev->dev, "can't request mem region\n");
>                 error = -EBUSY;
> @@ -307,15 +299,11 @@ static int __devinit omap4_keypad_probe(struct platform_device *pdev)
>                 goto err_release_mem;
>         }
>
> -       keypad_data->rows = num_rows;
> -       keypad_data->cols = num_cols;
> -       keypad_data->irq = irq;
> -       keypad_data->row_shift = row_shift;
>
>         /*
> -       * Enable clocks for the keypad module so that we can read
> -       * revision register.
> -       */
> +        * Enable clocks for the keypad module so that we can read
> +        * revision register.
> +        */
>         pm_runtime_enable(&pdev->dev);
>         error = pm_runtime_get_sync(&pdev->dev);
>         if (error) {
> @@ -358,29 +346,30 @@ static int __devinit omap4_keypad_probe(struct platform_device *pdev)
>         input_dev->open = omap4_keypad_open;
>         input_dev->close = omap4_keypad_close;
>
> -       if (np) {
> -               keypad_data = omap_keypad_parse_dt(&pdev->dev,
> -                               keypad_data->rows, keypad_data->cols,
> -                               input_dev);
> -       } else {
> -               keypad_data->keymap_data =
> -                       (struct matrix_keymap_data *)pdata->keymap_data;
> -               error = matrix_keypad_build_keymap(keypad_data->keymap_data,
> -                               NULL, keypad_data->rows, keypad_data->cols,
> -                               keypad_data->keymap, input_dev);
> -               if (error) {
> -                       dev_err(&pdev->dev, "failed to build keymap\n");
> -                       goto err_free_input;
> -               }
> -       }
> -
> +       input_set_capability(input_dev, EV_MSC, MSC_SCAN);
>         if (!keypad_data->no_autorepeat)
>                 __set_bit(EV_REP, input_dev->evbit);
>
> -       input_set_capability(input_dev, EV_MSC, MSC_SCAN);
> -
>         input_set_drvdata(input_dev, keypad_data);
>
> +       keypad_data->row_shift = get_count_order(keypad_data->cols);
> +       max_keys = keypad_data->rows << keypad_data->row_shift;
> +       keypad_data->keymap = kzalloc(max_keys * sizeof(keypad_data->keymap[0]),
> +                                     GFP_KERNEL);
> +       if (!keypad_data->keymap) {
> +               dev_err(&pdev->dev, "Not enough memory for keymap\n");
> +               error = -ENOMEM;
> +               goto err_free_input;
> +       }
> +
> +       error = matrix_keypad_build_keymap(keymap_data, NULL,
> +                                          keypad_data->rows, keypad_data->cols,
> +                                          keypad_data->keymap, input_dev);
> +       if (error) {
> +               dev_err(&pdev->dev, "failed to build keymap\n");
> +               goto err_free_keymap;
> +       }
> +
>         error = request_irq(keypad_data->irq, omap4_keypad_interrupt,
>                              IRQF_TRIGGER_RISING,
>                              "omap4-keypad", keypad_data);
> @@ -397,11 +386,14 @@ static int __devinit omap4_keypad_probe(struct platform_device *pdev)
>                 goto err_pm_disable;
>         }
>
> +       platform_set_drvdata(pdev, keypad_data);
>         return 0;
>
>  err_pm_disable:
>         pm_runtime_disable(&pdev->dev);
>         free_irq(keypad_data->irq, keypad_data);
> +err_free_keymap:
> +       kfree(keypad_data->keymap);
>  err_free_input:
>         input_free_device(input_dev);
>  err_pm_put_sync:
> @@ -409,7 +401,7 @@ err_pm_put_sync:
>  err_unmap:
>         iounmap(keypad_data->base);
>  err_release_mem:
> -       release_mem_region(res->start, size);
> +       release_mem_region(res->start, resource_size(res));
>  err_free_keypad:
>         kfree(keypad_data);
>         return error;
> @@ -431,17 +423,21 @@ static int __devexit omap4_keypad_remove(struct platform_device *pdev)
>         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>         release_mem_region(res->start, resource_size(res));
>
> +       kfree(keypad_data->keymap);
>         kfree(keypad_data);
> +
>         platform_set_drvdata(pdev, NULL);
>
>         return 0;
>  }
>
> +#ifdef CONFIG_OF
>  static const struct of_device_id omap_keypad_dt_match[] = {
>         { .compatible = "ti,omap4-keypad" },
>         {},
>  };
>  MODULE_DEVICE_TABLE(of, omap_keypad_dt_match);
> +#endif
>
>  static struct platform_driver omap4_keypad_driver = {
>         .probe          = omap4_keypad_probe,
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/