Re: [PATCH v2] dt-bindings: Convert active-semi PMIC docs to YAML schemas

[Paul Cercueil <paul@xxxxxxxxxxxxxxx>]

Hi Krzysztof,

Le dim. 6 nov. 2022 à 10:55:11 +0100, Krzysztof Kozlowski 
<krzysztof.kozlowski@xxxxxxxxxx> a écrit :
> On 05/11/2022 23:58, Paul Cercueil wrote:
> >  Create YAML bindings for the Active-semi PMICs and remove the old 
> > text
> >  files.
> >
> >  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>
> >  ---
> >
> >  Notes:
> >      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
> >
> >      Note:
> >      I set Liam Girdwood and Mark Brown as the maintainers by 
> > default, since
> >      it doesn't appear that anybody is managing the Active-semi 
> > drivers, but
> >      if anybody steps up I can update it.
>
> It should not be Liam and Mark, but someone having/knowing this
> particular hardware.

Well, who would that be?

I do have a board with the ACT8600, but that's about it.

-Paul

> >   .../bindings/regulator/act8865-regulator.txt  | 117 --------
> >   .../bindings/regulator/act8945a-regulator.txt | 113 --------
> >   .../regulator/active-semi,act8600.yaml        | 141 ++++++++++
> >   .../regulator/active-semi,act8846.yaml        | 207 ++++++++++++++
> >   .../regulator/active-semi,act8865.yaml        | 162 +++++++++++
> >   .../regulator/active-semi,act8945a.yaml       | 261 
> > ++++++++++++++++++
> >   6 files changed, 771 insertions(+), 230 deletions(-)
> >   delete mode 100644 
> > Documentation/devicetree/bindings/regulator/act8865-regulator.txt
> >   delete mode 100644 
> > Documentation/devicetree/bindings/regulator/act8945a-regulator.txt
> >   create mode 100644 
> > Documentation/devicetree/bindings/regulator/active-semi,act8600.yaml
> >   create mode 100644 
> > Documentation/devicetree/bindings/regulator/active-semi,act8846.yaml
> >   create mode 100644 
> > Documentation/devicetree/bindings/regulator/active-semi,act8865.yaml
> >   create mode 100644 
> > Documentation/devicetree/bindings/regulator/active-semi,act8945a.yaml
> >
> >  diff --git 
> > a/Documentation/devicetree/bindings/regulator/act8865-regulator.txt 
> > b/Documentation/devicetree/bindings/regulator/act8865-regulator.txt
> >  deleted file mode 100644
> >  index b9f58e480349..000000000000
> >  --- 
> > a/Documentation/devicetree/bindings/regulator/act8865-regulator.txt
> >  +++ /dev/null
> >  @@ -1,117 +0,0 @@
> >  -ACT88xx regulators
> >  --------------------
> >  -
> >  -Required properties:
> >  -- compatible: "active-semi,act8846" or "active-semi,act8865" or 
> > "active-semi,act8600"
> >  -- reg: I2C slave address
> >  -
> >  -Optional properties:
> >  -- system-power-controller: Telling whether or not this pmic is 
> > controlling
> >  -  the system power. See 
> > Documentation/devicetree/bindings/power/power-controller.txt .
> >  -- active-semi,vsel-high: Indicates the VSEL pin is high.
> >  -  If this property is missing, assume the VSEL pin is low(0).
> >  -
> >  -Optional input supply properties:
> >  -- for act8600:
> >  -  - vp1-supply: The input supply for DCDC_REG1
> >  -  - vp2-supply: The input supply for DCDC_REG2
> >  -  - vp3-supply: The input supply for DCDC_REG3
> >  -  - inl-supply: The input supply for LDO_REG5, LDO_REG6, LDO_REG7 
> > and LDO_REG8
> >  -  SUDCDC_REG4, LDO_REG9 and LDO_REG10 do not have separate 
> > supplies.
> >  -- for act8846:
> >  -  - vp1-supply: The input supply for REG1
> >  -  - vp2-supply: The input supply for REG2
> >  -  - vp3-supply: The input supply for REG3
> >  -  - vp4-supply: The input supply for REG4
> >  -  - inl1-supply: The input supply for REG5, REG6 and REG7
> >  -  - inl2-supply: The input supply for REG8 and LDO_REG9
> >  -  - inl3-supply: The input supply for REG10, REG11 and REG12
> >  -- for act8865:
> >  -  - vp1-supply: The input supply for DCDC_REG1
> >  -  - vp2-supply: The input supply for DCDC_REG2
> >  -  - vp3-supply: The input supply for DCDC_REG3
> >  -  - inl45-supply: The input supply for LDO_REG1 and LDO_REG2
> >  -  - inl67-supply: The input supply for LDO_REG3 and LDO_REG4
> >  -
> >  -Any standard regulator properties can be used to configure the 
> > single regulator.
> >  -regulator-initial-mode, regulator-allowed-modes and regulator-mode 
> > could be specified
> >  -for act8865 using mode values from 
> > dt-bindings/regulator/active-semi,8865-regulator.h
> >  -file.
> >  -
> >  -The valid names for regulators are:
> >  -	- for act8846:
> >  -	REG1, REG2, REG3, REG4, REG5, REG6, REG7, REG8, REG9, REG10, 
> > REG11, REG12
> >  -	- for act8865:
> >  -	DCDC_REG1, DCDC_REG2, DCDC_REG3, LDO_REG1, LDO_REG2, LDO_REG3, 
> > LDO_REG4.
> >  -	- for act8600:
> >  -	DCDC_REG1, DCDC_REG2, DCDC_REG3, SUDCDC_REG4, LDO_REG5, LDO_REG6, 
> > LDO_REG7,
> >  -	LDO_REG8, LDO_REG9, LDO_REG10,
> >  -
> >  -Example:
> >  ---------
> >  -
> >  -#include <dt-bindings/regulator/active-semi,8865-regulator.h>
> >  -
> >  -		i2c1: i2c@f0018000 {
> >  -			pmic: act8865@5b {
> >  -				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/act8945a-regulator.txt 
> > b/Documentation/devicetree/bindings/regulator/act8945a-regulator.txt
> >  deleted file mode 100644
> >  index 4017527619ab..000000000000
> >  --- 
> > a/Documentation/devicetree/bindings/regulator/act8945a-regulator.txt
> >  +++ /dev/null
> >  @@ -1,113 +0,0 @@
> >  -Device-Tree bindings for regulators of Active-semi ACT8945A 
> > Multi-Function Device
> >  -
> >  -Required properties:
> >  - - compatible: "active-semi,act8945a", please refer to 
> > ../mfd/act8945a.txt.
> >  -
> >  -Optional properties:
> >  -- active-semi,vsel-high: Indicates if the VSEL pin is set to 
> > logic-high.
> >  -  If this property is missing, assume the VSEL pin is set to 
> > logic-low.
> >  -
> >  -Optional input supply properties:
> >  -  - vp1-supply: The input supply for REG_DCDC1
> >  -  - vp2-supply: The input supply for REG_DCDC2
> >  -  - vp3-supply: The input supply for REG_DCDC3
> >  -  - inl45-supply: The input supply for REG_LDO1 and REG_LDO2
> >  -  - inl67-supply: The input supply for REG_LDO3 and REG_LDO4
> >  -
> >  -Any standard regulator properties can be used to configure the 
> > single regulator.
> >  -regulator-initial-mode, regulator-allowed-modes and regulator-mode 
> > could be
> >  -specified using mode values from 
> > dt-bindings/regulator/active-semi,8945a-regulator.h
> >  -file.
> >  -
> >  -The valid names for regulators are:
> >  -	REG_DCDC1, REG_DCDC2, REG_DCDC3, REG_LDO1, REG_LDO2, REG_LDO3, 
> > REG_LDO4.
> >  -
> >  -Example:
> >  -
> >  -#include <dt-bindings/regulator/active-semi,8945a-regulator.h>
> >  -
> >  -	pmic@5b {
> >  -		compatible = "active-semi,act8945a";
> >  -		reg = <0x5b>;
> >  -
> >  -		active-semi,vsel-high;
> >  -
> >  -		regulators {
> >  -			vdd_1v35_reg: REG_DCDC1 {
> >  -				regulator-name = "VDD_1V35";
> >  -				regulator-min-microvolt = <1350000>;
> >  -				regulator-max-microvolt = <1350000>;
> >  -				regulator-always-on;
> >  -
> >  -				regulator-allowed-modes = <ACT8945A_REGULATOR_MODE_FIXED>,
> >  -							  <ACT8945A_REGULATOR_MODE_LOWPOWER>;
> >  -				regulator-initial-mode = <ACT8945A_REGULATOR_MODE_FIXED>;
> >  -
> >  -				regulator-state-mem {
> >  -					regulator-on-in-suspend;
> >  -					regulator-suspend-min-microvolt=<1400000>;
> >  -					regulator-suspend-max-microvolt=<1400000>;
> >  -					regulator-changeable-in-suspend;
> >  -					regulator-mode=<ACT8945A_REGULATOR_MODE_LOWPOWER>;
> >  -				};
> >  -			};
> >  -
> >  -			vdd_1v2_reg: REG_DCDC2 {
> >  -				regulator-name = "VDD_1V2";
> >  -				regulator-min-microvolt = <1100000>;
> >  -				regulator-max-microvolt = <1300000>;
> >  -				regulator-always-on;
> >  -
> >  -				regulator-allowed-modes = <ACT8945A_REGULATOR_MODE_FIXED>,
> >  -							  <ACT8945A_REGULATOR_MODE_LOWPOWER>;
> >  -				regulator-initial-mode = <ACT8945A_REGULATOR_MODE_FIXED>;
> >  -
> >  -				regulator-state-mem {
> >  -					regulator-off-in-suspend;
> >  -				};
> >  -			};
> >  -
> >  -			vdd_3v3_reg: REG_DCDC3 {
> >  -				regulator-name = "VDD_3V3";
> >  -				regulator-min-microvolt = <3300000>;
> >  -				regulator-max-microvolt = <3300000>;
> >  -				regulator-always-on;
> >  -			};
> >  -
> >  -			vdd_fuse_reg: REG_LDO1 {
> >  -				regulator-name = "VDD_FUSE";
> >  -				regulator-min-microvolt = <2500000>;
> >  -				regulator-max-microvolt = <2500000>;
> >  -				regulator-always-on;
> >  -
> >  -				regulator-allowed-modes = <ACT8945A_REGULATOR_MODE_NORMAL>,
> >  -							  <ACT8945A_REGULATOR_MODE_LOWPOWER>;
> >  -				regulator-initial-mode = <ACT8945A_REGULATOR_MODE_NORMAL>;
> >  -
> >  -				regulator-state-mem {
> >  -					regulator-off-in-suspend;
> >  -				};
> >  -			};
> >  -
> >  -			vdd_3v3_lp_reg: REG_LDO2 {
> >  -				regulator-name = "VDD_3V3_LP";
> >  -				regulator-min-microvolt = <3300000>;
> >  -				regulator-max-microvolt = <3300000>;
> >  -				regulator-always-on;
> >  -			};
> >  -
> >  -			vdd_led_reg: REG_LDO3 {
> >  -				regulator-name = "VDD_LED";
> >  -				regulator-min-microvolt = <3300000>;
> >  -				regulator-max-microvolt = <3300000>;
> >  -				regulator-always-on;
> >  -			};
> >  -
> >  -			vdd_sdhc_1v8_reg: REG_LDO4 {
> >  -				regulator-name = "VDD_SDHC_1V8";
> >  -				regulator-min-microvolt = <1800000>;
> >  -				regulator-max-microvolt = <1800000>;
> >  -				regulator-always-on;
> >  -			};
> >  -		};
> >  -	};
> >  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..ca0ac316dff2
> >  --- /dev/null
> >  +++ 
> > b/Documentation/devicetree/bindings/regulator/active-semi,act8600.yaml
> >  @@ -0,0 +1,141 @@
> >  +# 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:
> >  +  - Liam Girdwood <lgirdwood@xxxxxxxxx>
> >  +  - Mark Brown <broonie@xxxxxxxxxx>
> >  +
> >  +properties:
> >  +  compatible:
> >  +    const: active-semi,act8600
> >  +
> >  +  reg:
> >  +    description: I2C address
>
> Drop description, it's obvious. The same in other files.
>
> >  +    maxItems: 1
> >  +
> >  +  system-power-controller:
> >  +    description:
> >  +      Indicates that the ACT8600 is responsible for powering OFF
> >  +      the system.
> >  +    type: boolean
> >  +
>
> (...)
>
> >  +examples:
> >  +  - |
> >  +    #include <dt-bindings/regulator/active-semi,8865-regulator.h>
> >  +
> >  +    i2c1 {
> >  +      #address-cells = <1>;
> >  +      #size-cells = <0>;
> >  +
> >  +      pmic: act8865@5b {
> >  +        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>;
>
> Align it with previous <.
>
> >  +            regulator-initial-mode = 
> > <ACT8865_REGULATOR_MODE_NORMAL>;
>
>
> (...)
>
> >  +
> >  +        charger {
> >  +          compatible = "active-semi,act8945a-charger";
> >  +          pinctrl-names = "default";
> >  +          pinctrl-0 = <&pinctrl_charger_chglev 
> > &pinctrl_charger_lbo &pinctrl_charger_irq>;
> >  +          interrupt-parent = <&pioA>;
> >  +          interrupts = <45 IRQ_TYPE_EDGE_RISING>;
> >  +
> >  +          active-semi,chglev-gpios = <&pioA 12 GPIO_ACTIVE_HIGH>;
> >  +          active-semi,lbo-gpios = <&pioA 72 GPIO_ACTIVE_LOW>;
> >  +          active-semi,input-voltage-threshold-microvolt = <6600>;
> >  +          active-semi,precondition-timeout = <40>;
> >  +          active-semi,total-timeout = <3>;
> >  +          status = "okay";
>
> Drop status
>
> >  +        };
> >  +      };
> >  +    };
>
> Best regards,
> Krzysztof