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

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

[H. Nikolaus Schaller]

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

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

[Jerry Lv]

________________________________________
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.
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.

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.

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


Best Regards,
Jerry Lv

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

[H. Nikolaus Schaller]

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?

> 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

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

[Jerry Lv]

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

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

[H. Nikolaus Schaller]

Hi Jerry,
sorry it appears that I have missed your mail.

> Am 08.08.2025 um 11:13 schrieb 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:

But the "bug" is only for the bq27z561.

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

It may be a matter of a better code comment (see below)...

> 
> 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 */

Ok, this depends on what the gauges != bq27000 and != bq27Z561 would expect. This is something I
do not know, but maybe the community / maintainers.

And, there is a comment for the definition of BQ27000 that it is also meant for the bq27200, which has an i2c interface.
That one is obsolete but still may be used somewhere...

But if it does never return 0xff for the status register it would be fine for me.

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

I can't acccess the git repo at the moment, so here is what think this would mean:

@@ -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)
+      if ((di->chip == BQ27000) && (cache.flags & 0xff) == 0xff)
-               cache.flags = -1; /* read error */
+               cache.flags = -ENODEV; /* bq27000 hdq read error */
       if (cache.flags >= 0) {
               cache.capacity = bq27xxx_battery_read_soc(di);

With the changed comment it should be more clear what it is doing and why.

Maybe can you test this? As soon as I can test, I'll report and submit a v2.

BR and thanks,
Nikolaus

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

[H. Nikolaus Schaller]

Hi Jerry,

> Am 05.08.2025 um 10:53 schrieb Jerry Lv <Jerry.Lv@axis.com>:
> 
> 
> 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?
> 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?

I tried to move the 0xff detection to bq27xxx_battery_hdq_read() and make it trigger only
for register 0x0a (BQ27XXX_REG_FLAGS), but there are other locations where bq27xxx_read()
is called for this register. And those emit error messages in case the battery is removed
while user-space is polling.

So I'll post a v2 with two patches (for different bugs):
a) set cache.flags to -ENODEV to fix the -EPERM bug
b) restrict the check for the 0xff condition to the bq27000 to avoid false positives for your bq27z561

BR and thanks,
Nikolaus

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

[Andreas Kemnade]

Hi,

Am Mon, 21 Jul 2025 14:46:09 +0200
schrieb "H. Nikolaus Schaller" <hns@goldelico.com>:

> 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.
> 
This all is a bit inconsistent, the offending commit makes it worse. 
Normally devices appear only in /sys if they exist. Regarding stuff in
/sys/class/power_supply, input power supplies might be there or not,
but there you can argument that the entry in /sys/class/power_supply
only means that there is a connector for connecting a supply.
But having the battery entry everywhere looks like waste. If would
expect the existence of a battery bay in the device where the common
battery is one with a bq27xxx.

Regards,
Andreas

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

[H. Nikolaus Schaller]

> Am 21.08.2025 um 20:15 schrieb Andreas Kemnade <andreas@kemnade.info>:
> 
> Hi,
> 
> Am Mon, 21 Jul 2025 14:46:09 +0200
> schrieb "H. Nikolaus Schaller" <hns@goldelico.com>:
> 
>> 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.
>> 
> This all is a bit inconsistent, the offending commit makes it worse. 
> Normally devices appear only in /sys if they exist. Regarding stuff in
> /sys/class/power_supply, input power supplies might be there or not,
> but there you can argument that the entry in /sys/class/power_supply
> only means that there is a connector for connecting a supply.

Indeed. If there is an optional bq27000 hdq battery the entry exists.

> But having the battery entry everywhere looks like waste. If would
> expect the existence of a battery bay in the device where the common
> battery is one with a bq27xxx.

I think the flaw you are mentioning is a completely diffent one. It comes from that
the 1-wire or hdq interface of some omap processors is enabled in the .dtsi by default
instead of disabling it like other interfaces (e.g. mcbsp1). E.g. for omap3 hdqw1w:

https://elixir.bootlin.com/linux/v6.16.1/source/arch/arm/boot/dts/ti/omap/omap3.dtsi#L502

And we should have the dts for the boards enable it only if the hdq interface is really
in use and there is a chance that a bq27000 can be connected. In that case the full
/sys entry is prepared but returns -ENODEV if the battery is missing, which is then
exactly the right error return (instead of -EPERM triggering the console message).

Or is there something related to power-management, so that the hdq silicon always
needs the hdq driver to properly idle?

