Re: [PATCH v3] dt-bindings: Convert active-semi PMIC docs to YAML schemas

From: Paul Cercueil
Date: Fri Dec 09 2022 - 05:46:04 EST

Next message: patchwork-bot+netdevbpf: "Re: [PATCH net] net: defxx: Fix missing err handling in dfx_init()"
Previous message: Johan Hovold: "Re: [PATCH v5 12/12] arm64: dts: qcom: sa8295-adp: Enable DP instances"
In reply to: Krzysztof Kozlowski: "Re: [PATCH v3] dt-bindings: Convert active-semi PMIC docs to YAML schemas"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Krzysztof,

Le vendredi 09 décembre 2022 à 10:05 +0100, Krzysztof Kozlowski a
écrit :
> On 07/12/2022 21:13, Paul Cercueil wrote:
> > Create YAML bindings for the Active-semi PMICs and remove the old
> > text
> > files.
>
> Use subject prefixes matching the subsystem (git log --oneline --
> ...),
> so: regulator: dt-bindings: Convert active-semi PMIC to DT schema
>
> >
> > The bindings aren't perfect, for instance I couldn't find good
> > descriptions for the vendor properties in the "charger" node of the
> > ACT8945A because I am not familiar with the hardware and these
> > properties were not documented anywhere.
> >
> > The YAML schemas are a bit different than what is described in the
> > old
> > text files, because these were sometimes wrong or had missing
> > information. This is the case for the ACT8600 documentation, which
> > specified the valid node names for the regulators, while the driver
> > was
> > expecting different names. This led to the current situation where
> > we
> > have two different boards using different names for the regulators:
> > - arch/mips/boot/dts/ingenic/ci20.dts uses the names documented in
> > the
> > text file,
> > - arch/mips/boot/dts/ingenic/gcw0.dts uses the names that the
> > driver
> > expects.
> > In theory, the driver should be fixed to follow the documentation,
> > and
> > accept both naming schemes. In practice though, when the PMIC node
> > was
> > added to the ci20.dts board file, the names were already wrong in
> > regards to what the driver expected, so it never really worked
> > correctly and wasn't tested properly. Furthermore, in that board
> > the
> > consumers of the regulators aren't working for various other
> > reasons
> > (invalid GPIOs, etc.).
> >
> > For that reason, for the ACT8600 bindings I decided to only use the
> > node
> > names that the driver expects (and that gcw0.dts uses), instead of
> > accepting both old and new names. A follow-up patch will update the
> > CI20
> > board to use the new regulator names.
> >
> > Signed-off-by: Paul Cercueil <paul@xxxxxxxxxxxxxxx>
> >
> > ---
> > v2:
> > - Avoid | character in descriptions that can be single-line
> > - Remove unevaluatedProperties when additionalProperties is also
> > present
> > - Remove useless inner parentheses in regular expressions
> > - Rename I2C nodes to just... i2c
> > - Remove node handles
> >
> > v3:
> > - Fix alignment in examples
> > - Drop useless status = "okay"; in examples
> > - I set myself as the maintainer, which I only did because nobody
> > else
> > seems to care.
> >
> > Cheers,
> > -Paul
>
> (...)
>
>
> > diff --git a/Documentation/devicetree/bindings/regulator/active-
> > semi,act8600.yaml
> > b/Documentation/devicetree/bindings/regulator/active-
> > semi,act8600.yaml
> > new file mode 100644
> > index 000000000000..d8cc9cd527ef
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/regulator/active-
> > semi,act8600.yaml
> > @@ -0,0 +1,140 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id:
> > http://devicetree.org/schemas/regulator/active-semi,act8600.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Active-semi ACT8600 regulator
> > +
> > +maintainers:
> > + - Paul Cercueil <paul@xxxxxxxxxxxxxxx>
> > +
> > +properties:
> > + compatible:
> > +    const: active-semi,act8600
> > +
> > + reg:
> > +    description: I2C address
>
> This is a friendly reminder during the review process.
>
> It seems my previous comments were not fully addressed. Maybe my
> feedback got lost between the quotes, maybe you just forgot to apply
> it.
> Please go back to the previous discussion and either implement all
> requested changes or keep discussing them.
>
> Thank you.

Sorry, I moved to a new email client (geary -> evolution) and in-line
responses are harder to see, I missed the previous comment about "reg".

I'll address this in v4 then.

Cheers,
-Paul

