Message-ID: <acb209ce-7cb6-9a07-c913-9931b980c8c7@rocketmail.com>
Date:   Tue, 18 Apr 2023 23:37:33 +0200
From:   Jakob Hauser <jahau@...ketmail.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc:     Sebastian Reichel <sre@...nel.org>, Lee Jones <lee@...nel.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Beomho Seo <beomho.seo@...sung.com>,
        Chanwoo Choi <cw00.choi@...sung.com>,
        Stephan Gerhold <stephan@...hold.net>,
        Raymond Hackley <raymondhackley@...tonmail.com>,
        Pavel Machek <pavel@....cz>, Axel Lin <axel.lin@...ics.com>,
        ChiYuan Huang <cy_huang@...htek.com>, linux-pm@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        phone-devel@...r.kernel.org, ~postmarketos/upstreaming@...ts.sr.ht
Subject: Re: [PATCH v2 9/9] dt-bindings: Add documentation for rt5033 mfd,
 regulator and charger

Hi Krzysztof,

On 16.04.23 20:39, Krzysztof Kozlowski wrote:
> On 16/04/2023 14:44, Jakob Hauser wrote:
>> Add device tree binding documentation for rt5033 multifunction device, voltage
>> regulator and battery charger.
> 
> Subject: drop second/last, redundant "documentation". The "dt-bindings"
> prefix is already stating that these are documentation.

If I understand correctly, the new subject would be:
"dt-bindings: Add rt5033 mfd, regulator and charger"

...

>> +    i2c {
>> +        #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 {
> 
> If you could change it: No underscores in node names... but you cannot.
> This is old driver so you will break the users.

As discussed on patch 5, I'll leave the regulator names unchanged. Thus, 
I'll reset them to the original uppercase spelling.

>> +                    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>;
>> +                };
>> +            };

...

>> 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..439e0b7962f3
>> --- /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@...ketmail.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-microamp:
>> +    description:
>> +      Current of pre-charge mode. The pre-charge current levels are 350 mA to
>> +      650 mA programmed by I2C per 100 mA.
> 
> minimum:
> maximum:
> multipleOf: 100
> 
> Same for other cases.

The "multipleOf: 100" doesn't seen appropriate to me when the choice is 
350, 450, 550, 650. Those are not multiples of 100. It's more of a step 
size. I didn't find a general property for step size. Listing them as 
"enum" would be another possibility, I guess, but not an elegant one. 
Especially for property "richtek,const-microvolt" there are 30 
possibilities.

Using "multipleOf" and unit microamp, the block would then look like this:

   richtek,pre-microamp:
     description:
       Current of pre-charge mode. The pre-charge current levels are
       350 mA to 650 mA programmed by I2C per 100 mA.
     minimum: 350000
     maximum: 650000
     multipleOf: 100000
     maxItems: 1

Or possibly better readable the following way:

   richtek,pre-microamp:
     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
     items:
       minimum: 350000
       maximum: 650000
       multipleOf: 100000

>> +    maxItems: 1
>> +
>> +  richtek,fast-microamp:
>> +    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-microamp:
>> +    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

Here are two different step sizes. The first few are 50 mA steps (150, 
200, 250, 300 mA) and then it changes to 100 mA steps (300, 400, 500, 
600 mA). How to deal with that? Again I guess "enum" would be a 
possibility, but again not a nice one.

>> +
>> +  richtek,pre-threshold-microvolt:
>> +    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-microvolt:
>> +    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:
>> +    description:
>> +      Phandle to the extcon device.
>> +    maxItems: 1

...

>> 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..66c8a0692e10
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/regulator/richtek,rt5033-regulator.yaml
>> @@ -0,0 +1,24 @@
>> +# 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
> 
> You should explain in commit msg that you document existing driver in
> the Linux kernel. We would not cut some slack, e.g. stricter rules
> applied to new bindings.
> 
>> +
>> +maintainers:
>> +  - Jakob Hauser <jahau@...ketmail.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)$":
>> +    type: object
>> +    $ref: /schemas/regulator/regulator.yaml#
> 
> Just squash it with parent schema. No real benefits of having regulators
> separate - it's very small one.

OK, I'll squash the regulator schema into the mfd schema.

Kind regards,
Jakob

Powered by blists - more mailing lists