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

[Vaittinen, Matti]

On Tue, 2020-08-25 at 10:27 +0200, krzk@xxxxxxxxxx wrote:
> On Tue, Aug 25, 2020 at 08:22:18AM +0000, Vaittinen, Matti wrote:
> > Hello Krzysztof,
> > 
> > On Tue, 2020-08-25 at 09:50 +0200, krzk@xxxxxxxxxx wrote:
> > > On Tue, Aug 25, 2020 at 09:45:00AM +0200, krzk@xxxxxxxxxx wrote:
> > > > On Tue, Aug 25, 2020 at 09:25:37AM +0200, krzk@xxxxxxxxxx
> > > > wrote:
> > > > > On Tue, Aug 25, 2020 at 06:51:33AM +0000, Vaittinen, Matti
> > > > > wrote:
> > > > > > 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:
> > > > > > > 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@xxxxxxxxxx>
> > > > > > > ---
> > > > > > >  .../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.
> > 
> > > > 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).

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