Anyways I think this is a different topic for separate cleanup. For the moment we
should fix the hdq27000 missing battery detection which was broken by changing
the return value logic.

BR,
Nikolaus

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

[Andreas Kemnade]

Am Thu, 21 Aug 2025 20:54:41 +0200
schrieb "H. Nikolaus Schaller" <hns@goldelico.com>:

> > Am 21.08.2025 um 20:15 schrieb Andreas Kemnade <andreas@kemnade.info>:
> > 
> > Hi,
> > 
> > Am Mon, 21 Jul 2025 14:46:09 +0200
> > schrieb "H. Nikolaus Schaller" <hns@goldelico.com>:
> >   
> >> 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.
> >>   
> > This all is a bit inconsistent, the offending commit makes it worse. 
> > Normally devices appear only in /sys if they exist. Regarding stuff in
> > /sys/class/power_supply, input power supplies might be there or not,
> > but there you can argument that the entry in /sys/class/power_supply
> > only means that there is a connector for connecting a supply.  
> 
> Indeed. If there is an optional bq27000 hdq battery the entry exists.
> 
Which is the condition that there is an optional bq27000 battery?
w1 might be enabled for other reasons. The bq27000 is not the only w1
chip in the world. BTW: I have removed the battery from my macbook and
there is no battery entry in /sys/class/power_supply. Same with another
laptop.

> > But having the battery entry everywhere looks like waste. If would
> > expect the existence of a battery bay in the device where the common
> > battery is one with a bq27xxx.  
> 
> I think the flaw you are mentioning is a completely diffent one. It comes from that
> the 1-wire or hdq interface of some omap processors is enabled in the .dtsi by default
> instead of disabling it like other interfaces (e.g. mcbsp1). E.g. for omap3 hdqw1w:
> 
> https://elixir.bootlin.com/linux/v6.16.1/source/arch/arm/boot/dts/ti/omap/omap3.dtsi#L502
> 
> And we should have the dts for the boards enable it only if the hdq interface is really
> in use and there is a chance that a bq27000 can be connected. In that case the full
> /sys entry is prepared but returns -ENODEV if the battery is missing, which is then
> exactly the right error return (instead of -EPERM triggering the console message).
>

And why do you think bq27000 should behave different than
max1721x_battery or ds2780_battery or ds2781_battery? If I enable the
drivers there is no additional battery in /sys/class/power_supply! Why
should everyone have a bq27000 in /sys/class/power_supply if the driver
is enabled and w1 is used for something? I wonder if the -ENODEV should
be catched earlier.

Regards,
Andreas

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

[H. Nikolaus Schaller]

Hi,

> Am 21.08.2025 um 22:05 schrieb Andreas Kemnade <andreas@kemnade.info>:
> 
> Am Thu, 21 Aug 2025 20:54:41 +0200
> schrieb "H. Nikolaus Schaller" <hns@goldelico.com>:
> 
>>> Am 21.08.2025 um 20:15 schrieb Andreas Kemnade <andreas@kemnade.info>:
>>> 
>>> Hi,
>>> 
>>> Am Mon, 21 Jul 2025 14:46:09 +0200
>>> schrieb "H. Nikolaus Schaller" <hns@goldelico.com>:
>>> 
>>>> 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.
>>>> 
>>> This all is a bit inconsistent, the offending commit makes it worse. 
>>> Normally devices appear only in /sys if they exist. Regarding stuff in
>>> /sys/class/power_supply, input power supplies might be there or not,
>>> but there you can argument that the entry in /sys/class/power_supply
>>> only means that there is a connector for connecting a supply.  
>> 
>> Indeed. If there is an optional bq27000 hdq battery the entry exists.
>> 
> Which is the condition that there is an optional bq27000 battery?

If there is no bq27000 battery, hdq reads 8 bits of 0xff (no client on the
1 bit serial bus with pull-up) as the "value" of the battery status register.
If there is a battery connected, the value is defined and not 0xff.

See 3dd843e1c26a023dc8d776e5d984c635c642785f

> w1 might be enabled for other reasons. The bq27000 is not the only w1
> chip in the world.

In these cases the bq27xxx driver does not need to be present and does
not interfere.

BTW: the bq27000 is unconditionally added to the hdq subsystem as soon as
CONFIG_BATTERY_BQ27XXX_HDQ is configured. On every system. So the alternative
to disabling hdq on the processor and enabling in the board specific DTB is
to unconfigure CONFIG_BATTERY_BQ27XXX_HDQ if hdq is used for something else.

