lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Fri, 09 Dec 2022 10:45:51 +0000
From:   Paul Cercueil <paul@...pouillou.net>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>
Cc:     linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v3] dt-bindings: Convert active-semi PMIC docs to YAML
 schemas

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@...pouillou.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@...pouillou.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.

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@...pouillou.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@...il.com>
> > +  - Mark Brown <broonie@...nel.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@...pouillou.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
> 

Powered by blists - more mailing lists