Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 25 Aug 2020 09:50:38 +0200
From:   "krzk@kernel.org" <krzk@kernel.org>
To:     "Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>
Cc:     "miquel.raynal@bootlin.com" <miquel.raynal@bootlin.com>,
        "linux-imx@nxp.com" <linux-imx@nxp.com>,
        "han.xu@nxp.com" <han.xu@nxp.com>,
        "Anson.Huang@nxp.com" <Anson.Huang@nxp.com>,
        "yibin.gong@nxp.com" <yibin.gong@nxp.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "shawnguo@kernel.org" <shawnguo@kernel.org>,
        "aford173@gmail.com" <aford173@gmail.com>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "kernel@pengutronix.de" <kernel@pengutronix.de>,
        "s.hauer@pengutronix.de" <s.hauer@pengutronix.de>,
        "richard@nod.at" <richard@nod.at>,
        "linux-mtd@lists.infradead.org" <linux-mtd@lists.infradead.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "daniel.baluta@nxp.com" <daniel.baluta@nxp.com>,
        "lee.jones@linaro.org" <lee.jones@linaro.org>,
        "festevam@gmail.com" <festevam@gmail.com>,
        "vigneshr@ti.com" <vigneshr@ti.com>,
        "jun.li@nxp.com" <jun.li@nxp.com>
Subject: Re: [PATCH 03/16] arm64: dts: imx8mm-beacon-som.dtsi: Align
 regulator names with schema
Message-ID: <20200825075038.GA10148@kozik-lap>
References: <20200824190701.8447-1-krzk@kernel.org>
 <20200824190701.8447-3-krzk@kernel.org>
 <fa042a4f670775f340e88fca8f363252112fd538.camel@fi.rohmeurope.com>
 <20200825072537.GA5914@kozik-lap>
 <20200825074500.GA19323@kozik-lap>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20200825074500.GA19323@kozik-lap>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.
> 
> The driver internally already uses lowercase names.
> 
> 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  1000mV  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   850mV   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 unknown  1200mV     0mA   900mV  1800mV
 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                             0    0      0 unknown  3300mV     0mA     0mV     0mV

Best regards,
Krzysztof