Re: [PATCH v4 7/8] power: supply: rt5033_battery: Adopt status property from charger

[Jakob Hauser]

Hi Sebastian,

On 09.05.23 00:06, Sebastian Reichel wrote:
> Hi,
>
> On Mon, May 08, 2023 at 11:18:28PM +0200, Jakob Hauser wrote:
> > On 08.05.23 13:35, Sebastian Reichel wrote:
> > > On Sat, May 06, 2023 at 05:54:34PM +0200, Jakob Hauser wrote:
> > > > The rt5033-battery fuelgauge can't get a status by itself. The rt5033-charger
> > > > can, let's get this value.
> > > >
> > > > Tested-by: Raymond Hackley <raymondhackley@xxxxxxxxxxxxxx>
> > > > Signed-off-by: Jakob Hauser <jahau@xxxxxxxxxxxxxx>
> > > > ---
> > > >    drivers/power/supply/rt5033_battery.c | 24 ++++++++++++++++++++++++
> > > >    1 file changed, 24 insertions(+)
> > > >
> > > > diff --git a/drivers/power/supply/rt5033_battery.c b/drivers/power/supply/rt5033_battery.c
> > > > index 5c04cf305219..a6520716d813 100644
> > > > --- a/drivers/power/supply/rt5033_battery.c
> > > > +++ b/drivers/power/supply/rt5033_battery.c
> > > > @@ -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 POWER_SUPPLY_STATUS_UNKNOWN;
> > > > +
> > > > +	ret = power_supply_get_property(charger, POWER_SUPPLY_PROP_STATUS, &val);
> > > > +	if (ret) {
> > > > +		power_supply_put(charger);
> > > > +		return POWER_SUPPLY_STATUS_UNKNOWN;
> > > > +	}
> > >
> > > struct rt5033_battery *battery = i2c_get_clientdata(client);
> > > ret = power_supply_get_property_from_supplier(battery->psy, POWER_SUPPLY_PROP_STATUS, &val);
> > > if (ret)
> > >       val.intval = POWER_SUPPLY_STATUS_UNKNOWN;
> >
> > I don't think this works. There is no direct relationship between
> > rt5033-charger and rt5033-battery. They operate independently from each
> > other.
>
> That should be fine as long as the supply dependency is properly declared.
>
> > I had a short try and the status property of rt5033-battery was "unknown".
> >
> > Just for the record, the full function I tried was:
> >
> > static int rt5033_battery_get_status(struct i2c_client *client)
> > {
> >          struct rt5033_battery *battery = i2c_get_clientdata(client);
> >          union power_supply_propval val;
> >          int ret;
> >
> >          ret = power_supply_get_property_from_supplier(battery->psy,
> >                                               POWER_SUPPLY_PROP_STATUS,
> >                                               &val);
> >          if (ret)
> >                  val.intval = POWER_SUPPLY_STATUS_UNKNOWN;
> >
> >          return val.intval;
> > }
> >
> > Later on I added a read-out of the "ret" value. It is "-19". I guess that's
> > the "return -ENODEV;" from function
> > power_supply_get_property_from_supplier(). [2]
> >
> > [2] https://github.com/torvalds/linux/blob/v6.4-rc1/drivers/power/supply/power_supply_core.c#L397-L421
>
> I suppose your DT is missing the connection between the charger and
> the battery:
>
> rt5033_charger: charger {
>      compatible = "rt5033-charger";
>      ...
> }
>
> fuel-gauge {
>      compatible = "rt5033-battery";
>      ...
>      power-supplies = <&rt5033_charger>; // you are probably missing this
> };
>
> See also Documentation/devicetree/bindings/power/supply/power-supply.yaml

...

Thanks for the hints.

This leads to updating the dt-bindings because adding the 
"power-supplies" property is important to be aware of.

Btw. first it didn't work. It took me quite some time to debug. I needed 
to add "psy_cfg.of_node = client->dev.of_node;" to the rt5033-battery 
probe function.

Now it works. However, there is a new problem. The battery driver gets 
probed first. The charger driver a bit later. In the meantime the 
battery driver spams dmesg with several "Failed to register power 
supply" because the charger driver isn't available yet. Once the charger 
driver is there, it works fine and dmesg becomes silent.

With the current state of the patchset:
dmesg | grep rt5033
[   13.628030] rt5033 6-0034: Device found (rev. 6)
[   13.633552] rt5033-led: Failed to locate of_node [id: -1]
[   13.790478] rt5033-charger rt5033-charger: DMA mask not set

With the changes discussed here:
dmesg | grep rt5033
[   15.741915] rt5033-battery 4-0035: Failed to register power supply
[   15.752894] rt5033-battery 4-0035: Failed to register power supply
[   15.795458] rt5033-battery 4-0035: Failed to register power supply
[   15.910760] rt5033-battery 4-0035: Failed to register power supply
[   15.913187] rt5033 6-0034: Device found (rev. 6)
[   15.914341] rt5033-led: Failed to locate of_node [id: -1]
[   15.920052] rt5033-battery 4-0035: Failed to register power supply
[   15.927262] rt5033-battery 4-0035: Failed to register power supply
[   16.017131] rt5033-battery 4-0035: Failed to register power supply
[   16.017401] rt5033-charger rt5033-charger: DMA mask not set

The message is comming from the rt5033-battery probe function, it's the 
power_supply_register() that fails.

Any ideas what could be done about this?

Kind regards,
Jakob