Hi Pavel,

On Sun, Oct 27, 2013 at 12:17:15PM +0100, Pavel Machek wrote:

quoted

Add device tree support for twl4030 keypad driver and update the
Documentation with twl4030 keypad device tree binding information.

Tested on Nokia N900.

It looks pretty good.

Thanks.

quoted

+ * keypad,num-rows and keypad,num-columns are required.

Is "keypad," prefix neccessary here?

See Documentation/devicetree/bindings/input/matrix-keymap.txt

quoted

+Optional Properties specific to linux:
+- linux,keypad-no-autorepeat: do no enable autorepeat feature.

"do not autorepeat". Plus I do not see what is Linux specifc about not
autorepeating... Other systems will likely know about autorepeat, too.

This property has already been used like this for
samsung-keypad, stmpe-keypad, omap-keypad and
gpio-matrix-keypad.

quoted

+#if IS_ENABLED(CONFIG_OF)

I'm probably missing something here, but why not #ifdef CONFIG_OF?

I have been told for other drivers, that IS_ENABLED() is
the prefered way to check for configuration these days.

quoted

@@ -381,7 +426,7 @@ static int twl4030_kp_probe(struct platform_device *pdev)
 
 	input_set_capability(input, EV_MSC, MSC_SCAN);
 	/* Enable auto repeat feature of Linux input subsystem */
-	if (pdata->rep)
+	if (!kp->no_autorepeat)
 		__set_bit(EV_REP, input->evbit);

Double negation is nasty to read. I believe code would be more
readable if you switched logic to kp->autorepeat.

I was thinking about the same when writing it, but decided
against it, since it will just move the double negation to
the variable initialization.

-- Sebastian