Hi Grant,

On 22 September 2011 23:10, Grant Likely [off-list ref] wrote:

On Mon, Sep 19, 2011 at 03:49:13PM +0530, Thomas Abraham wrote:

quoted

Add device tree based discovery support for Samsung's keypad controller.

Cc: Joonyoung Shim <redacted>
Cc: Donghwa Lee <redacted>
Signed-off-by: Thomas Abraham <redacted>

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.

[...]

quoted

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

[...]

quoted

+
+ ? ? 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.

quoted

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

[...]

quoted

+ ? ? 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.

[...]