[1/3] power: supply: bq27xxx: fix sign of current_now for newer ICs

Message ID 20210223141122.9574-1-matthias.schiffer@ew.tq-group.com
State New
Headers show
Series
  • [1/3] power: supply: bq27xxx: fix sign of current_now for newer ICs
Related show

Commit Message

Matthias Schiffer Feb. 23, 2021, 2:11 p.m.
Commit cd060b4d0868 ("power: supply: bq27xxx: fix polarity of current_now")
changed the sign of current_now for all bq27xxx variants, but on BQ28Z610
I'm now seeing negated values *with* that patch.

The GTA04/Openmoko device that was used for testing uses a BQ27000 or
BQ27010 IC, so I assume only the BQ27XXX_O_ZERO code path was incorrect.
Revert the behaviour for newer ICs.

Fixes: cd060b4d0868 "power: supply: bq27xxx: fix polarity of current_now"
Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---

@Andreas Kemnade: It would be great to get a confirmation that the
Openmoko battery indeed uses BQ27000/BQ27010 - I was having some trouble
finding that information.


 drivers/power/supply/bq27xxx_battery.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Pali Rohár Feb. 23, 2021, 2:53 p.m. | #1
On Tuesday 23 February 2021 15:49:46 Andreas Kemnade wrote:
> On Tue, 23 Feb 2021 15:11:20 +0100
> Matthias Schiffer <matthias.schiffer@ew.tq-group.com> wrote:
> 
> > Commit cd060b4d0868 ("power: supply: bq27xxx: fix polarity of current_now")
> > changed the sign of current_now for all bq27xxx variants, but on BQ28Z610
> > I'm now seeing negated values *with* that patch.
> > 
> > The GTA04/Openmoko device that was used for testing uses a BQ27000 or
> > BQ27010 IC, so I assume only the BQ27XXX_O_ZERO code path was incorrect.
> > Revert the behaviour for newer ICs.
> > 
> > Fixes: cd060b4d0868 "power: supply: bq27xxx: fix polarity of current_now"
> > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > ---
> > 
> > @Andreas Kemnade: It would be great to get a confirmation that the
> > Openmoko battery indeed uses BQ27000/BQ27010 - I was having some trouble
> > finding that information.
> > 
> I can confirm that.
> here is the corresponding schematic:
> 
> http://people.openmoko.org/tony_tu/GTA02/hardware/GTA02/CT-GTA02.pdf
> 
> Regards,
> Andreas

Nokia N900 has BQ27200 connected via i2c. CCing Maemo mailing list maemo-leste@lists.dyne.org
Matthias Schiffer Feb. 24, 2021, 11:50 a.m. | #2
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 <matthias.schiffer@ew.tq-group.com>

> ---

>  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;

>  };

>

Patch

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 4c4a7b1c64c5..cb6ebd2f905e 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -1827,7 +1827,7 @@  static int bq27xxx_battery_current(struct bq27xxx_device_info *di,
 		val->intval = curr * BQ27XXX_CURRENT_CONSTANT / BQ27XXX_RS;
 	} else {
 		/* Other gauges return signed value */
-		val->intval = -(int)((s16)curr) * 1000;
+		val->intval = (int)((s16)curr) * 1000;
 	}
 
 	return 0;