bq27x00: use cached flags

PATCH In review Arthur Demchenkov 3 participants 3 messages Feb 18, 2019 – Feb 19, 2019

Subsystems (3)

Power supply Nokia N900 power supply drivers TI BQ27XXX power supply driver

lore

Share & tools

lore

Arthur Demchenkov

author

2019-02-18 06:59 UTC #0 lore raw

PATCH 1/1 bq27x00: use cached flags

The flags were just read by bq27xxx_battery_update(), no need to read them again.

Signed-off-by Arthur Demchenkov <[email protected]>

---
 drivers/power/supply/bq27xxx_battery.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

drivers/power/supply/bq27xxx_battery.c +4 ~~-16~~

index 6dbbe95844a3..29b3a4056865 100644
@@ -1555,27 +1555,14 @@ static bool bq27xxx_battery_dead(struct bq27xxx_device_info *di, u16 flags)
 		return flags & (BQ27XXX_FLAG_SOC1 | BQ27XXX_FLAG_SOCF);
 }
 
-/*
- * Read flag register.
- * Return < 0 if something fails.
- */
 static int bq27xxx_battery_read_health(struct bq27xxx_device_info *di)
 {
-	int flags;
-	bool has_singe_flag = di->opts & BQ27XXX_O_ZERO;
-
-	flags = bq27xxx_read(di, BQ27XXX_REG_FLAGS, has_singe_flag);
-	if (flags < 0) {
-		dev_err(di->dev, "error reading flag register:%d\n", flags);
-		return flags;
-	}
-
 	/* Unlikely but important to return first */
-	if (unlikely(bq27xxx_battery_overtemp(di, flags)))
+	if (unlikely(bq27xxx_battery_overtemp(di, di->cache.flags)))
 		return POWER_SUPPLY_HEALTH_OVERHEAT;
-	if (unlikely(bq27xxx_battery_undertemp(di, flags)))
+	if (unlikely(bq27xxx_battery_undertemp(di, di->cache.flags)))
 		return POWER_SUPPLY_HEALTH_COLD;
-	if (unlikely(bq27xxx_battery_dead(di, flags)))
+	if (unlikely(bq27xxx_battery_dead(di, di->cache.flags)))
 		return POWER_SUPPLY_HEALTH_DEAD;
 
 	return POWER_SUPPLY_HEALTH_GOOD;

@@ -1612,6 +1599,7 @@ void bq27xxx_battery_update(struct bq27xxx_device_info *di)
 			cache.capacity = bq27xxx_battery_read_soc(di);
 			if (di->regs[BQ27XXX_REG_AE] != INVALID_REG_ADDR)
 				cache.energy = bq27xxx_battery_read_energy(di);
+			di->cache.flags = cache.flags;
 			cache.health = bq27xxx_battery_read_health(di);
 		}
 		if (di->regs[BQ27XXX_REG_CYCT] != INVALID_REG_ADDR)

Signature · 1 line

2.11.0

Andrew F. Davis

Reviewed-by on v1 1/1

2019-02-19 16:55 UTC #1 lore raw

Quoted 5 lines · Arthur Demchenkov

The flags were just read by bq27xxx_battery_update(), no need to read them again.
Signed-off-by Arthur Demchenkov <[email protected]>
---

Nothing obviously wrong with this patch so:

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

At this point we have W1 regmap and so we now have everything we should need to convert this driver over to regmap. Then the caching comes for free and a lot of checks and such can be dropped.

Thanks,
Andrew

Quoted 47 lines

 drivers/power/supply/bq27xxx_battery.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

drivers/power/supply/bq27xxx_battery.c +4 ~~-16~~

index 6dbbe95844a3..29b3a4056865 100644
@@ -1555,27 +1555,14 @@ static bool bq27xxx_battery_dead(struct bq27xxx_device_info *di, u16 flags)
 		return flags & (BQ27XXX_FLAG_SOC1 | BQ27XXX_FLAG_SOCF);
 }
 
