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

[Sebastian Reichel]

Hi,

On Tue, May 09, 2023 at 03:01:32AM +0200, Jakob Hauser wrote:
> On 09.05.23 00:06, Sebastian Reichel wrote:
> > 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.

It should already be part of the binding, because richtek,rt5033-battery.yaml has

allOf:
  - $ref: power-supply.yaml#

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

Replace the dev_err() with dev_err_probe():

if (IS_ERR(battery->psy))
    return dev_err_probe(&client->dev, PTR_ERR(battery->psy), "Failed to register power supply\n");

That will avoid printing an error for -EPROBE_DEFER.

Greetings,

-- Sebastian

Attachment: signature.asc
Description: PGP signature