[PATCH v2 0/3] Fix RT5033 battery device tree probing

[PATCH v2 0/3] Fix RT5033 battery device tree probing

[Stephan Gerhold]

At the moment, the RT5033 MFD and battery driver suggest that the
battery driver should probe as a sub-device of the MFD driver. However,
this does not make any sense since the fuel gauge part of RT5033 has its
own I2C device and interrupt line.

It was also documented as separate I2C device in the original device
tree bindings [1] (that were never finished up and merged) but for some
reason the code does not match the documentation (and reality). :/

Given other fairly critical mistakes like setting the wrong bits
in the regulator driver (see [2]), unfortunately I get the feeling
that none of the RT5033 drivers were ever tested properly. :(

This patch sets adds a proper of_match_table to rt5033-battery
and removes the rt5033-battery sub-device from the MFD driver.
There is no compile/runtime dependency of the power supply / MFD patch
so they can just be applied separately through the power supply / MFD tree.

With these changes, rt5033-battery seems to work fine on the
Samsung Galaxy A5 (2015) at least (it reports a reasonable
battery percentage).

[1]: https://lore.kernel.org/linux-pm/1425864191-4121-3-git-send-email-beomho.seo@samsung.com/
[2]: https://lore.kernel.org/lkml/20201110130047.8097-1-michael.srba@seznam.cz/

Changes in v2: Fix stupid typo in second patch :(
v1: Honestly, not worth looking at :)

Stephan Gerhold (3):
  dt-bindings: power: supply: Add DT schema for richtek,rt5033-battery
  power: supply: rt5033_battery: Fix device tree enumeration
  mfd: rt5033: Drop rt5033-battery sub-device

 .../power/supply/richtek,rt5033-battery.yaml  | 54 +++++++++++++++++++
 drivers/mfd/rt5033.c                          |  3 --
 drivers/power/supply/Kconfig                  |  3 +-
 drivers/power/supply/rt5033_battery.c         |  7 +++
 4 files changed, 63 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/power/supply/richtek,rt5033-battery.yaml

-- 
2.31.1

[PATCH v2 1/3] dt-bindings: power: supply: Add DT schema for richtek,rt5033-battery

[Stephan Gerhold]

The RT5033 PMIC provides a simple fuel gauge via I2C.
Add a DT schema to describe how to set it up in the device tree.

Note that although RT5033 is a MFD with lots of functionality
(also charger, regulator, LEDs, ...) the fuel gauge has a separate
I2C bus and is not part of the MFD.

Cc: Beomho Seo <beomho.seo@samsung.com>
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 .../power/supply/richtek,rt5033-battery.yaml  | 54 +++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/richtek,rt5033-battery.yaml

diff --git a/Documentation/devicetree/bindings/power/supply/richtek,rt5033-battery.yaml b/Documentation/devicetree/bindings/power/supply/richtek,rt5033-battery.yaml
new file mode 100644
index 000000000000..ae647d3355a2
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/richtek,rt5033-battery.yaml
@@ -0,0 +1,54 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/power/supply/richtek,rt5033-battery.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Richtek RT5033 PMIC Fuel Gauge
+
+maintainers:
+  - Stephan Gerhold <stephan@gerhold.net>
+
+allOf:
+  - $ref: power-supply.yaml#
+
+properties:
+  compatible:
+    const: richtek,rt5033-battery
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      battery@35 {
+        compatible = "richtek,rt5033-battery";
+        reg = <0x35>;
+      };
+    };
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      battery@35 {
+        compatible = "richtek,rt5033-battery";
+        reg = <0x35>;
+        interrupt-parent = <&msmgpio>;
+        interrupts = <121 IRQ_TYPE_EDGE_FALLING>;
+      };
+    };
-- 
2.31.1

[PATCH v2 2/3] power: supply: rt5033_battery: Fix device tree enumeration

[Stephan Gerhold]

