| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 9 May 2023 03:01:32 +0200
From: Jakob Hauser <jahau@...ketmail.com>
To: Sebastian Reichel <sebastian.reichel@...labora.com>
Cc: Lee Jones <lee@...nel.org>, Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Beomho Seo <beomho.seo@...sung.com>,
Chanwoo Choi <cw00.choi@...sung.com>,
Stephan Gerhold <stephan@...hold.net>,
Raymond Hackley <raymondhackley@...tonmail.com>,
Pavel Machek <pavel@....cz>, Axel Lin <axel.lin@...ics.com>,
ChiYuan Huang <cy_huang@...htek.com>,
Linus Walleij <linus.walleij@...aro.org>,
Henrik Grimler <henrik@...mler.se>, linux-pm@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
phone-devel@...r.kernel.org, ~postmarketos/upstreaming@...ts.sr.ht
Subject: Re: [PATCH v4 7/8] power: supply: rt5033_battery: Adopt status
property from charger
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@...tonmail.com>
>>>> Signed-off-by: Jakob Hauser <jahau@...ketmail.com>
>>>> ---
>>>> 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
Powered by blists - more mailing lists