devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] ARM: dts: imx6qdl-sabresd: Do not place regulator nodes under simple-bus
@ 2014-02-12  0:31 Fabio Estevam
       [not found] ` <1392165078-25794-1-git-send-email-festevam-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
  0 siblings, 1 reply; 8+ messages in thread
From: Fabio Estevam @ 2014-02-12  0:31 UTC (permalink / raw)
  To: shawn.guo-QSEj5FYQhm4dnm+yROfE0A
  Cc: mark.rutland-5wv7dgnIgG8,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Fabio Estevam

From: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>

According to Documentation/devicetree/bindings/regulator/regulator.txt 
regulator nodes should not be placed under 'simple-bus'.

Mark Rutland also explains about it at:
http://www.spinics.net/lists/linux-usb/msg101497.html 
 
Signed-off-by: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
Shawn,

I can convert other dts files if you are fine with this approach.

 arch/arm/boot/dts/imx6qdl-sabresd.dtsi | 51 ++++++++++++++--------------------
 1 file changed, 21 insertions(+), 30 deletions(-)

diff --git a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
index 0d816d3..d7df5b2 100644
--- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
@@ -18,38 +18,29 @@
 		reg = <0x10000000 0x40000000>;
 	};
 
-	regulators {
-		compatible = "simple-bus";
-		#address-cells = <1>;
-		#size-cells = <0>;
-
-		reg_usb_otg_vbus: regulator@0 {
-			compatible = "regulator-fixed";
-			reg = <0>;
-			regulator-name = "usb_otg_vbus";
-			regulator-min-microvolt = <5000000>;
-			regulator-max-microvolt = <5000000>;
-			gpio = <&gpio3 22 0>;
-			enable-active-high;
-		};
+	reg_usb_otg_vbus: regulator@0 {
+		compatible = "regulator-fixed";
+		regulator-name = "usb_otg_vbus";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		gpio = <&gpio3 22 0>;
+		enable-active-high;
+	};
 
-		reg_usb_h1_vbus: regulator@1 {
-			compatible = "regulator-fixed";
-			reg = <1>;
-			regulator-name = "usb_h1_vbus";
-			regulator-min-microvolt = <5000000>;
-			regulator-max-microvolt = <5000000>;
-			gpio = <&gpio1 29 0>;
-			enable-active-high;
-		};
+	reg_usb_h1_vbus: regulator@1 {
+		compatible = "regulator-fixed";
+		regulator-name = "usb_h1_vbus";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		gpio = <&gpio1 29 0>;
+		enable-active-high;
+	};
 
-		reg_audio: regulator@2 {
-			compatible = "regulator-fixed";
-			reg = <2>;
-			regulator-name = "wm8962-supply";
-			gpio = <&gpio4 10 0>;
-			enable-active-high;
-		};
+	reg_audio: regulator@2 {
+		compatible = "regulator-fixed";
+		regulator-name = "wm8962-supply";
+		gpio = <&gpio4 10 0>;
+		enable-active-high;
 	};
 
 	gpio-keys {
-- 
1.8.1.2

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] ARM: dts: imx6qdl-sabresd: Do not place regulator nodes under simple-bus
       [not found] ` <1392165078-25794-1-git-send-email-festevam-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2014-02-12  9:22   ` Shawn Guo
       [not found]     ` <20140212092227.GA24834-rvtDTF3kK1ictlrPMvKcciBecyulp+rMXqFh9Ls21Oc@public.gmane.org>
  0 siblings, 1 reply; 8+ messages in thread
From: Shawn Guo @ 2014-02-12  9:22 UTC (permalink / raw)
  To: Fabio Estevam
  Cc: mark.rutland-5wv7dgnIgG8,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Fabio Estevam

On Tue, Feb 11, 2014 at 10:31:18PM -0200, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> 
> According to Documentation/devicetree/bindings/regulator/regulator.txt 
> regulator nodes should not be placed under 'simple-bus'.

I failed to read that statement from the binding doc.  Can you quote the
doc specifically on that statement?

> 
> Mark Rutland also explains about it at:
> http://www.spinics.net/lists/linux-usb/msg101497.html 
>  
> Signed-off-by: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> ---
> Shawn,
> 
> I can convert other dts files if you are fine with this approach.

I take it as an unnecessary churn, unless I see the consensus from most
of DT maintainers and arm-soc folks that we should make this change.
And see comment below ...

