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

[H. Nikolaus Schaller]

Another comment (in code section)

> Am 27.02.2025 um 18:14 schrieb H. Nikolaus Schaller <hns at goldelico.com>:
> 
> Hi,
> 
> 
>> Am 07.02.2025 um 22:51 schrieb Sicelo A. Mhlongo <absicsz at gmail.com>:
>> 
>> On the bq27x00 and bq27x10 variants, a number of conditions may reset the value
>> stored in the NAC register. This will cause capacity, energy, and charge to
>> report the value 0, even when the battery is full. On the other hand, the chip
>> also provides a flag, EDVF, which accurately detects the true battery empty
>> condition, when capacity, charge, and energy are really 0.  Therefore, discard
>> readings for these properties if their value is 0 while EDVF is unset.
>> 
>> Tested on the Nokia N900 with bq27200.
> 
> 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.
> 
> BR and thanks,
> Nikolaus
> 
>> Signed-off-by: Sicelo A. Mhlongo <absicsz at gmail.com>
>> ---
>> drivers/power/supply/bq27xxx_battery.c | 17 +++++++++++++++--
>> 1 file changed, 15 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
>> index 90a5bccfc6b9..df92f55c63f6 100644
>> --- a/drivers/power/supply/bq27xxx_battery.c
>> +++ b/drivers/power/supply/bq27xxx_battery.c
>> @@ -2115,6 +2115,10 @@ static int bq27xxx_battery_get_property(struct power_supply *psy,
>> break;
>> case POWER_SUPPLY_PROP_CAPACITY:
>> ret = bq27xxx_simple_value(di->cache.capacity, val);
>> + /* If 0 is reported, it is expected that EDVF is also set */
>> + if (!ret && di->opts & BQ27XXX_O_ZERO &&
>> +   !(di->cache.flags & BQ27000_FLAG_EDVF))
>> + return -EINVAL;

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.

>> break;
>> case POWER_SUPPLY_PROP_CAPACITY_LEVEL:
>> ret = bq27xxx_battery_capacity_level(di, val);
>> @@ -2138,10 +2142,15 @@ static int bq27xxx_battery_get_property(struct power_supply *psy,
>> val->intval = POWER_SUPPLY_TECHNOLOGY_LION;
>> break;
>> case POWER_SUPPLY_PROP_CHARGE_NOW:
>> - if (di->regs[BQ27XXX_REG_NAC] != INVALID_REG_ADDR)
>> + if (di->regs[BQ27XXX_REG_NAC] != INVALID_REG_ADDR) {
>> ret = bq27xxx_battery_read_nac(di, val);
>> - else
>> + /* If 0 is reported, it is expected that EDVF is also set */
>> + if (!ret && di->opts & BQ27XXX_O_ZERO &&
>> +   !(di->cache.flags & BQ27000_FLAG_EDVF))
>> + return -EINVAL;
>> + } else {
>> ret = bq27xxx_battery_read_rc(di, val);
>> + }
>> break;
>> case POWER_SUPPLY_PROP_CHARGE_FULL:
>> ret = bq27xxx_battery_read_fcc(di, val);
>> @@ -2163,6 +2172,10 @@ static int bq27xxx_battery_get_property(struct power_supply *psy,
>> break;
>> case POWER_SUPPLY_PROP_ENERGY_NOW:
>> ret = bq27xxx_battery_read_energy(di, val);
>> + /* If 0 is reported, it is expected that EDVF is also set */
>> + if (!ret && di->opts & BQ27XXX_O_ZERO &&
>> +   !(di->cache.flags & BQ27000_FLAG_EDVF))
>> + return -EINVAL;
>> break;
>> case POWER_SUPPLY_PROP_POWER_AVG:
>> ret = bq27xxx_battery_pwr_avg(di, val);
>> -- 
>> 2.47.2
>> 
>> 
> 
>