On all newer bq27xxx ICs, the AveragePower register contains a signed
value; in addition to handling the raw value as unsigned, the driver
code also didn't convert it to µW as expected.
At least for the BQ28Z610, the reference manual incorrectly states that
the value is in units of 1mA and not 10mA. I have no way of knowing
whether the manuals of other supported ICs contain the same error, or if
there are models that actually use 1mA. At least, the new code shouldn't
be *less* correct than the old version for any device.
power_avg is removed from the cache structure, se we don't have to
extend it to store both a signed value and an error code. Always getting
an up-to-date value may be desirable anyways, as it avoids inconsistent
current and power readings when switching between charging and
discharging.
Signed-off-by: Matthias Schiffer <[email protected]>
---
drivers/power/supply/bq27xxx_battery.c | 51 ++++++++++++++------------
include/linux/power/bq27xxx_battery.h | 1 -
2 files changed, 27 insertions(+), 25 deletions(-)
diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index cb6ebd2f905e..20e1dc8a87cf 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -1661,27 +1661,6 @@ static int bq27xxx_battery_read_time(struct bq27xxx_device_info *di, u8 reg)
return tval * 60;
}
-/*
- * Read an average power register.
- * Return < 0 if something fails.
- */
-static int bq27xxx_battery_read_pwr_avg(struct bq27xxx_device_info *di)
-{
- int tval;
-
- tval = bq27xxx_read(di, BQ27XXX_REG_AP, false);
- if (tval < 0) {
- dev_err(di->dev, "error reading average power register %02x: %d\n",
- BQ27XXX_REG_AP, tval);
- return tval;
- }
-
- if (di->opts & BQ27XXX_O_ZERO)
- return (tval * BQ27XXX_POWER_CONSTANT) / BQ27XXX_RS;
- else
- return tval;
-}
-
/*
* Returns true if a battery over temperature condition is detected
*/
@@ -1769,8 +1748,6 @@ void bq27xxx_battery_update(struct bq27xxx_device_info *di)
}
if (di->regs[BQ27XXX_REG_CYCT] != INVALID_REG_ADDR)
cache.cycle_count = bq27xxx_battery_read_cyct(di);
- if (di->regs[BQ27XXX_REG_AP] != INVALID_REG_ADDR)
- cache.power_avg = bq27xxx_battery_read_pwr_avg(di);
/* We only have to read charge design full once */
if (di->charge_design_full <= 0)
@@ -1833,6 +1810,32 @@ static int bq27xxx_battery_current(struct bq27xxx_device_info *di,
return 0;
}
+/*
+ * Get the average power in µW
+ * Return < 0 if something fails.
+ */
+static int bq27xxx_battery_pwr_avg(struct bq27xxx_device_info *di,
+ union power_supply_propval *val)
+{
+ int power;
+
+ power = bq27xxx_read(di, BQ27XXX_REG_AP, false);
+ if (power < 0) {
+ dev_err(di->dev,
+ "error reading average power register %02x: %d\n",
+ BQ27XXX_REG_AP, power);
+ return power;
+ }
+
+ if (di->opts & BQ27XXX_O_ZERO)
+ val->intval = (power * BQ27XXX_POWER_CONSTANT) / BQ27XXX_RS;
+ else
+ /* Other gauges return a signed value in units of 10mW */
+ val->intval = (int)((s16)power) * 10000;
+
+ return 0;
+}
+
static int bq27xxx_battery_status(struct bq27xxx_device_info *di,
union power_supply_propval *val)
{
@@ -2020,7 +2023,7 @@ static int bq27xxx_battery_get_property(struct power_supply *psy,
ret = bq27xxx_simple_value(di->cache.energy, val);
break;
case POWER_SUPPLY_PROP_POWER_AVG:
- ret = bq27xxx_simple_value(di->cache.power_avg, val);
+ ret = bq27xxx_battery_pwr_avg(di, val);
break;
case POWER_SUPPLY_PROP_HEALTH:
ret = bq27xxx_simple_value(di->cache.health, val);
diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
index 111a40d0d3d5..8d5f4f40fb41 100644
--- a/include/linux/power/bq27xxx_battery.h
+++ b/include/linux/power/bq27xxx_battery.h
@@ -53,7 +53,6 @@ struct bq27xxx_reg_cache {
int capacity;
int energy;
int flags;
- int power_avg;
int health;
};
--
2.17.1
On Tue, 2021-02-23 at 15:11 +0100, Matthias Schiffer wrote:
> On all newer bq27xxx ICs, the AveragePower register contains a signed
> value; in addition to handling the raw value as unsigned, the driver
> code also didn't convert it to µW as expected.
>
> At least for the BQ28Z610, the reference manual incorrectly states that
> the value is in units of 1mA and not 10mA. I have no way of knowing
> whether the manuals of other supported ICs contain the same error, or if
> there are models that actually use 1mA. At least, the new code shouldn't
> be *less* correct than the old version for any device
Ugh, of course this section should talk about mW and not mA. I'll wait
if there is more feedback and then send a v2.
> .
>
> power_avg is removed from the cache structure, se we don't have to
> extend it to store both a signed value and an error code. Always getting
> an up-to-date value may be desirable anyways, as it avoids inconsistent
> current and power readings when switching between charging and
> discharging.
>
> Signed-off-by: Matthias Schiffer <[email protected]>
> ---
> drivers/power/supply/bq27xxx_battery.c | 51 ++++++++++++++------------
> include/linux/power/bq27xxx_battery.h | 1 -
> 2 files changed, 27 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> index cb6ebd2f905e..20e1dc8a87cf 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -1661,27 +1661,6 @@ static int bq27xxx_battery_read_time(struct bq27xxx_device_info *di, u8 reg)
> return tval * 60;
> }
>
> -/*
> - * Read an average power register.
> - * Return < 0 if something fails.
> - */
> -static int bq27xxx_battery_read_pwr_avg(struct bq27xxx_device_info *di)
> -{
> - int tval;
> -
> - tval = bq27xxx_read(di, BQ27XXX_REG_AP, false);
> - if (tval < 0) {
> - dev_err(di->dev, "error reading average power register %02x: %d\n",
> - BQ27XXX_REG_AP, tval);
> - return tval;
> - }
> -
> - if (di->opts & BQ27XXX_O_ZERO)
> - return (tval * BQ27XXX_POWER_CONSTANT) / BQ27XXX_RS;
> - else
> - return tval;
> -}
> -
> /*
> * Returns true if a battery over temperature condition is detected
> */
> @@ -1769,8 +1748,6 @@ void bq27xxx_battery_update(struct bq27xxx_device_info *di)
> }
> if (di->regs[BQ27XXX_REG_CYCT] != INVALID_REG_ADDR)
> cache.cycle_count = bq27xxx_battery_read_cyct(di);
> - if (di->regs[BQ27XXX_REG_AP] != INVALID_REG_ADDR)
> - cache.power_avg = bq27xxx_battery_read_pwr_avg(di);
>
> /* We only have to read charge design full once */
> if (di->charge_design_full <= 0)
> @@ -1833,6 +1810,32 @@ static int bq27xxx_battery_current(struct bq27xxx_device_info *di,
> return 0;
> }
>
> +/*
> + * Get the average power in µW
> + * Return < 0 if something fails.
> + */
> +static int bq27xxx_battery_pwr_avg(struct bq27xxx_device_info *di,
> + union power_supply_propval *val)
> +{
> + int power;
> +
> + power = bq27xxx_read(di, BQ27XXX_REG_AP, false);
> + if (power < 0) {
> + dev_err(di->dev,
> + "error reading average power register %02x: %d\n",
> + BQ27XXX_REG_AP, power);
> + return power;
> + }
> +
> + if (di->opts & BQ27XXX_O_ZERO)
> + val->intval = (power * BQ27XXX_POWER_CONSTANT) / BQ27XXX_RS;
> + else
> + /* Other gauges return a signed value in units of 10mW */
> + val->intval = (int)((s16)power) * 10000;
> +
> + return 0;
> +}
> +
> static int bq27xxx_battery_status(struct bq27xxx_device_info *di,
> union power_supply_propval *val)
> {
> @@ -2020,7 +2023,7 @@ static int bq27xxx_battery_get_property(struct power_supply *psy,
> ret = bq27xxx_simple_value(di->cache.energy, val);
> break;
> case POWER_SUPPLY_PROP_POWER_AVG:
> - ret = bq27xxx_simple_value(di->cache.power_avg, val);
> + ret = bq27xxx_battery_pwr_avg(di, val);
> break;
> case POWER_SUPPLY_PROP_HEALTH:
> ret = bq27xxx_simple_value(di->cache.health, val);
> diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
> index 111a40d0d3d5..8d5f4f40fb41 100644
> --- a/include/linux/power/bq27xxx_battery.h
> +++ b/include/linux/power/bq27xxx_battery.h
> @@ -53,7 +53,6 @@ struct bq27xxx_reg_cache {
> int capacity;
> int energy;
> int flags;
> - int power_avg;
> int health;
> };
>