/sys/class/power_supply/bq27200-0/capacity changed meaning between 4.1 and 4.4?

PATCH Pavel Machek 4 participants 12 messages Jan 9, 2016 – Jan 13, 2016

lore

Share & tools

lore

Pavel Machek

author

2016-01-09 23:07 UTC #0 lore raw

Hi!

Did /sys/class/power_supply/bq27200-0/capacity change meaning between 4.1 and 4.4?

It used to report battery capacity remaining in percent.

Not sure what it reports now, but ain't in percent....

pavel at n900:/my/tui/ofone$ cat /sys/class/power_supply/bq27200-0/capacity
7497
pavel at n900:/my/tui/ofone$ uname -a
Linux n900 4.4.0-rc8-omap3-149467-g69aafd8-dirty #114 PREEMPT Sat Jan 9 21:44:00 CET 2016 armv7l GNU/Linux
pavel at n900:/my/tui/ofone$

And power_supply_class.txt says:

~ ~ ~ ~ ~ ~ ~ Charge/Energy/Capacity - how to not confuse ~ ~ ~ ~ ~ ~ ~
~
~
~ Because both "charge" (?Ah) and "energy" (?Wh) represents "capacity" ~
~ of battery, this class distinguish these terms. Don't mix them! ~
~
~
~ CHARGE_* attributes represents capacity in ?Ah only.
~
~ ENERGY_* attributes represents capacity in ?Wh only.
~
~ CAPACITY attribute represents capacity in *percents*, from 0 to
100. ~

--- hmm. So that seems to be a mistake.

pavel at n900:/my/tui/ofone$ cat /sys/class/power_supply/bq27200-0/capacity 7497
pavel at n900:/my/tui/ofone$ cat /sys/class/power_supply/bq27200-0/capacity 16713
pavel at n900:/my/tui/ofone$ cat /sys/class/power_supply/bq27200-0/capacity 25162

Converting them to hex gives rather strange values.

Hmm. .... what is this? Git blames Andrew F. Davis. The first part of code suggests whole cache needs to be initialized... but the second part of the code only initializes cache.time_to_empty (for example) conditionaly.

Are you sure?

                if (has_ci_flag && (cache.flags & BQ27000_FLAG_CI)) {
                        dev_info(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_TT\
ECP);
                        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);
                        cache.health = bq27xxx_battery_read_health(di);
                }

But that does not explain the wrong capacity..

