two fixes for new bq27000/10 and bq27500 driver

PATCH In review H. Nikolaus Schaller 4 participants 9 messages Dec 17, 2015 – Dec 25, 2015

Share & tools

H. Nikolaus Schaller

author

2015-12-17 10:13 UTC #0 lore raw

cover two fixes for new bq27000/10 and bq27500 driver

H. Nikolaus Schaller (2):
power:bq27xxx: fix reading for bq27000 and bq27010 power:bq27xxx: fix register numbers of bq27500

 drivers/power/bq27xxx_battery.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

Signature · 1 line

2.5.1

H. Nikolaus Schaller

author

2015-12-17 10:12 UTC #1 lore raw

PATCH 1/2 power:bq27xxx: fix reading for bq27000 and bq27010

bug: the driver reports funny capacity values:

root@letux:/sys/class/power_supply/bq27000-battery# cat uevent
POWER_SUPPLY_NAME=bq27000-battery
POWER_SUPPLY_STATUS=Charging
POWER_SUPPLY_PRESENT=1
POWER_SUPPLY_VOLTAGE_NOW=3702000
POWER_SUPPLY_CURRENT_NOW=-464635
POWER_SUPPLY_CAPACITY=1536			<- over 100% is magic
POWER_SUPPLY_CAPACITY_LEVEL=Normal
POWER_SUPPLY_TEMP=311
POWER_SUPPLY_TIME_TO_FULL_NOW=10440
POWER_SUPPLY_TECHNOLOGY=Li-ion
POWER_SUPPLY_CHARGE_FULL=805450
POWER_SUPPLY_CHARGE_NOW=1068
POWER_SUPPLY_CHARGE_FULL_DESIGN=8844998	<- battery has just 1200 mAh
POWER_SUPPLY_CYCLE_COUNT=21
POWER_SUPPLY_ENERGY_NOW=0
POWER_SUPPLY_POWER_AVG=0
POWER_SUPPLY_HEALTH=Good
POWER_SUPPLY_MANUFACTURER=Texas Instruments

reason: the state of charge and the design capacity register are single byte only. The design capacity returns the higer order byte.

tested: GTA04 with Openmoko/FIC HF08x battery (using hdq)

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
 drivers/power/bq27xxx_battery.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

drivers/power/bq27xxx_battery.c +9 -3

