| Message ID | 20200824190701.8447-3-krzk@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/16] dt-bindings: mfd: rohm, bd71847-pmic: Correct clock properties requirements | expand |
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? > 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?) Best Regards Matti Vaittinen
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? > > > 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?) The names will take regulator names from the driver. The problem is with matching the of_node. Dear Rob, Maybe you have an idea how to fix this driver-binding ABI incompatibility? Or better just leave it? Best regards, Krzysztof
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. > > > > > > 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. > > The names will take regulator names from the driver. The problem is with > matching the of_node. > > > Dear Rob, > > Maybe you have an idea how to fix this driver-binding ABI > incompatibility? Or better just leave it? Not valid anymore, I just got confused... Best regards, Krzysztof
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
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. 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. > > > > 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. Best regards Matti Vaittinen
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
On Mon, Aug 24, 2020 at 09:06:48PM +0200, Krzysztof Kozlowski wrote: > Device tree schema expects regulator names to be lowercase. This fixes > dtbs_check warnings like: The subject here is not correct. Copy/paste error. I'll fix it up. Best regards, Krzysztof
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: > > 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. 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)
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"; 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"; regulator-min-microvolt = <1605000>; regulator-max-microvolt = <1995000>; regulator-boot-on; @@ -104,7 +104,7 @@ buck6_reg: BUCK6 { // BUCK8 in datasheet - regulator-name = "BUCK6"; + regulator-name = "buck6"; regulator-min-microvolt = <800000>; regulator-max-microvolt = <1400000>; regulator-boot-on; @@ -112,7 +112,7 @@ }; ldo1_reg: LDO1 { - regulator-name = "LDO1"; + regulator-name = "ldo1"; regulator-min-microvolt = <1600000>; regulator-max-microvolt = <3300000>; regulator-boot-on; @@ -120,7 +120,7 @@ }; ldo2_reg: LDO2 { - regulator-name = "LDO2"; + regulator-name = "ldo2"; regulator-min-microvolt = <800000>; regulator-max-microvolt = <900000>; regulator-boot-on; @@ -128,7 +128,7 @@ }; ldo3_reg: LDO3 { - regulator-name = "LDO3"; + regulator-name = "ldo3"; regulator-min-microvolt = <1800000>; regulator-max-microvolt = <3300000>; regulator-boot-on; @@ -136,7 +136,7 @@ }; ldo4_reg: LDO4 { - regulator-name = "LDO4"; + regulator-name = "ldo4"; regulator-min-microvolt = <900000>; regulator-max-microvolt = <1800000>; regulator-boot-on; @@ -144,7 +144,7 @@ }; ldo6_reg: LDO6 { - regulator-name = "LDO6"; + regulator-name = "ldo6"; regulator-min-microvolt = <900000>; regulator-max-microvolt = <1800000>; regulator-boot-on;
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(-)