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