Received-SPF: pass (google.com: domain of linux-kernel+bounces-112307-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <3c0d5aa2-55fb-4827-bcd4-256e6ae55ee9@gmail.com>
Date: Sat, 23 Mar 2024 11:56:20 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 1/5] dt-bindings: mfd: Add rk816 binding
Content-Language: en-US, de-DE
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
 Lee Jones <lee@kernel.org>, Rob Herring <robh@kernel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
 Conor Dooley <conor+dt@kernel.org>, Heiko Stuebner <heiko@sntech.de>,
 Linus Walleij <linus.walleij@linaro.org>, Liam Girdwood
 <lgirdwood@gmail.com>, Mark Brown <broonie@kernel.org>
Cc: Chris Zhong <zyw@rock-chips.com>, Zhang Qing <zhangqing@rock-chips.com>,
 devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
 linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org,
 linux-gpio@vger.kernel.org
References: <20240323085852.116756-1-knaerzche@gmail.com>
 <20240323085852.116756-2-knaerzche@gmail.com>
 <bfe79f3d-1615-45a4-81f7-7e30740308d7@linaro.org>
From: Alex Bee <knaerzche@gmail.com>
In-Reply-To: <bfe79f3d-1615-45a4-81f7-7e30740308d7@linaro.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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
>