diff mbox series

[01/16] dt-bindings: mfd: rohm,bd71847-pmic: Correct clock properties requirements

Message ID 20200824190701.8447-1-krzk@kernel.org
State Superseded
Headers show
Series [01/16] dt-bindings: mfd: rohm,bd71847-pmic: Correct clock properties requirements | expand

Checks

Context Check Description
robh/dt-meta-schema success
robh/checkpatch success

Commit Message

Krzysztof Kozlowski Aug. 24, 2020, 7:06 p.m. UTC
The input clock and number of clock provider cells are not required for
the PMIC to operate.  They are needed only for the optional bd718x7
clock driver.

Add also clock-output-names as driver takes use of it.

This fixes dtbs_check warnings like:

    arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dt.yaml: pmic@4b: 'clocks' is a required property
    arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dt.yaml: pmic@4b: '#clock-cells' is a required property

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 .../devicetree/bindings/mfd/rohm,bd71847-pmic.yaml       | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Fabio Estevam Aug. 24, 2020, 8:07 p.m. UTC | #1
Hi Krzysztof,

On Mon, Aug 24, 2020 at 4:07 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:

> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-beacon-baseboard.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-beacon-baseboard.dtsi
> index bf0859f1e1fa..5b5af8b381df 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mm-beacon-baseboard.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mm-beacon-baseboard.dtsi
> @@ -70,7 +70,7 @@
>  &ecspi2 {
>         pinctrl-names = "default";
>         pinctrl-0 = <&pinctrl_espi2>;
> -       cs-gpios = <&gpio5 9 0>;
> +       cs-gpios = <&gpio5 9 GPIO_ACTIVE_LOW>;

I have already sent a patch fixing this:
https://www.spinics.net/lists/arm-kernel/msg830895.html
Vaittinen, Matti Aug. 25, 2020, 6:23 a.m. UTC | #2
Hello Krzysztof,

On Mon, 2020-08-24 at 21:06 +0200, Krzysztof Kozlowski wrote:
> The input clock and number of clock provider cells are not required
> for
> the PMIC to operate.  They are needed only for the optional bd718x7
> clock driver.

I have always found the DT bindings hard to do. I quite often end up
having a different view with Rob so I probably could just shut-up and
watch how this evolves :)

But as keeping my mouth is so difficult...

...All of the drivers are optional. The PMIC can power-on without any
drivers. Drivers are mostly used just for disabling the voltage from
graphics accelerator block when it is not needed (optional). Or some
DVS (optional). But yes, maybe the clk driver is "more optional" than
the rest. XD So, I am not against this.

> Add also clock-output-names as driver takes use of it.
> 
> This fixes dtbs_check warnings like:
> 
>     arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dt.yaml: pmic@4b:
> 'clocks' is a required property
>     arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dt.yaml: pmic@4b:
> '#clock-cells' is a required property
> 
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
>  .../devicetree/bindings/mfd/rohm,bd71847-pmic.yaml       | 9
> +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/rohm,bd71847-
> pmic.yaml b/Documentation/devicetree/bindings/mfd/rohm,bd71847-
> pmic.yaml
> index 77bcca2d414f..5d531051a153 100644
> --- a/Documentation/devicetree/bindings/mfd/rohm,bd71847-pmic.yaml
> +++ b/Documentation/devicetree/bindings/mfd/rohm,bd71847-pmic.yaml
> @@ -38,6 +38,9 @@ properties:
>    "#clock-cells":
>      const: 0
>  
> +  clock-output-names:
> +    maxItems: 1

I had this in original binding (text) document patch series. For some
reason it was later dropped. Unfortunately I didn't easily find a
reason as to why. Adding it back now is absolutely fine for me though.

> +
>  # The BD71847 abd BD71850 support two different HW states as reset
> target
>  # states. States are called as SNVS and READY. At READY state all
> the PMIC
>  # power outputs go down and OTP is reload. At the SNVS state all
> other logic
> @@ -116,12 +119,14 @@ required:
>    - compatible
>    - reg
>    - interrupts
> -  - clocks
> -  - "#clock-cells"
>    - regulators
>  
>  additionalProperties: false
>  
> +dependencies:
> +  '#clock-cells': [clocks]
> +  clocks: ['#clock-cells']

This is new to me. Please educate me - does this simply mean that if
'#clock-cells' is given, then also the 'clocks' must be given - and the
other way around?

If so, then:
Acked-By: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>



