| 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: <12c375c4-d7d0-7a7b-5688-2210491df8e6@linaro.org>
Date: Fri, 9 Dec 2022 10:05:29 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Paul Cercueil <paul@...pouillou.net>,
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
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.
> + 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