Re: [PATCH 10/10] dt-bindings: Add documentation for rt5033 mfd, regulator and charger

[Jakob Hauser <jahau@rocketmail.com>]

Hi Rob,

thanks for the remarks and sorry for not running 'make 
dt_binding_check'. I'm not familiar with devicetree bindings and 
obviously missed to read the file 
Documentation/devicetree/bindings/submitting-patches.rst.

On 01.03.23 03:35, Rob Herring wrote:
> On Tue, Feb 28, 2023 at 11:32:27PM +0100, Jakob Hauser wrote:
>> Add device tree binding documentation for rt5033 multifunction device, voltage
>> regulator and battery charger.
>>
>> Cc: Beomho Seo <beomho.seo@samsung.com>
>> Cc: Chanwoo Choi <cw00.choi@samsung.com>
>> Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
>> ---
>>   .../bindings/mfd/richtek,rt5033.yaml          | 102 ++++++++++++++++++
>>   .../power/supply/richtek,rt5033-charger.yaml  |  76 +++++++++++++
>>   .../regulator/richtek,rt5033-regulator.yaml   |  45 ++++++++
>>   3 files changed, 223 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/mfd/richtek,rt5033.yaml
>>   create mode 100644 Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml
>>   create mode 100644 Documentation/devicetree/bindings/regulator/richtek,rt5033-regulator.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/richtek,rt5033.yaml b/Documentation/devicetree/bindings/mfd/richtek,rt5033.yaml
>> new file mode 100644
>> index 000000000000..f1a58694c81e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/richtek,rt5033.yaml
>> @@ -0,0 +1,102 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/mfd/richtek,rt5033.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Richtek RT5033 Power Management Integrated Circuit
>> +
>> +maintainers:
>> +  - Jakob Hauser <jahau@rocketmail.com>
>> +
>> +description: |
> 
> Don't need '|' unless you care about line endings.

OK

