Re: [PATCH 2/2] ARM: dts: AM33XX: Add lis331dlh device tree data to am335x-evm

On Friday 14 September 2012, AnilKumar, Chimata wrote:

quoted

On Fri, Sep 14, 2012 at 13:56:06, Arnd Bergmann wrote:
On Thursday 13 September 2012, AnilKumar Ch wrote:

quoted

Why do you put the "reg" property here

Here I specified reg property because lis331dlh I2C slave address is 0x18.

quoted

quoted

 		dcan1: d_can@481d0000 {
 			status = "okay";
 			pinctrl-names = "default";

@@ -61,6 +70,39 @@
 		regulator-max-microvolt = <5000000>;
 		regulator-boot-on;
 	};
+
+	lis3_reg: fixedregulator@1 {
+		compatible = "regulator-fixed";
+		regulator-name = "lis3_reg";
+		regulator-boot-on;
+	};
+};
+&lis331dlh {
+	compatible = "st,lis3lv02d-i2c";

and all the rest here? At least I would expect the "compatible" property
to be in the same place above.

This data is appended to above one, to make it readable I moved remaining
properties to here.

I don't follow how this is making things more readable.

Maybe a more logical way to do this would be use the existing i2c2 label
and write all the additions as

i2c2: {
	status = "okay";
	clock-frequency = <400000>;

	lis331dlh@18 {
		compatible = "st,lis3lv02d";
		reg = <0x18>;

		vdd-supply = <&lis3_reg>;
		vdd-io-supply = <&lis3_reg>;
		...
	};

quoted

Also, I think you should remove the "-i2c" postfix from the name, that
is already implied by the parent bus.

I will remove, but in case of spi the compatible name is lis3lv02d_spi.
By mistake I have uses "-i2c" instead of "_i2c".

The normal convention is to use '-', not '_', so that part was ok.

I think naming the other one lis3lv02d_spi was a mistake, it should
be named 'st,lis3lv02d' independent of the bus IMHO.

Document is already present, 
http://git.kernel.org/?p=linux/kernel/git/next/linux-next.git;a=commit;h=2f2ff3cc8d930493f9a598b9192706c09403e12e

Some minor changes in docs, in my next version I will update document
as well. I will send V3 if there are no comments on v2.

Ok.

	Arnd