index 880233c..e54a125 100644
@@ -471,7 +471,10 @@ static int bq27xxx_battery_read_soc(struct bq27xxx_device_info *di)
 {
 	int soc;
 
-	soc = bq27xxx_read(di, BQ27XXX_REG_SOC, false);
+	if (di->chip == BQ27000 || di->chip == BQ27010)
+		soc = bq27xxx_read(di, BQ27XXX_REG_SOC, true);
+	else
+		soc = bq27xxx_read(di, BQ27XXX_REG_SOC, false);
 
 	if (soc < 0)
 		dev_dbg(di->dev, "error reading State-of-Charge\n");

@@ -536,7 +539,10 @@ static int bq27xxx_battery_read_dcap(struct bq27xxx_device_info *di)
 {
 	int dcap;
 
-	dcap = bq27xxx_read(di, BQ27XXX_REG_DCAP, false);
+	if (di->chip == BQ27000 || di->chip == BQ27010)
+		dcap = bq27xxx_read(di, BQ27XXX_REG_DCAP, true);
+	else
+		dcap = bq27xxx_read(di, BQ27XXX_REG_DCAP, false);
 
 	if (dcap < 0) {
 		dev_dbg(di->dev, "error reading initial last measured discharge\n");

@@ -544,7 +550,7 @@ static int bq27xxx_battery_read_dcap(struct bq27xxx_device_info *di)
 	}
 
 	if (di->chip == BQ27000 || di->chip == BQ27010)
-		dcap *= BQ27XXX_CURRENT_CONSTANT / BQ27XXX_RS;
+		dcap = (dcap << 8) * BQ27XXX_CURRENT_CONSTANT / BQ27XXX_RS;
 	else
 		dcap *= 1000;

Signature · 1 line

2.5.1

Andrew F. Davis

Acked-by on v1 1/2

2015-12-24 02:46 UTC #2 lore raw

Re: [PATCH 1/2] power:bq27xxx: fix reading for bq27000 and bq27010

Quoted 49 lines · H. Nikolaus Schaller

bug: the driver reports funny capacity values:

root@letux:/sys/class/power_supply/bq27000-battery# cat uevent
POWER_SUPPLY_NAME=bq27000-battery
POWER_SUPPLY_STATUS=Charging
POWER_SUPPLY_PRESENT=1
POWER_SUPPLY_VOLTAGE_NOW=3702000
POWER_SUPPLY_CURRENT_NOW=-464635
POWER_SUPPLY_CAPACITY=1536			<- over 100% is magic
POWER_SUPPLY_CAPACITY_LEVEL=Normal
POWER_SUPPLY_TEMP=311
POWER_SUPPLY_TIME_TO_FULL_NOW=10440
POWER_SUPPLY_TECHNOLOGY=Li-ion
POWER_SUPPLY_CHARGE_FULL=805450
POWER_SUPPLY_CHARGE_NOW=1068
POWER_SUPPLY_CHARGE_FULL_DESIGN=8844998	<- battery has just 1200 mAh
POWER_SUPPLY_CYCLE_COUNT=21
POWER_SUPPLY_ENERGY_NOW=0
POWER_SUPPLY_POWER_AVG=0
POWER_SUPPLY_HEALTH=Good
POWER_SUPPLY_MANUFACTURER=Texas Instruments

reason: the state of charge and the design capacity register are single byte only. The design capacity returns the higer order byte.

tested: GTA04 with Openmoko/FIC HF08x battery (using hdq)

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
  drivers/power/bq27xxx_battery.c | 12 +++++++++---
  1 file changed, 9 insertions(+), 3 deletions(-)

drivers/power/bq27xxx_battery.c +4 -1

index 880233c..e54a125 100644
@@ -471,7 +471,10 @@ static int bq27xxx_battery_read_soc(struct bq27xxx_device_info *di)
  {
  	int soc;
 
-	soc = bq27xxx_read(di, BQ27XXX_REG_SOC, false);
+	if (di->chip == BQ27000 || di->chip == BQ27010)
+		soc = bq27xxx_read(di, BQ27XXX_REG_SOC, true);
+	else
+		soc = bq27xxx_read(di, BQ27XXX_REG_SOC, false);
 
  	if (soc < 0)
  		dev_dbg(di->dev, "error reading State-of-Charge\n");

drivers/power/bq27xxx_battery.c static int bq27xxx_battery_read_dcap(struct bq27xxx_device_info *di) · line 546

Quoted 9 lines

drivers/power/bq27xxx_battery.c +4 -1

@@ -536,7 +539,10 @@ static int bq27xxx_battery_read_dcap(struct bq27xxx_device_info *di)
  {
  	int dcap;
 
-	dcap = bq27xxx_read(di, BQ27XXX_REG_DCAP, false);
+	if (di->chip == BQ27000 || di->chip == BQ27010)
+		dcap = bq27xxx_read(di, BQ27XXX_REG_DCAP, true);
+	else
+		dcap = bq27xxx_read(di, BQ27XXX_REG_DCAP, false);

I'm wondering how the I2C versions will be effected by this change, will we get the right byte when we only read one? Maybe the fix should be to still read 2-bytes but mask out the right bits from the result? The data-sheet doesn't seem clear on this.

Well, it is more right than it was before, I'll look for a bq27200 to test and make changes if needed some other time, but for now:

Acked-by Andrew F. Davis <[email protected]>

Quoted 12 lines

drivers/power/bq27xxx_battery.c +1 -1

  	if (dcap < 0) {
  		dev_dbg(di->dev, "error reading initial last measured discharge\n");

@@ -544,7 +550,7 @@ static int bq27xxx_battery_read_dcap(struct bq27xxx_device_info *di)
  	}
 
  	if (di->chip == BQ27000 || di->chip == BQ27010)
-		dcap *= BQ27XXX_CURRENT_CONSTANT / BQ27XXX_RS;
+		dcap = (dcap << 8) * BQ27XXX_CURRENT_CONSTANT / BQ27XXX_RS;
  	else
  		dcap *= 1000;

Signature · 1 line

Andrew F. Davis

Pali Rohár

Reviewed-by on v1 1/2

2015-12-25 13:20 UTC #3 lore raw

Re: [PATCH 1/2] power:bq27xxx: fix reading for bq27000 and bq27010

In reply to H. Nikolaus Schaller

Quoted 63 lines · On Thursday 17 December 2015 11:12:53 H. Nikolaus Schaller wrote:

bug: the driver reports funny capacity values:

root@letux:/sys/class/power_supply/bq27000-battery# cat uevent
POWER_SUPPLY_NAME=bq27000-battery
POWER_SUPPLY_STATUS=Charging
POWER_SUPPLY_PRESENT=1
POWER_SUPPLY_VOLTAGE_NOW=3702000
POWER_SUPPLY_CURRENT_NOW=-464635
POWER_SUPPLY_CAPACITY=1536			<- over 100% is magic
POWER_SUPPLY_CAPACITY_LEVEL=Normal
POWER_SUPPLY_TEMP=311
POWER_SUPPLY_TIME_TO_FULL_NOW=10440
POWER_SUPPLY_TECHNOLOGY=Li-ion
POWER_SUPPLY_CHARGE_FULL=805450
POWER_SUPPLY_CHARGE_NOW=1068
POWER_SUPPLY_CHARGE_FULL_DESIGN=8844998	<- battery has just 1200 mAh
POWER_SUPPLY_CYCLE_COUNT=21
POWER_SUPPLY_ENERGY_NOW=0
POWER_SUPPLY_POWER_AVG=0
POWER_SUPPLY_HEALTH=Good
POWER_SUPPLY_MANUFACTURER=Texas Instruments

reason: the state of charge and the design capacity register are single byte only. The design capacity returns the higer order byte.

tested: GTA04 with Openmoko/FIC HF08x battery (using hdq)

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
 drivers/power/bq27xxx_battery.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

(quoted diff) +0 -0

b/drivers/power/bq27xxx_battery.c index 880233c..e54a125 100644

drivers/power/bq27xxx_battery.c +8 -2

@@ -471,7 +471,10 @@ static int bq27xxx_battery_read_soc(struct
 bq27xxx_device_info *di) {
 	int soc;
 
-	soc = bq27xxx_read(di, BQ27XXX_REG_SOC, false);
+	if (di->chip == BQ27000 || di->chip == BQ27010)
+		soc = bq27xxx_read(di, BQ27XXX_REG_SOC, true);
+	else
+		soc = bq27xxx_read(di, BQ27XXX_REG_SOC, false);
 
 	if (soc < 0)
 		dev_dbg(di->dev, "error reading State-of-Charge\n");

@@ -536,7 +539,10 @@ static int bq27xxx_battery_read_dcap(struct
 bq27xxx_device_info *di) {
 	int dcap;
 
-	dcap = bq27xxx_read(di, BQ27XXX_REG_DCAP, false);
+	if (di->chip == BQ27000 || di->chip == BQ27010)
+		dcap = bq27xxx_read(di, BQ27XXX_REG_DCAP, true);
+	else
+		dcap = bq27xxx_read(di, BQ27XXX_REG_DCAP, false);
 
 	if (dcap < 0) {
 		dev_dbg(di->dev, "error reading initial last measured

discharge\n"); @@ -544,7 +550,7 @@ static int bq27xxx_battery_read_dcap(struct bq27xxx_device_info *di) }

drivers/power/bq27xxx_battery.c static int bq27xxx_battery_read_dcap(struct bq27xxx_device_info *di) · line 555

Quoted 5 lines

drivers/power/bq27xxx_battery.c +1 -1

 	if (di->chip == BQ27000 || di->chip == BQ27010)
-		dcap *= BQ27XXX_CURRENT_CONSTANT / BQ27XXX_RS;
+		dcap = (dcap << 8) * BQ27XXX_CURRENT_CONSTANT / BQ27XXX_RS;
 	else
 		dcap *= 1000;

Hi!

This patch fixes commit d74534c27775857cb09abd0f92ed9539dc8d0a93 (power:
bq27xxx_battery: Add support for additional bq27xxx family devices) in which this bug (all 3 patch chunks) were introduced.

Before that commit code was very similar after applying this patch. So:

Reviewed-by Pali Rohár <[email protected]>

Signature · 2 lines

Pali Rohár
[email protected]

H. Nikolaus Schaller

author

2015-12-17 10:13 UTC #4 lore raw

PATCH 2/2 power:bq27xxx: fix register numbers of bq27500

In reply to H. Nikolaus Schaller

bug: according to data sheet some register numbers are wrong.

tested: no

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
 drivers/power/bq27xxx_battery.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/power/bq27xxx_battery.c +3 -3

index e54a125..f9a5f4f 100644
@@ -198,10 +198,10 @@ static u8 bq27500_regs[] = {
 	INVALID_REG_ADDR,	/* TTECP - NA	*/
 	0x0c,	/* NAC		*/
 	0x12,	/* LMD(FCC)	*/
-	0x1e,	/* CYCT		*/
+	0x2a,	/* CYCT		*/
 	INVALID_REG_ADDR,	/* AE - NA	*/
-	0x20,	/* SOC(RSOC)	*/
-	0x2e,	/* DCAP(ILMD)	*/
+	0x2c,	/* SOC(RSOC)	*/
+	0x3c,	/* DCAP(ILMD)	*/
 	INVALID_REG_ADDR,	/* AP - NA	*/
 };

Signature · 1 line

2.5.1

Andrew F. Davis

Acked-by on v1 2/2

2015-12-24 02:53 UTC #5 lore raw

Re: [PATCH 2/2] power:bq27xxx: fix register numbers of bq27500

Quoted 5 lines · H. Nikolaus Schaller

bug: according to data sheet some register numbers are wrong.
tested: no
Signed-off-by H. Nikolaus Schaller <[email protected]>

Indeed they are, good catch.

Acked-by Andrew F. Davis <[email protected]>

Quoted 23 lines

---
  drivers/power/bq27xxx_battery.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

drivers/power/bq27xxx_battery.c +3 -3

index e54a125..f9a5f4f 100644
@@ -198,10 +198,10 @@ static u8 bq27500_regs[] = {
  	INVALID_REG_ADDR,	/* TTECP - NA	*/
  	0x0c,	/* NAC		*/
  	0x12,	/* LMD(FCC)	*/
-	0x1e,	/* CYCT		*/
+	0x2a,	/* CYCT		*/
  	INVALID_REG_ADDR,	/* AE - NA	*/
-	0x20,	/* SOC(RSOC)	*/
-	0x2e,	/* DCAP(ILMD)	*/
+	0x2c,	/* SOC(RSOC)	*/
+	0x3c,	/* DCAP(ILMD)	*/
  	INVALID_REG_ADDR,	/* AP - NA	*/
  };

Sebastian Reichel

2015-12-19 08:00 UTC #6 lore raw

In reply to H. Nikolaus Schaller

Hi Andrew,

Quoted 6 lines · H. Nikolaus Schaller

H. Nikolaus Schaller (2):
power:bq27xxx: fix reading for bq27000 and bq27010 power:bq27xxx: fix register numbers of bq27500
 drivers/power/bq27xxx_battery.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

Can you give me your Acked-By (or Tested-By) for those patches? Also I wonder if you would be available as official reviewer for the driver.

-- Sebastian

Andrew F. Davis

2015-12-24 13:58 UTC #7 lore raw

Quoted 11 lines · Sebastian Reichel

Hi Andrew,
Quoted 6 lines · H. Nikolaus Schaller
H. Nikolaus Schaller (2):
power:bq27xxx: fix reading for bq27000 and bq27010 power:bq27xxx: fix register numbers of bq27500
  drivers/power/bq27xxx_battery.c | 18 ++++++++++++------
  1 file changed, 12 insertions(+), 6 deletions(-)
Can you give me your Acked-By (or Tested-By) for those patches?

Done

Quoted 2 lines

Also I wonder if you would be available as official reviewer for the driver.

Sure, what's the normal procedure for this, should I submit a patch adding myself to that driver in MAINTAINERS?

Pali Rohár

2015-12-25 13:22 UTC #8 lore raw

Quoted 5 lines · On Thursday 24 December 2015 14:57:56 Andrew F. Davis wrote:

Quoted 2 lines
Also I wonder if you would be available as official reviewer for the driver.
Sure, what's the normal procedure for this, should I submit a patch adding myself to that driver in MAINTAINERS?

Yes, send patch.

Signature · 2 lines

Pali Rohár
[email protected]