Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <12c375c4-d7d0-7a7b-5688-2210491df8e6@linaro.org>
Date:   Fri, 9 Dec 2022 10:05:29 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.1
Subject: Re: [PATCH v3] dt-bindings: Convert active-semi PMIC docs to YAML
 schemas
Content-Language: en-US
To:     Paul Cercueil <paul@crapouillou.net>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>
Cc:     linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
References: <20221207201341.191682-1-paul@crapouillou.net>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20221207201341.191682-1-paul@crapouillou.net>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@crapouillou.net>
> 
> ---
> 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@crapouillou.net>
> +
> +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.

> +    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@crapouillou.net>
> +
> +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@gmail.com>
> +  - Mark Brown <broonie@kernel.org>
> +
> +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@crapouillou.net>
> +
> +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