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 >