> BTW: I have removed the battery from my macbook and
> there is no battery entry in /sys/class/power_supply. Same with another
> laptop.

Does the entry disappear if you remove the battery while powered from AC and
come back on re-insertion of the battery?

Do they use hdq at all? With i2c it is easier to detect a "no response" during
probe or operation. But still i2c assumes that a chip responds at boot
or never (or user-space must run a timer to reprobe every now and then).

IMHO they are not prepared to handle the use case we have for the bq27000
and should therefore not be the role model.

> 
>>> But having the battery entry everywhere looks like waste. If would
>>> expect the existence of a battery bay in the device where the common
>>> battery is one with a bq27xxx.  
>> 
>> I think the flaw you are mentioning is a completely diffent one. It comes from that
>> the 1-wire or hdq interface of some omap processors is enabled in the .dtsi by default
>> instead of disabling it like other interfaces (e.g. mcbsp1). E.g. for omap3 hdqw1w:
>> 
>> https://elixir.bootlin.com/linux/v6.16.1/source/arch/arm/boot/dts/ti/omap/omap3.dtsi#L502
>> 
>> And we should have the dts for the boards enable it only if the hdq interface is really
>> in use and there is a chance that a bq27000 can be connected. In that case the full
>> /sys entry is prepared but returns -ENODEV if the battery is missing, which is then
>> exactly the right error return (instead of -EPERM triggering the console message).
>> 
> 
> And why do you think bq27000 should behave different than
> max1721x_battery or ds2780_battery or ds2781_battery?

I have looked into the ds2780 code but do not understand how they handle the case
that the battery is removed or swapped or inserted during operation (while on external
power supply).

The max1721x is different. At least from looking into code it behaves exactly the same
as the bq27000. There is a POWER_SUPPLY_PROP_PRESENT. Which can always be read. It does
this by reading the MAX172XX_REG_STATUS and detecting that some bit (MAX172XX_BAT_PRESENT)
is not set. This can either mean no battery connected to the chip or the chip (built into
the battery case) is not connected to the hdq bus.

I can not test but would therefore assume the same for the max1721. As soon as you configure
it for a hdq capable device/kernel there should be a /sys entry for it with present == 0.

BTW: all bq27xxx gauges have this property, not only the bq27000.

> If I enable the
> drivers there is no additional battery in /sys/class/power_supply!

Which is surprising because then the max1721x can never report "no battery present".
Unless it is always sitting on the main board and the battery is connected or not.

Or there is some special logic in the max1721x probe which can detect during boot that
the chip is really connected. But then you can not remove a battery with built-in
max1721x because it must be installed on first boot and can not be inserted later.

> Why
> should everyone have a bq27000 in /sys/class/power_supply if the driver
> is enabled and w1 is used for something? I wonder if the -ENODEV should
> be catched earlier.

What do you mean with "catched earlier"? What is your proposal?

Well, as proposed by Jerry earlier, it appears as if it can also be handled in bq27xxx_battery_hdq_read()
by detecting the register BQ27XXX_REG_FLAGS and the read value 0xff and return -ENODEV.

Then it would be constrained to the bq27000 - but still not solve your issue that boards
may not have a bq27000 option on their hdq bus...

Anyways this discussion now goes far beyond fixing the regression introduced by

f16d9fb6cf03 ("power: supply: bq27xxx: Retrieve again when busy")

So I'd suggest to fix the regression first and then add new functions or changes (for handling
removable chips and removable batteries reasonably) in a separate patch or series. Having separate
patches improves bisectability and is easier to track what has been changed (for different reasons).

BR,
Nikolaus

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

[H. Nikolaus Schaller]

Hi,

> Am 22.08.2025 um 08:51 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
> 
> 
> What do you mean with "catched earlier"? What is your proposal?
> 
> Well, as proposed by Jerry earlier, it appears as if it can also be handled in bq27xxx_battery_hdq_read()
> by detecting the register BQ27XXX_REG_FLAGS and the read value 0xff and return -ENODEV.

I tried this but there are more locations where BQ27XXX_REG_FLAGS are read and where the reading
code is not prepared to receive an -ENODEV. This will for example emit

[  293.389831] w1_slave_driver 01-000000000000: error reading flags

each time the battery is removed. And in some race cases (a read of the full /sys properties is
already in progress), there may be more than one such message. That is not nice to replace one
console message with another...

So I am not sure if it is a good idea to make the lowest layer ("catched earlier") read function
detect -ENODEV based on the register number. It can not know what the next layer wants to do with
the result.

BR,
Nikolaus