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
