RE: [PATCH v2 1/2] dt-bindings: regulator: add adi,adp5055-regulator

[Torreno, Alexis Czezar]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
> Sent: Thursday, March 20, 2025 5:07 PM
> To: Torreno, Alexis Czezar <AlexisCzezar.Torreno@xxxxxxxxxx>
> Cc: Liam Girdwood <lgirdwood@xxxxxxxxx>; Mark Brown
> <broonie@xxxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>; Krzysztof Kozlowski
> <krzk+dt@xxxxxxxxxx>; Conor Dooley <conor+dt@xxxxxxxxxx>; linux-
> kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 1/2] dt-bindings: regulator: add adi,adp5055-regulator
> 
> [External]
> 
> On Thu, Mar 20, 2025 at 02:53:54PM +0800, Alexis Czezar Torreno wrote:
> > Add documentation for devicetree bindings for ADP5055. The device
> > consists of 3 buck regulators able to connect to high input voltages
> > of up to 18V with no preregulators.
> >
> 
> Please use subject prefixes matching the subsystem. You can get them for
> example with 'git log --oneline -- DIRECTORY_OR_FILE' on the directory your
> patch is touching. For bindings, the preferred subjects are explained here:
> https://urldefense.com/v3/__https://www.kernel.org/doc/html/latest/devic
> etree/bindings/submitting-patches.html*i-for-patch-
> submitters__;Iw!!A3Ni8CS0y2Y!-
> jfDnjTsrIleNV3xmOgakxTfgfPymC_1VWaNuF4unhOr23s35UCRin2d9qUc5Zo
> 4m92ovjLsDFzFBmdfkrzhqg$
> 

Will take note, I guess 'dt_bindings' and 'sub_system' are switched on a few 
subsystem like regulators/etc

> 
> > Signed-off-by: Alexis Czezar Torreno <alexisczezar.torreno@xxxxxxxxxx>
> > ---
> >  .../bindings/regulator/adi,adp5055-regulator.yaml  | 161
> +++++++++++++++++++++
> >  MAINTAINERS                                        |   6 +
> >  2 files changed, 167 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/regulator/adi,adp5055-regulator.ya
> > ml
> > b/Documentation/devicetree/bindings/regulator/adi,adp5055-regulator.ya
> > ml
> > new file mode 100644
> > index
> >
> 0000000000000000000000000000000000000000..fc8f1e61ba321f8b4c6f
> 8c1e3d0e
> > 91d570fb4953
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/regulator/adi,adp5055-regulato
> > +++ r.yaml
> > @@ -0,0 +1,161 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id:
> > +https://urldefense.com/v3/__http://devicetree.org/schemas/regulator/a
> > +di,adp5055-regulator.yaml*__;Iw!!A3Ni8CS0y2Y!-
> jfDnjTsrIleNV3xmOgakxTf
> >
> +gfPymC_1VWaNuF4unhOr23s35UCRin2d9qUc5Zo4m92ovjLsDFzFBmdSUR
> y26w$
> > +$schema:
> > +https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.y
> > +aml*__;Iw!!A3Ni8CS0y2Y!-
> jfDnjTsrIleNV3xmOgakxTfgfPymC_1VWaNuF4unhOr23
> > +s35UCRin2d9qUc5Zo4m92ovjLsDFzFBmdXh_3TTw$
> > +
> > +title: Analog Devices ADP5055 Triple Buck Regulator
> > +
> > +maintainers:
> > +  - Alexis Czezar Torreno <alexisczezar.torreno@xxxxxxxxxx>
> > +
> > +description: |
> > +  The ADP5055 combines three high performance buck regulators.
> > +  The device enables direct connection to high input voltages
> > +  up to 18 V with no preregulators.
> > +
> > +https://www.analog.com/media/en/technical-documentation/data-
> sheets/a
> > +dp5055.pdf
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - adi,adp5055
> > +
> > +  reg:
> > +    enum:
> > +      - 0x70
> > +      - 0x71
> > +
> > +  adi,tset-us:
> > +    description:
> > +      Setting time used by the device. This is changed via soldering
> > +      specific resistor values on the CFG2 pin.
> > +    enum: [2600, 20800]
> > +    default: 2600
> > +
> > +  adi,hw-en-array-gpios:
> 
> Drop prefix, drop "array" and this probably will be changed anyway.

Noted but yes, will probably move/change depending on discussion below
on the gpios/enables

> 
> > +    description:
> > +      Asserted during driver probe. Each array entry acts as the
> 
> s/Asserted during driver probe.//
> If driver moves this code to other place, does it mean bindings are wrong?

Will remove/edit. You're right, it shouldn't imply the bindings are wrong if ever.

> 
> > +      hardware enable for channels 0-2. Should be marked 0 for active
> 
> What does it mean "0" for active low? No, active low has its own flag.
> 
> Use proper flags and implement it properly in the driver.

