| Message ID | 20250322161718.253093-1-absicsz@gmail.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | [v2] power: supply: bq27xxx: do not report incorrect zero values | expand |
Hi Sicelo, > Am 22.03.2025 um 16:43 schrieb Sicelo A. Mhlongo <absicsz@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@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@letux:~# echo off >/sys/class/power_supply/twl4030_usb/mode root@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 >
Hi > > > > +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@letux:~# echo off >/sys/class/power_supply/twl4030_usb/mode > root@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? I can see the problem. Thanks for testing. Let us retract this patch. I think we can solve the issue in userspace. Sincerely Sicelo
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); +} + 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);
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@gmail.com> --- drivers/power/supply/bq27xxx_battery.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-)