diff mbox series

[v2,1/5] dt-bindings: mfd: Add rk816 binding

Message ID 20240323085852.116756-2-knaerzche@gmail.com (mailing list archive)
State New, archived
Headers show
Series Add RK816 PMIC support | expand

Commit Message

Alex Bee March 23, 2024, 8:58 a.m. UTC
Add DT binding document for Rockchip's RK816 PMIC

Signed-off-by: Alex Bee <knaerzche@gmail.com>
---
changes since v1:
  - lowercase/hyphens for regulator node names
  - rename "-reg" to "-regulator" to make node names generic
  - dropped superfluous description for clock-output-names and
    wakeup-source
  - dropped "|" for text blocks that don't require to preserve formatting
  - use full path for `$ref`s
  - added pins description to the binding
  - added charger function to description
 
 .../bindings/mfd/rockchip,rk816.yaml          | 269 ++++++++++++++++++
 1 file changed, 269 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/rockchip,rk816.yaml

Comments

Krzysztof Kozlowski March 23, 2024, 10:32 a.m. UTC | #1
On 23/03/2024 09:58, Alex Bee wrote:
> Add DT binding document for Rockchip's RK816 PMIC
> 
> Signed-off-by: Alex Bee <knaerzche@gmail.com>
> ---
> changes since v1:
>   - lowercase/hyphens for regulator node names
>   - rename "-reg" to "-regulator" to make node names generic

I don't understand why did you do it. I did not ask for it. If you want
to rename, drop redundant regulator or reg suffix from node names.

>   - dropped superfluous description for clock-output-names and
>     wakeup-source
>   - dropped "|" for text blocks that don't require to preserve formatting
>   - use full path for `$ref`s
>   - added pins description to the binding
>   - added charger function to description
>  
>  .../bindings/mfd/rockchip,rk816.yaml          | 269 ++++++++++++++++++
>  1 file changed, 269 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/rockchip,rk816.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mfd/rockchip,rk816.yaml b/Documentation/devicetree/bindings/mfd/rockchip,rk816.yaml
> new file mode 100644
> index 000000000000..9664162f4f75
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/rockchip,rk816.yaml
> @@ -0,0 +1,269 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/rockchip,rk816.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: RK816 Power Management Integrated Circuit
> +
> +maintainers:
> +  - Chris Zhong <zyw@rock-chips.com>
> +  - Zhang Qing <zhangqing@rock-chips.com>
> +
> +description:
> +  Rockchip RK816 series PMIC. This device consists of an i2c controlled MFD
> +  that includes regulators, a RTC, a GPIO controller, a power button, and a
> +  battery charger manager with fuel gauge.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - rockchip,rk816
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  '#clock-cells':
> +    description:
> +      See <dt-bindings/clock/rockchip,rk808.h> for clock IDs.
> +    const: 1
> +
> +  clock-output-names:
> +    maxItems: 2
> +
> +  gpio-controller: true
> +
> +  '#gpio-cells':
> +    const: 2
> +
> +  system-power-controller:
> +    type: boolean
> +    description:
> +      Telling whether or not this PMIC is controlling the system power.
> +
> +  wakeup-source:
> +    type: boolean
> +
> +  vcc1-supply:
> +    description:
> +      The input supply for dcdc1-regulator.
> +
> +  vcc2-supply:
> +    description:
> +      The input supply for dcdc2-regulator.
> +
> +  vcc3-supply:
> +    description:
> +      The input supply for dcdc3-regulator.
> +
> +  vcc4-supply:
> +    description:
> +      The input supply for dcdc4-regulator.
> +
> +  vcc5-supply:
> +    description:
> +      The input supply for ldo1-regulator, ldo2-regulator, and ldo3-regulator.
> +
> +  vcc6-supply:
> +    description:
> +      The input supply for ldo4-regulator, ldo5-regulator, and ldo6-regulator.
> +
> +  vcc7-supply:
> +    description:
> +      The input supply for boost.
> +
> +  vcc8-supply:
> +    description:
> +      The input supply for otg-switch.
> +
> +  regulators:
> +    type: object
> +    patternProperties:
> +      "^(boost|dcdc[1-4]-regulator|ldo[1-6]-regulator|otg-switch)$":
> +        type: object
> +        $ref: /schemas/regulator/regulator.yaml#
> +        unevaluatedProperties: false
> +    unevaluatedProperties: false
> +
> +patternProperties:
> +  '-pins$':

