Re: [PATCH] power: supply: bq27xxx: fix error return in case of no bq27000 hdq battery

[Jerry Lv <jerrylv@axis.com>]

Hello Nikolaus,

On 8/5/2025 5:28 PM, H. Nikolaus Schaller wrote:
> Hi Jerry,
>
>> Am 05.08.2025 um 10:53 schrieb Jerry Lv <Jerry.Lv@axis.com>:
>>
>>
>>
>>
>> ________________________________________
>> From: H. Nikolaus Schaller <hns@goldelico.com>
>> Sent: Monday, July 21, 2025 8:46 PM
>> To: Sebastian Reichel; Jerry Lv
>> Cc: Pali Rohár; linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org; letux-kernel@openphoenux.org; stable@vger.kernel.org; kernel@pyra-handheld.com; andreas@kemnade.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@goldelico.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@axis.com>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.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?
It's the data returned by i2c_transfer(). I have reported this issue to 
TI, and wait for their further investigation.
Not sure whether other gauges behave like this or not.
>> 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.
This is not only for bq27z561, it's the general mechanism in the driver 
bq27xxx_battery.c for all gauges:

        static int bq27xxx_battery_get_property() {

       ...

       if (psp != POWER_SUPPLY_PROP_PRESENT && di->cache.flags < 0)

             return di->cache.flags;

       }

>> 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.
Since this is the special check only needed for bq27000,

suggest to check the chip type before changing the cache.flags to 
-ENODEV manually, see my comments in later part.

>
>>         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?
This change works for my device, but just as you said, this change makes 
the existing code more difficult to understand.

Since changing the cache.flags to -ENODEV manually is only needed for 
the bq27000 with the HDQ driver,

suggest to check the chip type first like below:

        if ((di->chip == BQ27000) && (cache.flags & 0xff) == 0xff)

              cache.flags = -ENODEV; /* read error */


This will not break the well tested bq27000 logic, and also works fine 
with other gauges, and it's more easy to understand.
What's your opinion?
>
> BR and thanks,
> Nikolaus
>
Best Regards,

Jerry Lv