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

["krzk@kernel.org" <krzk@kernel.org>]

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@kernel.org wrote:
> > On Tue, Aug 25, 2020 at 09:45:00AM +0200, krzk@kernel.org wrote:
> > > On Tue, Aug 25, 2020 at 09:25:37AM +0200, krzk@kernel.org 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@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.
> 
> > > 
> > > 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.

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

> 
> > > 
> > > Everything looks good. I will just double check whether the
> > > constraints
> > > did not change on the board after boot.
> > 
> > Constraints look good.
> > 
> > > > > >  				regulator-min-microvolt =
> > > > > > <700000>;
> > > > > >  				regulator-max-microvolt =
> > > > > > <1300000>;
> > > > > >  				regulator-boot-on;
> > > > > > @@ -69,7 +69,7 @@
> > > > > >  			};
> > > > > >  
> > > > > >  			buck2_reg: BUCK2 {
> > > > > > -				regulator-name = "BUCK2";
> > > > > > +				regulator-name = "buck2";
> > > > > >  				regulator-min-microvolt =
> > > > > > <700000>;
> > > > > >  				regulator-max-microvolt =
> > > > > > <1300000>;
> > > > > >  				regulator-boot-on;
> > > > > > @@ -79,14 +79,14 @@
> > > > > >  
> > > > > >  			buck3_reg: BUCK3 {
> > > > > >  				// BUCK5 in datasheet
> > > > > > -				regulator-name = "BUCK3";
> > > > > > +				regulator-name = "buck3";
> > > > > >  				regulator-min-microvolt =
> > > > > > <700000>;
> > > > > >  				regulator-max-microvolt =
> > > > > > <1350000>;
> > > > > >  			};
> > > > > >  
> > > > > >  			buck4_reg: BUCK4 {
> > > > > >  				// BUCK6 in datasheet
> > > > > > -				regulator-name = "BUCK4";
> > > > > > +				regulator-name = "buck4";
> > > > > >  				regulator-min-microvolt =
> > > > > > <3000000>;
> > > > > >  				regulator-max-microvolt =
> > > > > > <3300000>;
> > > > > >  				regulator-boot-on;
> > > > > > @@ -95,7 +95,7 @@
> > > > > >  
> > > > > >  			buck5_reg: BUCK5 {
> > > > > >  				// BUCK7 in datasheet
> > > > > > -				regulator-name = "BUCK5";
> > > > > > +				regulator-name = "buck5";
> > > > > 
> > > > > What I see in bd718x7-regulator.c for LDO6 desc is:
> > > > > 
> > > > >                         /* LDO6 is supplied by buck5 */
> > > > >                         .supply_name = "buck5",
> > > > > 
> > > > > So, is this change going to change the supply-chain for the
> > > > > board? Is
> > > > > this intended? (Or am I mistaken on what is the impact of
> > > > > regulator-
> > > > > name property?)
> > > 
> > > Good point, let me check the supplies.
> > 
> > This patch actually fixes the supplies which before were not working
> > because of case mismatch.
> > Before:
> > 
> >  regulator                      use open bypass  opmode voltage
> > current     min     max
> > -------------------------------------------------------------------
> > --------------------
> >  regulator-dummy                  4    5      0
> > unknown     0mV     0mA     0mV     0mV
> >     LDO6                          1    0      0
> > unknown  1200mV     0mA   900mV  1800mV
> >  BUCK1                            1    0      0
> > unknown   850mV     0mA   700mV  1300mV
> >  BUCK2                            2    1      0
> > unknown  1000mV     0mA   700mV  1300mV
> >     cpu0-
> > cpu                      1                                 0mA  1000m
> > V  1000mV
> >  BUCK3                            1    0      0
> > unknown   975mV     0mA   700mV  1350mV
> >  BUCK4                            1    0      0
> > unknown  3300mV     0mA  3000mV  3300mV
> >  BUCK5                            1    0      0
> > unknown  1800mV     0mA  1605mV  1995mV
> >  BUCK6                            1    0      0
> > unknown  1200mV     0mA   800mV  1400mV
> >  LDO1                             1    0      0
> > unknown  1800mV     0mA  1600mV  1900mV
> >  LDO2                             1    0      0
> > unknown   800mV     0mA   800mV   900mV
> >  LDO3                             1    0      0
> > unknown  1800mV     0mA  1800mV  3300mV
> >  LDO4                             1    0      0
> > unknown   900mV     0mA   900mV  1800mV
> >  ldo5                             1    4      0
> > unknown  1800mV     0mA  1800mV  1800mV
> > 
> > 
> > 
> > After:
> >  regulator                      use open bypass  opmode voltage
> > current     min     max
> > -------------------------------------------------------------------
> > --------------------
> > buck1                            1    0      0
> > unknown   850mV     0mA   700mV  1300mV
> >  buck2                            2    1      0
> > unknown   850mV     0mA   700mV  1300mV
> >     cpu0-
> > cpu                      1                                 0mA   850m
> > V   850mV
> >  buck3                            1    0      0
> > unknown   975mV     0mA   700mV  1350mV
> >  buck4                            1    0      0
> > unknown  3300mV     0mA  3000mV  3300mV
> >  buck5                            2    1      0
> > unknown  1800mV     0mA  1605mV  1995mV
> >     ldo6                          1    0      0 
> 
> That was my point :) Before this commit the system has acted
> differently - either by accident or by purpose. In any case, the DTS
> change will change supply logic and this should probably be mentioned
> in commit log to help bisecting possible issues :)
> 
> But as I said, I am not opposed to this change - I am merely somewhat
> cautious with changes like this.

Thanks for the hints.

Best regards,
Krzysztof


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