> > +    maxItems: 1
> > +
> > + system-power-controller:
> > +    description:
> > +      Indicates that the ACT8600 is responsible for powering OFF
> > +      the system.
> > +    type: boolean
> > +
> > + active-semi,vsel-high:
> > +    description:
> > +      Indicates the VSEL pin is high. If this property is missing,
> > +      the VSEL pin is assumed to be low.
> > +    type: boolean
> > +
> > + regulators:
> > +    type: object
> > +    additionalProperties: false
> > +
> > +    properties:
> > +      DCDC1:
> > +        type: object
> > +        $ref: /schemas/regulator/regulator.yaml#
> > +        unevaluatedProperties: false
> > +
> > +        properties:
> > +          vp1-supply:
> > +            description: Handle to the VP1 input supply
> > +
> > +      DCDC2:
> > +        type: object
> > +        $ref: /schemas/regulator/regulator.yaml#
> > +        unevaluatedProperties: false
> > +
> > +        properties:
> > +          vp2-supply:
> > +            description: Handle to the VP2 input supply
> > +
> > +      DCDC3:
> > +        type: object
> > +        $ref: /schemas/regulator/regulator.yaml#
> > +        unevaluatedProperties: false
> > +
> > +        properties:
> > +          vp3-supply:
> > +            description: Handle to the VP3 input supply
> > +
> > +    patternProperties:
> > +      "^(SUDCDC_REG4|LDO_REG9|LDO_REG10)$":
> > +        type: object
> > +        $ref: /schemas/regulator/regulator.yaml#
> > +        unevaluatedProperties: false
> > +
> > +      "^LDO[5-8]$":
> > +        type: object
> > +        $ref: /schemas/regulator/regulator.yaml#
> > +        unevaluatedProperties: false
> > +
> > +        properties:
> > +          inl-supply:
> > +            description: Handle to the INL input supply
> > +
> > +additionalProperties: false
> > +
> > +required:
> > + - reg
> > + - compatible
> > + - regulators
> > +
> > +examples:
> > + - |
> > +    i2c {
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +
> > +      pmic@5a {
> > +        compatible = "active-semi,act8600";
> > +        reg = <0x5a>;
> > +
> > +        regulators {
> > +          SUDCDC_REG4 {
> > +            regulator-min-microvolt = <5300000>;
> > +            regulator-max-microvolt = <5300000>;
> > +            inl-supply = <&vcc>;
> > +          };
> > +
> > +          LDO5 {
> > +            regulator-min-microvolt = <2500000>;
> > +            regulator-max-microvolt = <2500000>;
> > +            inl-supply = <&vcc>;
> > +          };
> > +
> > +          LDO6 {
> > +            regulator-min-microvolt = <3300000>;
> > +            regulator-max-microvolt = <3300000>;
> > +            inl-supply = <&vcc>;
> > +          };
> > +
> > +          LDO7 {
> > +            regulator-min-microvolt = <3300000>;
> > +            regulator-max-microvolt = <3300000>;
> > +            inl-supply = <&vcc>;
> > +          };
> > +
> > +          LDO8 {
> > +            regulator-min-microvolt = <1800000>;
> > +            regulator-max-microvolt = <1800000>;
> > +            regulator-always-on;
> > +            inl-supply = <&vcc>;
> > +          };
> > +
> > +          LDO_REG9 {
> > +            regulator-min-microvolt = <3300000>;
> > +            regulator-max-microvolt = <3300000>;
> > +            regulator-always-on;
> > +            inl-supply = <&vcc>;
> > +          };
> > +
> > +          LDO_REG10 {
> > +            inl-supply = <&vcc>;
> > +          };
> > +        };
> > +      };
> > +    };
> > diff --git a/Documentation/devicetree/bindings/regulator/active-
> > semi,act8846.yaml
> > b/Documentation/devicetree/bindings/regulator/active-
> > semi,act8846.yaml
> > new file mode 100644
> > index 000000000000..f276dec59b3d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/regulator/active-
> > semi,act8846.yaml
> > @@ -0,0 +1,206 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id:
> > http://devicetree.org/schemas/regulator/active-semi,act8846.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Active-semi ACT8846 regulator
> > +
> > +maintainers:
> > + - Paul Cercueil <paul@xxxxxxxxxxxxxxx>
> > +
> > +properties:
> > + compatible:
> > +    const: active-semi,act8846
> > +
> > + reg:
> > +    description: I2C address
>
> Ditto
>
> > +    maxItems: 1
> > +
> > + system-power-controller:
> > +    description:
> > +      Indicates that the ACT8846 is responsible for powering OFF
> > +      the system.
> > +    type: boolean
> > +
> > + active-semi,vsel-high:
> > +    description:
> > +      Indicates the VSEL pin is high. If this property is missing,
> > +      the VSEL pin is assumed to be low.
> > +    type: boolean
> > +
> > + regulators:
> > +    type: object
> > +    additionalProperties: false
> > +
> > +    properties:
> > +      REG1:
> > +        type: object
> > +        $ref: /schemas/regulator/regulator.yaml#
> > +        unevaluatedProperties: false
> > +
> > +        properties:
> > +          vp1-supply:
> > +            description: Handle to the VP1 input supply
> > +
> > +      REG2:
> > +        type: object
> > +        $ref: /schemas/regulator/regulator.yaml#
> > +        unevaluatedProperties: false
> > +
> > +        properties:
> > +          vp2-supply:
> > +            description: Handle to the VP2 input supply
> > +
> > +      REG3:
> > +        type: object
> > +        $ref: /schemas/regulator/regulator.yaml#
> > +        unevaluatedProperties: false
> > +
> > +        properties:
> > +          vp3-supply:
> > +            description: Handle to the VP3 input supply
> > +
> > +      REG4:
> > +        type: object
> > +        $ref: /schemas/regulator/regulator.yaml#
> > +        unevaluatedProperties: false
> > +
> > +        properties:
> > +          vp4-supply:
> > +            description: Handle to the VP4 input supply
> > +
> > +    patternProperties:
> > +      "^REG[5-7]$":
> > +        type: object
> > +        $ref: /schemas/regulator/regulator.yaml#
> > +        unevaluatedProperties: false
> > +
> > +        properties:
> > +          inl1-supply:
> > +            description: Handle to the INL1 input supply
> > +
> > +      "^REG[8-9]$":
> > +        type: object
> > +        $ref: /schemas/regulator/regulator.yaml#
> > +        unevaluatedProperties: false
> > +
> > +        properties:
> > +          inl2-supply:
> > +            description: Handle to the INL2 input supply
> > +
> > +      "^REG1[0-2]$":
> > +        type: object
> > +        $ref: /schemas/regulator/regulator.yaml#
> > +        unevaluatedProperties: false
> > +
> > +        properties:
> > +          inl3-supply:
> > +            description: Handle to the INL3 input supply
> > +
> > +additionalProperties: false
> > +
> > +required:
> > + - reg
> > + - compatible
> > + - regulators
> > +
> > +examples:
> > + - |
> > +    i2c {
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +
> > +      pmic@5a {
> > +        compatible = "active-semi,act8846";
> > +        reg = <0x5a>;
> > +
> > +        system-power-controller;
> > +
> > +        regulators {
> > +          REG1 {
> > +            regulator-name = "VCC_DDR";
> > +            regulator-min-microvolt = <1200000>;
> > +            regulator-max-microvolt = <1200000>;
> > +            regulator-always-on;
> > +          };
> > +
> > +          REG2 {
> > +            regulator-name = "VCC_IO";
> > +            regulator-min-microvolt = <3300000>;
> > +            regulator-max-microvolt = <3300000>;
> > +            regulator-always-on;
> > +          };
> > +
> > +          REG3 {
> > +            regulator-name = "VDD_LOG";
> > +            regulator-min-microvolt = <1000000>;
> > +            regulator-max-microvolt = <1000000>;
> > +            regulator-always-on;
> > +          };
> > +
> > +          REG4 {
> > +            regulator-name = "VCC_20";
> > +            regulator-min-microvolt = <2000000>;
> > +            regulator-max-microvolt = <2000000>;
> > +            regulator-always-on;
> > +          };
> > +
> > +          REG5 {
> > +            regulator-name = "VCCIO_SD";
> > +            regulator-min-microvolt = <3300000>;
> > +            regulator-max-microvolt = <3300000>;
> > +            regulator-always-on;
> > +          };
> > +
> > +          REG6 {
> > +            regulator-name = "VDD10_LCD";
> > +            regulator-min-microvolt = <1000000>;
> > +            regulator-max-microvolt = <1000000>;
> > +            regulator-always-on;
> > +          };
> > +
> > +          REG7 {
> > +            regulator-name = "VCC_WL";
> > +            regulator-min-microvolt = <3300000>;
> > +            regulator-max-microvolt = <3300000>;
> > +            regulator-always-on;
> > +          };
> > +
> > +          REG8 {
> > +            regulator-name = "VCCA_33";
> > +            regulator-min-microvolt = <3300000>;
> > +            regulator-max-microvolt = <3300000>;
> > +            regulator-always-on;
> > +          };
> > +
> > +          REG9 {
> > +            regulator-name = "VCC_LAN";
> > +            regulator-min-microvolt = <3300000>;
> > +            regulator-max-microvolt = <3300000>;
> > +            regulator-always-on;
> > +          };
> > +
> > +          REG10 {
> > +            regulator-name = "VDD_10";
> > +            regulator-min-microvolt = <1000000>;
> > +            regulator-max-microvolt = <1000000>;
> > +            regulator-always-on;
> > +          };
> > +
> > +          REG11 {
> > +            regulator-name = "VCC_18";
> > +            regulator-min-microvolt = <1800000>;
> > +            regulator-max-microvolt = <1800000>;
> > +            regulator-always-on;
> > +          };
> > +
> > +          REG12 {
> > +            regulator-name = "VCC18_LCD";
> > +            regulator-min-microvolt = <1800000>;
> > +            regulator-max-microvolt = <1800000>;
> > +            regulator-always-on;
> > +          };
> > +        };
> > +      };
> > +    };
> > diff --git a/Documentation/devicetree/bindings/regulator/active-
> > semi,act8865.yaml
> > b/Documentation/devicetree/bindings/regulator/active-
> > semi,act8865.yaml
> > new file mode 100644
> > index 000000000000..cf36ab7c82c4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/regulator/active-
> > semi,act8865.yaml
> > @@ -0,0 +1,162 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id:
> > http://devicetree.org/schemas/regulator/active-semi,act8865.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Active-semi ACT8865 regulator
> > +
> > +maintainers:
> > + - Liam Girdwood <lgirdwood@xxxxxxxxx>
> > + - Mark Brown <broonie@xxxxxxxxxx>
> > +
> > +properties:
> > + compatible:
> > +    const: active-semi,act8865
> > +
> > + reg:
> > +    description: I2C address
>
> ditto
>
> > +    maxItems: 1
> > +
> > + system-power-controller:
> > +    description: |
> > +      Indicates that the ACT8865 is responsible for powering OFF
> > +      the system.
> > +    type: boolean
> > +
> > + active-semi,vsel-high:
> > +    description: |
> > +      Indicates the VSEL pin is high. If this property is missing,
> > +      the VSEL pin is assumed to be low.
> > +    type: boolean
> > +
> > + regulators:
> > +    type: object
> > +    unevaluatedProperties: false
> > +
> > +    properties:
> > +      DCDC_REG1:
> > +        type: object
> > +        $ref: /schemas/regulator/regulator.yaml#
> > +        unevaluatedProperties: false
> > +
> > +        properties:
> > +          vp1-supply:
> > +            description: Handle to the VP1 input supply
> > +
> > +      DCDC_REG2:
> > +        type: object
> > +        $ref: /schemas/regulator/regulator.yaml#
> > +        unevaluatedProperties: false
> > +
> > +        properties:
> > +          vp2-supply:
> > +            description: Handle to the VP2 input supply
> > +
> > +      DCDC_REG3:
> > +        type: object
> > +        $ref: /schemas/regulator/regulator.yaml#
> > +        unevaluatedProperties: false
> > +
> > +        properties:
> > +          vp3-supply:
> > +            description: Handle to the VP3 input supply
> > +
> > +    patternProperties:
> > +      "^LDO_REG[1-2]$":
> > +        type: object
> > +        $ref: /schemas/regulator/regulator.yaml#
> > +        unevaluatedProperties: false
> > +
> > +        properties:
> > +          inl45-supply:
> > +            description: Handle to the INL45 input supply
> > +
> > +      "^LDO_REG[3-4]$":
> > +        type: object
> > +        $ref: /schemas/regulator/regulator.yaml#
> > +        unevaluatedProperties: false
> > +
> > +        properties:
> > +          inl67-supply:
> > +            description: Handle to the INL67 input supply
> > +
> > +    additionalProperties: false
> > +
> > +additionalProperties: false
> > +
> > +required:
> > + - reg
> > + - compatible
> > + - regulators
> > +
> > +examples:
> > + - |
> > +    #include <dt-bindings/regulator/active-semi,8865-regulator.h>
> > +
> > +    i2c1 {
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +
> > +      pmic: act8865@5b {
>
> Node names should be generic.
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
>
> > +        compatible = "active-semi,act8865";
> > +        reg = <0x5b>;
> > +        active-semi,vsel-high;
> > +
> > +        regulators {
> > +          vcc_1v8_reg: DCDC_REG1 {
> > +            regulator-name = "VCC_1V8";
> > +            regulator-min-microvolt = <1800000>;
> > +            regulator-max-microvolt = <1800000>;
> > +            regulator-always-on;
> > +          };
> > +
> > +          vcc_1v2_reg: DCDC_REG2 {
> > +            regulator-name = "VCC_1V2";
> > +            regulator-min-microvolt = <1100000>;
> > +            regulator-max-microvolt = <1300000>;
> > +            regulator-always-on;
> > +
> > +            regulator-allowed-modes =
> > <ACT8865_REGULATOR_MODE_FIXED>,
> > +
> > <ACT8865_REGULATOR_MODE_LOWPOWER>;
> > +            regulator-initial-mode =
> > <ACT8865_REGULATOR_MODE_FIXED>;
> > +
> > +            regulator-state-mem {
> > +              regulator-on-in-suspend;
> > +              regulator-suspend-min-microvolt = <1150000>;
> > +              regulator-suspend-max-microvolt = <1150000>;
> > +              regulator-changeable-in-suspend;
> > +              regulator-mode = <ACT8865_REGULATOR_MODE_LOWPOWER>;
> > +            };
> > +          };
> > +
> > +          vcc_3v3_reg: DCDC_REG3 {
> > +            regulator-name = "VCC_3V3";
> > +            regulator-min-microvolt = <3300000>;
> > +            regulator-max-microvolt = <3300000>;
> > +            regulator-always-on;
> > +          };
> > +
> > +          vddana_reg: LDO_REG1 {
> > +            regulator-name = "VDDANA";
> > +            regulator-min-microvolt = <3300000>;
> > +            regulator-max-microvolt = <3300000>;
> > +            regulator-always-on;
> > +
> > +            regulator-allowed-modes =
> > <ACT8865_REGULATOR_MODE_NORMAL>,
> > +            <ACT8865_REGULATOR_MODE_LOWPOWER>;
> > +            regulator-initial-mode =
> > <ACT8865_REGULATOR_MODE_NORMAL>;
> > +
> > +            regulator-state-mem {
> > +              regulator-off-in-suspend;
> > +            };
> > +          };
> > +
> > +          vddfuse_reg: LDO_REG2 {
> > +            regulator-name = "FUSE_2V5";
> > +            regulator-min-microvolt = <2500000>;
> > +            regulator-max-microvolt = <2500000>;
> > +          };
> > +        };
> > +      };
> > +    };
> > diff --git a/Documentation/devicetree/bindings/regulator/active-
> > semi,act8945a.yaml
> > b/Documentation/devicetree/bindings/regulator/active-
> > semi,act8945a.yaml
> > new file mode 100644
> > index 000000000000..b8c0ba8247ef
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/regulator/active-
> > semi,act8945a.yaml
> > @@ -0,0 +1,259 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id:
> > http://devicetree.org/schemas/regulator/active-semi,act8945a.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Active-semi ACT8945a regulator
> > +
> > +maintainers:
> > + - Paul Cercueil <paul@xxxxxxxxxxxxxxx>
> > +
> > +properties:
> > + compatible:
> > +    const: active-semi,act8945a
> > +
> > + reg:
> > +    description: I2C address
>
> ditto
>
> > +    maxItems: 1
> > +
> > + system-power-controller:
> > +    description:
> > +      Indicates that the ACT8945a is responsible for powering OFF
> > +      the system.
> > +    type: boolean
> > +
>
> Best regards,
> Krzysztof
>

Next message: patchwork-bot+netdevbpf: "Re: [PATCH net] net: defxx: Fix missing err handling in dfx_init()"
Previous message: Johan Hovold: "Re: [PATCH v5 12/12] arm64: dts: qcom: sa8295-adp: Enable DP instances"
In reply to: Krzysztof Kozlowski: "Re: [PATCH v3] dt-bindings: Convert active-semi PMIC docs to YAML schemas"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]