Keep consistent quotes, so either ' or ".

> +    type: object
> +    additionalProperties: false
> +    $ref: /schemas/pinctrl/pinmux-node.yaml
> +
> +    properties:
> +      function:
> +        enum: [pin_fun_gpio, pin_fun_thermistor]

gpio, thermistor
(pin_fun is redundant)

> +
> +      pins:
> +        $ref: /schemas/types.yaml#/definitions/string
> +        const: gpio0
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - "#clock-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/pinctrl/rockchip.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        rk816: pmic@1a {
> +            compatible = "rockchip,rk816";
> +            reg = <0x1a>;
> +            interrupt-parent = <&gpio0>;
> +            interrupts = <RK_PA2 IRQ_TYPE_LEVEL_LOW>;
> +            clock-output-names = "xin32k", "rk816-clkout2";
> +            pinctrl-names = "default";
> +            pinctrl-0 = <&pmic_int_l>;
> +            gpio-controller;
> +            system-power-controller;
> +            wakeup-source;
> +            #clock-cells = <1>;
> +            #gpio-cells = <2>;
> +
> +            vcc1-supply = <&vcc_sys>;
> +            vcc2-supply = <&vcc_sys>;
> +            vcc3-supply = <&vcc_sys>;
> +            vcc4-supply = <&vcc_sys>;
> +            vcc5-supply = <&vcc33_io>;
> +            vcc6-supply = <&vcc_sys>;

Add pins node to the example.

Best regards,
Krzysztof
Alex Bee March 23, 2024, 10:56 a.m. UTC | #2
Hi Krzysztof,

Am 23.03.24 um 11:32 schrieb Krzysztof Kozlowski:
> On 23/03/2024 09:58, Alex Bee wrote:
>> Add DT binding document for Rockchip's RK816 PMIC
>>
>> Signed-off-by: Alex Bee <knaerzche@gmail.com>
>> ---
>> changes since v1:
>>    - lowercase/hyphens for regulator node names
>>    - rename "-reg" to "-regulator" to make node names generic
> I don't understand why did you do it. I did not ask for it. If you want
> to rename, drop redundant regulator or reg suffix from node names.
You didn't ask for that, thats true.

I did it regardless, since node names should be generic and the "-reg"
suffix is not. They are all subnodes of "regulators", indeed, but I don't
think dropping the suffix completely makes the binding any better, since
there is a boost(-switch) and an otg-switch which are also subnodes of
"regulators" and they are just switches.

