| Message ID | cover.1681646904.git.jahau@rocketmail.com |
|---|---|
| Headers | show |
| Series | Add RT5033 charger device driver | 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 Krzysztof, On 19.04.23 10:42, Krzysztof Kozlowski wrote: > On 18/04/2023 23:37, Jakob Hauser wrote: > >>>> +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. > > Ahh, right. You can use enum here and min/max for other cases, where > multipleOf cannot be used. > >>>> + 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. > > enum Thanks for the reply. Looking through the properties, I get the following result. 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 enum: [350000, 450000, 550000, 650000] 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 minimum: 700000 maximum: 2000000 multipleOf: 100000 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 enum: [150000, 200000, 250000, 300000, 400000, 500000, 600000] 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 minimum: 2300000 maximum: 3800000 multipleOf: 100000 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 minimum: 3650000 maximum: 4400000 multipleOf: 25000 Kind regards, Jakob
This patchset adds the charger driver "rt5033-charger". It is part of the multifunction device rt5033. The patchset is based on an older version by Beomho Seo of March 2015. For more information on the history and setup of the patchset see the cover sheet of version v1, there is a link further down below the changelog. In patch 9 I didn't change the extcon phandle, I haven't received any answer on this. Changes in v2: - Rebased to linux-next (20230413), as suggested by Lee. - The v1 patch 3 "mfd: rt5033: Fix comments and style in includes" vanished as it got applied already. - Dropped the v1 patch 8 "power: supply: rt5033_charger: Make use of high impedance mode". The high impedance mode is kind of a sleep mode for power saving. It turned out that it might complicate a future implementation of an rt5033 flash LED driver. Therefore drop it for now. The high impedance mode could be added at a later date as a power saving improvement. - Patch 2: Changed variable name "data" back to original "dev_id". - New patch 5: Changed name of regulators to lowercase, as suggested by Rob. - Patch 6: In function "rt5033_charger_dt_init" replaced the devicetree units "uamp" to "microamp" and "uvolt" to "microvolt". However, I didn't change the unit names of the driver-internal variables in order to keep the variable names short. Let me know if you think they should be changed too. - Patch 9: Removed '|' after all "description" blocks in all three files. - Patch 9: In the example of "mfd/richtek,rt5033.yaml" changed "i2c@0" to "i2c". - Patch 9: In the example of "mfd/richtek,rt5033.yaml" removed the last part on the battery fuelgauge. It has its own I2C line and is therefore not a subsidiary of the rt5033 MFD driver. - Patch 9: Replaced units "uamp" by "microamp" and "uvolt" by "microvolt" in the example of "mfd/richtek,rt5033.yaml" and the file "power/supply/richtek,rt5033-charger.yaml". - Patch 9: Changed name of regulators to lowercase in "regulator/richtek,rt5033-regulator.yaml" and in the example of "mfd/richtek,rt5033.yaml" (related to patch 5). - Patch 9: Removed example from "regulator/richtek,rt5033-regulator.yaml". It is already part of the example in "mfd/richtek,rt5033.yaml". v1: https://lore.kernel.org/linux-pm/cover.1677620677.git.jahau@rocketmail.com/T/#t The result of the patchset v2 can be seen at: https://github.com/Jakko3/linux/blob/rt5033-charger_v2/drivers/power/supply/rt5033_charger.c Jakob Hauser (8): mfd: rt5033: Fix chip revision readout mfd: rt5033: Fix STAT_MASK, HZ_MASK and AICR defines mfd: rt5033: Apply preparatory changes before adding rt5033-charger driver regulator: rt5033: Change regulator names to lowercase power: supply: rt5033_charger: Add RT5033 charger device driver power: supply: rt5033_charger: Add cable detection and USB OTG supply power: supply: rt5033_battery: Adopt status property from charger dt-bindings: Add documentation for rt5033 mfd, regulator and charger Stephan Gerhold (1): mfd: rt5033: Drop rt5033-battery sub-device .../bindings/mfd/richtek,rt5033.yaml | 90 +++ .../power/supply/richtek,rt5033-charger.yaml | 76 ++ .../regulator/richtek,rt5033-regulator.yaml | 24 + drivers/mfd/rt5033.c | 8 +- drivers/power/supply/Kconfig | 8 + drivers/power/supply/Makefile | 1 + drivers/power/supply/rt5033_battery.c | 24 + drivers/power/supply/rt5033_charger.c | 724 ++++++++++++++++++ drivers/regulator/rt5033-regulator.c | 12 +- include/linux/mfd/rt5033-private.h | 64 +- include/linux/mfd/rt5033.h | 10 +- 11 files changed, 1008 insertions(+), 33 deletions(-) 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 create mode 100644 drivers/power/supply/rt5033_charger.c