[PATCH v3 2/2] input: samsung-keypad: Add device tree support

[Thomas Abraham]

Hi Grant,

On 22 September 2011 23:10, Grant Likely <grant.likely at secretlab.ca> wrote:
> On Mon, Sep 19, 2011 at 03:49:13PM +0530, Thomas Abraham wrote:
>> Add device tree based discovery support for Samsung's keypad controller.
>>
>> Cc: Joonyoung Shim <jy0922.shim at samsung.com>
>> Cc: Donghwa Lee <dh09.lee at samsung.com>
>> Signed-off-by: Thomas Abraham <thomas.abraham at linaro.org>
>
> A few things to fix below, but you can add my acked-by when you've
> addressed them.

Ok. Thanks for your review. I will fix as per your comments.

[...]

>
>> diff --git a/drivers/input/keyboard/samsung-keypad.c b/drivers/input/keyboard/samsung-keypad.c
>> index f689f49..2a477bc 100644
>> --- a/drivers/input/keyboard/samsung-keypad.c
>> +++ b/drivers/input/keyboard/samsung-keypad.c

[...]

>> +
>> +     of_property_read_u32(np, "samsung,keypad-num-rows", &num_rows);
>> +     of_property_read_u32(np, "samsung,keypad-num-columns", &num_cols);
>
> property_read doesn't touch the values of num_rows or num_cols on
> failure.  The values need to be initialized if you're going to test
> the result value.

Ok. num_rows and num_cols variables will be set to zero before the
property read call.

>
>> +     if (!num_rows || !num_cols) {
>> +             dev_err(dev, "number of keypad rows/columns not specified\n");
>> +             return NULL;
>> +     }

[...]

>>
>> +     if (pdev->dev.of_node) {
>> +             samsung_keypad_parse_dt_gpio(&pdev->dev, keypad);
>> +#ifdef CONFIG_OF
>> +             keypad->type = of_device_is_compatible(pdev->dev.of_node,
>> +                                     "samsung,s5pv210-keypad");
>> +#endif
>
> This still looks odd.  If you move the #ifdef up one line, then you
> can remove the empty version of samsung_keypad_parse_dt_gpio(), and
> this block won't look so weird.
>

That would be a better approach. Thanks for the suggestion.

Regards,
Thomas.

[...]