Re: [PATCH 1/4] dt-binding: mfd: Add Richtek RT5120 PMIC support
From: Krzysztof Kozlowski
Date: Tue Jun 07 2022 - 07:53:59 EST
On 07/06/2022 07:52, cy_huang wrote:
> From: ChiYuan Huang <cy_huang@xxxxxxxxxxx>
>
> Add Richtek RT5120 PMIC devicetree document.
>
> Signed-off-by: ChiYuan Huang <cy_huang@xxxxxxxxxxx>
> ---
> .../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@xxxxxxxxxxx>
> +
> +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