[PATCH 0/1] power: supply: bq27xxx: expose battery data when CI=1

[PATCH 0/1] power: supply: bq27xxx: expose battery data when CI=1

[Sicelo A. Mhlongo]

We currently discard capacity information provided by the chip when the
Capacity Inaccurate flag is set. However, in many cases, having this
stale data is better than no information at all.

Even if the chip has been fully reset, the datasheet shows that capacity 
values (NAC specifically) can be seeded by using the WRTNAC facility of 
the Control and Mode registers. After seeding, CI remains set, but the
capacity values are as accurate as the NAC value provided to WRTNAC is.

On the Nokia N900, such seed value can be obtained from rx51_battery, 
which reads battery data directly from the battery's BSI pin. This can
be done in userspace via i2c access to the registers. With this patch,
once seeded, capacity values are available through the driver.

The patch was successfully tested on bq27200 on the Nokia N900.

Signed-off-by: Sicelo A. Mhlongo <absicsz@gmail.com>

Sicelo A. Mhlongo (1):
  power: supply: bq27xxx: expose battery data when CI=1

 drivers/power/supply/bq27xxx_battery.c | 58 +++++++++++++-------------
 1 file changed, 28 insertions(+), 30 deletions(-)

-- 
2.35.2

[PATCH 1/1] power: supply: bq27xxx: expose battery data when CI=1

[Sicelo A. Mhlongo]

When the Capacity Inaccurate flag is set, the chip still provides data
about the battery, albeit inaccurate. Instead of discarding capacity
values for CI=1, expose the stale data and use the
POWER_SUPPLY_HEALTH_CALIBRATION_REQUIRED property to indicate that the
values should be used with care.

Signed-off-by: Sicelo A. Mhlongo <absicsz@gmail.com>
---
 drivers/power/supply/bq27xxx_battery.c | 58 +++++++++++++-------------
 1 file changed, 28 insertions(+), 30 deletions(-)

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 72e727cd31e8..a524237b758e 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -1572,14 +1572,6 @@ static int bq27xxx_battery_read_charge(struct bq27xxx_device_info *di, u8 reg)
  */
 static inline int bq27xxx_battery_read_nac(struct bq27xxx_device_info *di)
 {
-	int flags;
-
-	if (di->opts & BQ27XXX_O_ZERO) {
-		flags = bq27xxx_read(di, BQ27XXX_REG_FLAGS, true);
-		if (flags >= 0 && (flags & BQ27000_FLAG_CI))
-			return -ENODATA;
-	}
-
 	return bq27xxx_battery_read_charge(di, BQ27XXX_REG_NAC);
 }
 
@@ -1742,6 +1734,18 @@ static bool bq27xxx_battery_dead(struct bq27xxx_device_info *di, u16 flags)
 		return flags & (BQ27XXX_FLAG_SOC1 | BQ27XXX_FLAG_SOCF);
 }
 
+/*
+ * Returns true if reported battery capacity is inaccurate
+ */
+static bool bq27xxx_battery_capacity_inaccurate(struct bq27xxx_device_info *di,
+						 u16 flags)
+{
+	if (di->opts & BQ27XXX_O_HAS_CI)
+		return (flags & BQ27000_FLAG_CI);
+	else
+		return false;
+}
+
 static int bq27xxx_battery_read_health(struct bq27xxx_device_info *di)
 {
 	/* Unlikely but important to return first */
@@ -1751,6 +1755,8 @@ static int bq27xxx_battery_read_health(struct bq27xxx_device_info *di)
 		return POWER_SUPPLY_HEALTH_COLD;
 	if (unlikely(bq27xxx_battery_dead(di, di->cache.flags)))
 		return POWER_SUPPLY_HEALTH_DEAD;
+	if (unlikely(bq27xxx_battery_capacity_inaccurate(di, di->cache.flags)))
+		return POWER_SUPPLY_HEALTH_CALIBRATION_REQUIRED;
 
 	return POWER_SUPPLY_HEALTH_GOOD;
 }
