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

[H. Nikolaus Schaller]

Hi Sicelo,

> Am 22.03.2025 um 16:43 schrieb Sicelo A. Mhlongo <absicsz at gmail.com>:
> 
> On the bq27x00 and bq27x10 variants, a number of conditions can reset the
> value stored in the NAC register. This will cause capacity, time-to-empty,
> energy, and charge to report the value 0, even when the battery is full.
> On the other hand, the chips provide a flag, EDVF, which reliably reports
> the true battery empty condition, when these properties are really zero.
> Therefore, discard readings for these properties if their value is 0 while
> EDVF is unset.
> 
> Tested on the Nokia N900 with bq27200.
> 
> Signed-off-by: Sicelo A. Mhlongo <absicsz at gmail.com>
> ---
> drivers/power/supply/bq27xxx_battery.c | 24 ++++++++++++++++++++++--
> 1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> index 2f31d750a4c1..8e5795c5754e 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -2107,6 +2107,15 @@ static int bq27xxx_battery_read_dmin_volt(struct bq27xxx_device_info *di,
> return 0;
> }
> 
> +static bool bq27xxx_value_is_valid(struct bq27xxx_device_info *di, int value)
> +{
> + /*
> + * On bq27xxx_0_zero, consider zero values invalid if EDVF is not set
> + */
> + return value || !(di->opts & BQ27XXX_O_ZERO) ||
> +       (di->cache.flags & BQ27000_FLAG_EDVF);

Hm. This still can return -EINVAL for a valid and successful readout of value == 0 which
could happen for a fully drained HF08 in the GTA04 with bq27000.

Here is an excerpt from running a battery until depletion:

root at letux:~# echo off >/sys/class/power_supply/twl4030_usb/mode
root at letux:~# while true; do echo $(date) $(cat /sys/class/power_supply/bq27000-battery/capacity)%; sleep 1; done

Mon Mar 24 14:37:11 UTC 2025 1%
Mon Mar 24 14:37:12 UTC 2025 1%
Mon Mar 24 14:37:13 UTC 2025 1%
...

Mon Mar 24 14:38:24 UTC 2025 1%
Mon Mar 24 14:38:25 UTC 2025 1%
Mon Mar 24 14:38:26 UTC 2025 1%
Mon Mar 24 14:38:27 UTC 2025 1%
cat: /sys/class/power_supply/bq27000-battery/capacity: Invalid argument
Mon Mar 24 14:38:28 UTC 2025 %
cat: /sys/class/power_supply/bq27000-battery/capacity: Invalid argument
Mon Mar 24 14:38:29 UTC 2025 %
cat: /sys/class/power_supply/bq27000-battery/capacity: Invalid argument
Mon Mar 24 14:38:30 UTC 2025 %
cat: /sys/class/power_supply/bq27000-battery/capacity: Invalid argument
Mon Mar 24 14:38:32 UTC 2025 %
...
cat: /sys/class/power_supply/bq27000-battery/capacity: Invalid argument
Mon Mar 24 14:53:39 UTC 2025 %
cat: /sys/class/power_supply/bq27000-battery/capacity: Invalid argument
Mon Mar 24 14:53:40 UTC 2025 %
cat: /sys/class/power_supply/bq27000-battery/capacity: Invalid argument
Mon Mar 24 14:53:41 UTC 2025 %
cat: /sys/class/power_supply/bq27000-battery/capacity: Invalid argument
Mon Mar 24 14:53:42 UTC 2025 %
Mon Mar 24 14:53:43 UTC 2025 0%
Mon Mar 24 14:53:44 UTC 2025 0%
Mon Mar 24 14:53:45 UTC 2025 0%
Mon Mar 24 14:53:46 UTC 2025 0%
Mon Mar 24 14:53:47 UTC 2025 0%
Mon Mar 24 14:53:48 UTC 2025 0%
Mon Mar 24 14:53:49 UTC 2025 0%
Mon Mar 24 14:53:50 UTC 2025 0%
Mon Mar 24 14:53:51 UTC 2025 0%
Mon Mar 24 14:53:53 UTC 2025 0%

...

Mon Mar 24 15:10:12 UTC 2025 0%
Mon Mar 24 15:10:14 UTC 2025 0%
Mon Mar 24 15:10:15 UTC 2025 0%
--- here device did shut down ---

This means that the capacity can go to 0% for approx. 15 minutes before
the BQ27000_FLAG_EDVF becomes set and 0% is reported. I would prefer not
to see the EINVAL in between.

This all may end that we have to add another DT property to tell the driver
if the bq27xxx is inside a battery or the battery is removeable and only check
for value=0 in the latter case?

Or can you limit this logic to the bq27200 because it is unlikely that the
i2c variant is used inside a battery pack and the HF08 battery uses the hdq
variant?

BR,
Nikolaus

> +}
> +
> static int bq27xxx_simple_value(int value,
> union power_supply_propval *val)
> {
> @@ -2147,6 +2156,8 @@ static int bq27xxx_battery_get_property(struct power_supply *psy,
> ret = bq27xxx_battery_current_and_status(di, val, NULL, NULL);
> break;
> case POWER_SUPPLY_PROP_CAPACITY:
> + if (!bq27xxx_value_is_valid(di, di->cache.capacity))
> + return -EINVAL;
> ret = bq27xxx_simple_value(di->cache.capacity, val);
> break;

> case POWER_SUPPLY_PROP_CAPACITY_LEVEL:
> @@ -2157,9 +2168,13 @@ static int bq27xxx_battery_get_property(struct power_supply *psy,
> break;
> case POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW:
> ret = bq27xxx_battery_read_time(di, BQ27XXX_REG_TTE, val);
> + if (!ret && !bq27xxx_value_is_valid(di, val->intval))
> + return -EINVAL;
> break;
> case POWER_SUPPLY_PROP_TIME_TO_EMPTY_AVG:
> ret = bq27xxx_battery_read_time(di, BQ27XXX_REG_TTECP, val);
> + if (!ret && !bq27xxx_value_is_valid(di, val->intval))
> + return -EINVAL;
> break;
> case POWER_SUPPLY_PROP_TIME_TO_FULL_NOW:
> ret = bq27xxx_battery_read_time(di, BQ27XXX_REG_TTF, val);
> @@ -2171,10 +2186,13 @@ 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 (!ret && !bq27xxx_value_is_valid(di, val->intval))
> + return -EINVAL;
> + } else {
> ret = bq27xxx_battery_read_rc(di, val);
> + }
> break;
> case POWER_SUPPLY_PROP_CHARGE_FULL:
> ret = bq27xxx_battery_read_fcc(di, val);
> @@ -2199,6 +2217,8 @@ 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 (!ret && !bq27xxx_value_is_valid(di, val->intval))
> + return -EINVAL;
> break;
> case POWER_SUPPLY_PROP_POWER_AVG:
> ret = bq27xxx_battery_pwr_avg(di, val);
> -- 
> 2.49.0
>