| Message ID | 9275af790e6e21b5cf661a2444effe4caf2be02e.1681646904.git.jahau@rocketmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/9] mfd: rt5033: Drop rt5033-battery sub-device | expand |
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. > > Cc: Beomho Seo <beomho.seo@samsung.com> > Cc: Chanwoo Choi <cw00.choi@samsung.com> > Signed-off-by: Jakob Hauser <jahau@rocketmail.com> > --- > The patch is based on linux-next (tag "next-20230413"). > > .../bindings/mfd/richtek,rt5033.yaml | 90 +++++++++++++++++++ > .../power/supply/richtek,rt5033-charger.yaml | 76 ++++++++++++++++ > .../regulator/richtek,rt5033-regulator.yaml | 24 +++++ > 3 files changed, 190 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 > > + 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. > + 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-microamp = <450000>; > + richtek,fast-microamp = <1000000>; > + richtek,eoc-microamp = <150000>; > + richtek,pre-threshold-microvolt = <3500000>; > + richtek,const-microvolt = <4350000>; > + extcon = <&muic>; > + }; > + }; > + }; > 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@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-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. > + 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 > + > + 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 > + > +required: > + - richtek,pre-microamp > + - richtek,fast-microamp > + - richtek,eoc-microamp > + - richtek,pre-threshold-microvolt > + - richtek,const-microvolt > + > +additionalProperties: false > + > +examples: > + - | > + charger { > + compatible = "richtek,rt5033-charger"; > + richtek,pre-microamp = <450000>; > + richtek,fast-microamp = <1000000>; > + richtek,eoc-microamp = <150000>; > + richtek,pre-threshold-microvolt = <3500000>; > + richtek,const-microvolt = <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..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@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)$": > + 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. Best regards, Krzysztof
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@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-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@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)$": >> + 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
Hi Jakob, thanks for your patch! The following caught my eye: On Sun, Apr 16, 2023 at 2:50 PM Jakob Hauser <jahau@rocketmail.com> 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> > --- > The patch is based on linux-next (tag "next-20230413"). (...) > --- /dev/null > +++ b/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml (...) > + 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 > + > + 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 > + > + 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 These are very generic currents and voltages, and their usage is well known and generic. So they should not be prefixed "richtek,". Use the properties already defined in Documentation/devicetree/bindings/power/supply/battery.yaml for these: precharge-current-microamp constant-charge-current-max-microamp charge-term-current-microamp precharge-upper-limit-microvolt constant-charge-voltage-max-microvolt Please double-check, I think those are the ones you need. Perhaps it is possible to just $ref these properties directly and add the additional restrictions on top. Yours, Linus Walleij
Hi Linus! On 20.04.23 10:03, Linus Walleij wrote: > On Thu, Apr 20, 2023 at 9:59 AM Linus Walleij <linus.walleij@linaro.org> wrote: >> >> Hi Jakob, >> >> thanks for your patch! >> >> The following caught my eye: >> >> On Sun, Apr 16, 2023 at 2:50 PM Jakob Hauser <jahau@rocketmail.com> 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> >>> --- >>> The patch is based on linux-next (tag "next-20230413"). >> (...) >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml >> (...) >>> + 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 >>> + >>> + 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 >>> + >>> + 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 >> >> These are very generic currents and voltages, and their usage is well known >> and generic. So they should not be prefixed "richtek,". >> >> Use the properties already defined in >> Documentation/devicetree/bindings/power/supply/battery.yaml >> for these: >> >> precharge-current-microamp >> constant-charge-current-max-microamp >> charge-term-current-microamp >> precharge-upper-limit-microvolt >> constant-charge-voltage-max-microvolt >> >> Please double-check, I think those are the ones you need. >> >> Perhaps it is possible to just $ref these properties directly and add >> the additional restrictions on top. > > On second thought, these are really weird properties to have on the > *charger* isn't it? > > It is really *battery* restrictions. > > A charger can charge many different batteries with different CC/CV > settings. > > I think your charger should contain a phandle to a battery and the battery > node should contain these limits. > > monitored-battery: > $ref: /schemas/types.yaml#/definitions/phandle > description: phandle to battery node > > Then you can just use the standard battery bindings for these properties > on the battery. > > See for example: > Documentation/devicetree/bindings/power/supply/stericsson,ab8500-charger.yaml > > There will be driver changes needed too, but this will be way cleaner. > > Yours, > Linus Walleij These are interesting hints! I was first a bit confused by the term "battery". I associated that term with the driver "rt5033-battery". But I think that thought was wrong. The driver "rt5033-battery" is just the fuel gauge. Hardware-wise, the mfd (incl. charger) and fuel gauge are on different I2C lines. Therefore, the different drivers access different registers. Charger registers: https://github.com/torvalds/linux/blob/v6.3-rc7/include/linux/mfd/rt5033-private.h#L13-L23 Fuel gauge registers: https://github.com/torvalds/linux/blob/v6.3-rc7/include/linux/mfd/rt5033-private.h#L215-L243 The things being set or read in those registers kind of determine which things are done in which driver. The properties we talk about here are the settings for the charger. They tell the charger how it should behave. It makes sense to process those settings within the charger driver. The fuel gauge, on the other hand, returns information like actual voltage and percentage. The only thing that seems not placed well is the "status" property like charging/discharging/not-charging/etc. At RT5033 this information is within the charger register. Userspace layer "UPower" expects this from the "battery" device. Patch 8 of this series v2 carries that property from the charger over to the fuel gauge. Though this is a bit of a quirk. And it creates dependencies between two drivers which actually would be independent from each other. --------------------------------- Back to the topic. When we talk about "battery" here, it seems to me that it's about the representation in the devicetree. Currently it is: pmic@34 { compatible = "richtek,rt5033"; .... charger { compatible = "richtek,rt5033-charger"; richtek,pre-microamp = <450000>; richtek,fast-microamp = <1000000>; richtek,eoc-microamp = <150000>; richtek,pre-threshold-microvolt = <3500000>; richtek,const-microvolt = <4350000>; extcon = <&muic>; }; }; According to your remarks, the properties could be "outsourced" into a battery node. (Btw. I have double-checked the property names.) battery: battery { compatible = "simple-battery"; precharge-current-microamp = <450000>; constant-charge-current-max-microamp = <1000000>; charge-term-current-microamp = <150000>; precharge-upper-limit-microvolt = <3500000>; constant-charge-voltage-max-microvolt = <4350000>; }; pmic@34 { compatible = "richtek,rt5033"; .... charger { compatible = "richtek,rt5033-charger"; monitored-battery = <&battery>; extcon = <&muic>; }; }; Personally I would choose the current implementation for two reasons (possibly weak ones): 1) The original author of the driver and documentation is Beomho Seo. I tried to preserve the original structure as far as possible. This is probably rather a question of editing than a technical one. 2) At least in my mind it's still the setup for the charger. It sets up a the charging behavior of a certain consumer device. And the choice of their values is limited to the hardware of the charger. Accordingly the dt-bindings would say what the charger hardware is capable to do. Therefore I'd say it's reasonable to have those values in the charger node and use vendor properties. I agree to you that actually the physical battery is determining how these values should be set. In the end, as far as I can see, it is a representation thing in the devicetree. At least in our case here. Not sure how to proceed here. I would stick to the current implementation. If someone strongly prefers the "battery" representation style, I'm open to switch to this. However, I'm not sure how the dt-bindings would look like in that case. Those battery properties would not be part of the RT5033 node, thus they basically would not be part of the RT5033 documentation. Again I think it makes sense to handle those properties within the charger node as "charger settings" properties. Kind regards, Jakob
Hi Jakob, On Thu, Apr 20, 2023 at 11:16 PM Jakob Hauser <jahau@rocketmail.com> wrote: > > On Thu, Apr 20, 2023 at 9:59 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > On second thought, these are really weird properties to have on the > > *charger* isn't it? > > > > It is really *battery* restrictions. > > > > A charger can charge many different batteries with different CC/CV > > settings. (...) > I was first a bit confused by the term "battery". I associated that term > with the driver "rt5033-battery". But I think that thought was wrong. > The driver "rt5033-battery" is just the fuel gauge. Yeah that is a different thing altogether, it should be named rt5033-fuel-gauge if I could turn back time, I would review it and say this, perhaps the confusion can be fixed. Mistakes were made. > The properties we talk about here are the settings for the charger. They > tell the charger how it should behave. It makes sense to process those > settings within the charger driver. It may make sense to *parse* this in the charger driver, by following the monitored-battery phandle to inspect the battery properties and get the information you need. The architecture of any Linux driver does not really concern the DT bindings, the bindings should reflect the hardware. The hardware has a charger, and the charger is monitoring a battery, so it needs to be its own DT node. > The fuel gauge, on the other hand, > returns information like actual voltage and percentage. The fuel gauge should probably have a phandle to the same battery for compleness sake, but may not need it. If it ever needs any battery properties, it should definately have that. > According to your remarks, the properties could be "outsourced" into a > battery node. (Btw. I have double-checked the property names.) > > battery: battery { > compatible = "simple-battery"; > precharge-current-microamp = <450000>; > constant-charge-current-max-microamp = <1000000>; > charge-term-current-microamp = <150000>; > precharge-upper-limit-microvolt = <3500000>; > constant-charge-voltage-max-microvolt = <4350000>; > }; > > pmic@34 { > compatible = "richtek,rt5033"; > .... > charger { > compatible = "richtek,rt5033-charger"; > monitored-battery = <&battery>; > extcon = <&muic>; > }; > }; Yups this is how it should look :) > Personally I would choose the current implementation for two reasons > (possibly weak ones): The device tree binding isn't any "implementation", and make sure to not go into the trap that DT bindings should be done to be convenient for any specific Linux driver to use. > 2) At least in my mind it's still the setup for the charger. It sets up > a the charging behavior of a certain consumer device. And the choice of > their values is limited to the hardware of the charger. Accordingly the > dt-bindings would say what the charger hardware is capable to do. > Therefore I'd say it's reasonable to have those values in the charger > node and use vendor properties. > > I agree to you that actually the physical battery is determining how > these values should be set. In the end, as far as I can see, it is a > representation thing in the devicetree. At least in our case here. The DT bindings should reflect the hardware, and not what some or any driver "would like to see" (to make its life easier...) As these things are programmed into registers, clearly the hardware is adoptable for different batteries, and the purpose of these registers is to support different batteries. Ergo: they belong in a battery node. > Not sure how to proceed here. I would stick to the current > implementation. If someone strongly prefers the "battery" representation > style, I'm open to switch to this. Again this is not an implementation but a hardware description. It should use a phandle to a montored-battery and follow that to read the battery properties. > However, I'm not sure how the dt-bindings would look like in that case. Just like you sketched above, just reuse simple-battery if the battery is hardcoded into the platform, such as soldered in or has a form factor such that no different battery can be fitted. > Those battery properties would not be part of the RT5033 node, thus they > basically would not be part of the RT5033 documentation. Again I think > it makes sense to handle those properties within the charger node as > "charger settings" properties. Why? This is like saying that the number of pixels on your monitor should be part of the graphics card DT node as "configuration". And we clearly do not do that. Yours, Linus Walleij
Hi Linus, On 21.04.23 11:20, Linus Walleij wrote: > On Thu, Apr 20, 2023 at 11:16 PM Jakob Hauser <jahau@rocketmail.com> wrote: ... >> I agree to you that actually the physical battery is determining how >> these values should be set. In the end, as far as I can see, it is a >> representation thing in the devicetree. At least in our case here. > > The DT bindings should reflect the hardware, and not what some > or any driver "would like to see" (to make its life easier...) > > As these things are programmed into registers, clearly the > hardware is adoptable for different batteries, and the purpose > of these registers is to support different batteries. Ergo: they > belong in a battery node. > >> Not sure how to proceed here. I would stick to the current >> implementation. If someone strongly prefers the "battery" representation >> style, I'm open to switch to this. > > Again this is not an implementation but a hardware description. > > It should use a phandle to a montored-battery and follow that to > read the battery properties. OK, this is expressing a strong preference. I’ll switch to the "battery" node in v3. >> However, I'm not sure how the dt-bindings would look like in that case. > > Just like you sketched above, just reuse simple-battery if the battery > is hardcoded into the platform, such as soldered in or has a form > factor such that no different battery can be fitted. > >> Those battery properties would not be part of the RT5033 node, thus they >> basically would not be part of the RT5033 documentation. Again I think >> it makes sense to handle those properties within the charger node as >> "charger settings" properties. > > Why? > > This is like saying that the number of pixels on your monitor should > be part of the graphics card DT node as "configuration". And we > clearly do not do that. The charger driver needs five parameters. And the person writing the dts file should know about what range and granularity is possible for those values. This information could be put into the description of the "monitored-battery", as shown below, but that's less strict and clear as it is currently in patch 9. 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 monitored-battery: description: | Phandle to the monitored battery according to battery.yaml. The battery node needs to contain five parameters. precharge-current-microamp: Current of pre-charge mode. The pre-charge current levels are 350 mA to 650 mA programmed by I2C per 100 mA. constant-charge-current-max-microamp: Current of fast-charge mode. The fast-charge current levels are 700 mA to 2000 mA programmed by I2C per 100 mA. charge-term-current-microamp: 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. precharge-upper-limit-microvolt: 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. constant-charge-voltage-max-microvolt: Battery regulation voltage of constant voltage mode. This voltage levels from 3.65 V to 4.4 V by I2C per 0.025 V. extcon: description: Phandle to the extcon device. maxItems: 1 required: - monitored-battery additionalProperties: false examples: - | charger { compatible = "richtek,rt5033-charger"; monitored-battery = <&battery>; extcon = <&muic>; }; Kind regards, Jakob
diff --git a/Documentation/devicetree/bindings/mfd/richtek,rt5033.yaml b/Documentation/devicetree/bindings/mfd/richtek,rt5033.yaml new file mode 100644 index 000000000000..158036a57291 --- /dev/null +++ b/Documentation/devicetree/bindings/mfd/richtek,rt5033.yaml @@ -0,0 +1,90 @@ +# 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: + 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 { + #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-microamp = <450000>; + richtek,fast-microamp = <1000000>; + richtek,eoc-microamp = <150000>; + richtek,pre-threshold-microvolt = <3500000>; + richtek,const-microvolt = <4350000>; + extcon = <&muic>; + }; + }; + }; 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@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-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 + + 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 + + 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 + +required: + - richtek,pre-microamp + - richtek,fast-microamp + - richtek,eoc-microamp + - richtek,pre-threshold-microvolt + - richtek,const-microvolt + +additionalProperties: false + +examples: + - | + charger { + compatible = "richtek,rt5033-charger"; + richtek,pre-microamp = <450000>; + richtek,fast-microamp = <1000000>; + richtek,eoc-microamp = <150000>; + richtek,pre-threshold-microvolt = <3500000>; + richtek,const-microvolt = <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..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 + +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)$": + type: object + $ref: /schemas/regulator/regulator.yaml# + unevaluatedProperties: false + +additionalProperties: false
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> --- The patch is based on linux-next (tag "next-20230413"). .../bindings/mfd/richtek,rt5033.yaml | 90 +++++++++++++++++++ .../power/supply/richtek,rt5033-charger.yaml | 76 ++++++++++++++++ .../regulator/richtek,rt5033-regulator.yaml | 24 +++++ 3 files changed, 190 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