Message-Id: <2437B077-0F51-4724-8861-7E0BEE9DB5F0@goldelico.com>
Date: Tue, 5 Aug 2025 11:28:46 +0200
From: "H. Nikolaus Schaller" <hns@...delico.com>
To: Jerry Lv <Jerry.Lv@...s.com>
Cc: Sebastian Reichel <sre@...nel.org>,
 Pali Rohár <pali@...nel.org>,
 "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "letux-kernel@...nphoenux.org" <letux-kernel@...nphoenux.org>,
 "stable@...r.kernel.org" <stable@...r.kernel.org>,
 "kernel@...a-handheld.com" <kernel@...a-handheld.com>,
 "andreas@...nade.info" <andreas@...nade.info>,
 Hermes Zhang <Hermes.Zhang@...s.com>
Subject: Re: [PATCH] power: supply: bq27xxx: fix error return in case of no
 bq27000 hdq battery

Hi Jerry,

> Am 05.08.2025 um 10:53 schrieb Jerry Lv <Jerry.Lv@...s.com>:
> 
> 
> 
> 
> ________________________________________
> From: H. Nikolaus Schaller <hns@...delico.com>
> Sent: Monday, July 21, 2025 8:46 PM
> To: Sebastian Reichel; Jerry Lv
> Cc: Pali Rohár; linux-pm@...r.kernel.org; linux-kernel@...r.kernel.org; letux-kernel@...nphoenux.org; stable@...r.kernel.org; kernel@...a-handheld.com; andreas@...nade.info; H. Nikolaus Schaller
> Subject: [PATCH] power: supply: bq27xxx: fix error return in case of no bq27000 hdq battery
> 
> [You don't often get email from hns@...delico.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> Since commit
> 
> commit f16d9fb6cf03 ("power: supply: bq27xxx: Retrieve again when busy")
> 
> the console log of some devices with hdq but no bq27000 battery
> (like the Pandaboard) is flooded with messages like:
> 
> [   34.247833] power_supply bq27000-battery: driver failed to report 'status' property: -1
> 
> as soon as user-space is finding a /sys entry and trying to read the
> "status" property.
> 
> It turns out that the offending commit changes the logic to now return the
> value of cache.flags if it is <0. This is likely under the assumption that
> it is an error number. In normal errors from bq27xxx_read() this is indeed
> the case.
> 
> But there is special code to detect if no bq27000 is installed or accessible
> through hdq/1wire and wants to report this. In that case, the cache.flags
> are set (historically) to constant -1 which did make reading properties
> return -ENODEV. So everything appeared to be fine before the return value was
> fixed. Now the -1 is returned as -ENOPERM instead of -ENODEV, triggering the
> error condition in power_supply_format_property() which then floods the
> console log.
> 
> So we change the detection of missing bq27000 battery to simply set
> 
>        cache.flags = -ENODEV
> 
> instead of -1.
> 
> Fixes: f16d9fb6cf03 ("power: supply: bq27xxx: Retrieve again when busy")
> Cc: Jerry Lv <Jerry.Lv@...s.com>
> Cc: stable@...r.kernel.org
> Signed-off-by: H. Nikolaus Schaller <hns@...delico.com>
> ---
> drivers/power/supply/bq27xxx_battery.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> index 93dcebbe11417..efe02ad695a62 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -1920,7 +1920,7 @@ static void bq27xxx_battery_update_unlocked(struct bq27xxx_device_info *di)
> 
>        cache.flags = bq27xxx_read(di, BQ27XXX_REG_FLAGS, has_singe_flag);
>        if ((cache.flags & 0xff) == 0xff)
> -               cache.flags = -1; /* read error */
> +               cache.flags = -ENODEV; /* read error */
>        if (cache.flags >= 0) {
>                cache.capacity = bq27xxx_battery_read_soc(di);
> 
> --
> 2.50.0
> 
> 
> 
> In our device, we use the I2C to get data from the gauge bq27z561. 
> During our test, when try to get the status register by bq27xxx_read() in the bq27xxx_battery_update_unlocked(), 
> we found sometimes the returned value is 0xFFFF, but it will update to some other value very quickly.

Strange. Do you have an idea if this is an I2C communication effect or really reported from the bq27z561 chip?

> So the returned 0xFFFF does not indicate "No such device", if we force to set the cache.flags to "-ENODEV" or "-1" manually in this case, 
> the bq27xxx_battery_get_property() will just return the cache.flags until it is updated at lease 5 seconds later,
> it means we cannot get any property in these 5 seconds.

Ok I see. So there should be a different rule for the bq27z561.

> 
> In fact, for the I2C driver, if no bq27000 is installed or accessible, 
> the bq27xxx_battery_i2c_read() will return "-ENODEV" directly when no device,
> or the i2c_transfer() will return the negative error according to real case.

Yes, that is what I2C can easily report. But for AFAIK for HDQ there is no -ENODEV
detection in the protocol. So the bq27000 has this special check.

> 
>        bq27xxx_battery_i2c_read() {
>                ...
>        if (!client->adapter)
>         return -ENODEV;
>                ...
>                ret = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
>                ...
>                if (ret < 0)
>        return ret;
>                ...
>        }
> 
> But there is no similar check in the bq27xxx_battery_hdq_read() for the HDQ/1-wire driver.
> 
> Could we do the same check in the bq27xxx_battery_hdq_read(),
> instead of changing the cache.flags manually when the last byte in the returned data is 0xFF?

So your suggestion is to modify bq27xxx_battery_hdq_read to check for BQ27XXX_REG_FLAGS and
value 0xff and convert to -ENODEV?

Well, it depends on the data that has been successfully reported. So making bq27xxx_battery_hdq_read()
have some logic to evaluate the data seems to just move the problem to a different place.
Especially as this is a generic function that can read any register it is counter-intuitive to
analyse the data.

> Or could we just force to set the returned value to "-ENODEV" only when the last byte get from bq27xxx_battery_hdq_read() is 0xFF?

In summary I am not sure if that improves anything. It just makes the existing code more difficult 
to understand.

What about checking bq27xxx_battery_update_unlocked() for

       if (!(di->opts & BQ27Z561_O_BITS) && (cache.flags & 0xff) == 0xff)

to protect your driver from this logic?

This would not touch or break the well tested bq27000 logic and prevent the new bq27z561
driver to trigger a false positive?

BR and thanks,
Nikolaus

Powered by blists - more mailing lists