> 
>  arch/arm/boot/dts/imx6qdl-sabresd.dtsi | 51 ++++++++++++++--------------------
>  1 file changed, 21 insertions(+), 30 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> index 0d816d3..d7df5b2 100644
> --- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> @@ -18,38 +18,29 @@
>  		reg = <0x10000000 0x40000000>;
>  	};
>  
> -	regulators {
> -		compatible = "simple-bus";
> -		#address-cells = <1>;
> -		#size-cells = <0>;
> -
> -		reg_usb_otg_vbus: regulator@0 {
> -			compatible = "regulator-fixed";
> -			reg = <0>;
> -			regulator-name = "usb_otg_vbus";
> -			regulator-min-microvolt = <5000000>;
> -			regulator-max-microvolt = <5000000>;
> -			gpio = <&gpio3 22 0>;
> -			enable-active-high;
> -		};
> +	reg_usb_otg_vbus: regulator@0 {

nodename@num should only be used for nodes that have 'reg' property.
Why are you dropping 'reg' property here?  Right, it does not compile if
you do not drop it.  You can take it as a reason of why I endorse
simple-bus regulators container.

Shawn

> +		compatible = "regulator-fixed";
> +		regulator-name = "usb_otg_vbus";
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +		gpio = <&gpio3 22 0>;
> +		enable-active-high;
> +	};
>  
> -		reg_usb_h1_vbus: regulator@1 {
> -			compatible = "regulator-fixed";
> -			reg = <1>;
> -			regulator-name = "usb_h1_vbus";
> -			regulator-min-microvolt = <5000000>;
> -			regulator-max-microvolt = <5000000>;
> -			gpio = <&gpio1 29 0>;
> -			enable-active-high;
> -		};
> +	reg_usb_h1_vbus: regulator@1 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "usb_h1_vbus";
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +		gpio = <&gpio1 29 0>;
> +		enable-active-high;
> +	};
>  
> -		reg_audio: regulator@2 {
> -			compatible = "regulator-fixed";
> -			reg = <2>;
> -			regulator-name = "wm8962-supply";
> -			gpio = <&gpio4 10 0>;
> -			enable-active-high;
> -		};
> +	reg_audio: regulator@2 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "wm8962-supply";
> +		gpio = <&gpio4 10 0>;
> +		enable-active-high;
>  	};
>  
>  	gpio-keys {
> -- 
> 1.8.1.2
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ARM: dts: imx6qdl-sabresd: Do not place regulator nodes under simple-bus
       [not found]     ` <20140212092227.GA24834-rvtDTF3kK1ictlrPMvKcciBecyulp+rMXqFh9Ls21Oc@public.gmane.org>
@ 2014-02-12  9:54       ` Mark Rutland
       [not found]         ` <20140212095450.GB21992-NuALmloUBlrZROr8t4l/smS4ubULX0JqMm0uRHvK7Nw@public.gmane.org>
  0 siblings, 1 reply; 8+ messages in thread
From: Mark Rutland @ 2014-02-12  9:54 UTC (permalink / raw)
  To: Shawn Guo
  Cc: Fabio Estevam,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Fabio Estevam

On Wed, Feb 12, 2014 at 09:22:30AM +0000, Shawn Guo wrote:
> On Tue, Feb 11, 2014 at 10:31:18PM -0200, Fabio Estevam wrote:
> > From: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > 
> > According to Documentation/devicetree/bindings/regulator/regulator.txt 
> > regulator nodes should not be placed under 'simple-bus'.
> 
> I failed to read that statement from the binding doc.  Can you quote the
> doc specifically on that statement?

The statement is slightly wrong. Rather, regulators are not required to
be on a simple bus.

In this particular case the simple-bus is being used incorrectly. I
describe some of this in the link Fabio provided immediately below, and
I have further comments specific to this case.

> 
> > 
> > Mark Rutland also explains about it at:
> > http://www.spinics.net/lists/linux-usb/msg101497.html 
> >  
> > Signed-off-by: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > ---
> > Shawn,
> > 
> > I can convert other dts files if you are fine with this approach.
> 
> I take it as an unnecessary churn, unless I see the consensus from most
> of DT maintainers and arm-soc folks that we should make this change.
> And see comment below ...

My concern is that the simple-bus is being used in a nonsensical way,
with a meaningless address space and reg values on children. While this
currently works it is not correct, and it's not even necessary. I would
like to limit the misuse of these constructs so as to prevent others
from making the same mistakes.

> 
> > 
> >  arch/arm/boot/dts/imx6qdl-sabresd.dtsi | 51 ++++++++++++++--------------------
> >  1 file changed, 21 insertions(+), 30 deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> > index 0d816d3..d7df5b2 100644
> > --- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> > +++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> > @@ -18,38 +18,29 @@
> >  		reg = <0x10000000 0x40000000>;
> >  	};
> >  
> > -	regulators {
> > -		compatible = "simple-bus";
> > -		#address-cells = <1>;
> > -		#size-cells = <0>;
> > -
> > -		reg_usb_otg_vbus: regulator@0 {
> > -			compatible = "regulator-fixed";
> > -			reg = <0>;
> > -			regulator-name = "usb_otg_vbus";
> > -			regulator-min-microvolt = <5000000>;
> > -			regulator-max-microvolt = <5000000>;
> > -			gpio = <&gpio3 22 0>;
> > -			enable-active-high;
> > -		};
> > +	reg_usb_otg_vbus: regulator@0 {
> 
> nodename@num should only be used for nodes that have 'reg' property.
> Why are you dropping 'reg' property here?  Right, it does not compile if
> you do not drop it.  You can take it as a reason of why I endorse
> simple-bus regulators container.

I don't think the logical jump from "it does not compile" to "I endorse
simple-bus regulators container" makes any sense. They're two separate
issues.

The unit-addresses and reg values can be dropped. They are not in the
regulator-fixed binding, and were never necessary. All that's necessary
is a unique name, and unit-addresses (which required a reg) were misused
to provide uniqueness. With that fixed, this will compile.

The question to ask is what address space does the simple-bus represent?
The regulators never defined one in their binding, and the simple-bus is
missing a _required_ ranges property, so it's not in the MMIO space of
the parent. Yet somehow it has a single address cell and no size cells.

This is _clearly_ not a simple-bus. Either it needs to map to the parent
address space in some way (which would mean dropping the reg values and
unit addresses as they would be clearly wrong), or it needs to be
replaced by something that isn't a simple-bus. For the latter case that
would mean you couldn't have MMIO regulators in the same container as
fixed regulators, which would be a bit useless.

As far as I can see, a regulator container node is pointless. It
describes no topological information yet pretends to.

Mark.

> 
> Shawn
> 
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "usb_otg_vbus";
> > +		regulator-min-microvolt = <5000000>;
> > +		regulator-max-microvolt = <5000000>;
> > +		gpio = <&gpio3 22 0>;
> > +		enable-active-high;
> > +	};
> >  
> > -		reg_usb_h1_vbus: regulator@1 {
> > -			compatible = "regulator-fixed";
> > -			reg = <1>;
> > -			regulator-name = "usb_h1_vbus";
> > -			regulator-min-microvolt = <5000000>;
> > -			regulator-max-microvolt = <5000000>;
> > -			gpio = <&gpio1 29 0>;
> > -			enable-active-high;
> > -		};
> > +	reg_usb_h1_vbus: regulator@1 {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "usb_h1_vbus";
> > +		regulator-min-microvolt = <5000000>;
> > +		regulator-max-microvolt = <5000000>;
> > +		gpio = <&gpio1 29 0>;
> > +		enable-active-high;
> > +	};
> >  
> > -		reg_audio: regulator@2 {
> > -			compatible = "regulator-fixed";
> > -			reg = <2>;
> > -			regulator-name = "wm8962-supply";
> > -			gpio = <&gpio4 10 0>;
> > -			enable-active-high;
> > -		};
> > +	reg_audio: regulator@2 {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "wm8962-supply";
> > +		gpio = <&gpio4 10 0>;
> > +		enable-active-high;
> >  	};
> >  
> >  	gpio-keys {
> > -- 
> > 1.8.1.2
> > 
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ARM: dts: imx6qdl-sabresd: Do not place regulator nodes under simple-bus
       [not found]         ` <20140212095450.GB21992-NuALmloUBlrZROr8t4l/smS4ubULX0JqMm0uRHvK7Nw@public.gmane.org>
@ 2014-02-12 12:28           ` Shawn Guo
  2014-02-12 13:57             ` Mark Rutland
  0 siblings, 1 reply; 8+ messages in thread
From: Shawn Guo @ 2014-02-12 12:28 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Fabio Estevam,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Fabio Estevam

On Wed, Feb 12, 2014 at 09:54:50AM +0000, Mark Rutland wrote:
> > I take it as an unnecessary churn, unless I see the consensus from most
> > of DT maintainers and arm-soc folks that we should make this change.
> > And see comment below ...
> 
> My concern is that the simple-bus is being used in a nonsensical way,
> with a meaningless address space and reg values on children. While this
> currently works it is not correct, and it's not even necessary. I would
> like to limit the misuse of these constructs so as to prevent others
> from making the same mistakes.

Unfortunately, it's already been used quite widely.  This is not IMX
specific, and you can grep arch/arm/boot/dts to get the idea.  I'm not
sure if it's worth the churn to 'fix' it.  In these days, arm-soc folks
are quite sensitive to the number of IMX patches and change sets.  50
LOC change for one board dts, and we now have ~70 files for IMX.  That's
why I would like to get the agreement from arm-soc folks that we should
really make such change. 

> 
> > 
> > > 
> > >  arch/arm/boot/dts/imx6qdl-sabresd.dtsi | 51 ++++++++++++++--------------------
> > >  1 file changed, 21 insertions(+), 30 deletions(-)
> > > 
> > > diff --git a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> > > index 0d816d3..d7df5b2 100644
> > > --- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> > > +++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> > > @@ -18,38 +18,29 @@
> > >  		reg = <0x10000000 0x40000000>;
> > >  	};
> > >  
> > > -	regulators {
> > > -		compatible = "simple-bus";
> > > -		#address-cells = <1>;
> > > -		#size-cells = <0>;
> > > -
> > > -		reg_usb_otg_vbus: regulator@0 {
> > > -			compatible = "regulator-fixed";
> > > -			reg = <0>;
> > > -			regulator-name = "usb_otg_vbus";
> > > -			regulator-min-microvolt = <5000000>;
> > > -			regulator-max-microvolt = <5000000>;
> > > -			gpio = <&gpio3 22 0>;
> > > -			enable-active-high;
> > > -		};
> > > +	reg_usb_otg_vbus: regulator@0 {
> > 
> > nodename@num should only be used for nodes that have 'reg' property.
> > Why are you dropping 'reg' property here?  Right, it does not compile if
> > you do not drop it.  You can take it as a reason of why I endorse
> > simple-bus regulators container.
> 
> I don't think the logical jump from "it does not compile" to "I endorse
> simple-bus regulators container" makes any sense. They're two separate
> issues.

You're right :)

> 
> The unit-addresses and reg values can be dropped. They are not in the
> regulator-fixed binding, and were never necessary. All that's necessary
> is a unique name, and unit-addresses (which required a reg) were misused
> to provide uniqueness. With that fixed, this will compile.

IIRC, ePAPR recommends to use generic node name and have
unit-addresses for uniqueness.  That's why I sent patch [1] to follow
the way that tegra and other platforms names them.  Now you're
suggesting to go back?

Shawn

[1] http://www.spinics.net/lists/arm-kernel/msg284474.html

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ARM: dts: imx6qdl-sabresd: Do not place regulator nodes under simple-bus
  2014-02-12 12:28           ` Shawn Guo
@ 2014-02-12 13:57             ` Mark Rutland
       [not found]               ` <20140212135736.GB25957-NuALmloUBlrZROr8t4l/smS4ubULX0JqMm0uRHvK7Nw@public.gmane.org>
  0 siblings, 1 reply; 8+ messages in thread
From: Mark Rutland @ 2014-02-12 13:57 UTC (permalink / raw)
  To: Shawn Guo
  Cc: Fabio Estevam, devicetree@vger.kernel.org, Fabio Estevam,
	linux-arm-kernel@lists.infradead.org

On Wed, Feb 12, 2014 at 12:28:39PM +0000, Shawn Guo wrote:
> On Wed, Feb 12, 2014 at 09:54:50AM +0000, Mark Rutland wrote:
> > > I take it as an unnecessary churn, unless I see the consensus from most
> > > of DT maintainers and arm-soc folks that we should make this change.
> > > And see comment below ...
> > 
> > My concern is that the simple-bus is being used in a nonsensical way,
> > with a meaningless address space and reg values on children. While this
> > currently works it is not correct, and it's not even necessary. I would
> > like to limit the misuse of these constructs so as to prevent others
> > from making the same mistakes.
> 
> Unfortunately, it's already been used quite widely.  This is not IMX
> specific, and you can grep arch/arm/boot/dts to get the idea.  I'm not
> sure if it's worth the churn to 'fix' it.  In these days, arm-soc folks
> are quite sensitive to the number of IMX patches and change sets.  50
> LOC change for one board dts, and we now have ~70 files for IMX.  That's
> why I would like to get the agreement from arm-soc folks that we should
> really make such change. 

As it stands, the dts are buggy. I can appreciate that you don't feel
this is important, but I do. It's not just an IMX issue, there is
widespread misunderstanding and abuse of simple-bus.

Said abuse is relying on current Linux implementation details, and that
can and will create problems if and when probing code is changed.
There's no good reason for abusing the binding when we can get it right
today.

We don't necessarily have to get rid of every simple bus in use as a
regulator container. While I don't like that use, I am happy to accept
them as long as they are used correctly, with a valid ranges property
and with children having sensible reg values (or no reg values if the
children don't need them as part of their binding).

> 
> > 
> > > 
> > > > 
> > > >  arch/arm/boot/dts/imx6qdl-sabresd.dtsi | 51 ++++++++++++++--------------------
> > > >  1 file changed, 21 insertions(+), 30 deletions(-)
> > > > 
> > > > diff --git a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> > > > index 0d816d3..d7df5b2 100644
> > > > --- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> > > > +++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> > > > @@ -18,38 +18,29 @@
> > > >  		reg = <0x10000000 0x40000000>;
> > > >  	};
> > > >  
> > > > -	regulators {
> > > > -		compatible = "simple-bus";
> > > > -		#address-cells = <1>;
> > > > -		#size-cells = <0>;
> > > > -
> > > > -		reg_usb_otg_vbus: regulator@0 {
> > > > -			compatible = "regulator-fixed";
> > > > -			reg = <0>;
> > > > -			regulator-name = "usb_otg_vbus";
> > > > -			regulator-min-microvolt = <5000000>;
> > > > -			regulator-max-microvolt = <5000000>;
> > > > -			gpio = <&gpio3 22 0>;
> > > > -			enable-active-high;
> > > > -		};
> > > > +	reg_usb_otg_vbus: regulator@0 {
> > > 
> > > nodename@num should only be used for nodes that have 'reg' property.
> > > Why are you dropping 'reg' property here?  Right, it does not compile if
> > > you do not drop it.  You can take it as a reason of why I endorse
> > > simple-bus regulators container.
> > 
> > I don't think the logical jump from "it does not compile" to "I endorse
> > simple-bus regulators container" makes any sense. They're two separate
> > issues.
> 
> You're right :)
> 
> > 
> > The unit-addresses and reg values can be dropped. They are not in the
> > regulator-fixed binding, and were never necessary. All that's necessary
> > is a unique name, and unit-addresses (which required a reg) were misused
> > to provide uniqueness. With that fixed, this will compile.
> 
> IIRC, ePAPR recommends to use generic node name and have
> unit-addresses for uniqueness.  That's why I sent patch [1] to follow
> the way that tegra and other platforms names them.  Now you're
> suggesting to go back?

As far as I can see, that's not what ePAPR says:

  If the node has no reg property, the @ and unit-address must be
  omitted and the node-name alone differentiates the node from other
  nodes at the same level in the tree.

If a binding doesn't require a reg property, then the node shouldn't
have a reg property. If that's true then the node should not have a
unit-address.

Using generic names can certainly help to make dts more legible, and we
don't have to go against that. It's just as easy to understand
regulator_1 and regulator@1.

I can appreciate that this area may be unclear, and that fixing this up
creates churn for what you feel is a minor issue. However, I don't see
that as a reason to perpetuate wrongness.

Thanks,
Mark.

> 
> Shawn
> 
> [1] http://www.spinics.net/lists/arm-kernel/msg284474.html
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ARM: dts: imx6qdl-sabresd: Do not place regulator nodes under simple-bus
       [not found]               ` <20140212135736.GB25957-NuALmloUBlrZROr8t4l/smS4ubULX0JqMm0uRHvK7Nw@public.gmane.org>
@ 2014-02-12 15:20                 ` Shawn Guo
       [not found]                   ` <20140212152050.GA28749-rvtDTF3kK1ictlrPMvKcciBecyulp+rMXqFh9Ls21Oc@public.gmane.org>
  0 siblings, 1 reply; 8+ messages in thread
From: Shawn Guo @ 2014-02-12 15:20 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Fabio Estevam,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Fabio Estevam

On Wed, Feb 12, 2014 at 01:57:36PM +0000, Mark Rutland wrote:
> As it stands, the dts are buggy. I can appreciate that you don't feel
> this is important, but I do. It's not just an IMX issue, there is
> widespread misunderstanding and abuse of simple-bus.
> 
> Said abuse is relying on current Linux implementation details, and that
> can and will create problems if and when probing code is changed.

The reality is the code already gets no chance to change on this regard,
considering the requirement that we need to maintain the interface
between kernel and DT as ABI.  The dts have been there like this for
10 kernel releases or so.

> There's no good reason for abusing the binding when we can get it right
> today.

As I said from the beginning, though I personally take it as a churn, I
will still be happy take the change, as long as my upstream folks are
also fine with it.

Shawn

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ARM: dts: imx6qdl-sabresd: Do not place regulator nodes under simple-bus
       [not found]                   ` <20140212152050.GA28749-rvtDTF3kK1ictlrPMvKcciBecyulp+rMXqFh9Ls21Oc@public.gmane.org>
@ 2014-02-12 17:22                     ` Fabio Estevam
  2014-02-13  1:59                       ` Shawn Guo
  0 siblings, 1 reply; 8+ messages in thread
From: Fabio Estevam @ 2014-02-12 17:22 UTC (permalink / raw)
  To: Shawn Guo
  Cc: Mark Rutland,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Fabio Estevam

On Wed, Feb 12, 2014 at 1:20 PM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Wed, Feb 12, 2014 at 01:57:36PM +0000, Mark Rutland wrote:
>> As it stands, the dts are buggy. I can appreciate that you don't feel
>> this is important, but I do. It's not just an IMX issue, there is
>> widespread misunderstanding and abuse of simple-bus.
>>
>> Said abuse is relying on current Linux implementation details, and that
>> can and will create problems if and when probing code is changed.
>
> The reality is the code already gets no chance to change on this regard,
> considering the requirement that we need to maintain the interface
> between kernel and DT as ABI.  The dts have been there like this for
> 10 kernel releases or so.

What breakage do you see with this patch?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ARM: dts: imx6qdl-sabresd: Do not place regulator nodes under simple-bus
  2014-02-12 17:22                     ` Fabio Estevam
@ 2014-02-13  1:59                       ` Shawn Guo
  0 siblings, 0 replies; 8+ messages in thread
From: Shawn Guo @ 2014-02-13  1:59 UTC (permalink / raw)
  To: Fabio Estevam
  Cc: Mark Rutland, devicetree@vger.kernel.org, Fabio Estevam,
	linux-arm-kernel@lists.infradead.org

On Wed, Feb 12, 2014 at 03:22:05PM -0200, Fabio Estevam wrote:
> On Wed, Feb 12, 2014 at 1:20 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
> > On Wed, Feb 12, 2014 at 01:57:36PM +0000, Mark Rutland wrote:
> >> As it stands, the dts are buggy. I can appreciate that you don't feel
> >> this is important, but I do. It's not just an IMX issue, there is
> >> widespread misunderstanding and abuse of simple-bus.
> >>
> >> Said abuse is relying on current Linux implementation details, and that
> >> can and will create problems if and when probing code is changed.
> >
> > The reality is the code already gets no chance to change on this regard,
> > considering the requirement that we need to maintain the interface
> > between kernel and DT as ABI.  The dts have been there like this for
> > 10 kernel releases or so.
> 
> What breakage do you see with this patch?

I'm not talking about this patch.  I'm replying to Mark's comment saying
some day the kernel probing code is changed.

Shawn

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2014-02-13  1:59 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-02-12  0:31 [PATCH] ARM: dts: imx6qdl-sabresd: Do not place regulator nodes under simple-bus Fabio Estevam
     [not found] ` <1392165078-25794-1-git-send-email-festevam-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2014-02-12  9:22   ` Shawn Guo
     [not found]     ` <20140212092227.GA24834-rvtDTF3kK1ictlrPMvKcciBecyulp+rMXqFh9Ls21Oc@public.gmane.org>
2014-02-12  9:54       ` Mark Rutland
     [not found]         ` <20140212095450.GB21992-NuALmloUBlrZROr8t4l/smS4ubULX0JqMm0uRHvK7Nw@public.gmane.org>
2014-02-12 12:28           ` Shawn Guo
2014-02-12 13:57             ` Mark Rutland
     [not found]               ` <20140212135736.GB25957-NuALmloUBlrZROr8t4l/smS4ubULX0JqMm0uRHvK7Nw@public.gmane.org>
2014-02-12 15:20                 ` Shawn Guo
     [not found]                   ` <20140212152050.GA28749-rvtDTF3kK1ictlrPMvKcciBecyulp+rMXqFh9Ls21Oc@public.gmane.org>
2014-02-12 17:22                     ` Fabio Estevam
2014-02-13  1:59                       ` Shawn Guo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).