diff mbox

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

Message ID 20160109230709.GA30551@amd (mailing list archive)
State New, archived
Headers show

Commit Message

Pavel Machek Jan. 9, 2016, 11:07 p.m. UTC
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@n900:/my/tui/ofone$ cat
/sys/class/power_supply/bq27200-0/capacity
7497
pavel@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@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@n900:/my/tui/ofone$ cat /sys/class/power_supply/bq27200-0/capacity
7497
pavel@n900:/my/tui/ofone$ cat /sys/class/power_supply/bq27200-0/capacity
16713
pavel@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

Comments

Pavel Machek Jan. 10, 2016, 8:06 a.m. UTC | #1
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....

> 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
Andrew Davis Jan. 11, 2016, 2:25 p.m. UTC | #2
On 01/09/2016 05:07 PM, Pavel Machek wrote:
> 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@n900:/my/tui/ofone$ cat
> /sys/class/power_supply/bq27200-0/capacity
> 7497
> pavel@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@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@n900:/my/tui/ofone$ cat /sys/class/power_supply/bq27200-0/capacity
> 7497
> pavel@n900:/my/tui/ofone$ cat /sys/class/power_supply/bq27200-0/capacity
> 16713
> pavel@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
>
> diff --git a/drivers/power/bq27xxx_battery.c b/drivers/power/bq27xxx_battery.c
> index 880233c..e9f26f5 100644
> --- a/drivers/power/bq27xxx_battery.c
> +++ b/drivers/power/bq27xxx_battery.c
> @@ -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)
>

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andrew Davis Jan. 11, 2016, 2:44 p.m. UTC | #3
On 01/10/2016 02:06 AM, Pavel Machek wrote:
> 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....
>
>> 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... */
> 	

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?

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

> ...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)
> 					

ACK

>
> 									Pavel
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pavel Machek Jan. 11, 2016, 9:42 p.m. UTC | #4
Hi!

> >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@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
Andrew Davis Jan. 11, 2016, 9:48 p.m. UTC | #5
On 01/11/2016 03:42 PM, Pavel Machek wrote:
> Hi!
>
>>> 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.
>

Makes sense.

> n900 has this variant:
>
>        	 bq27200: bq27200@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)).

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

> Thanks,
> 									Pavel
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/power/bq27xxx_battery.c b/drivers/power/bq27xxx_battery.c
index 880233c..e9f26f5 100644
--- a/drivers/power/bq27xxx_battery.c
+++ b/drivers/power/bq27xxx_battery.c
@@ -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)