-static inline int bq27xxx_read(struct bq27xxx_device_info *di, u8 reg, +static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index,

                                bool single)
				 {

This is pretty anti-social function now has completely different meaning, yet same name and compatible enough not to catch mistakes.

And sure enough:

static int bq27xxx_battery_read_charge(struct bq27xxx_device_info *di,u8 reg)

{
        int charge;

        charge = bq27xxx_read(di, reg, false);

The internal functions already check for non-existent registers. We can kill the debug prints and reuse that... its better than not initializing half the struct.

The types should in bq27xxx_battery_read_time should be certainly fixed like the patch below suggests.

The patch does not compile, but I should be sleeping, not trying to understand crazy code. Whoever wrote it, please fix it. Maybe you can just do

                        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;

undonditionaly so that half of file does not need updating.

Pavel

drivers/power/bq27xxx_battery.c +10 ~~-23~~

index 880233c..e9f26f5 100644
@@ -483,11 +483,11 @@ static int bq27xxx_battery_read_soc(struct bq27xxx_device_info *di)
  * Return a battery charge value in ?Ah
  * Or < 0 if something fails.
  */
-static int bq27xxx_battery_read_charge(struct bq27xxx_device_info *di, u8 reg)
+static int bq27xxx_battery_read_charge(struct bq27xxx_device_info *di, int reg_index)
 {
 	int charge;
 
-	charge = bq27xxx_read(di, reg, false);
+	charge = bq27xxx_read(di, reg_index, false);
 	if (charge < 0) {
 		dev_dbg(di->dev, "error reading charge register %02x: %d\n",
 			reg, charge);

@@ -561,7 +561,6 @@ static int bq27xxx_battery_read_energy(struct bq27xxx_device_info *di)
 
 	ae = bq27xxx_read(di, BQ27XXX_REG_AE, false);
 	if (ae < 0) {
-		dev_dbg(di->dev, "error reading available energy\n");
 		return ae;
 	}

@@ -602,8 +601,6 @@ static int bq27xxx_battery_read_cyct(struct bq27xxx_device_info *di)
 	int cyct;
 
 	cyct = bq27xxx_read(di, BQ27XXX_REG_CYCT, false);
-	if (cyct < 0)
-		dev_err(di->dev, "error reading cycle count total\n");
 
 	return cyct;
 }

@@ -612,14 +609,12 @@ static int bq27xxx_battery_read_cyct(struct bq27xxx_device_info *di)
  * Read a time register.
  * Return < 0 if something fails.
  */
-static int bq27xxx_battery_read_time(struct bq27xxx_device_info *di, u8 reg)
+static int bq27xxx_battery_read_time(struct bq27xxx_device_info *di, int reg_index)
 {
 	int tval;
 
-	tval = bq27xxx_read(di, reg, false);
+	tval = bq27xxx_read(di, reg_index, false);
 	if (tval < 0) {
-		dev_dbg(di->dev, "error reading time register %02x: %d\n",
-			reg, tval);
 		return tval;
 	}

@@ -639,8 +634,6 @@ static int bq27xxx_battery_read_pwr_avg(struct bq27xxx_device_info *di)
 
 	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;
 	}

@@ -731,22 +724,16 @@ static void bq27xxx_battery_update(struct bq27xxx_device_info *di)
 			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.time_to_empty = bq27xxx_battery_read_time(di, BQ27XXX_REG_TTE);
+			cache.time_to_empty_avg = bq27xxx_battery_read_time(di, BQ27XXX_REG_TTECP);
+			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);
+			cache.energy = bq27xxx_battery_read_energy(di);
 			cache.health = bq27xxx_battery_read_health(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);
+		cache.cycle_count = bq27xxx_battery_read_cyct(di);
+		cache.power_avg = bq27xxx_battery_read_pwr_avg(di);
 
 		/* We only have to read charge design full once */
 		if (di->charge_design_full <= 0)

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Pavel Machek

author

2016-01-10 08:06 UTC #1 lore raw

Hi!

Quoted 10 lines

Did /sys/class/power_supply/bq27200-0/capacity change meaning between 4.1 and 4.4?
It used to report battery capacity remaining in percent.
Not sure what it reports now, but ain't in percent....
The patch does not compile, but I should be sleeping, not trying to understand crazy code. Whoever wrote it, please fix it. Maybe you can just do

...and more crazy code :-(.

        cache.flags = bq27xxx_read(di, BQ27XXX_REG_FLAGS, has_singe_flag);
		if ((cache.flags & 0xff) == 0xff)
		                cache.flags = -1; /* read error */
				        /* WTF? bq27xxx returns -ERRNO
        on error, we mask some bits off it, and then make it -1... */

...and one crazy optimalization...

        if (memcmp(&di->cache, &cache, sizeof(cache)) != 0)
	                di->cache = cache;

...are we playing obfuscated C code contest, yet?

        case POWER_SUPPLY_PROP_PRESENT:
	                val->intval = di->cache.flags < 0 ? 0 : 1;

...to decidegree C?

                if (ret == 0)
		   	        pval->intval -= 2731; /* convert decidegree k to c */

as read takes enum, make it enum like this?
static inline int bq27xxx_read(struct bq27xxx_device_info *di, enum bq27xxx_reg_index reg_index,
bool single)

Pavel

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Andrew F. Davis

2016-01-11 14:44 UTC #2 lore raw

Quoted 13 lines · Pavel Machek

Hi!
Quoted 10 lines
Did /sys/class/power_supply/bq27200-0/capacity change meaning between 4.1 and 4.4?
It used to report battery capacity remaining in percent.
Not sure what it reports now, but ain't in percent....
The patch does not compile, but I should be sleeping, not trying to understand crazy code. Whoever wrote it, please fix it. Maybe you can just do

Quoted 8 lines

...and more crazy code :-(.

         cache.flags = bq27xxx_read(di, BQ27XXX_REG_FLAGS, has_singe_flag);
		if ((cache.flags & 0xff) == 0xff)
		                cache.flags = -1; /* read error */
				        /* WTF? bq27xxx returns -ERRNO
         on error, we mask some bits off it, and then make it -1... */

This is probably left over from when the driver was 1wire only, which seems to fail by just reading back all ones. Not sure why the -1 though?

Quoted 6 lines

...and one crazy optimalization...

         if (memcmp(&di->cache, &cache, sizeof(cache)) != 0)
	                di->cache = cache;

Hmmm, I think the lines above it:

	if (di->cache.capacity != cache.capacity)
		power_supply_changed(di->bat);

were at one point rolled into this as so:

	if (memcmp(&di->cache, &cache, sizeof(cache)) != 0) {
		di->cache = cache;
		power_supply_changed(di->bat);
	}

Otherwise that isn't an optimization, it probably takes more time comparing than just doing the copy every time...

Quoted 5 lines

...are we playing obfuscated C code contest, yet?

         case POWER_SUPPLY_PROP_PRESENT:
	                val->intval = di->cache.flags < 0 ? 0 : 1;

Quoted 8 lines

...to decidegree C?
                 if (ret == 0)
		   	        pval->intval -= 2731; /* convert decidegree k to c */
as read takes enum, make it enum like this?
static inline int bq27xxx_read(struct bq27xxx_device_info *di, enum bq27xxx_reg_index reg_index,
bool single)

ACK

Quoted 3 lines

Pavel

Andrew F. Davis

2016-01-11 14:25 UTC #3 lore raw

In reply to Pavel Machek

Quoted 203 lines · Pavel Machek

Hi!

Did /sys/class/power_supply/bq27200-0/capacity change meaning between 4.1 and 4.4?

It used to report battery capacity remaining in percent.

Not sure what it reports now, but ain't in percent....

And power_supply_class.txt says:

--- hmm. So that seems to be a mistake.

Converting them to hex gives rather strange values.

Hmm. .... what is this? Git blames Andrew F. Davis.The first part of code suggests whole cache needs to be initialized... but the second part of the code only initializes cache.time_to_empty (for example) conditionaly.

Are you sure?

                 if (has_ci_flag && (cache.flags & BQ27000_FLAG_CI)) {
                         dev_info(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_TT\
ECP);
                         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);
                         cache.health = bq27xxx_battery_read_health(di);
                 }

But that does not explain the wrong capacity..

(quoted diff) +1 -1

-static inline int bq27xxx_read(struct bq27xxx_device_info *di, u8  reg,
+static inline int bq27xxx_read(struct bq27xxx_device_info *di, int  reg_index,
                                 bool single)

This is pretty anti-social function now has completely different meaning, yet same name and compatible enough not to catch mistakes.

And sure enough:

static int bq27xxx_battery_read_charge(struct bq27xxx_device_info *di,u8 reg)

{
         int charge;

         charge = bq27xxx_read(di, reg, false);

The internal functions already check for non-existent registers. We can kill the debug prints and reuse that... its better than not initializing half the struct.

The types should in bq27xxx_battery_read_time should be certainly fixed like the patch below suggests.

The patch does not compile, but I should be sleeping, not trying to understand crazy code. Whoever wrote it, please fix it. Maybe you can just do

                         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;

undonditionaly so that half of file does not need updating.

Pavel

drivers/power/bq27xxx_battery.c +8 ~~-19~~

index 880233c..e9f26f5 100644
@@ -483,11 +483,11 @@ static int bq27xxx_battery_read_soc(struct bq27xxx_device_info *di)
   * Return a battery charge value in ?Ah
   * Or < 0 if something fails.
   */
-static int bq27xxx_battery_read_charge(struct bq27xxx_device_info *di, u8 reg)
+static int bq27xxx_battery_read_charge(struct bq27xxx_device_info *di, int reg_index)
  {
  	int charge;
 
-	charge = bq27xxx_read(di, reg, false);
+	charge = bq27xxx_read(di, reg_index, false);
  	if (charge < 0) {
  		dev_dbg(di->dev, "error reading charge register %02x: %d\n",
  			reg, charge);

@@ -561,7 +561,6 @@ static int bq27xxx_battery_read_energy(struct bq27xxx_device_info *di)
 
  	ae = bq27xxx_read(di, BQ27XXX_REG_AE, false);
  	if (ae < 0) {
-		dev_dbg(di->dev, "error reading available energy\n");
  		return ae;
  	}

@@ -602,8 +601,6 @@ static int bq27xxx_battery_read_cyct(struct bq27xxx_device_info *di)
  	int cyct;
 
  	cyct = bq27xxx_read(di, BQ27XXX_REG_CYCT, false);
-	if (cyct < 0)
-		dev_err(di->dev, "error reading cycle count total\n");
 
  	return cyct;
  }

@@ -612,14 +609,12 @@ static int bq27xxx_battery_read_cyct(struct bq27xxx_device_info *di)
   * Read a time register.
   * Return < 0 if something fails.
   */
-static int bq27xxx_battery_read_time(struct bq27xxx_device_info *di, u8 reg)
+static int bq27xxx_battery_read_time(struct bq27xxx_device_info *di, int reg_index)
  {
  	int tval;
 
-	tval = bq27xxx_read(di, reg, false);
+	tval = bq27xxx_read(di, reg_index, false);
  	if (tval < 0) {
-		dev_dbg(di->dev, "error reading time register %02x: %d\n",
-			reg, tval);
  		return tval;
  	}

@@ -639,8 +634,6 @@ static int bq27xxx_battery_read_pwr_avg(struct bq27xxx_device_info *di)
 
  	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;
  	}

@@ -731,22 +724,16 @@ static void bq27xxx_battery_update(struct bq27xxx_device_info *di)
  			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.time_to_empty = bq27xxx_battery_read_time(di, BQ27XXX_REG_TTE);
+			cache.time_to_empty_avg = bq27xxx_battery_read_time(di, BQ27XXX_REG_TTECP);
+			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);
+			cache.energy = bq27xxx_battery_read_energy(di);
  			cache.health = bq27xxx_battery_read_health(di);

drivers/power/bq27xxx_battery.c static void bq27xxx_battery_update(struct bq27xxx_device_info *di) · line 739

Quoted 10 lines

drivers/power/bq27xxx_battery.c +2 -4

  		}
-		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);
+		cache.cycle_count = bq27xxx_battery_read_cyct(di);
+		cache.power_avg = bq27xxx_battery_read_pwr_avg(di);
 
  		/* We only have to read charge design full once */
  		if (di->charge_design_full <= 0)

There was a resent overhaul of this driver and a lot of code is vestigial as you have seen. I've been meaning to continue cleaning it up, my next step would probably to bring up regmap on the 1wire bus, so that we could cut out a whole bunch of this stuff by using its built-in caching instead of this attempt at rolling our own.

Beyond this, I see no immediate problems with these suggested changes, although attempting to call a function to read a value when that associated register is invalid doesn't make much sense ether, maybe clearing the cache to start would work better? Or maybe even just trashing the whole caching thing.

Pavel Machek

author

2016-01-11 21:42 UTC #4 lore raw

Hi!

Quoted 6 lines

Quoted 2 lines
Did /sys/class/power_supply/bq27200-0/capacity change meaning between 4.1 and 4.4?
There was a resent overhaul of this driver and a lot of code is vestigial as you have seen. I've been meaning to continue cleaning it up, my next step would probably to bring up regmap

Ok, 1wire is probably reasonable approach. But we have an regression between 4.1 and 4.4, and to debug it is probably by reading code.

I can't prove you caused the regression, but you basically rewrote the driver, so it should be easier for you to spot the bug.

n900 has this variant:

      	 bq27200: bq27200 at 55 {
	                 compatible = "ti,bq27200";

reg = <0x55>;
};

Will you try?

If not, tell me, and I can do it, but it will involve a lot of swaring...

Thanks,
Pavel

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Andrew F. Davis

2016-01-11 21:48 UTC #5 lore raw

Quoted 9 lines · Pavel Machek

Hi!
Quoted 6 lines
Quoted 2 lines
Did /sys/class/power_supply/bq27200-0/capacity change meaning between 4.1 and 4.4?
There was a resent overhaul of this driver and a lot of code is vestigial as you have seen. I've been meaning to continue cleaning it up, my next step would probably to bring up regmap

Quoted 6 lines

Ok, 1wire is probably reasonable approach. But we have an regression between 4.1 and 4.4, and to debug it is probably by reading code.
I can't prove you caused the regression, but you basically rewrote the driver, so it should be easier for you to spot the bug.

Makes sense.

Quoted 7 lines

n900 has this variant:

       	 bq27200: bq27200 at 55 {
	                 compatible = "ti,bq27200";

reg = <0x55>;
};

Hmmm, not sure if I have this one on hand, I'll see if I can find an n900 around here (they seem to be pretty popular around here for testing (had a lot of TI parts)).

Quoted 4 lines

Will you try?
If not, tell me, and I can do it, but it will involve a lot of swaring...

I'll give it a look over.

Quoted 3 lines

Thanks,
Pavel

Andrew F. Davis

2016-01-12 15:22 UTC #6 lore raw

Quoted 31 lines · Andrew F. Davis

Quoted 9 lines · Pavel Machek
Hi!
Quoted 6 lines
Quoted 2 lines
Did /sys/class/power_supply/bq27200-0/capacity change meaning between 4.1 and 4.4?
There was a resent overhaul of this driver and a lot of code is vestigial as you have seen. I've been meaning to continue cleaning it up, my next step would probably to bring up regmap
Quoted 6 lines
Ok, 1wire is probably reasonable approach. But we have an regression between 4.1 and 4.4, and to debug it is probably by reading code.
I can't prove you caused the regression, but you basically rewrote the driver, so it should be easier for you to spot the bug.
Makes sense.
Quoted 7 lines
n900 has this variant:
            bq27200: bq27200 at 55 {
                     compatible = "ti,bq27200";
               reg = <0x55>;
      };
Hmmm, not sure if I have this one on hand, I'll see if I can find an n900 around here (they seem to be pretty popular around here for testing (had a lot of TI parts)).

Quoted 6 lines

Quoted 4 lines
Will you try?
If not, tell me, and I can do it, but it will involve a lot of swaring...
I'll give it a look over.

OK, I'm still looking for a test setup, but this was definitely a problem, could you give this a try?:

drivers/power/bq27xxx_battery.c +2 -1

index 880233c..4fec3cf 100644
@@ -470,8 +470,9 @@ static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index,
  static int bq27xxx_battery_read_soc(struct bq27xxx_device_info *di)
  {
  	int soc;
+	bool single = di->chip == BQ27000 || di->chip == BQ27010;
  
-	soc = bq27xxx_read(di, BQ27XXX_REG_SOC, false);
+	soc = bq27xxx_read(di, BQ27XXX_REG_SOC, single);
  
  	if (soc < 0)
  		dev_dbg(di->dev, "error reading State-of-Charge\n");

Pavel Machek

author Tested-by Acked-by

2016-01-12 21:53 UTC #7 lore raw

Hi!

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
If not, tell me, and I can do it, but it will involve a lot of swaring...
I'll give it a look over.
OK, I'm still looking for a test setup, but this was definitely a problem, could you give this a try?:

Well, search no more. I tested it and seems to work ok. There's one more blank line in the source, so I had to apply patch by hand.

Tested-by Pavel Machek <[email protected]>

Acked-by Pavel Machek <[email protected]>

Reported-by Pavel Machek <[email protected]>

Cc stable at vger.kernel.org

? :-).

And... thanks!
Pavel

Quoted 13 lines

drivers/power/bq27xxx_battery.c +2 -1

index 880233c..4fec3cf 100644
@@ -470,8 +470,9 @@ static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index,
 static int bq27xxx_battery_read_soc(struct bq27xxx_device_info *di)
 {
 	int soc;
+	bool single = di->chip == BQ27000 || di->chip == BQ27010;
-	soc = bq27xxx_read(di, BQ27XXX_REG_SOC, false);
+	soc = bq27xxx_read(di, BQ27XXX_REG_SOC, single);
 	if (soc < 0)
 		dev_dbg(di->dev, "error reading State-of-Charge\n");

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Pali Rohár

2016-01-13 08:44 UTC #8 lore raw

Quoted 17 lines · On Tuesday 12 January 2016 22:53:52 Pavel Machek wrote:

Hi!
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
If not, tell me, and I can do it, but it will involve a lot of swaring...
I'll give it a look over.
OK, I'm still looking for a test setup, but this was definitely a problem, could you give this a try?:
Well, search no more. I tested it and seems to work ok. There's one more blank line in the source, so I had to apply patch by hand.
Tested-by Pavel Machek <[email protected]>
Acked-by Pavel Machek <[email protected]>
Reported-by Pavel Machek <[email protected]>
Cc stable at vger.kernel.org

Quoted 4 lines

? :-).
And... thanks!
Pavel

There are more pending fixes for bq27xxx... Maybe your is duplicate?

http://thread.gmane.org/gmane.linux.power-management.general/70090
http://thread.gmane.org/gmane.linux.power-management.general/69716/focus=69714
http://thread.gmane.org/gmane.linux.power-management.general/69716/focus=2110580

Sebastian, please take them and ideally send to stable@ for backporting.

Quoted 15 lines

Quoted 14 lines

drivers/power/bq27xxx_battery.c +2 -1

index 880233c..4fec3cf 100644
@@ -470,8 +470,9 @@ static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index,
 static int bq27xxx_battery_read_soc(struct bq27xxx_device_info *di)
 {
 	int soc;
+	bool single = di->chip == BQ27000 || di->chip == BQ27010;
-	soc = bq27xxx_read(di, BQ27XXX_REG_SOC, false);
+	soc = bq27xxx_read(di, BQ27XXX_REG_SOC, single);
 	if (soc < 0)
 		dev_dbg(di->dev, "error reading State-of-Charge\n");

Signature · 2 lines

Pali Roh?r
pali.rohar at gmail.com

Sebastian Reichel

2016-01-13 10:26 UTC #9 lore raw

Quoted 18 lines · Pali Roh?r

Quoted 17 lines · On Tuesday 12 January 2016 22:53:52 Pavel Machek wrote:
Hi!
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
If not, tell me, and I can do it, but it will involve a lot of swaring...
I'll give it a look over.
OK, I'm still looking for a test setup, but this was definitely a problem, could you give this a try?:
Well, search no more. I tested it and seems to work ok. There's one more blank line in the source, so I had to apply patch by hand.
Tested-by Pavel Machek <[email protected]>
Acked-by Pavel Machek <[email protected]>
Reported-by Pavel Machek <[email protected]>
Cc stable at vger.kernel.org

Quoted 6 lines

Quoted 4 lines
? :-).
And... thanks!
Pavel
There are more pending fixes for bq27xxx... Maybe your is duplicate?

No, it's not a duplicate.

Quoted 5 lines

http://thread.gmane.org/gmane.linux.power-management.general/70090
http://thread.gmane.org/gmane.linux.power-management.general/69716/focus=69714
http://thread.gmane.org/gmane.linux.power-management.general/69716/focus=2110580
Sebastian, please take them and ideally send to stable@ for backporting.

There is no need to send them to stable@, if they are properly tagged with a 'Fixes: <sha1> ("title")' tag, so please always include it in your commit messages, if possible :)

-- Sebastian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160113/b74fb73d/attachment-0001.sig>

Pali Rohár

2016-01-13 10:32 UTC #10 lore raw

Quoted 19 lines · On Wednesday 13 January 2016 11:26:13 Sebastian Reichel wrote:

Quoted 18 lines · Pali Roh?r
Quoted 17 lines · On Tuesday 12 January 2016 22:53:52 Pavel Machek wrote:
Hi!
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
If not, tell me, and I can do it, but it will involve a lot of swaring...
I'll give it a look over.
OK, I'm still looking for a test setup, but this was definitely a problem, could you give this a try?:
Well, search no more. I tested it and seems to work ok. There's one more blank line in the source, so I had to apply patch by hand.
Tested-by Pavel Machek <[email protected]>
Acked-by Pavel Machek <[email protected]>
Reported-by Pavel Machek <[email protected]>
Cc stable at vger.kernel.org

Quoted 12 lines

Quoted 6 lines
Quoted 4 lines
? :-).
And... thanks!
Pavel
There are more pending fixes for bq27xxx... Maybe your is duplicate?
No, it's not a duplicate.
Quoted 3 lines
http://thread.gmane.org/gmane.linux.power-management.general/70090
http://thread.gmane.org/gmane.linux.power-management.general/69716/focus=69714
http://thread.gmane.org/gmane.linux.power-management.general/69716/focus=2110580

Hm... It looks like Pavel's patch is subset of this 3rd patch as it fix function bq27xxx_battery_read_soc()

Quoted 5 lines

Quoted 1 line
Sebastian, please take them and ideally send to stable@ for backporting.
There is no need to send them to stable@, if they are properly tagged with a 'Fixes: <sha1> ("title")' tag, so please always include it in your commit messages, if possible :)