Alex
>
>>    - dropped superfluous description for clock-output-names and
>>      wakeup-source
>>    - dropped "|" for text blocks that don't require to preserve formatting
>>    - use full path for `$ref`s
>>    - added pins description to the binding
>>    - added charger function to description
>>   
>>   .../bindings/mfd/rockchip,rk816.yaml          | 269 ++++++++++++++++++
>>   1 file changed, 269 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/mfd/rockchip,rk816.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/rockchip,rk816.yaml b/Documentation/devicetree/bindings/mfd/rockchip,rk816.yaml
>> new file mode 100644
>> index 000000000000..9664162f4f75
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/rockchip,rk816.yaml
>> @@ -0,0 +1,269 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/mfd/rockchip,rk816.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: RK816 Power Management Integrated Circuit
>> +
>> +maintainers:
>> +  - Chris Zhong <zyw@rock-chips.com>
>> +  - Zhang Qing <zhangqing@rock-chips.com>
>> +
>> +description:
>> +  Rockchip RK816 series PMIC. This device consists of an i2c controlled MFD
>> +  that includes regulators, a RTC, a GPIO controller, a power button, and a
>> +  battery charger manager with fuel gauge.
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - rockchip,rk816
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  '#clock-cells':
>> +    description:
>> +      See <dt-bindings/clock/rockchip,rk808.h> for clock IDs.
>> +    const: 1
>> +
>> +  clock-output-names:
>> +    maxItems: 2
>> +
>> +  gpio-controller: true
>> +
>> +  '#gpio-cells':
>> +    const: 2
>> +
>> +  system-power-controller:
>> +    type: boolean
>> +    description:
>> +      Telling whether or not this PMIC is controlling the system power.
>> +
>> +  wakeup-source:
>> +    type: boolean
>> +
>> +  vcc1-supply:
>> +    description:
>> +      The input supply for dcdc1-regulator.
>> +
>> +  vcc2-supply:
>> +    description:
>> +      The input supply for dcdc2-regulator.
>> +
>> +  vcc3-supply:
>> +    description:
>> +      The input supply for dcdc3-regulator.
>> +
>> +  vcc4-supply:
>> +    description:
>> +      The input supply for dcdc4-regulator.
>> +
>> +  vcc5-supply:
>> +    description:
>> +      The input supply for ldo1-regulator, ldo2-regulator, and ldo3-regulator.
>> +
>> +  vcc6-supply:
>> +    description:
>> +      The input supply for ldo4-regulator, ldo5-regulator, and ldo6-regulator.
>> +
>> +  vcc7-supply:
>> +    description:
>> +      The input supply for boost.
>> +
>> +  vcc8-supply:
>> +    description:
>> +      The input supply for otg-switch.
>> +
>> +  regulators:
>> +    type: object
>> +    patternProperties:
>> +      "^(boost|dcdc[1-4]-regulator|ldo[1-6]-regulator|otg-switch)$":
>> +        type: object
>> +        $ref: /schemas/regulator/regulator.yaml#
>> +        unevaluatedProperties: false
>> +    unevaluatedProperties: false
>> +
>> +patternProperties:
>> +  '-pins$':
> Keep consistent quotes, so either ' or ".
>
>> +    type: object
>> +    additionalProperties: false
>> +    $ref: /schemas/pinctrl/pinmux-node.yaml
>> +
>> +    properties:
>> +      function:
>> +        enum: [pin_fun_gpio, pin_fun_thermistor]
> gpio, thermistor
> (pin_fun is redundant)
>
>> +
>> +      pins:
>> +        $ref: /schemas/types.yaml#/definitions/string
>> +        const: gpio0
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - "#clock-cells"
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/pinctrl/rockchip.h>
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +    #include <dt-bindings/gpio/gpio.h>
>> +
>> +    i2c {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        rk816: pmic@1a {
>> +            compatible = "rockchip,rk816";
>> +            reg = <0x1a>;
>> +            interrupt-parent = <&gpio0>;
>> +            interrupts = <RK_PA2 IRQ_TYPE_LEVEL_LOW>;
>> +            clock-output-names = "xin32k", "rk816-clkout2";
>> +            pinctrl-names = "default";
>> +            pinctrl-0 = <&pmic_int_l>;
>> +            gpio-controller;
>> +            system-power-controller;
>> +            wakeup-source;
>> +            #clock-cells = <1>;
>> +            #gpio-cells = <2>;
>> +
>> +            vcc1-supply = <&vcc_sys>;
>> +            vcc2-supply = <&vcc_sys>;
>> +            vcc3-supply = <&vcc_sys>;
>> +            vcc4-supply = <&vcc_sys>;
>> +            vcc5-supply = <&vcc33_io>;
>> +            vcc6-supply = <&vcc_sys>;
> Add pins node to the example.
>
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski March 23, 2024, 11:02 a.m. UTC | #3
On 23/03/2024 11:56, Alex Bee wrote:
> Hi Krzysztof,
> 
> Am 23.03.24 um 11:32 schrieb Krzysztof Kozlowski:
>> On 23/03/2024 09:58, Alex Bee wrote:
>>> Add DT binding document for Rockchip's RK816 PMIC
>>>
>>> Signed-off-by: Alex Bee <knaerzche@gmail.com>
>>> ---
>>> changes since v1:
>>>    - lowercase/hyphens for regulator node names
>>>    - rename "-reg" to "-regulator" to make node names generic
>> I don't understand why did you do it. I did not ask for it. If you want
>> to rename, drop redundant regulator or reg suffix from node names.
> You didn't ask for that, thats true.
> 
> I did it regardless, since node names should be generic and the "-reg"

device node names, here you do not have devices.

> suffix is not. They are all subnodes of "regulators", indeed, but I don't
> think dropping the suffix completely makes the binding any better, since
> there is a boost(-switch) and an otg-switch which are also subnodes of
> "regulators" and they are just switches.

Adding "regulator" suffix does not make them more generic... anyway,
that's not expected pattern. First, do you see such code anywhere?
Second, the regulator node names should be usually named as the name of
physical component. That's the most common pattern.

Please do not bring some exceptions from coding style just for your
device. Your device is not special.

You did not respond to rest of my comments, so I assume you agree 100%
with them.

Also, please trim the replies from unneeded context.

Best regards,
Krzysztof
Alex Bee March 23, 2024, 11:15 a.m. UTC | #4
Am 23.03.24 um 12:02 schrieb Krzysztof Kozlowski:
> On 23/03/2024 11:56, Alex Bee wrote:
>> Hi Krzysztof,
>>
>> Am 23.03.24 um 11:32 schrieb Krzysztof Kozlowski:
>>> On 23/03/2024 09:58, Alex Bee wrote:
>>>> Add DT binding document for Rockchip's RK816 PMIC
>>>>
>>>> Signed-off-by: Alex Bee <knaerzche@gmail.com>
>>>> ---
>>>> changes since v1:
>>>>     - lowercase/hyphens for regulator node names
>>>>     - rename "-reg" to "-regulator" to make node names generic
>>> I don't understand why did you do it. I did not ask for it. If you want
>>> to rename, drop redundant regulator or reg suffix from node names.
>> You didn't ask for that, thats true.
>>
>> I did it regardless, since node names should be generic and the "-reg"
> device node names, here you do not have devices.
OK. I wasn't aware there's a difference between device nodes and 
devices. I'll drop the suffix from the regulators.
> You did not respond to rest of my comments, so I assume you agree 100%
> with them.

I do.

Alex.

>
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/mfd/rockchip,rk816.yaml b/Documentation/devicetree/bindings/mfd/rockchip,rk816.yaml
new file mode 100644
index 000000000000..9664162f4f75
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/rockchip,rk816.yaml
@@ -0,0 +1,269 @@ 
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/rockchip,rk816.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: RK816 Power Management Integrated Circuit
+
+maintainers:
+  - Chris Zhong <zyw@rock-chips.com>
+  - Zhang Qing <zhangqing@rock-chips.com>
+
+description:
+  Rockchip RK816 series PMIC. This device consists of an i2c controlled MFD
+  that includes regulators, a RTC, a GPIO controller, a power button, and a
+  battery charger manager with fuel gauge.
+
+properties:
+  compatible:
+    enum:
+      - rockchip,rk816
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  '#clock-cells':
+    description:
+      See <dt-bindings/clock/rockchip,rk808.h> for clock IDs.
+    const: 1
+
+  clock-output-names:
+    maxItems: 2
+
+  gpio-controller: true
+
+  '#gpio-cells':
+    const: 2
+
+  system-power-controller:
+    type: boolean
+    description:
+      Telling whether or not this PMIC is controlling the system power.
+
+  wakeup-source:
+    type: boolean
+
+  vcc1-supply:
+    description:
+      The input supply for dcdc1-regulator.
+
+  vcc2-supply:
+    description:
+      The input supply for dcdc2-regulator.
+
+  vcc3-supply:
+    description:
+      The input supply for dcdc3-regulator.
+
+  vcc4-supply:
+    description:
+      The input supply for dcdc4-regulator.
+
+  vcc5-supply:
+    description:
+      The input supply for ldo1-regulator, ldo2-regulator, and ldo3-regulator.
+
+  vcc6-supply:
+    description:
+      The input supply for ldo4-regulator, ldo5-regulator, and ldo6-regulator.
+
+  vcc7-supply:
+    description:
+      The input supply for boost.
+
+  vcc8-supply:
+    description:
+      The input supply for otg-switch.
+
+  regulators:
+    type: object
+    patternProperties:
+      "^(boost|dcdc[1-4]-regulator|ldo[1-6]-regulator|otg-switch)$":
+        type: object
+        $ref: /schemas/regulator/regulator.yaml#
+        unevaluatedProperties: false
+    unevaluatedProperties: false
+
+patternProperties:
+  '-pins$':
+    type: object
+    additionalProperties: false
+    $ref: /schemas/pinctrl/pinmux-node.yaml
+
+    properties:
+      function:
+        enum: [pin_fun_gpio, pin_fun_thermistor]
+
+      pins:
+        $ref: /schemas/types.yaml#/definitions/string
+        const: gpio0
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - "#clock-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/pinctrl/rockchip.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        rk816: pmic@1a {
+            compatible = "rockchip,rk816";
+            reg = <0x1a>;
+            interrupt-parent = <&gpio0>;
+            interrupts = <RK_PA2 IRQ_TYPE_LEVEL_LOW>;
+            clock-output-names = "xin32k", "rk816-clkout2";
+            pinctrl-names = "default";
+            pinctrl-0 = <&pmic_int_l>;
+            gpio-controller;
+            system-power-controller;
+            wakeup-source;
+            #clock-cells = <1>;
+            #gpio-cells = <2>;
+
+            vcc1-supply = <&vcc_sys>;
+            vcc2-supply = <&vcc_sys>;
+            vcc3-supply = <&vcc_sys>;
+            vcc4-supply = <&vcc_sys>;
+            vcc5-supply = <&vcc33_io>;
+            vcc6-supply = <&vcc_sys>;
+
+            regulators {
+                vdd_cpu: dcdc1-regulator {
+                    regulator-name = "vdd_cpu";
+                    regulator-min-microvolt = <750000>;
+                    regulator-max-microvolt = <1450000>;
+                    regulator-ramp-delay = <6001>;
+                    regulator-initial-mode = <1>;
+                    regulator-always-on;
+                    regulator-boot-on;
+
+                    regulator-state-mem {
+                        regulator-off-in-suspend;
+                    };
+                };
+
+                vdd_logic: dcdc2-regulator {
+                    regulator-name = "vdd_logic";
+                    regulator-min-microvolt = <800000>;
+                    regulator-max-microvolt = <1250000>;
+                    regulator-ramp-delay = <6001>;
+                    regulator-initial-mode = <1>;
+                    regulator-always-on;
+                    regulator-boot-on;
+
+                    regulator-state-mem {
+                        regulator-on-in-suspend;
+                        regulator-suspend-microvolt = <1000000>;
+                    };
+                };
+
+                vcc_ddr: dcdc3-regulator {
+                    regulator-name = "vcc_ddr";
+                    regulator-initial-mode = <1>;
+                    regulator-always-on;
+                    regulator-boot-on;
+
+                    regulator-state-mem {
+                        regulator-on-in-suspend;
+                    };
+                };
+
+                vcc33_io: dcdc4-regulator {
+                    regulator-min-microvolt = <3300000>;
+                    regulator-max-microvolt = <3300000>;
+                    regulator-name = "vcc33_io";
+                    regulator-initial-mode = <1>;
+                    regulator-always-on;
+                    regulator-boot-on;
+
+                    regulator-state-mem {
+                        regulator-on-in-suspend;
+                        regulator-suspend-microvolt = <3300000>;
+                    };
+                };
+
+                vccio_pmu: ldo1-regulator {
+                    regulator-min-microvolt = <3300000>;
+                    regulator-max-microvolt = <3300000>;
+                    regulator-name = "vccio_pmu";
+                    regulator-always-on;
+                    regulator-boot-on;
+
+                    regulator-state-mem {
+                        regulator-on-in-suspend;
+                        regulator-suspend-microvolt = <3300000>;
+                    };
+                };
+
+                vcc_tp: ldo2-regulator {
+                    regulator-min-microvolt = <3300000>;
+                    regulator-max-microvolt = <3300000>;
+                    regulator-name = "vcc_tp";
+
+                    regulator-state-mem {
+                        regulator-off-in-suspend;
+                    };
+                };
+
+                vdd_10: ldo3-regulator {
+                    regulator-min-microvolt = <1000000>;
+                    regulator-max-microvolt = <1000000>;
+                    regulator-name = "vdd_10";
+                    regulator-always-on;
+                    regulator-boot-on;
+
+                    regulator-state-mem {
+                        regulator-on-in-suspend;
+                        regulator-suspend-microvolt = <1000000>;
+                    };
+                };
+
+                vcc18_lcd: ldo4-regulator {
+                    regulator-min-microvolt = <1800000>;
+                    regulator-max-microvolt = <1800000>;
+                    regulator-name = "vcc18_lcd";
+
+                    regulator-state-mem {
+                        regulator-on-in-suspend;
+                        regulator-suspend-microvolt = <1800000>;
+                    };
+                };
+
+                vccio_sd: ldo5-regulator {
+                    regulator-min-microvolt = <1800000>;
+                    regulator-max-microvolt = <3300000>;
+                    regulator-name = "vccio_sd";
+
+                    regulator-state-mem {
+                        regulator-on-in-suspend;
+                        regulator-suspend-microvolt = <3300000>;
+                    };
+                };
+
+                vdd10_lcd: ldo6-regulator {
+                    regulator-min-microvolt = <1000000>;
+                    regulator-max-microvolt = <1000000>;
+                    regulator-name = "vdd10_lcd";
+
+                    regulator-state-mem {
+                        regulator-on-in-suspend;
+                        regulator-suspend-microvolt = <1000000>;
+                    };
+                };
+            };
+        };
+    };