The fuel gauge in the RT5033 PMIC has its own I2C bus and interrupt
line. Therefore, it is not actually part of the RT5033 MFD and needs
its own of_match_table to probe properly.

Also, given that it's independent of the MFD, there is actually
no need to make the Kconfig depend on MFD_RT5033. Although the driver
uses the shared <linux/mfd/rt5033.h> header, there is no compile
or runtime dependency on the RT5033 MFD driver.

Cc: Beomho Seo <beomho.seo@samsung.com>
Cc: Chanwoo Choi <cw00.choi@samsung.com>
Fixes: b847dd96e659 ("power: rt5033_battery: Add RT5033 Fuel gauge device driver")
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
Changes in v2: Fix stupid typo :(
---
 drivers/power/supply/Kconfig          | 3 ++-
 drivers/power/supply/rt5033_battery.c | 7 +++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index e696364126f1..20a2f93252f9 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -712,7 +712,8 @@ config BATTERY_GOLDFISH
 
 config BATTERY_RT5033
 	tristate "RT5033 fuel gauge support"
-	depends on MFD_RT5033
+	depends on I2C
+	select REGMAP_I2C
 	help
 	  This adds support for battery fuel gauge in Richtek RT5033 PMIC.
 	  The fuelgauge calculates and determines the battery state of charge
diff --git a/drivers/power/supply/rt5033_battery.c b/drivers/power/supply/rt5033_battery.c
index f330452341f0..9ad0afe83d1b 100644
--- a/drivers/power/supply/rt5033_battery.c
+++ b/drivers/power/supply/rt5033_battery.c
@@ -164,9 +164,16 @@ static const struct i2c_device_id rt5033_battery_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, rt5033_battery_id);
 
