* [PATCH v2] power: supply: bq27xxx: do not report incorrect zero values
@ 2025-03-22 15:43 Sicelo A. Mhlongo
2025-03-24 15:12 ` H. Nikolaus Schaller
0 siblings, 1 reply; 3+ messages in thread
From: Sicelo A. Mhlongo @ 2025-03-22 15:43 UTC (permalink / raw)
To: Sebastian Reichel, linux-pm
Cc: hns, pali, phone-devel, letux-kernel, akemnade, Sicelo A. Mhlongo
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);
+}
+
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] power: supply: bq27xxx: do not report incorrect zero values
2025-03-22 15:43 [PATCH v2] power: supply: bq27xxx: do not report incorrect zero values Sicelo A. Mhlongo
@ 2025-03-24 15:12 ` H. Nikolaus Schaller
2025-03-24 15:42 ` Sicelo
0 siblings, 1 reply; 3+ messages in thread
From: H. Nikolaus Schaller @ 2025-03-24 15:12 UTC (permalink / raw)
To: Sicelo A. Mhlongo
Cc: Sebastian Reichel, linux-pm, pali, phone-devel,
Discussions about the Letux Kernel, akemnade
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] power: supply: bq27xxx: do not report incorrect zero values
2025-03-24 15:12 ` H. Nikolaus Schaller
@ 2025-03-24 15:42 ` Sicelo
0 siblings, 0 replies; 3+ messages in thread
From: Sicelo @ 2025-03-24 15:42 UTC (permalink / raw)
To: H. Nikolaus Schaller
Cc: Sebastian Reichel, linux-pm, pali, phone-devel,
Discussions about the Letux Kernel, akemnade
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-03-24 15:42 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-22 15:43 [PATCH v2] power: supply: bq27xxx: do not report incorrect zero values Sicelo A. Mhlongo
2025-03-24 15:12 ` H. Nikolaus Schaller
2025-03-24 15:42 ` Sicelo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox