diff mbox

[4/5] ARM: dts: imx28-apf28dev: add support for can0

Message ID 1423836725-86061-4-git-send-email-gwenhael.goavec-merou@armadeus.com (mailing list archive)
State New, archived
Headers show

Commit Message

Gwenhael Goavec-Merou Feb. 13, 2015, 2:12 p.m. UTC 
Signed-off-by: Gwenhael Goavec-Merou <gwenhael.goavec-merou@armadeus.com>
Signed-off-by: Sebastien Szymanski <sebastien.szymanski@armadeus.com>
---
 arch/arm/boot/dts/imx28-apf28dev.dts | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Comments

Lothar Waßmann Feb. 13, 2015, 2:53 p.m. UTC | #1 
Hi,

Gwenhael Goavec-Merou wrote:
> Signed-off-by: Gwenhael Goavec-Merou <gwenhael.goavec-merou@armadeus.com>
> Signed-off-by: Sebastien Szymanski <sebastien.szymanski@armadeus.com>
> ---
>  arch/arm/boot/dts/imx28-apf28dev.dts | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/imx28-apf28dev.dts b/arch/arm/boot/dts/imx28-apf28dev.dts
> index 68405c3..a052d3e 100644
> --- a/arch/arm/boot/dts/imx28-apf28dev.dts
> +++ b/arch/arm/boot/dts/imx28-apf28dev.dts
> @@ -110,6 +110,13 @@
>  					};
>  				};
>  			};
> +
> +			can0: can@80032000 {
>
Did you compile-test this? It should produce a build error, since
the label can0 already exists in imx28.dtsi.
You should use:
|&can0 {
|				pinctrl-names = "default";
|				pinctrl-0 = <&can0_pins_a>;
|				xceiver-supply = <&reg_can0_vcc>;
|				status = "okay";
|};

> @@ -176,6 +183,14 @@
>  			gpio = <&gpio1 23 1>;
s/ 1/ GPIO_ACTIVE_LOW/ ?


Lothar Waßmann
Gwenhael Goavec-Merou Feb. 13, 2015, 3:31 p.m. UTC | #2 
Hi,

On Fri, 13 Feb 2015 15:53:58 +0100
Lothar Waßmann <LW@KARO-electronics.de> wrote:

> Hi,
> 
> Gwenhael Goavec-Merou wrote:
> > Signed-off-by: Gwenhael Goavec-Merou <gwenhael.goavec-merou@armadeus.com>
> > Signed-off-by: Sebastien Szymanski <sebastien.szymanski@armadeus.com>
> > ---
> >  arch/arm/boot/dts/imx28-apf28dev.dts | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/imx28-apf28dev.dts b/arch/arm/boot/dts/imx28-apf28dev.dts
> > index 68405c3..a052d3e 100644
> > --- a/arch/arm/boot/dts/imx28-apf28dev.dts
> > +++ b/arch/arm/boot/dts/imx28-apf28dev.dts
> > @@ -110,6 +110,13 @@
> >  					};
> >  				};
> >  			};
> > +
> > +			can0: can@80032000 {
> >
> Did you compile-test this? It should produce a build error, since
> the label can0 already exists in imx28.dtsi.
Compile fine and without error. It's the same thing for all labels in 
this dts file.
> You should use:
> |&can0 {
> |				pinctrl-names = "default";
> |				pinctrl-0 = <&can0_pins_a>;
> |				xceiver-supply = <&reg_can0_vcc>;
> |				status = "okay";
> |};
> 
> > @@ -176,6 +183,14 @@
> >  			gpio = <&gpio1 23 1>;
> s/ 1/ GPIO_ACTIVE_LOW/ ?
> 
This change is out of the context to this patch. This change will be done in
other patch.
> 
> Lothar Waßmann
> -- 
Gwenhael Goavec-Merou
Lothar Waßmann Feb. 13, 2015, 4:59 p.m. UTC | #3 
Hi,

gwenhael.goavec wrote:
> Hi,
> 
> On Fri, 13 Feb 2015 15:53:58 +0100
> Lothar Waßmann <LW@KARO-electronics.de> wrote:
> 
> > Hi,
> > 
> > Gwenhael Goavec-Merou wrote:
> > > Signed-off-by: Gwenhael Goavec-Merou <gwenhael.goavec-merou@armadeus.com>
> > > Signed-off-by: Sebastien Szymanski <sebastien.szymanski@armadeus.com>
> > > ---
> > >  arch/arm/boot/dts/imx28-apf28dev.dts | 15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> > > 
> > > diff --git a/arch/arm/boot/dts/imx28-apf28dev.dts b/arch/arm/boot/dts/imx28-apf28dev.dts
> > > index 68405c3..a052d3e 100644
> > > --- a/arch/arm/boot/dts/imx28-apf28dev.dts
> > > +++ b/arch/arm/boot/dts/imx28-apf28dev.dts
> > > @@ -110,6 +110,13 @@
> > >  					};
> > >  				};
> > >  			};
> > > +
> > > +			can0: can@80032000 {
> > >
> > Did you compile-test this? It should produce a build error, since
> > the label can0 already exists in imx28.dtsi.
> Compile fine and without error. It's the same thing for all labels in 
> this dts file.
> > You should use:
> > |&can0 {
> > |				pinctrl-names = "default";
> > |				pinctrl-0 = <&can0_pins_a>;
> > |				xceiver-supply = <&reg_can0_vcc>;
> > |				status = "okay";
> > |};
>
I also noticed now, that defining a label for the same node twice is OK
with the dtc-compiler.
But you could simplify your dts files and make them more readable by
referencing the existing labels (like all the other platforms do)
rather than reproducing the whole tree structrure in your files.

Thus instead of:
|	apb@80000000 {
|		apbh@80000000 {
|			gpmi-nand@8000c000 {
[...]
|		};
|
|		apbx@80040000 {
|			duart: serial@80074000 {
[...]

you would only have:
|&duart {
	[...]
|};
|
|&gpmi {
|	[...]
|};
in your dts files.
Sorting the labels alphabetically additionally helps readability.


Lothar Waßmann
Gwenhael Goavec-Merou Feb. 16, 2015, 9:59 a.m. UTC | #4 
Hi,

On Fri, 13 Feb 2015 17:59:36 +0100
Lothar Waßmann <LW@KARO-electronics.de> wrote:

> Hi,
> 
> gwenhael.goavec wrote:
> > Hi,
> > 
> > On Fri, 13 Feb 2015 15:53:58 +0100
> > Lothar Waßmann <LW@KARO-electronics.de> wrote:
> > 
> > > Hi,
> > > 
> > > Gwenhael Goavec-Merou wrote:
> > > > Signed-off-by: Gwenhael Goavec-Merou <gwenhael.goavec-merou@armadeus.com>
> > > > Signed-off-by: Sebastien Szymanski <sebastien.szymanski@armadeus.com>
> > > > ---
> > > >  arch/arm/boot/dts/imx28-apf28dev.dts | 15 +++++++++++++++
> > > >  1 file changed, 15 insertions(+)
> > > > 
> > > > diff --git a/arch/arm/boot/dts/imx28-apf28dev.dts b/arch/arm/boot/dts/imx28-apf28dev.dts
> > > > index 68405c3..a052d3e 100644
> > > > --- a/arch/arm/boot/dts/imx28-apf28dev.dts
> > > > +++ b/arch/arm/boot/dts/imx28-apf28dev.dts
> > > > @@ -110,6 +110,13 @@
> > > >  					};
> > > >  				};
> > > >  			};
> > > > +
> > > > +			can0: can@80032000 {
> > > >
> > > Did you compile-test this? It should produce a build error, since
> > > the label can0 already exists in imx28.dtsi.
> > Compile fine and without error. It's the same thing for all labels in 
> > this dts file.
> > > You should use:
> > > |&can0 {
> > > |				pinctrl-names = "default";
> > > |				pinctrl-0 = <&can0_pins_a>;
> > > |				xceiver-supply = <&reg_can0_vcc>;
> > > |				status = "okay";
> > > |};
> >
> I also noticed now, that defining a label for the same node twice is OK
> with the dtc-compiler.
> But you could simplify your dts files and make them more readable by
> referencing the existing labels (like all the other platforms do)
> rather than reproducing the whole tree structrure in your files.
> 
> Thus instead of:
> |	apb@80000000 {
> |		apbh@80000000 {
> |			gpmi-nand@8000c000 {
> [...]
> |		};
> |
> |		apbx@80040000 {
> |			duart: serial@80074000 {
> [...]
> 
> you would only have:
> |&duart {
> 	[...]
> |};
> |
> |&gpmi {
> |	[...]
> |};
> in your dts files.
> Sorting the labels alphabetically additionally helps readability.
> 
Yes it's possible but, according to [1], this change does not seem desirable.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/255375.html

Gwenhael Goavec-Merou
diff mbox

Patch

diff --git a/arch/arm/boot/dts/imx28-apf28dev.dts b/arch/arm/boot/dts/imx28-apf28dev.dts
index 68405c3..a052d3e 100644
--- a/arch/arm/boot/dts/imx28-apf28dev.dts
+++ b/arch/arm/boot/dts/imx28-apf28dev.dts
@@ -110,6 +110,13 @@ 
 					};
 				};
 			};
+
+			can0: can@80032000 {
+				pinctrl-names = "default";
+				pinctrl-0 = <&can0_pins_a>;
+				xceiver-supply = <&reg_can0_vcc>;
+				status = "okay";
+			};
 		};
 
 		apbx@80040000 {
@@ -176,6 +183,14 @@ 
 			gpio = <&gpio1 23 1>;
 			enable-active-high;
 		};
+
+		reg_can0_vcc: regulator@1 {
+			compatible = "regulator-fixed";
+			reg = <1>;
+			regulator-name = "can0_vcc";
+			regulator-min-microvolt = <5000000>;
+			regulator-max-microvolt = <5000000>;
+		};
 	};
 
 	leds {