Re: [PATCH 2/2] ARM: dts: AM33XX: Add lis331dlh device tree data to am335x-evm
On Friday 14 September 2012, AnilKumar, Chimata wrote:
> Small question here, in my v2 version I have specified both
> the compatible names lis3lv02d and lis331dlh is it fine or
> only one is sufficient?
>
> +static struct of_device_id lis3lv02d_i2c_dt_ids[] = {
> + { .compatible = "st,lis3lv02d" },
> + { .compatible = "st,lis331dlh" },
> + {}
> +};
>
That's ok. In most cases, people will need the data field to point to a
data structure with the differences between two chips, but not if they are
identical from the software side.
It's also fine if you just list one entry here and put both values as
"compatible" in the device tree, to signify that the device is backwards
compatible with the older variant.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
RE: [PATCH 2/2] ARM: dts: AM33XX: Add lis331dlh device tree data to am335x-evm
+Daniel
On Fri, Sep 14, 2012 at 15:58:37, Arnd Bergmann wrote:
> On Friday 14 September 2012, AnilKumar, Chimata wrote:
> >> On Fri, Sep 14, 2012 at 13:56:06, Arnd Bergmann wrote:
> > > On Thursday 13 September 2012, AnilKumar Ch wrote:
>
> > >
> > > Why do you put the "reg" property here
> >
> > Here I specified reg property because lis331dlh I2C slave address is 0x18.
> >
> > >
> > > > dcan1: d_can@481d {
> > > > status = "okay";
> > > > pinctrl-names = "default";
> > > > @@ -61,6 +70,39 @@
> > > > regulator-max-microvolt = <500>;
> > > > 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.
Basic lis3 parameter is in i2c2 node to tell the hierarchy of
nodes and remaining parameters at the bottom. With this way
we can understand easily what is start & end braces so that
node hierarchy is cleaner. (This is my view)
>
> 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 = <40>;
>
> lis331dlh@18 {
> compatible = "st,lis3lv02d";
> reg = <0x18>;
>
> vdd-supply = <&lis3_reg>;
> vdd-io-supply = <&lis3_reg>;
> ...
> };
>
If this is better/preferable way then I will change.
> > > 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".
Then, I will change to 'st,lis3lv02d' in lis3lv02d_i2c driver
and same will added to .dts file.
Small question here, in my v2 version I have specified both
the compatible names lis3lv02d and lis331dlh is it fine or
only one is sufficient?
+static struct of_device_id lis3lv02d_i2c_dt_ids[] = {
+ { .compatible = "st,lis3lv02d" },
+ { .compatible = "st,lis331dlh" },
+ {}
+};
>
> 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.
Yes, initially I expected the same. I think driver name is used
as a compatible string.
>
> > 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.
>
Can you just quickly review v2 series? So that I will send
V3.
Thanks
AnilKumar
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 2/2] ARM: dts: AM33XX: Add lis331dlh device tree data to am335x-evm
On Friday 14 September 2012, AnilKumar, Chimata wrote:
>> On Fri, Sep 14, 2012 at 13:56:06, Arnd Bergmann wrote:
> > On Thursday 13 September 2012, AnilKumar Ch wrote:
> >
> > Why do you put the "reg" property here
>
> Here I specified reg property because lis331dlh I2C slave address is 0x18.
>
> >
> > > dcan1: d_can@481d {
> > > status = "okay";
> > > pinctrl-names = "default";
> > > @@ -61,6 +70,39 @@
> > > regulator-max-microvolt = <500>;
> > > 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 = <40>;
lis331dlh@18 {
compatible = "st,lis3lv02d";
reg = <0x18>;
vdd-supply = <&lis3_reg>;
vdd-io-supply = <&lis3_reg>;
...
};
> > 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
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
RE: [PATCH 2/2] ARM: dts: AM33XX: Add lis331dlh device tree data to am335x-evm
Hi Arnd,
Thanks for the review,
On Fri, Sep 14, 2012 at 13:56:06, Arnd Bergmann wrote:
> On Thursday 13 September 2012, AnilKumar Ch wrote:
> > Add lis331dlh device tree data to am335x-evm.dts. In AM335x EVM
> > lis331dlh accelerometer is connected to I2C2 bus. So this patch
> > change the status to "okay" to use I2C2 bus. Also added all the
> > required platform data to am335x-evm.
> >
> > Signed-off-by: AnilKumar Ch
> > ---
> > arch/arm/boot/dts/am335x-evm.dts | 42
> > ++
> > 1 file changed, 42 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/am335x-evm.dts
> > b/arch/arm/boot/dts/am335x-evm.dts
> > index 9fb59c5..9e5a878 100644
> > --- a/arch/arm/boot/dts/am335x-evm.dts
> > +++ b/arch/arm/boot/dts/am335x-evm.dts
> > @@ -47,6 +47,15 @@
> > };
> > };
> >
> > + i2c2: i2c@4802a000 {
> > + status = "okay";
> > + clock-frequency = <40>;
> > +
> > + lis331dlh: lis331dlh@18 {
> > + reg = <0x18>;
> > + };
> > + };
>
> Why do you put the "reg" property here
Here I specified reg property because lis331dlh I2C slave address is 0x18.
>
> > dcan1: d_can@481d {
> > status = "okay";
> > pinctrl-names = "default";
> > @@ -61,6 +70,39 @@
> > regulator-max-microvolt = <500>;
> > 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.
>
> 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".
>
> > + Vdd-supply = <&lis3_reg>;
> > + Vdd_IO-supply = <&lis3_reg>;
> > +
> > + st,click-single-x;
> > + st,click-single-y;
> > + st,click-single-z;
> > + st,click-thresh-x = <10>;
> > + st,click-thresh-y = <10>;
> > + st,click-thresh-z = <10>;
> > + st,irq1-click;
> > + st,irq2-click;
> > + st,wakeup-x-lo;
> > + st,wakeup-x-hi;
> > + st,wakeup-y-lo;
> > + st,wakeup-y-hi;
> > + st,wakeup-z-lo;
> > + st,wakeup-z-hi;
> > + st,min-limit-x = <120>;
> > + st,min-limit-y = <120>;
> > + st,min-limit-z = <140>;
> > + st,max-limit-x = <550>;
> > + st,max-limit-y = <550>;
> > + st,max-limit-z = <750>;
>
> Is there a binding document that describes all these?
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.
Thanks
AnilKumar
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 2/2] ARM: dts: AM33XX: Add lis331dlh device tree data to am335x-evm
On Thursday 13 September 2012, AnilKumar Ch wrote:
> Add lis331dlh device tree data to am335x-evm.dts. In AM335x EVM
> lis331dlh accelerometer is connected to I2C2 bus. So this patch
> change the status to "okay" to use I2C2 bus. Also added all the
> required platform data to am335x-evm.
>
> Signed-off-by: AnilKumar Ch
> ---
> arch/arm/boot/dts/am335x-evm.dts | 42
> ++
> 1 file changed, 42 insertions(+)
>
> diff --git a/arch/arm/boot/dts/am335x-evm.dts
> b/arch/arm/boot/dts/am335x-evm.dts
> index 9fb59c5..9e5a878 100644
> --- a/arch/arm/boot/dts/am335x-evm.dts
> +++ b/arch/arm/boot/dts/am335x-evm.dts
> @@ -47,6 +47,15 @@
> };
> };
>
> + i2c2: i2c@4802a000 {
> + status = "okay";
> + clock-frequency = <40>;
> +
> + lis331dlh: lis331dlh@18 {
> + reg = <0x18>;
> + };
> + };
Why do you put the "reg" property here
> dcan1: d_can@481d {
> status = "okay";
> pinctrl-names = "default";
> @@ -61,6 +70,39 @@
> regulator-max-microvolt = <500>;
> 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.
Also, I think you should remove the "-i2c" postfix from the name, that
is already implied by the parent bus.
> + Vdd-supply = <&lis3_reg>;
> + Vdd_IO-supply = <&lis3_reg>;
> +
> + st,click-single-x;
> + st,click-single-y;
> + st,click-single-z;
> + st,click-thresh-x = <10>;
> + st,click-thresh-y = <10>;
> + st,click-thresh-z = <10>;
> + st,irq1-click;
> + st,irq2-click;
> + st,wakeup-x-lo;
> + st,wakeup-x-hi;
> + st,wakeup-y-lo;
> + st,wakeup-y-hi;
> + st,wakeup-z-lo;
> + st,wakeup-z-hi;
> + st,min-limit-x = <120>;
> + st,min-limit-y = <120>;
> + st,min-limit-z = <140>;
> + st,max-limit-x = <550>;
> + st,max-limit-y = <550>;
> + st,max-limit-z = <750>;
Is there a binding document that describes all these?
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
[PATCH 2/2] ARM: dts: AM33XX: Add lis331dlh device tree data to am335x-evm
Add lis331dlh device tree data to am335x-evm.dts. In AM335x EVM
lis331dlh accelerometer is connected to I2C2 bus. So this patch
change the status to "okay" to use I2C2 bus. Also added all the
required platform data to am335x-evm.
Signed-off-by: AnilKumar Ch
---
arch/arm/boot/dts/am335x-evm.dts | 42 ++
1 file changed, 42 insertions(+)
diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
index 9fb59c5..9e5a878 100644
--- a/arch/arm/boot/dts/am335x-evm.dts
+++ b/arch/arm/boot/dts/am335x-evm.dts
@@ -47,6 +47,15 @@
};
};
+ i2c2: i2c@4802a000 {
+ status = "okay";
+ clock-frequency = <40>;
+
+ lis331dlh: lis331dlh@18 {
+ reg = <0x18>;
+ };
+ };
+
dcan1: d_can@481d {
status = "okay";
pinctrl-names = "default";
@@ -61,6 +70,39 @@
regulator-max-microvolt = <500>;
regulator-boot-on;
};
+
+ lis3_reg: fixedregulator@1 {
+ compatible = "regulator-fixed";
+ regulator-name = "lis3_reg";
+ regulator-boot-on;
+ };
+};
+
+&lis331dlh {
+ compatible = "st,lis3lv02d-i2c";
+ Vdd-supply = <&lis3_reg>;
+ Vdd_IO-supply = <&lis3_reg>;
+
+ st,click-single-x;
+ st,click-single-y;
+ st,click-single-z;
+ st,click-thresh-x = <10>;
+ st,click-thresh-y = <10>;
+ st,click-thresh-z = <10>;
+ st,irq1-click;
+ st,irq2-click;
+ st,wakeup-x-lo;
+ st,wakeup-x-hi;
+ st,wakeup-y-lo;
+ st,wakeup-y-hi;
+ st,wakeup-z-lo;
+ st,wakeup-z-hi;
+ st,min-limit-x = <120>;
+ st,min-limit-y = <120>;
+ st,min-limit-z = <140>;
+ st,max-limit-x = <550>;
+ st,max-limit-y = <550>;
+ st,max-limit-z = <750>;
};
/include/ "tps65910.dtsi"
--
1.7.9.5
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
