Add RT5033 charger device driver

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".

Lowercase is preferred for node names.

Add device tree binding documentation for rt5033 multifunction device, voltage regulator and battery charger.

Cc Beomho Seo <[email protected]>

Cc Chanwoo Choi <[email protected]>

Signed-off-by Jakob Hauser <[email protected]>

---
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

Documentation/devicetree/bindings/mfd/richtek,rt5033.yaml new file +6 -0

+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        pmic@34 {
+            compatible = "richtek,rt5033";

Documentation/devicetree/bindings/mfd/richtek,rt5033.yaml new file +28 -0

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

Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml new file +13 -0

new file mode 100644
index 000000000000..439e0b7962f3
@@ -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 <[email protected]>
+
+description:
+  The battery charger of the multifunction device RT5033 has to be instantiated

Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml new file +53 -0

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

Documentation/devicetree/bindings/regulator/richtek,rt5033-regulator.yaml new file +0 -0

new file mode 100644
index 000000000000..66c8a0692e10

Documentation/devicetree/bindings/regulator/richtek,rt5033-regulator.yaml new file +4 -0

+
+maintainers:
+  - Jakob Hauser <[email protected]>
+

Quoted 1 line · Jakob Hauser

Lowercase is preferred for node names.

This will break all existing users. In-tree and out-of-tree. Where is the binding update?

Quoted 2 lines · Jakob Hauser

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.

Quoted 11 lines

Documentation/devicetree/bindings/mfd/richtek,rt5033.yaml new file +11 -0

+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        pmic@34 {
+            compatible = "richtek,rt5033";
+            reg = <0x34>;
+
+            interrupt-parent = <&msmgpio>;
+            interrupts = <62 IRQ_TYPE_EDGE_FALLING>;
+

Documentation/devicetree/bindings/mfd/richtek,rt5033.yaml line 62

Quoted 5 lines

Documentation/devicetree/bindings/mfd/richtek,rt5033.yaml new file +5 -0

+            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.

Quoted 16 lines

Documentation/devicetree/bindings/mfd/richtek,rt5033.yaml new file +16 -0

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

Quoted 19 lines

Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml new file +13 -0

new file mode 100644
index 000000000000..439e0b7962f3
@@ -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 <[email protected]>
+
+description:
+  The battery charger of the multifunction device RT5033 has to be instantiated

Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml line 23

Quoted 10 lines

Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml new file +10 -0

+  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.

Quoted 6 lines

Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml new file +6 -0

+    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.

Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml line 37

Quoted 8 lines

Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml new file +8 -0

+    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

Quoted 18 lines

Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml new file +18 -0

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

Documentation/devicetree/bindings/regulator/richtek,rt5033-regulator.yaml line 7

Quoted 13 lines

Documentation/devicetree/bindings/regulator/richtek,rt5033-regulator.yaml new file +7 -0

new file mode 100644
index 000000000000..66c8a0692e10
@@ -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.

Quoted 9 lines

Documentation/devicetree/bindings/regulator/richtek,rt5033-regulator.yaml new file +9 -0

+
+maintainers:
+  - Jakob Hauser <[email protected]>
+
+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.

Documentation/devicetree/bindings/regulator/richtek,rt5033-regulator.yaml line 21

Quoted 5 lines

Documentation/devicetree/bindings/regulator/richtek,rt5033-regulator.yaml new file +5 -0

+
+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.

Hi Krzysztof,

Quoted 5 lines · Krzysztof Kozlowski

Quoted 1 line · Jakob Hauser

Lowercase is preferred for node names.

This will break all existing users. In-tree and out-of-tree. Where is the binding update?

In my reply to Rob's comments in v1 I was pointing out that this will affect an existing driver. There was no reaction.

As far as I can see, there is no in-tree usage yet. Though I can't tell about out-of-tree usage. Although if there is, adding the rt5033-charger driver might already causes the need for changes.

Well, to stay on the safe side, I'll drop this patch in v3 and will change the bindings (patch 9) back to uppercase.

Quoted 13 lines

Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml line 23

Quoted 8 lines

Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml new file +8 -0

+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

Quoted 1 line

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.

Quoted 6 lines

Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml line 37

Quoted 6 lines

Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml new file +6 -0

+  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.

Quoted 14 lines · Jakob Hauser

Quoted 5 lines · Krzysztof Kozlowski

Quoted 1 line · Jakob Hauser

Lowercase is preferred for node names.

This will break all existing users. In-tree and out-of-tree. Where is the binding update?

In my reply to Rob's comments in v1 I was pointing out that this will affect an existing driver. There was no reaction.

As far as I can see, there is no in-tree usage yet. Though I can't tell about out-of-tree usage. Although if there is, adding the rt5033-charger driver might already causes the need for changes.

Quoted 2 lines

Well, to stay on the safe side, I'll drop this patch in v3 and will change the bindings (patch 9) back to uppercase.

Your v1 binding patch did not explain that you document existing ABI, so you got comments like for a new binding. This is not really new binding, is it?

Quoted 13 lines · Jakob Hauser

Quoted 13 lines

Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml line 23

Quoted 8 lines

Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml new file +8 -0

+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

Quoted 8 lines

Quoted 1 line

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.

Quoted 7 lines

Quoted 6 lines

Quoted 6 lines

Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml new file +6 -0

+  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

Quoted 4 lines

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

Add device tree binding documentation for rt5033 multifunction device, voltage regulator and battery charger.

Cc Beomho Seo <[email protected]>

Cc Chanwoo Choi <[email protected]>

Signed-off-by Jakob Hauser <[email protected]>

---
The patch is based on linux-next (tag "next-20230413").

Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml new file +0 -0

Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml new file +21 -0

+  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:

Hi Jakob,

thanks for your patch!

The following caught my eye:

Quoted 8 lines · Jakob Hauser

Add device tree binding documentation for rt5033 multifunction device, voltage regulator and battery charger.

Cc Beomho Seo <[email protected]>

Cc Chanwoo Choi <[email protected]>

Signed-off-by Jakob Hauser <[email protected]>

---
The patch is based on linux-next (tag "next-20230413").

(...)

Quoted 2 lines

Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml new file +0 -0

(...)

Quoted 21 lines

Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml new file +21 -0

+  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:

Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml line 50

Quoted 10 lines

Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml new file +10 -0

+      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.

From: Stephan Gerhold <[email protected]>

The fuel gauge in the RT5033 PMIC (rt5033-battery) has its own I2C bus and interrupt lines. Therefore, it is not part of the MFD device and needs to be specified separately in the device tree.

Cc Beomho Seo <[email protected]>

Cc Chanwoo Choi <[email protected]>

Fixes 0b271258544b ("mfd: rt5033: Add Richtek RT5033 driver core.")

Signed-off-by Stephan Gerhold <[email protected]>

Acked-by Lee Jones <[email protected]>

Signed-off-by Jakob Hauser <[email protected]>

Quoted 66 lines · Linus Walleij

Hi Jakob,

thanks for your patch!

The following caught my eye:

Quoted 8 lines · Jakob Hauser

Add device tree binding documentation for rt5033 multifunction device, voltage regulator and battery charger.

Cc Beomho Seo <[email protected]>

Cc Chanwoo Choi <[email protected]>

Signed-off-by Jakob Hauser <[email protected]>

---
The patch is based on linux-next (tag "next-20230413").

(...)

Quoted 2 lines

Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml new file +0 -0

(...)

Quoted 21 lines

Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml new file +21 -0

+  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:

Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml line 50

Quoted 10 lines

Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml new file +10 -0

+      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

Quoted 4 lines

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

Quoted 9 lines · Linus Walleij

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.

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.

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):

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.

On Thu, Apr 20, 2023 at 11:16 PM Jakob Hauser <[email protected]> wrote:

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 1 line

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.

Quoted 4 lines

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 rt5033-battery fuelgauge can't get a status by itself. The rt5033-charger can, let's get this value.

Tested-by Raymond Hackley <[email protected]>

Signed-off-by Jakob Hauser <[email protected]>

---
  drivers/power/supply/rt5033_battery.c | 24 ++++++++++++++++++++++++
  1 file changed, 24 insertions(+)

drivers/power/supply/rt5033_battery.c +10 -0

index 5c04cf305219..48d4cccce4f6 100644
@@ -12,6 +12,26 @@
  #include <linux/mfd/rt5033-private.h>
  #include <linux/mfd/rt5033.h>
 
+static int rt5033_battery_get_status(struct i2c_client *client)
+{
+	struct power_supply *charger;
+	union power_supply_propval val;
+	int ret;
+
+	charger = power_supply_get_by_name("rt5033-charger");
+	if (!charger)
+		return -ENODEV;
+

drivers/power/supply/rt5033_battery.c +3 -0

  static int rt5033_battery_get_capacity(struct i2c_client *client)
  {
  	struct rt5033_battery *battery = i2c_get_clientdata(client);

@@ -84,6 +104,9 @@ static int rt5033_battery_get_property(struct power_supply *psy,
  	case POWER_SUPPLY_PROP_CAPACITY:
  		val->intval = rt5033_battery_get_capacity(battery->client);
  		break;
+	case POWER_SUPPLY_PROP_STATUS:
+		val->intval = rt5033_battery_get_status(battery->client);
+		break;
  	default:
  		return -EINVAL;
  	}

Hi Jakob,

kernel test robot noticed the following build warnings:

[auto build test WARNING on lee-mfd/for-mfd-next]
[also build test WARNING on next-20230421]
[cannot apply to sre-power-supply/for-next broonie-regulator/for-next linus/master lee-mfd/for-mfd-fixes v6.3-rc7] [If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jakob-Hauser/mfd-rt5033-Fix-chip-revision-readout/20230416-214502
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
patch link:    https://lore.kernel.org/r/665d8906ea7b84e0a248315e8395a80007b8bafb.1681646904.git.jahau%40rocketmail.com
patch subject: [PATCH v2 6/9] power: supply: rt5033_charger: Add RT5033 charger device driver
config: x86_64-allmodconfig (https://download.01.org/0day-ci/archive/20230423/[email protected]/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/0fbd385f9a1acd55a8d943560428b9d783f8047f
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Jakob-Hauser/mfd-rt5033-Fix-chip-revision-readout/20230416-214502
git checkout 0fbd385f9a1acd55a8d943560428b9d783f8047f
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash arch/x86/xen/ drivers/power/supply/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All warnings (new ones prefixed by >>):

Quoted 1 line

Quoted 1 line

drivers/power/supply/rt5033_charger.c:26:10: warning: variable 'state' is uninitialized when used here [-Wuninitialized]

                    return state;
                           ^~~~~
    drivers/power/supply/rt5033_charger.c:23:11: note: initialize the variable 'state' to silence this warning
            int state;
                     ^
                      = 0
    1 warning generated.

vim +/state +26 drivers/power/supply/rt5033_charger.c

     18	
     19	static int rt5033_get_charger_state(struct rt5033_charger *charger)
     20	{
     21		struct regmap *regmap = charger->rt5033->regmap;
     22		unsigned int reg_data;
     23		int state;
     24	
     25		if (!regmap)

Quoted 1 line

26 return state;

     27	
     28		regmap_read(regmap, RT5033_REG_CHG_STAT, &reg_data);
     29	
     30		switch (reg_data & RT5033_CHG_STAT_MASK) {
     31		case RT5033_CHG_STAT_DISCHARGING:
     32			state = POWER_SUPPLY_STATUS_DISCHARGING;
     33			break;
     34		case RT5033_CHG_STAT_CHARGING:
     35			state = POWER_SUPPLY_STATUS_CHARGING;
     36			break;
     37		case RT5033_CHG_STAT_FULL:
     38			state = POWER_SUPPLY_STATUS_FULL;
     39			break;
     40		case RT5033_CHG_STAT_NOT_CHARGING:
     41			state = POWER_SUPPLY_STATUS_NOT_CHARGING;

     42			break;
     43		default:
     44			state = POWER_SUPPLY_STATUS_UNKNOWN;
     45		}
     46	
     47		return state;
     48	}
     49