Hi Alexander,

Please see my comments inline.

On Saturday 17 of August 2013 09:45:40 Alexander Shiyan wrote:

You should not really be enforcing such strict node naming. If you have 
the "buttons" node, which aggregates all the buttons, all you need is just 
parsing all the child nodes of it, regardless of their names.

As a side note, the @unit-address suffix is used only when you have the 
reg property present in the node and must be equal to the address value 
set in this property.

If you need the button index (which is true, looking at the code), you 
should use the reg property instead, with appropriate #address-cells 
(probably 1 for your use case) and #size-cells (0, as size doesn't make 
sense in your use case) in "buttons" node.

+  Optional properties:

The properties below should be rather prefixed with "fsl," string to 
indicate that they are specific to this particular device.

+    active-high : Change active button level from 0 to 1.

It is a boolean property, isn't it? If yes, this should be noted. Also the 
"from 0 to 1" statement is a bit unfortunate.

What about:

	active-high : A boolean property present if the button is active 
high. Otherwise the button is assumed to be active low.

+    enable-reset: Performs hadware reset through PMIC.

Could you elaborate on meaning of this property a bit more, please?

So basically this example after addressing my comments would look like:

	buttons {
		#address-cells = <1>;
		#size-cells = <0>;

		irrelevant-name@1 {
			reg = <1>;
			linux,code = <0x1f>;
			fsl,debounce = <1>;
		};
	};

"buttons");

The of_find_node_by_name() function does not search for node with given 
name inside the node you specify, but rather starting from this node and 
going over all the rest of device tree.

of_get_child_by_name() seems to be what you need here.

Hmm, this can lead to stack corruption. The line above will print 9 
(including terminating zero) characters to the childname array, which can 
hold only 5 of them.

+			child = of_get_child_by_name(parent, childname);
+			if (!child)
+				continue;
+			prop = of_get_property(child, "linux,code", NULL);
+			if (prop)
+				code = be32_to_cpu(*prop) & 0xffff;

What about using of_property_read_u32() here? See include/linux/of.h for a 
whole lot of useful DT parsing functions and their descriptions in 
drivers/of/base.c (kernel-doc comments).

+			else {
+				dev_err(&pdev->dev,
+					"Button %i: Missing key code\n", i 

+ 1);

+				continue;
+			}
+			invert = of_property_read_bool(child, "active-

high");

+			reset = of_property_read_bool(child, "enable-

reset");

+			prop = of_get_property(child, "debounce", NULL);
+			debounce = prop ? be32_to_cpu(*prop) : 0;

Again, of_property_read_u32() would simplify the code. As a side note, 
there is also a be32_to_cpup() helper, which dereferences a __be32 pointer 
for you and returns a value in CPU endianess.

+		} else {
+			if (!(pdata->buttons[i].flags & 

MC13XXX_BUTTON_ENABLE))

+				continue;
+			code = pdata->buttons[i].keycode;
+			invert = !!(pdata->buttons[i].flags &
+				    MC13XXX_BUTTON_POL_INVERT);
+			reset = !!(pdata->buttons[i].flags &
+				   MC13XXX_BUTTON_RESET_EN);
+			debounce = pdata->buttons[i].flags;
+		}

Anyway, based on my comments wrt the bindings, I would rather separate the 
DT parsing code from this loop and, in DT case, parse the buttons in a 
for_each_child_of_node() loop, reading the reg property and using it as an 
index to the buttons array (after checking if the index isn't out of 
bounds, of course).

Best regards,
Tomasz