--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
K
iviharjunlenkki 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)
Vaittinen, Matti Aug. 25, 2020, 6:51 a.m. UTC | #3
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
Krzysztof Kozlowski Aug. 25, 2020, 6:55 a.m. UTC | #4
On Tue, Aug 25, 2020 at 06:23:36AM +0000, Vaittinen, Matti wrote:
> 
> Hello Krzysztof,
> 
> On Mon, 2020-08-24 at 21:06 +0200, Krzysztof Kozlowski wrote:
> > The input clock and number of clock provider cells are not required
> > for
> > the PMIC to operate.  They are needed only for the optional bd718x7
> > clock driver.
> 
> I have always found the DT bindings hard to do. I quite often end up
> having a different view with Rob so I probably could just shut-up and
> watch how this evolves :)
> 
> But as keeping my mouth is so difficult...
> 
> ...All of the drivers are optional. The PMIC can power-on without any
> drivers. Drivers are mostly used just for disabling the voltage from
> graphics accelerator block when it is not needed (optional). Or some
> DVS (optional). But yes, maybe the clk driver is "more optional" than
> the rest. XD So, I am not against this.

Each regulator node is optional, it can be skipped. And device will
work and regulator driver will bind. The difference here is that without
clocks the clock driver won't even bind... but if we keep clocks as
required, then multiple DTSes do not pass the bindings check.

I don't have strong feelings about dropping requirement for clocks, just
this looks easier to implement and logical to me (this is a PMIC so
clock is a secondary feature).

> 
> > Add also clock-output-names as driver takes use of it.
> > 
> > This fixes dtbs_check warnings like:
> > 
> >     arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dt.yaml: pmic@4b:
> > 'clocks' is a required property
> >     arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dt.yaml: pmic@4b:
> > '#clock-cells' is a required property
> > 
> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> > ---
> >  .../devicetree/bindings/mfd/rohm,bd71847-pmic.yaml       | 9
> > +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/rohm,bd71847-
> > pmic.yaml b/Documentation/devicetree/bindings/mfd/rohm,bd71847-
> > pmic.yaml
> > index 77bcca2d414f..5d531051a153 100644
> > --- a/Documentation/devicetree/bindings/mfd/rohm,bd71847-pmic.yaml
> > +++ b/Documentation/devicetree/bindings/mfd/rohm,bd71847-pmic.yaml
> > @@ -38,6 +38,9 @@ properties:
> >    "#clock-cells":
> >      const: 0
> >  
> > +  clock-output-names:
> > +    maxItems: 1
> 
> I had this in original binding (text) document patch series. For some
> reason it was later dropped. Unfortunately I didn't easily find a
> reason as to why. Adding it back now is absolutely fine for me though.
> 
> > +
> >  # The BD71847 abd BD71850 support two different HW states as reset
> > target
> >  # states. States are called as SNVS and READY. At READY state all
> > the PMIC
> >  # power outputs go down and OTP is reload. At the SNVS state all
> > other logic
> > @@ -116,12 +119,14 @@ required:
> >    - compatible
> >    - reg
> >    - interrupts
> > -  - clocks
> > -  - "#clock-cells"
> >    - regulators
> >  
> >  additionalProperties: false
> >  
> > +dependencies:
> > +  '#clock-cells': [clocks]
> > +  clocks: ['#clock-cells']
> 
> This is new to me. Please educate me - does this simply mean that if
> '#clock-cells' is given, then also the 'clocks' must be given - and the
> other way around?

Yes, because the clocks do not have sense without clock-cells and vice versa.

> 
> If so, then:
> Acked-By: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> 

Thanks.

Best regards,
Krzysztof
Krzysztof Kozlowski Aug. 25, 2020, 7:25 a.m. UTC | #5
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
Krzysztof Kozlowski Aug. 25, 2020, 7:45 a.m. UTC | #6
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
Krzysztof Kozlowski Aug. 25, 2020, 7:50 a.m. UTC | #7
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
Vaittinen, Matti Aug. 25, 2020, 8:22 a.m. UTC | #8
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
Krzysztof Kozlowski Aug. 25, 2020, 8:27 a.m. UTC | #9
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
Krzysztof Kozlowski Aug. 25, 2020, 8:29 a.m. UTC | #10
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
Vaittinen, Matti Aug. 25, 2020, 9:35 a.m. UTC | #11
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 mbox series

Patch

diff --git a/Documentation/devicetree/bindings/mfd/rohm,bd71847-pmic.yaml b/Documentation/devicetree/bindings/mfd/rohm,bd71847-pmic.yaml
index 77bcca2d414f..5d531051a153 100644
--- a/Documentation/devicetree/bindings/mfd/rohm,bd71847-pmic.yaml
+++ b/Documentation/devicetree/bindings/mfd/rohm,bd71847-pmic.yaml
@@ -38,6 +38,9 @@  properties:
   "#clock-cells":
     const: 0
 
+  clock-output-names:
+    maxItems: 1
+
 # The BD71847 abd BD71850 support two different HW states as reset target
 # states. States are called as SNVS and READY. At READY state all the PMIC
 # power outputs go down and OTP is reload. At the SNVS state all other logic
@@ -116,12 +119,14 @@  required:
   - compatible
   - reg
   - interrupts
-  - clocks
-  - "#clock-cells"
   - regulators
 
 additionalProperties: false
 
+dependencies:
+  '#clock-cells': [clocks]
+  clocks: ['#clock-cells']
+
 examples:
   - |
     #include <dt-bindings/interrupt-controller/irq.h>