I was using GPIO_ACTIVE_LOW before just '0' and it was generating errors.
I ended up wording the property this way due to it.

I was only made aware today of my colleagues that I was supposed to place
the flag back before sending my patch as this was more standard. 
The error is due to our setup.
Will correct it and be more careful.

> 
> What is hardware enable and software enable? Is it enable-gpios per
> regulator? Then why this isn't in the regulator node, just like we expect for all
> regulator bindings?

There's a register called CTRL_MODE1 and 2 bits of it configures how "enabling"
works. This is not per regulator but for the whole device
00 - only HW or gpios enables each regulator
01 - only SW ie register writes
10 - both gpio and register are needed to enable
11 - either sw/hw 

We decided to support 00 and 01, but the fact that all regulators are affected
there can't be 1 regulator enabled via software but the others are via gpio.

To handle this, our first idea was to create a gpio array to make sure all three gpios
are declared. If it is not exactly 3 gpios, the driver automatically uses SW enable.

Can move this to each regulator node as enable-gpios.

> 
> 
> > +      low. Requires all three channels to be initialized. Not adding
> > +      the property turns the system to a software only enable mode.
> > +    minItems: 3
> > +    maxItems: 3
> > +
> > +  adi,ocp-blanking:
> > +    description:
> > +      If present, the over current protection
> > +      blanking (OCP) for all channels is on.
> 
> Don't
> wrap
> at
> random
> places, plese.

My bad, edited the first line without considering the next.

> 
> > +    type: boolean
> > +
> > +  adi,delay-power-good:
> > +    description:
> > +      Configures delay timer of the power good (PWRGD) pin.
> > +      Delay is based on Tset which can be 2.6 ms or 20.8 ms.
> > +    type: boolean
> > +
> > +  '#address-cells':
> > +    const: 1
> > +
> > +  '#size-cells':
> > +    const: 0
> > +
> > +patternProperties:
> > +  "^channel@([0-2])$":
> 
> This is a mess... never tested and makes no sense. Either this is a regulator or a
> channel. Looks like regulator, but you called it a channel. If regulator, then
> missing ref to regulator schema.

I approached this as a channel initially but understood later that it should be 
treated as a regulator instead and failed to completely update the doc after. 
Will fix, including other properties that called it channel.

> 
> > +    type: object
> > +    additionalProperties: false
> > +
> > +    properties:
> > +      reg:
> > +        description: The channel number representing each buck converter.
> > +        maximum: 2
> > +
> > +      adi,dvs-limit-upper-microvolt:
> > +        description:
> > +          Configure the allowable upper side limit of the
> > +          voltage output of each channel in microvolt.
> > +          Voltages are in 12mV steps, value is autoadjusted.
> > +          Vout_high = Vout + DVS_upper_limit.
> 
> And how do you configure vout?

Calling this vout seem inaccurate when I read the datasheet again, might change.
But this vout is tied to some feedback resistors (600mV x (1 + Rt/Rb))
I added the equation for context, but the interest is only on DVS_upper_limit

> 
> > +        minimum: 12000
> > +        maximum: 192000
> > +        default: 192000
> > +
> > +      adi,dvs-limit-lower-microvolt:
> > +        description:
> > +          Configure the allowable lower side limit of the
> > +          voltage output of each channel in microvolt.
> > +          Voltages are in 12mV steps, value is autoadjusted.
> > +          Vout_low = Vout + DVS_lower_limit.
> > +        minimum: -190500
> > +        maximum: -10500
> > +        default: -190500
> > +
> > +      adi,fast-transient:
> > +        description:
> > +          Configures the fast transient sensitivity for each channel.
> > +          "none"    - No fast transient.
> > +          "3G_1.5%" - 1.5% window with 3*350uA/V
> > +          "5G_1.5%" - 1.5% window with 5*350uA/V
> > +          "5G_2.5%" - 2.5% window with 5*350uA/V
> > +        enum: [none, 3G_1.5%, 5G_1.5%, 5G_2.5%]
> > +        default: 5G_2.5%
> > +
> > +      adi,mask-power-good:
> > +        description:
> > +          If present, masks individual channels to the external
> > +          PWRGD hardware pin.
> > +        type: boolean
> > +
> > +    required:
> > +      - regulator-name
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        regulator@70 {
> > +            compatible = "adi,adp5055";
> > +            reg = <0x70>;
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +
> > +            adi,tset-us = <2600>;
> > +            adi,hw-en-array-gpios = <&gpio 17 0>,
> > +                                    <&gpio 18 0>,
> > +                                    <&gpio 19 0>;
> 
> No, use proper defines and proper flags.

Yes will fix as discussed above

> 
> > +
> > +            adi,ocp-blanking;
> > +            adi,delay-power-good;
> > +
> > +            DCDC0 {
> 
> Your schema said something else. Test your patches before sending, not
> through our systems.

Apology on this again, I forgot to run the dt_binding_check for v2

> 
> Best regards,
> Krzysztof