[Letux-kernel] [PATCH] power: supply: bq27xxx: do not report bogus zero values

[H. Nikolaus Schaller]

Hi,

> Am 28.02.2025 um 09:02 schrieb Sicelo <absicsz at gmail.com>:
> 
> Hi
> 
> On Thu, Feb 27, 2025 at 06:48:49PM +0100, H. Nikolaus Schaller wrote:
>>> 
>>> I finally found time to test this patch on the GTA04 which uses an OpenMoko HF08
>>> battery which comes with a built-in bq27000.
>>> 
>>> The result is that I can't read the /sys/class/power_supply/bq27000-battery/charge_now
>>> at all and always get EINVAL. Independently of the charge level reported without
>>> your patch.
>>> 
>>> If I remove this patch again, the values are reasonable as in all the years before.
>>> 
>>> What I don't know is how and to which values the EEPROM of the bq27000 has been
>>> factory programmed so that the HF08 battery maybehave differently from yours.
>>> 
>>> Finally, I am not aware of reports from GTA04 users that the value stored in the NAC
>>> register is being reset as you describe and capacity, energy and charge to be falsely
>>> reported as 0.
>>> 
>>> So please restrict this patch to the N900.
> 
> In the case of the HF08, the value will not get reset since the chip is
> inside the battery as describe.

Ah, ok. Of course it obviously will be reset if the chip becomes disconnected from all power.

> However, the patch should be fine for
> HF08 and N900, etc., since the EINVAL should only be emitted when it has
> been reset. Unfortunately, I made the mistake below...
> 
>> 
>> You write:
>> 
>>  Therefore, discard readings for these properties if their value is 0 while EDVF is unset.
>> 
>> but the 'val' is not checked at all. Only the error return value
>> 'ret' of bq27xxx_simple_value() which is 0 if reading was ok.
>> 
>> So shouldn't that be:
>> 
>> + if (!val->intval && di->opts & BQ27XXX_O_ZERO &&
>> +   !(di->cache.flags & BQ27000_FLAG_EDVF))
>> + return -EINVAL;
>> 
>> This could explain why I always get an -EINVAL here since BQ27000_FLAG_EDVF is likely not set.
> 
> You are absolutely correct, Thank you. It seems I spent more time
> testing the non-working condition than the working condition. Apologies.

No worry. That is what review is good for.

> 
> Will you submit the fix, or I should?

I think the patch has not yet been merged anywhere (but I am not sure), so
you better can send a v2 of the series.

And, I think I have found another potential issue.

The function bq27xxx_simple_value() returns the value passed as the first
parameter if it is negative (i.e. read error). And, then, it is not copies
to val->intval.

In that case val->intval may still be 0 and an -EINVAL may be returned
instead of the error return from what is passed as the first parameter to
bq27xxx_simple_value().

So I think it could be better to not check for 

>> if (!val->intval && 

but

>> if (!di->cache.capacity && 

Alternatively if it should not rely on knowledge about the internaly of
bq27xxx_simple_value()

>> + if (!ret && !val->intval && di->opts & BQ27XXX_O_ZERO &&

Or even a third and maybe best alternative: do the check before calling
bq27xxx_simple_value(). Since setting val->intval and then returning an
error is not needed.


 case POWER_SUPPLY_PROP_CAPACITY:
	/* If 0 is reported, it is expected that EDVF is also set */
	if (!di->cache.capacity && di->opts & BQ27XXX_O_ZERO &&
		!(di->cache.flags & BQ27000_FLAG_EDVF))
		return -EINVAL;
 	ret = bq27xxx_simple_value(di->cache.capacity, val);
	break;

etc.

This should still report the ret error if di->cache.capacity < 0.

BR and thanks,
Nikolaus