| 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
| ||
|
Message-ID: <cfee5b013237dec9e41878f029268510658a4cb1.camel@crapouillou.net>
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