I have not sent those patches, but OK. In that email thread you can find sha1 commit id which broke it.

Signature · 2 lines

Pali Roh?r
pali.rohar at gmail.com

Pavel Machek

author

2016-01-13 11:01 UTC #11 lore raw

In reply to Sebastian Reichel

Quoted 32 lines · Sebastian Reichel

Quoted 18 lines · Pali Roh?r
Quoted 17 lines · On Tuesday 12 January 2016 22:53:52 Pavel Machek wrote:
Hi!
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
If not, tell me, and I can do it, but it will involve a lot of swaring...
I'll give it a look over.
OK, I'm still looking for a test setup, but this was definitely a problem, could you give this a try?:
Well, search no more. I tested it and seems to work ok. There's one more blank line in the source, so I had to apply patch by hand.
Tested-by Pavel Machek <[email protected]>
Acked-by Pavel Machek <[email protected]>
Reported-by Pavel Machek <[email protected]>
Cc stable at vger.kernel.org
Quoted 6 lines
Quoted 4 lines
? :-).
And... thanks!
Pavel
There are more pending fixes for bq27xxx... Maybe your is duplicate?
No, it's not a duplicate.
Quoted 4 lines
http://thread.gmane.org/gmane.linux.power-management.general/70090
http://thread.gmane.org/gmane.linux.power-management.general/69716/focus=69714
http://thread.gmane.org/gmane.linux.power-management.general/69716/focus=2110580

Quoted 5 lines

Quoted 1 line
Sebastian, please take them and ideally send to stable@ for backporting.
There is no need to send them to stable@, if they are properly tagged with a 'Fixes: <sha1> ("title")' tag, so please always include it in your commit messages, if possible :)

Heh, sorry about that, but I don't know what it fixed. I guess git blame would tell us.

Anyway,
http://thread.gmane.org/gmane.linux.power-management.general/69716/focus=2110580
fixed an regression in 4.4 kernel, and did not get to 4.4, which is rather sad :-(.

Pavel

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html