>> +  RT5033 is a multifunction device which includes battery charger, fuel gauge,
>> +  flash LED current source, LDO and synchronous Buck converter for portable
>> +  applications. It is interfaced to host controller using I2C interface. The
>> +  battery fuel gauge uses a separate I2C bus.
>> +
>> +properties:
>> +  compatible:
>> +    const: richtek,rt5033
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  regulators:
>> +    type: object
>> +    $ref: /schemas/regulator/richtek,rt5033-regulator.yaml#
>> +
>> +  charger:
>> +    type: object
>> +    $ref: /schemas/power/supply/richtek,rt5033-charger.yaml#
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +
>> +    i2c@0 {
> 
> i2c {

OK

>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        pmic@34 {
>> +            compatible = "richtek,rt5033";
>> +            reg = <0x34>;
>> +
>> +            interrupt-parent = <&msmgpio>;
>> +            interrupts = <62 IRQ_TYPE_EDGE_FALLING>;
>> +
>> +            pinctrl-names = "default";
>> +            pinctrl-0 = <&pmic_int_default>;
>> +
>> +            regulators {
>> +                safe_ldo_reg: SAFE_LDO {
>> +                    regulator-name = "SAFE_LDO";
>> +                    regulator-min-microvolt = <4900000>;
>> +                    regulator-max-microvolt = <4900000>;
>> +                    regulator-always-on;
>> +                };
>> +                ldo_reg: LDO {
>> +                    regulator-name = "LDO";
>> +                    regulator-min-microvolt = <2800000>;
>> +                    regulator-max-microvolt = <2800000>;
>> +                };
>> +                buck_reg: BUCK {
>> +                    regulator-name = "BUCK";
>> +                    regulator-min-microvolt = <1200000>;
>> +                    regulator-max-microvolt = <1200000>;
>> +                };
>> +            };
>> +
>> +            charger {
>> +                compatible = "richtek,rt5033-charger";
>> +                richtek,pre-uamp = <450000>;
>> +                richtek,fast-uamp = <1000000>;
>> +                richtek,eoc-uamp = <150000>;
>> +                richtek,pre-threshold-uvolt = <3500000>;
>> +                richtek,const-uvolt = <4350000>;
>> +                extcon = <&muic>;
>> +            };
>> +        };
>> +    };
>> +
>> +    i2c@1 {
> 
> This should be a separate example entry.

I'll skip it then.

The battery fuel gauge is not handled as a part of the MFD, it has a 
separate I2C line. Accordingly, it has a separate documentation 
including examples [1].

I had implemented into the MFD example to make clear this is separated. 
But as it is not part of the MFD, I guess it shouldn't show up in the 
MFD documentation.

In the description of the MFD there is the statement "The battery fuel 
gauge uses a separate I2C bus." I hope this is clear enough, I'm not 
sure if I should modify/extent that statement or add some kind of 
reference to the battery fuel gauge after removing it from the example.

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/power/supply/richtek,rt5033-battery.yaml?h=v6.2

>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        battery@35 {
>> +            compatible = "richtek,rt5033-battery";
>> +            reg = <0x35>;
>> +            interrupt-parent = <&msmgpio>;
>> +            interrupts = <121 IRQ_TYPE_EDGE_FALLING>;
>> +        };
>> +    };
>> diff --git a/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml b/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml
>> new file mode 100644
>> index 000000000000..996c2932927d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml
>> @@ -0,0 +1,76 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/power/supply/richtek,rt5033-charger.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Richtek RT5033 PIMC Battery Charger
>> +
>> +maintainers:
>> +  - Jakob Hauser <jahau@rocketmail.com>
>> +
>> +description: |
>> +  The battery charger of the multifunction device RT5033 has to be instantiated
>> +  under sub-node named "charger" using the following format.
>> +
>> +properties:
>> +  compatible:
>> +    const: richtek,rt5033-charger
>> +
>> +  richtek,pre-uamp:
> 
> Use defined standard unit type suffixes.

That makes sense. I took the current names from the 2015 patchset and 
wasn't aware of the standard suffixes.

Just for the record: Chaning the names will also impact patch 06 "power: 
supply: rt5033_charger: Add RT5033 charger device driver", as the names 
are parsed there.

>> +    description: |
>> +      Current of pre-charge mode. The pre-charge current levels are 350 mA to
>> +      650 mA programmed by I2C per 100 mA.
>> +    maxItems: 1
>> +
>> +  richtek,fast-uamp:
>> +    description: |
>> +      Current of fast-charge mode. The fast-charge current levels are 700 mA
>> +      to 2000 mA programmed by I2C per 100 mA.
>> +    maxItems: 1
>> +
>> +  richtek,eoc-uamp:
>> +    description: |
>> +      This property is end of charge current. Its level ranges from 150 mA to
>> +      600 mA. Between 150 mA and 300 mA in 50 mA steps, between 300 mA and 600 mA
>> +      in 100 mA steps.
>> +    maxItems: 1
>> +
>> +  richtek,pre-threshold-uvolt:
>> +    description: |
>> +      Voltage of pre-charge mode. If the battery voltage is below the pre-charge
>> +      threshold voltage, the charger is in pre-charge mode with pre-charge current.
>> +      Its levels are 2.3 V to 3.8 V programmed by I2C per 0.1 V.
>> +    maxItems: 1
>> +
>> +  richtek,const-uvolt:
>> +    description: |
>> +      Battery regulation voltage of constant voltage mode. This voltage levels from
>> +      3.65 V to 4.4 V by I2C per 0.025 V.
>> +    maxItems: 1
>> +
>> +  extcon:
> 
> This is deprecated. There's standard connector bindings now.

How does this work? I couldn't find an example.

I found Documentation/devicetree/bindings/connector/usb-connector.yaml 
[2] but I don't see how this would be applied here.

The extcon device entry in the samsung-serranove devicetree [3] looks like:

i2c-muic {
         compatible = "i2c-gpio";
         sda-gpios = <&msmgpio 105 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
         scl-gpios = <&msmgpio 106 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;

         pinctrl-names = "default";
         pinctrl-0 = <&muic_i2c_default>;

         #address-cells = <1>;
         #size-cells = <0>;

         muic: extcon@14 {
                 compatible = "siliconmitus,sm5504-muic";
                 reg = <0x14>;

                 interrupt-parent = <&msmgpio>;
                 interrupts = <12 IRQ_TYPE_EDGE_FALLING>;

                 pinctrl-names = "default";
                 pinctrl-0 = <&muic_irq_default>;
         };
};

[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/connector/usb-connector.yaml?h=v6.2
[3] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/msm8916-samsung-serranove.dts?h=v6.2#n123

>> +    description: |
>> +      Phandle to the extcon device.
>> +    maxItems: 1
>> +
>> +required:
>> +  - richtek,pre-uamp
>> +  - richtek,fast-uamp
>> +  - richtek,eoc-uamp
>> +  - richtek,pre-threshold-uvolt
>> +  - richtek,const-uvolt
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    charger {
>> +        compatible = "richtek,rt5033-charger";
>> +        richtek,pre-uamp = <450000>;
>> +        richtek,fast-uamp = <1000000>;
>> +        richtek,eoc-uamp = <150000>;
>> +        richtek,pre-threshold-uvolt = <3500000>;
>> +        richtek,const-uvolt = <4350000>;
>> +        extcon = <&muic>;
>> +    };
>> diff --git a/Documentation/devicetree/bindings/regulator/richtek,rt5033-regulator.yaml b/Documentation/devicetree/bindings/regulator/richtek,rt5033-regulator.yaml
>> new file mode 100644
>> index 000000000000..61b074488db4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/regulator/richtek,rt5033-regulator.yaml
>> @@ -0,0 +1,45 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/regulator/richtek,rt5033-regulator.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Richtek RT5033 PIMC Voltage Regulator
>> +
>> +maintainers:
>> +  - Jakob Hauser <jahau@rocketmail.com>
>> +
>> +description: |
>> +  The regulators of RT5033 have to be instantiated under a sub-node named
>> +  "regulators". For SAFE_LDO voltage there is only one value of 4.9 V. LDO
>> +  voltage ranges from 1.2 V to 3.0 V in 0.1 V steps. BUCK voltage ranges from
>> +  1.0 V to 3.0 V in 0.1 V steps.
>> +
>> +patternProperties:
>> +  "^(SAFE_LDO|LDO|BUCK)$":
> 
> Lowercase preferred for node names.

OK, I can change to lowercase.

Though I have to change the already existing driver rt5033-regulator as 
well then [4]. I'm not sure if this has an impact on already existing 
implementations. Although within the upstream kernel I think there is no 
usage of the rt5033-regulator driver yet.

[4] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/regulator/rt5033-regulator.c?h=v6.2#n42

>> +    type: object
>> +    $ref: /schemas/regulator/regulator.yaml#
>> +    unevaluatedProperties: false
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    regulators {
> 
> Just 1 complete example in the MFD binding please.

OK, I'll skip the examples part here then.

>> +        safe_ldo_reg: SAFE_LDO {
>> +            regulator-name = "SAFE_LDO";
>> +            regulator-min-microvolt = <4900000>;
>> +            regulator-max-microvolt = <4900000>;
>> +            regulator-always-on;
>> +        };
>> +        ldo_reg: LDO {
>> +            regulator-name = "LDO";
>> +            regulator-min-microvolt = <2800000>;
>> +            regulator-max-microvolt = <2800000>;
>> +        };
>> +        buck_reg: BUCK {
>> +            regulator-name = "BUCK";
>> +            regulator-min-microvolt = <1200000>;
>> +            regulator-max-microvolt = <1200000>;
>> +        };
>> +     };
>> -- 
>> 2.39.1
>>

I'll wait with implementing the changes as there will be likely further 
comments on the other patches.

Kind regards,
Jakob