+static const struct of_device_id rt5033_battery_of_match[] = {
+	{ .compatible = "richtek,rt5033-battery", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, rt5033_battery_of_match);
+
 static struct i2c_driver rt5033_battery_driver = {
 	.driver = {
 		.name = "rt5033-battery",
+		.of_match_table = rt5033_battery_of_match,
 	},
 	.probe = rt5033_battery_probe,
 	.remove = rt5033_battery_remove,
-- 
2.31.1

[PATCH v2 3/3] mfd: rt5033: Drop rt5033-battery sub-device

[Stephan Gerhold]

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 <beomho.seo@samsung.com>
Cc: Chanwoo Choi <cw00.choi@samsung.com>
Fixes: 0b271258544b ("mfd: rt5033: Add Richtek RT5033 driver core.")
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 drivers/mfd/rt5033.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/mfd/rt5033.c b/drivers/mfd/rt5033.c
index f1236a9acf30..df095e91e266 100644
--- a/drivers/mfd/rt5033.c
+++ b/drivers/mfd/rt5033.c
@@ -41,9 +41,6 @@ static const struct mfd_cell rt5033_devs[] = {
 	{
 		.name = "rt5033-charger",
 		.of_compatible = "richtek,rt5033-charger",
-	}, {
-		.name = "rt5033-battery",
-		.of_compatible = "richtek,rt5033-battery",
 	}, {
 		.name = "rt5033-led",
 		.of_compatible = "richtek,rt5033-led",
-- 
2.31.1

Re: [PATCH v2 3/3] mfd: rt5033: Drop rt5033-battery sub-device

[Lee Jones]

On Mon, 17 May 2021, Stephan Gerhold wrote:

> 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 <beomho.seo@samsung.com>
> Cc: Chanwoo Choi <cw00.choi@samsung.com>
> Fixes: 0b271258544b ("mfd: rt5033: Add Richtek RT5033 driver core.")
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
>  drivers/mfd/rt5033.c | 3 ---
>  1 file changed, 3 deletions(-)

Acked-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 1/3] dt-bindings: power: supply: Add DT schema for richtek,rt5033-battery

[Rob Herring]

On Mon, 17 May 2021 12:51:11 +0200, Stephan Gerhold wrote:
> The RT5033 PMIC provides a simple fuel gauge via I2C.
> Add a DT schema to describe how to set it up in the device tree.
> 
> Note that although RT5033 is a MFD with lots of functionality
> (also charger, regulator, LEDs, ...) the fuel gauge has a separate
> I2C bus and is not part of the MFD.
> 
> Cc: Beomho Seo <beomho.seo@samsung.com>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
>  .../power/supply/richtek,rt5033-battery.yaml  | 54 +++++++++++++++++++
>  1 file changed, 54 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/supply/richtek,rt5033-battery.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 0/3] Fix RT5033 battery device tree probing

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2233 bytes --]

Hi,

On Mon, May 17, 2021 at 12:51:10PM +0200, Stephan Gerhold wrote:
> At the moment, the RT5033 MFD and battery driver suggest that the
> battery driver should probe as a sub-device of the MFD driver. However,
> this does not make any sense since the fuel gauge part of RT5033 has its
> own I2C device and interrupt line.
> 
> It was also documented as separate I2C device in the original device
> tree bindings [1] (that were never finished up and merged) but for some
> reason the code does not match the documentation (and reality). :/
> 
> Given other fairly critical mistakes like setting the wrong bits
> in the regulator driver (see [2]), unfortunately I get the feeling
> that none of the RT5033 drivers were ever tested properly. :(
> 
> This patch sets adds a proper of_match_table to rt5033-battery
> and removes the rt5033-battery sub-device from the MFD driver.
> There is no compile/runtime dependency of the power supply / MFD patch
> so they can just be applied separately through the power supply / MFD tree.

Thanks, I queued patches 1&2 to power-supply's for-next branch
and ignored patch 3.

-- Sebastian

> With these changes, rt5033-battery seems to work fine on the
> Samsung Galaxy A5 (2015) at least (it reports a reasonable
> battery percentage).
> 
> [1]: https://lore.kernel.org/linux-pm/1425864191-4121-3-git-send-email-beomho.seo@samsung.com/
> [2]: https://lore.kernel.org/lkml/20201110130047.8097-1-michael.srba@seznam.cz/
> 
> Changes in v2: Fix stupid typo in second patch :(
> v1: Honestly, not worth looking at :)
> 
> Stephan Gerhold (3):
>   dt-bindings: power: supply: Add DT schema for richtek,rt5033-battery
>   power: supply: rt5033_battery: Fix device tree enumeration
>   mfd: rt5033: Drop rt5033-battery sub-device
> 
>  .../power/supply/richtek,rt5033-battery.yaml  | 54 +++++++++++++++++++
>  drivers/mfd/rt5033.c                          |  3 --
>  drivers/power/supply/Kconfig                  |  3 +-
>  drivers/power/supply/rt5033_battery.c         |  7 +++
>  4 files changed, 63 insertions(+), 4 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/power/supply/richtek,rt5033-battery.yaml
> 
> -- 
> 2.31.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/3] mfd: rt5033: Drop rt5033-battery sub-device

[Stephan Gerhold]

Hi Lee,

On Wed, May 19, 2021 at 03:46:30PM +0100, Lee Jones wrote:
> On Mon, 17 May 2021, Stephan Gerhold wrote:
> 
> > 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 <beomho.seo@samsung.com>
> > Cc: Chanwoo Choi <cw00.choi@samsung.com>
> > Fixes: 0b271258544b ("mfd: rt5033: Add Richtek RT5033 driver core.")
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > ---
> >  drivers/mfd/rt5033.c | 3 ---
> >  1 file changed, 3 deletions(-)
> 
> Acked-by: Lee Jones <lee.jones@linaro.org>
> 

Since I mentioned in the cover letter that the MFD and power supply
changes can be applied independently, Sebastian only queued patch 1 and 2.

Can you queue this one through the MFD tree?

Thanks!
Stephan