-/*
- * Read flag register.
- * Return < 0 if something fails.
- */
 static int bq27xxx_battery_read_health(struct bq27xxx_device_info *di)
 {
-	int flags;
-	bool has_singe_flag = di->opts & BQ27XXX_O_ZERO;
-
-	flags = bq27xxx_read(di, BQ27XXX_REG_FLAGS, has_singe_flag);
-	if (flags < 0) {
-		dev_err(di->dev, "error reading flag register:%d\n", flags);
-		return flags;
-	}
-
 	/* Unlikely but important to return first */
-	if (unlikely(bq27xxx_battery_overtemp(di, flags)))
+	if (unlikely(bq27xxx_battery_overtemp(di, di->cache.flags)))
 		return POWER_SUPPLY_HEALTH_OVERHEAT;
-	if (unlikely(bq27xxx_battery_undertemp(di, flags)))
+	if (unlikely(bq27xxx_battery_undertemp(di, di->cache.flags)))
 		return POWER_SUPPLY_HEALTH_COLD;
-	if (unlikely(bq27xxx_battery_dead(di, flags)))
+	if (unlikely(bq27xxx_battery_dead(di, di->cache.flags)))
 		return POWER_SUPPLY_HEALTH_DEAD;
 
 	return POWER_SUPPLY_HEALTH_GOOD;

@@ -1612,6 +1599,7 @@ void bq27xxx_battery_update(struct bq27xxx_device_info *di)
 			cache.capacity = bq27xxx_battery_read_soc(di);
 			if (di->regs[BQ27XXX_REG_AE] != INVALID_REG_ADDR)
 				cache.energy = bq27xxx_battery_read_energy(di);
+			di->cache.flags = cache.flags;
 			cache.health = bq27xxx_battery_read_health(di);
 		}
 		if (di->regs[BQ27XXX_REG_CYCT] != INVALID_REG_ADDR)

Sebastian Reichel

maintainer

2019-02-19 23:30 UTC #2 lore raw

Hi,

Quoted 7 lines · Andrew F. Davis

Quoted 5 lines · Arthur Demchenkov
The flags were just read by bq27xxx_battery_update(), no need to read them again.
Signed-off-by Arthur Demchenkov <[email protected]>
---

Quoted 7 lines

Nothing obviously wrong with this patch so:
Reviewed-by Andrew F. Davis <[email protected]>
At this point we have W1 regmap and so we now have everything we should need to convert this driver over to regmap. Then the caching comes for free and a lot of checks and such can be dropped.

Yes, bq27xxx could be simplified a lot by converting to regmap and a patch would be appreciated. This register cannot be cached by regmap, though. Regmap caching is only for registers, that are not modified by the hardware (except during reset).

Thanks for the patch and the review, I just merged this to power-supply-next.

-- Sebastian

Quoted 51 lines

Thanks,
Andrew

Quoted 47 lines

 drivers/power/supply/bq27xxx_battery.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

drivers/power/supply/bq27xxx_battery.c +4 ~~-16~~

index 6dbbe95844a3..29b3a4056865 100644
@@ -1555,27 +1555,14 @@ static bool bq27xxx_battery_dead(struct bq27xxx_device_info *di, u16 flags)
 		return flags & (BQ27XXX_FLAG_SOC1 | BQ27XXX_FLAG_SOCF);
 }
 
-/*
- * Read flag register.
- * Return < 0 if something fails.
- */
 static int bq27xxx_battery_read_health(struct bq27xxx_device_info *di)
 {
-	int flags;
-	bool has_singe_flag = di->opts & BQ27XXX_O_ZERO;
-
-	flags = bq27xxx_read(di, BQ27XXX_REG_FLAGS, has_singe_flag);
-	if (flags < 0) {
-		dev_err(di->dev, "error reading flag register:%d\n", flags);
-		return flags;
-	}
-
 	/* Unlikely but important to return first */
-	if (unlikely(bq27xxx_battery_overtemp(di, flags)))
+	if (unlikely(bq27xxx_battery_overtemp(di, di->cache.flags)))
 		return POWER_SUPPLY_HEALTH_OVERHEAT;
-	if (unlikely(bq27xxx_battery_undertemp(di, flags)))
+	if (unlikely(bq27xxx_battery_undertemp(di, di->cache.flags)))
 		return POWER_SUPPLY_HEALTH_COLD;
-	if (unlikely(bq27xxx_battery_dead(di, flags)))
+	if (unlikely(bq27xxx_battery_dead(di, di->cache.flags)))
 		return POWER_SUPPLY_HEALTH_DEAD;
 
 	return POWER_SUPPLY_HEALTH_GOOD;

@@ -1612,6 +1599,7 @@ void bq27xxx_battery_update(struct bq27xxx_device_info *di)
 			cache.capacity = bq27xxx_battery_read_soc(di);
 			if (di->regs[BQ27XXX_REG_AE] != INVALID_REG_ADDR)
 				cache.energy = bq27xxx_battery_read_energy(di);
+			di->cache.flags = cache.flags;
 			cache.health = bq27xxx_battery_read_health(di);
 		}
 		if (di->regs[BQ27XXX_REG_CYCT] != INVALID_REG_ADDR)