Re: [PATCH 03/16] arm64: dts: imx8mm-beacon-som.dtsi: Align regulator names with schema

On Tue, 2020-08-25 at 10:27 +0200, krzk@kernel.org wrote:

On Tue, Aug 25, 2020 at 08:22:18AM +0000, Vaittinen, Matti wrote:

quoted

Hello Krzysztof,

On Tue, 2020-08-25 at 09:50 +0200, krzk@kernel.org wrote:

quoted

On Tue, Aug 25, 2020 at 09:45:00AM +0200, krzk@kernel.org wrote:

quoted

On Tue, Aug 25, 2020 at 09:25:37AM +0200, krzk@kernel.org
wrote:

quoted

On Tue, Aug 25, 2020 at 06:51:33AM +0000, Vaittinen, Matti
wrote:

quoted

Hello Krzysztof,

Just some questions - please ignore if I misunderstood the
impact of
the change.

On Mon, 2020-08-24 at 21:06 +0200, Krzysztof Kozlowski
wrote:

quoted

Device tree schema expects regulator names to be
lowercase.  This
fixes
dtbs_check warnings like:

    arch/arm64/boot/dts/freescale/imx8mn-ddr4-
evk.dt.yaml: 
pmic@4b:
regulators:LDO1:regulator-name:0: 'LDO1' does not match
'^ldo[1-6]$'

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 .../boot/dts/freescale/imx8mn-ddr4-evk.dts    | 22
+++++++++------
----
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/imx8mn-ddr4-

evk.dts
b/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
index a1e5483dbbbe..299caed5d46e 100644

--- a/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts

@@ -60,7 +60,7 @@
 
 		regulators {
 			buck1_reg: BUCK1 {
-				regulator-name = "BUCK1";
+				regulator-name = "buck1";

I am not against this change but I would expect seeing some
other
patches too? I guess this will change the regulator name in
regulator
core, right? So maybe I am mistaken but it looks to me this
change is
visible in suppliers, sysfs and debugfs too? Thus changing
this
sounds
a bit like asking for a nose bleed :) Am I right that the
impact of
this change has been thoroughly tested? Are there any other
patches
(that I have not seen) related to this change?

Oh, crap, the names of regulators in the driver are
lowercase,
but they
use of_match_ptr for upper case. Seriously, why making a
binding
which
is contradictory to the driver implementation on the first
day?

The driver goes with binding, right? One expects uppercase,
other
lowercase...

And tell me, what is now the ABI? The binding or the
incorrect
implementation?

Wait, my mistake. I got confused by my own change. The node
name
stays
the same, so of_match will be correct.

Yes. I think so too. Match will still work as earler.

quoted

quoted

The driver internally already uses lowercase names.

Yep. I was simply thinking that if anyone has been specifying the
regulators as suppliers by name - then this change will change
things
(as is seen for LDO5). Additionally, if any user-space SW has been
reading the regulator states from sysfs - then these names will
also
change the sysfs. Debugfs change is hopefully not such a big deal.

About user-space, I think the embedded DT is not part of kernel ABI,
so
there is no such requirement about keeping it stable. I agree though
it
might be annoying surprise.

Just to ensure we are talking about same thing:
I see you are talking about embedded DT not being an ABI. I agree with
you - DT itself is not ABI. But in case you missed this we have:

static ssize_t name_show(struct device *dev, struct device_attribute
*attr,
			 char *buf)
{
	struct regulator_dev *rdev = dev_get_drvdata(dev);

	return sprintf(buf, "%s\n", rdev_get_name(rdev));
}
static DEVICE_ATTR_RO(name);

in regulator core. I believe the rdev_get_name(rdev) shall change
according to regulator-name. (But as I said, I have no idea if this is
used by user-space on your board - I'll leave this for you & others to
judge).

quoted

Whether this really breaks anything is beyond my knowledge as I
don't
even have this board. Anyways, I think that by minimum the commit
message should point out that this change will be visible outside
DTS
and the BD718x7 driver - up to the user-space.

Good point, I will extend the commit msg about possible impact and
fixing supplies.

Thanks :)

--Matti

--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland
SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~

Simon says - in Latin please.
"non cogito me" dixit Rene Descarte, deinde evanescavit

(Thanks for the translation Simon)