Re: [PATCH] arm64: dts: renesas: r8a77995: draak: Fix backlight regulator name

Re: [PATCH] arm64: dts: renesas: r8a77995: draak: Fix backlight regulator name

[Laurent Pinchart]

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>;

-- 
Regards,

Laurent Pinchart

Re: [PATCH] arm64: dts: renesas: r8a77995: draak: Fix backlight regulator name

[Geert Uytterhoeven]

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

Re: [PATCH] arm64: dts: renesas: r8a77995: draak: Fix backlight regulator name

[Rob Herring]

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

Re: [PATCH] arm64: dts: renesas: r8a77995: draak: Fix backlight regulator name

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 438 bytes --]

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 :(

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: dts: renesas: r8a77995: draak: Fix backlight regulator name

[Rob Herring]

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