Re: [PATCH 1/4] dt-binding: mfd: Add Richtek RT5120 PMIC support

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 07/06/2022 07:52, cy_huang wrote:
> From: ChiYuan Huang <cy_huang@richtek.com>
> 
> Add Richtek RT5120 PMIC devicetree document.
> 
> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> ---
>  .../devicetree/bindings/mfd/richtek,rt5120.yaml    | 180 +++++++++++++++++++++
>  1 file changed, 180 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/richtek,rt5120.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mfd/richtek,rt5120.yaml b/Documentation/devicetree/bindings/mfd/richtek,rt5120.yaml
> new file mode 100644
> index 00000000..376bf73
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/richtek,rt5120.yaml
> @@ -0,0 +1,180 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/richtek,rt5120.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Richtek RT5120 PMIC
> +
> +maintainers:
> +  - ChiYuan Huang <cy_huang@richtek.com>
> +
> +description: |
> +  The RT5120 provides four high-efficiency buck converters and one LDO voltage
> +  regulator. The device is targeted at providingthe processor voltage, memory,
> +  I/O, and peripheral rails in home entertainment devices. The I2C interface is
> +  used for dynamic voltage scaling of the processor voltage, power rails on/off
> +  sequence control, operation mode selection.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - richtek,rt5120
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  interrupt-controller: true
> +
> +  "#interrupt-cells":
> +    const: 1
> +
> +  wakeup-source: true
> +
> +  richtek,enable-undervolt-hiccup:
> +    type: boolean
> +    description: |
> +      If used, under voltage protection trigger hiccup behavior, else latchup as
> +      default
> +
> +  richtek,enable-overvolt-hiccup:
> +    type: boolean
> +    description:
> +      Like as 'enable-uv-hiccup', it configures over voltage protection to
> +      hiccup, else latchup as default
> +
> +  vin1-supply:
> +    description: phandle for buck1 input power source
> +
> +  vin2-supply:
> +    description: phandle for buck2 input power source
> +
> +  vin3-supply:
> +    description: phandle for buck3 input power source
> +
> +  vin4-supply:
> +    description: phandle for buck4 input power source
> +
> +  vinldo-supply:
> +    description: phandle for ldo input power source
> +
> +  regulators:
> +    type: object
> +
> +    patternProperties:
> +      "^buck[1-4]$":
> +        type: object
> +        $ref: /schemas/regulator/regulator.yaml#
> +
> +        properties:
> +          regulator-allowed-modes:
> +            description: |
> +              Used to specify the allowed buck converter operating mode
> +              mode mapping:
> +                0: auto mode
> +                1: force pwm mode
> +            items:
> +              enum: [0, 1]
> +
> +        unevaluatedProperties: false

Better to put it after '$ref' for readability.

> +
> +      "^(ldo|exten)$":
> +        type: object
> +        $ref: /schemas/regulator/regulator.yaml#

You need here unevaluatedProperties:false as well (for the ldo/exten
properties)

> +
> +    additionalProperties: false
> +
> +  powerkey:
> +    type: object
> +    description:
> +      The power key driver may be optional. If not used, change node status to
> +      'disabled'

This description is not helpful, does not describe the hardware. Please
describe hardware, not Devicetree usage.

> +
> +    properties:
> +      compatible:
> +        enum:
> +          - richtek,rt5120-pwrkey
> +
> +    required:
> +      - compatible
> +
> +    additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - '#interrupt-cells'
> +  - interrupt-controller
> +  - regulators
> +  - powerkey

You wrote powerkey is optional... so the node should not be required, right?

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      pmic@62 {
> +        compatible = "richtek,rt5120";
> +        reg = <0x62>;
> +        interrupts-extended = <&gpio_intc 32 IRQ_TYPE_LEVEL_LOW>;
> +        interrupt-controller;
> +        #interrupt-cells = <1>;
> +        wakeup-source;
> +
> +        regulators {
> +          buck1 {
> +            regulator-name = "rt5120-buck1";
> +            regulator-min-microvolt = <600000>;
> +            regulator-max-microvolt = <1393750>;
> +            regulator-allowed-modes = <0 1>;
> +            regulator-boot-on;
> +          };
> +          buck2 {
> +            regulator-name = "rt5120-buck2";
> +            regulator-min-microvolt = <1100000>;
> +            regulator-max-microvolt = <1100000>;
> +            regulator-allowed-modes = <0 1>;
> +            regulator-always-on;
> +          };
> +          buck3 {
> +            regulator-name = "rt5120-buck3";
> +            regulator-min-microvolt = <1800000>;
> +            regulator-max-microvolt = <1800000>;
> +            regulator-allowed-modes = <0 1>;
> +            regulator-always-on;
> +          };
> +          buck4 {
> +            regulator-name = "rt5120-buck4";
> +            regulator-min-microvolt = <3300000>;
> +            regulator-max-microvolt = <3300000>;
> +            regulator-allowed-modes = <0 1>;
> +            regulator-always-on;
> +          };
> +          ldo {
> +            regulator-name = "rt5120-ldo";
> +            regulator-min-microvolt = <1800000>;
> +            regulator-max-microvolt = <1800000>;
> +            regulator-always-on;
> +          };
> +          exten {
> +            regulator-name = "rt5120-exten";
> +            regulator-min-microvolt = <3000000>;
> +            regulator-max-microvolt = <3000000>;
> +            regulator-always-on;
> +          };
> +        };
> +        powerkey {
> +                status = "okay";

Messed up indentation. No need for status in examples.

> +                compatible = "richtek,rt5120-pwrkey";
> +        };
> +      };
> +    };


Best regards,
Krzysztof