diff mbox series

arm64: dts: renesas: r8a77995: draak: Fix backlight regulator name

Message ID 20190731074801.5706-1-geert+renesas@glider.be (mailing list archive)
State Mainlined
Commit 45f5d5a9e34d3fe4140a9a3b5f7ebe86c252440a
Headers show
Series arm64: dts: renesas: r8a77995: draak: Fix backlight regulator name | expand

Commit Message

Geert Uytterhoeven July 31, 2019, 7:48 a.m. UTC
Currently there are two nodes named "regulator1" in the Draak DTS: a
3.3V regulator for the eMMC and the LVDS decoder, and a 12V regulator
for the backlight.  This causes the former to be overwritten by the
latter.

Fix this by renaming all regulators with numerical suffixes to use named
suffixes, which are less likely to conflict.

Fixes: 4fbd4158fe8967e9 ("arm64: dts: renesas: r8a77995: draak: Add backlight")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
I guess this is a fix for v5.3?

This fix takes a slightly different approach than commit
12105cec654cf906 ("arm64: dts: renesas: r8a77990: ebisu: Fix backlight
regulator numbering"), which just fixed the conflicting numerical
suffix.
---
 arch/arm64/boot/dts/renesas/r8a77995-draak.dts | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Laurent Pinchart July 31, 2019, 8:12 a.m. UTC | #1
Hi Geert,

(CC'ing the device tree mailing list)

Thank you for the patch.

On Wed, Jul 31, 2019 at 09:48:01AM +0200, Geert Uytterhoeven wrote:
> Currently there are two nodes named "regulator1" in the Draak DTS: a
> 3.3V regulator for the eMMC and the LVDS decoder, and a 12V regulator
> for the backlight.  This causes the former to be overwritten by the
> latter.
> 
> Fix this by renaming all regulators with numerical suffixes to use named
> suffixes, which are less likely to conflict.

Aren't DT node names supposed to describe the device type, not a
particular instance of the device ? This is something that has bothered
me too, but I believe the naming scheme should be decided globally, not
per board. Is there precedent for using this scheme that has been
explicitly approved by the DT maintainers ?

> Fixes: 4fbd4158fe8967e9 ("arm64: dts: renesas: r8a77995: draak: Add backlight")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> I guess this is a fix for v5.3?
> 
> This fix takes a slightly different approach than commit
> 12105cec654cf906 ("arm64: dts: renesas: r8a77990: ebisu: Fix backlight
> regulator numbering"), which just fixed the conflicting numerical
> suffix.
> ---
>  arch/arm64/boot/dts/renesas/r8a77995-draak.dts | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/renesas/r8a77995-draak.dts b/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
> index 0711170b26b1fe1c..3aa2564dfdc25fff 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
> +++ b/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
> @@ -97,7 +97,7 @@
>  		reg = <0x0 0x48000000 0x0 0x18000000>;
>  	};
>  
> -	reg_1p8v: regulator0 {
> +	reg_1p8v: regulator-1p8v {
>  		compatible = "regulator-fixed";
>  		regulator-name = "fixed-1.8V";
>  		regulator-min-microvolt = <1800000>;
> @@ -106,7 +106,7 @@
>  		regulator-always-on;
>  	};
>  
> -	reg_3p3v: regulator1 {
> +	reg_3p3v: regulator-3p3v {
>  		compatible = "regulator-fixed";
>  		regulator-name = "fixed-3.3V";
>  		regulator-min-microvolt = <3300000>;
> @@ -115,7 +115,7 @@
>  		regulator-always-on;
>  	};
>  
> -	reg_12p0v: regulator1 {
> +	reg_12p0v: regulator-12p0v {
>  		compatible = "regulator-fixed";
>  		regulator-name = "D12.0V";
>  		regulator-min-microvolt = <12000000>;
Geert Uytterhoeven July 31, 2019, 8:32 a.m. UTC | #2
Hi Laurent,

On Wed, Jul 31, 2019 at 10:12 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Wed, Jul 31, 2019 at 09:48:01AM +0200, Geert Uytterhoeven wrote:
> > Currently there are two nodes named "regulator1" in the Draak DTS: a
> > 3.3V regulator for the eMMC and the LVDS decoder, and a 12V regulator
> > for the backlight.  This causes the former to be overwritten by the
> > latter.
> >
> > Fix this by renaming all regulators with numerical suffixes to use named
> > suffixes, which are less likely to conflict.
>
> Aren't DT node names supposed to describe the device type, not a
> particular instance of the device ? This is something that has bothered
> me too, but I believe the naming scheme should be decided globally, not
> per board. Is there precedent for using this scheme that has been
> explicitly approved by the DT maintainers ?

The example in Documentation/devicetree/bindings/regulator/regulator.yaml
uses "regulator@0", which of course works only if #address-cells = 1, which
is usually not the case for discrete regulators.
BTW, the example lacks a "reg" property...

So some other suffix has to be added to distinguish individual "regulator"
nodes.

The example in Documentation/devicetree/bindings/regulator/fixed-regulator.yaml
uses "regulator-1v8" since commit b735f41dcb06ae06 ("dt-bindings: regulator:
update fixed-regulator example"), which received a Reviewed-by from Rob
after it was committed.
https://lore.kernel.org/lkml/CAL_Jsq+rRYazOqtjNms0cTK0HpkxCkmZ4JXoLM7ZaPivATEO8A@mail.gmail.com/

Looks good enough to me ;-)

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Rob Herring (Arm) July 31, 2019, 2:47 p.m. UTC | #3
On Wed, Jul 31, 2019 at 2:32 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Laurent,
>
> On Wed, Jul 31, 2019 at 10:12 AM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> > On Wed, Jul 31, 2019 at 09:48:01AM +0200, Geert Uytterhoeven wrote:
> > > Currently there are two nodes named "regulator1" in the Draak DTS: a
> > > 3.3V regulator for the eMMC and the LVDS decoder, and a 12V regulator
> > > for the backlight.  This causes the former to be overwritten by the
> > > latter.
> > >
> > > Fix this by renaming all regulators with numerical suffixes to use named
> > > suffixes, which are less likely to conflict.
> >
> > Aren't DT node names supposed to describe the device type, not a
> > particular instance of the device ? This is something that has bothered
> > me too, but I believe the naming scheme should be decided globally, not
> > per board. Is there precedent for using this scheme that has been
> > explicitly approved by the DT maintainers ?
>
> The example in Documentation/devicetree/bindings/regulator/regulator.yaml
> uses "regulator@0", which of course works only if #address-cells = 1, which
> is usually not the case for discrete regulators.
> BTW, the example lacks a "reg" property...

Yeah, that predates our being strict about unit-addresses.

> So some other suffix has to be added to distinguish individual "regulator"
> nodes.

<nodename>-<identifier> is basically the format we've been following
for cases without an address.

As long as we have a consistent base name that we can match schema
with, then I'm happy. But for regulators, we have a lot of node names
like 'buck1', 'LDO2', etc.

Rob
Mark Brown July 31, 2019, 3:09 p.m. UTC | #4
On Wed, Jul 31, 2019 at 08:47:38AM -0600, Rob Herring wrote:

> As long as we have a consistent base name that we can match schema
> with, then I'm happy. But for regulators, we have a lot of node names
> like 'buck1', 'LDO2', etc.

Those are all types of regulator (LDOs and DCDCs are the main types of
voltage regulator, and buck is another term for DCDC).

I'm still not clear what meaningful effect any of this node name stuff
has :(
Rob Herring (Arm) July 31, 2019, 4:40 p.m. UTC | #5
On Wed, Jul 31, 2019 at 9:09 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, Jul 31, 2019 at 08:47:38AM -0600, Rob Herring wrote:
>
> > As long as we have a consistent base name that we can match schema
> > with, then I'm happy. But for regulators, we have a lot of node names
> > like 'buck1', 'LDO2', etc.
>
> Those are all types of regulator (LDOs and DCDCs are the main types of
> voltage regulator, and buck is another term for DCDC).

Yes, I know.

> I'm still not clear what meaningful effect any of this node name stuff
> has :(

It is primarily just what I said. Standard names or patterns allow for
applying schemas. Otherwise, we only have schema checks when we have a
device specific schema. Of course, we do have those too, but generic
ones are useful when we don't. If there are errors in the DT causing
the device specific schema to not match (say a typo in the compatible
string), we still have some checking.

Rob
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/renesas/r8a77995-draak.dts b/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
index 0711170b26b1fe1c..3aa2564dfdc25fff 100644
--- a/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
+++ b/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
@@ -97,7 +97,7 @@ 
 		reg = <0x0 0x48000000 0x0 0x18000000>;
 	};
 
-	reg_1p8v: regulator0 {
+	reg_1p8v: regulator-1p8v {
 		compatible = "regulator-fixed";
 		regulator-name = "fixed-1.8V";
 		regulator-min-microvolt = <1800000>;
@@ -106,7 +106,7 @@ 
 		regulator-always-on;
 	};
 
-	reg_3p3v: regulator1 {
+	reg_3p3v: regulator-3p3v {
 		compatible = "regulator-fixed";
 		regulator-name = "fixed-3.3V";
 		regulator-min-microvolt = <3300000>;
@@ -115,7 +115,7 @@ 
 		regulator-always-on;
 	};
 
-	reg_12p0v: regulator1 {
+	reg_12p0v: regulator-12p0v {
 		compatible = "regulator-fixed";
 		regulator-name = "D12.0V";
 		regulator-min-microvolt = <12000000>;