@@ -1767,29 +1773,21 @@ void bq27xxx_battery_update(struct bq27xxx_device_info *di)
 	if (cache.flags >= 0) {
 		cache.temperature = bq27xxx_battery_read_temperature(di);
 		if (has_ci_flag && (cache.flags & BQ27000_FLAG_CI)) {
-			dev_info_once(di->dev, "battery is not calibrated! ignoring capacity values\n");
-			cache.capacity = -ENODATA;
-			cache.energy = -ENODATA;
-			cache.time_to_empty = -ENODATA;
-			cache.time_to_empty_avg = -ENODATA;
-			cache.time_to_full = -ENODATA;
-			cache.charge_full = -ENODATA;
-			cache.health = -ENODATA;
-		} else {
-			if (di->regs[BQ27XXX_REG_TTE] != INVALID_REG_ADDR)
-				cache.time_to_empty = bq27xxx_battery_read_time(di, BQ27XXX_REG_TTE);
-			if (di->regs[BQ27XXX_REG_TTECP] != INVALID_REG_ADDR)
-				cache.time_to_empty_avg = bq27xxx_battery_read_time(di, BQ27XXX_REG_TTECP);
-			if (di->regs[BQ27XXX_REG_TTF] != INVALID_REG_ADDR)
-				cache.time_to_full = bq27xxx_battery_read_time(di, BQ27XXX_REG_TTF);
-
-			cache.charge_full = bq27xxx_battery_read_fcc(di);
-			cache.capacity = bq27xxx_battery_read_soc(di);
-			if (di->regs[BQ27XXX_REG_AE] != INVALID_REG_ADDR)
-				cache.energy = bq27xxx_battery_read_energy(di);
-			di->cache.flags = cache.flags;
-			cache.health = bq27xxx_battery_read_health(di);
+			dev_info_once(di->dev, "battery is not calibrated! capacity values are inaccurate\n");
 		}
+		if (di->regs[BQ27XXX_REG_TTE] != INVALID_REG_ADDR)
+			cache.time_to_empty = bq27xxx_battery_read_time(di, BQ27XXX_REG_TTE);
+		if (di->regs[BQ27XXX_REG_TTECP] != INVALID_REG_ADDR)
+			cache.time_to_empty_avg = bq27xxx_battery_read_time(di, BQ27XXX_REG_TTECP);
+		if (di->regs[BQ27XXX_REG_TTF] != INVALID_REG_ADDR)
+			cache.time_to_full = bq27xxx_battery_read_time(di, BQ27XXX_REG_TTF);
+
+		cache.charge_full = bq27xxx_battery_read_fcc(di);
+		cache.capacity = bq27xxx_battery_read_soc(di);
+		if (di->regs[BQ27XXX_REG_AE] != INVALID_REG_ADDR)
+			cache.energy = bq27xxx_battery_read_energy(di);
+		di->cache.flags = cache.flags;
+		cache.health = bq27xxx_battery_read_health(di);
 		if (di->regs[BQ27XXX_REG_CYCT] != INVALID_REG_ADDR)
 			cache.cycle_count = bq27xxx_battery_read_cyct(di);
 
-- 
2.35.2

Re: [PATCH 1/1] power: supply: bq27xxx: expose battery data when CI=1

[Pali Rohár]

On Tuesday 19 April 2022 23:36:47 Sicelo A. Mhlongo wrote:
> When the Capacity Inaccurate flag is set, the chip still provides data
> about the battery, albeit inaccurate. Instead of discarding capacity
> values for CI=1, expose the stale data and use the
> POWER_SUPPLY_HEALTH_CALIBRATION_REQUIRED property to indicate that the
> values should be used with care.

Looks good! I did not know that there is CALIBRATION_REQUIRED property.

Reviewed-by: Pali Rohár <pali@kernel.org>

> Signed-off-by: Sicelo A. Mhlongo <absicsz@gmail.com>
> ---
>  drivers/power/supply/bq27xxx_battery.c | 58 +++++++++++++-------------
>  1 file changed, 28 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> index 72e727cd31e8..a524237b758e 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -1572,14 +1572,6 @@ static int bq27xxx_battery_read_charge(struct bq27xxx_device_info *di, u8 reg)
>   */
>  static inline int bq27xxx_battery_read_nac(struct bq27xxx_device_info *di)
>  {
> -	int flags;
> -
> -	if (di->opts & BQ27XXX_O_ZERO) {
> -		flags = bq27xxx_read(di, BQ27XXX_REG_FLAGS, true);
> -		if (flags >= 0 && (flags & BQ27000_FLAG_CI))
> -			return -ENODATA;
> -	}
> -
>  	return bq27xxx_battery_read_charge(di, BQ27XXX_REG_NAC);
>  }
>  
> @@ -1742,6 +1734,18 @@ static bool bq27xxx_battery_dead(struct bq27xxx_device_info *di, u16 flags)
>  		return flags & (BQ27XXX_FLAG_SOC1 | BQ27XXX_FLAG_SOCF);
>  }
>  
> +/*
> + * Returns true if reported battery capacity is inaccurate
> + */
> +static bool bq27xxx_battery_capacity_inaccurate(struct bq27xxx_device_info *di,
> +						 u16 flags)
> +{
> +	if (di->opts & BQ27XXX_O_HAS_CI)
> +		return (flags & BQ27000_FLAG_CI);
> +	else
> +		return false;
> +}
> +
>  static int bq27xxx_battery_read_health(struct bq27xxx_device_info *di)
>  {
>  	/* Unlikely but important to return first */
> @@ -1751,6 +1755,8 @@ static int bq27xxx_battery_read_health(struct bq27xxx_device_info *di)
>  		return POWER_SUPPLY_HEALTH_COLD;
>  	if (unlikely(bq27xxx_battery_dead(di, di->cache.flags)))
>  		return POWER_SUPPLY_HEALTH_DEAD;
> +	if (unlikely(bq27xxx_battery_capacity_inaccurate(di, di->cache.flags)))
> +		return POWER_SUPPLY_HEALTH_CALIBRATION_REQUIRED;
>  
>  	return POWER_SUPPLY_HEALTH_GOOD;
>  }
> @@ -1767,29 +1773,21 @@ void bq27xxx_battery_update(struct bq27xxx_device_info *di)
>  	if (cache.flags >= 0) {
>  		cache.temperature = bq27xxx_battery_read_temperature(di);
>  		if (has_ci_flag && (cache.flags & BQ27000_FLAG_CI)) {
> -			dev_info_once(di->dev, "battery is not calibrated! ignoring capacity values\n");
> -			cache.capacity = -ENODATA;
> -			cache.energy = -ENODATA;
> -			cache.time_to_empty = -ENODATA;
> -			cache.time_to_empty_avg = -ENODATA;
> -			cache.time_to_full = -ENODATA;
> -			cache.charge_full = -ENODATA;
> -			cache.health = -ENODATA;
> -		} else {
> -			if (di->regs[BQ27XXX_REG_TTE] != INVALID_REG_ADDR)
> -				cache.time_to_empty = bq27xxx_battery_read_time(di, BQ27XXX_REG_TTE);
> -			if (di->regs[BQ27XXX_REG_TTECP] != INVALID_REG_ADDR)
> -				cache.time_to_empty_avg = bq27xxx_battery_read_time(di, BQ27XXX_REG_TTECP);
> -			if (di->regs[BQ27XXX_REG_TTF] != INVALID_REG_ADDR)
> -				cache.time_to_full = bq27xxx_battery_read_time(di, BQ27XXX_REG_TTF);
> -
> -			cache.charge_full = bq27xxx_battery_read_fcc(di);
> -			cache.capacity = bq27xxx_battery_read_soc(di);
> -			if (di->regs[BQ27XXX_REG_AE] != INVALID_REG_ADDR)
> -				cache.energy = bq27xxx_battery_read_energy(di);
> -			di->cache.flags = cache.flags;
> -			cache.health = bq27xxx_battery_read_health(di);
> +			dev_info_once(di->dev, "battery is not calibrated! capacity values are inaccurate\n");

Just one note: Do we still need this info message in dmesg? Now when
this information is going to be reported via sysfs API, I'm not sure if
it still has some value...

>  		}
> +		if (di->regs[BQ27XXX_REG_TTE] != INVALID_REG_ADDR)
> +			cache.time_to_empty = bq27xxx_battery_read_time(di, BQ27XXX_REG_TTE);
> +		if (di->regs[BQ27XXX_REG_TTECP] != INVALID_REG_ADDR)
> +			cache.time_to_empty_avg = bq27xxx_battery_read_time(di, BQ27XXX_REG_TTECP);
> +		if (di->regs[BQ27XXX_REG_TTF] != INVALID_REG_ADDR)
> +			cache.time_to_full = bq27xxx_battery_read_time(di, BQ27XXX_REG_TTF);
> +
> +		cache.charge_full = bq27xxx_battery_read_fcc(di);
> +		cache.capacity = bq27xxx_battery_read_soc(di);
> +		if (di->regs[BQ27XXX_REG_AE] != INVALID_REG_ADDR)
> +			cache.energy = bq27xxx_battery_read_energy(di);
> +		di->cache.flags = cache.flags;
> +		cache.health = bq27xxx_battery_read_health(di);
>  		if (di->regs[BQ27XXX_REG_CYCT] != INVALID_REG_ADDR)
>  			cache.cycle_count = bq27xxx_battery_read_cyct(di);
>  
> -- 
> 2.35.2
> 

Re: [PATCH 0/1] power: supply: bq27xxx: expose battery data when CI=1

[Pali Rohár]

On Tuesday 19 April 2022 23:36:46 Sicelo A. Mhlongo wrote:
> We currently discard capacity information provided by the chip when the
> Capacity Inaccurate flag is set. However, in many cases, having this
> stale data is better than no information at all.
> 
> Even if the chip has been fully reset, the datasheet shows that capacity 
> values (NAC specifically) can be seeded by using the WRTNAC facility of 
> the Control and Mode registers. After seeding, CI remains set, but the
> capacity values are as accurate as the NAC value provided to WRTNAC is.

NAC (Nominal Available Capacity) is exported via CHARGE_NOW property.

So what about implementing CHARGE_NOW property also writable and write
operation would do this WRTNAC seeding? So userspace would be able to do
'echo new_value > charge_now' and for Nokia N900 case just directly
'cat .../rx51_battery/charge_full_design > .../bq27200/charge_now'

> On the Nokia N900, such seed value can be obtained from rx51_battery, 
> which reads battery data directly from the battery's BSI pin. This can
> be done in userspace via i2c access to the registers. With this patch,
> once seeded, capacity values are available through the driver.
> 
> The patch was successfully tested on bq27200 on the Nokia N900.
> 
> Signed-off-by: Sicelo A. Mhlongo <absicsz@gmail.com>
> 
> Sicelo A. Mhlongo (1):
>   power: supply: bq27xxx: expose battery data when CI=1
> 
>  drivers/power/supply/bq27xxx_battery.c | 58 +++++++++++++-------------
>  1 file changed, 28 insertions(+), 30 deletions(-)
> 
> -- 
> 2.35.2
> 

Re: [PATCH 0/1] power: supply: bq27xxx: expose battery data when CI=1

[Sicelo]

On Wed, Apr 20, 2022 at 12:09:59AM +0200, Pali Rohár wrote:
> NAC (Nominal Available Capacity) is exported via CHARGE_NOW property.
> 
> So what about implementing CHARGE_NOW property also writable and write
> operation would do this WRTNAC seeding? So userspace would be able to do
> 'echo new_value > charge_now' and for Nokia N900 case just directly
> 'cat .../rx51_battery/charge_full_design > .../bq27200/charge_now'
Thank you for this suggestion. I'm afraid implementing it would be a bit
too involved for my current level of experience.  If someone beats me to
implementing WRTNAC ability, all the better.  Otherwise I will
definitely want to look into it in the future.

The challenge with seeding via rx51_battery is that it provides
CHARGE_FULL_DESIGN, so one must still calculate a NAC value from the
voltage. In my userspace script [1], I am currently using a linear
calculation, but obviously this is not really correct.

I might mention, in passing, that I also wish to later add the 
VOLTAGE_MIN_DESIGN property (which will be taken from EDVF). It may help
userspace (e.g. upower) know when to shutdown the device if CI is set,
as capacity cannot be trusted.

Regards
Sicelo

[1] https://gist.github.com/sicelo/0a0e895d81a6b73b26555d215dee296d#file-rx51_seed_bq27xxx-sh-L34-L42