Message-ID: <20240306100904.1914263-1-Hermes.Zhang@axis.com>
Date: Wed, 6 Mar 2024 18:09:03 +0800
From: Hermes Zhang <Hermes.Zhang@...s.com>
To: Sebastian Reichel <sre@...nel.org>
CC: <kernel@...s.com>, Hermes Zhang <Hermes.Zhang@...s.com>,
	Pali Rohár <pali@...nel.org>, <linux-pm@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: [PATCH v2] power: supply: bq27xxx: Divide the reg cache to each register

The reg cache used to be grouped and activated for each property
access, regardless of whether it was part of the group. That will
lead to a significant increase in I2C transmission.
Divide the cache group and create a cache for every register. The
cache won't work until the register has been fetched. This will help
in reducing the quantity of pointless I2C communication and avoiding
the error -16 (EBUSY) that occurs while using an I2C bus that is
shared by many devices.

Signed-off-by: Hermes Zhang <Hermes.Zhang@...s.com>
---
v2:
 - Refactor implementation.

 drivers/power/supply/bq27xxx_battery.c | 231 +++++++++++++++++--------
 include/linux/power/bq27xxx_battery.h  |  30 ++--
 2 files changed, 179 insertions(+), 82 deletions(-)

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 1c4a9d137744..cc724322f4f0 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -1746,14 +1746,16 @@ static bool bq27xxx_battery_capacity_inaccurate(struct bq27xxx_device_info *di,
 
 static int bq27xxx_battery_read_health(struct bq27xxx_device_info *di)
 {
+	int flags = di->cache[CACHE_REG_FLAGS].value;
+
 	/* Unlikely but important to return first */
-	if (unlikely(bq27xxx_battery_overtemp(di, di->cache.flags)))
+	if (unlikely(bq27xxx_battery_overtemp(di, flags)))
 		return POWER_SUPPLY_HEALTH_OVERHEAT;
-	if (unlikely(bq27xxx_battery_undertemp(di, di->cache.flags)))
+	if (unlikely(bq27xxx_battery_undertemp(di, flags)))
 		return POWER_SUPPLY_HEALTH_COLD;
-	if (unlikely(bq27xxx_battery_dead(di, di->cache.flags)))
+	if (unlikely(bq27xxx_battery_dead(di, flags)))
 		return POWER_SUPPLY_HEALTH_DEAD;
-	if (unlikely(bq27xxx_battery_capacity_inaccurate(di, di->cache.flags)))
+	if (unlikely(bq27xxx_battery_capacity_inaccurate(di, flags)))
 		return POWER_SUPPLY_HEALTH_CALIBRATION_REQUIRED;
 
 	return POWER_SUPPLY_HEALTH_GOOD;
@@ -1778,7 +1780,7 @@ static int bq27xxx_battery_current_and_status(
 	struct bq27xxx_device_info *di,
 	union power_supply_propval *val_curr,
 	union power_supply_propval *val_status,
-	struct bq27xxx_reg_cache *cache)
+	struct bq27xxx_cache_reg *reg)
 {
 	bool single_flags = (di->opts & BQ27XXX_O_ZERO);
 	int curr;
@@ -1790,8 +1792,8 @@ static int bq27xxx_battery_current_and_status(
 		return curr;
 	}
 
-	if (cache) {
-		flags = cache->flags;
+	if (reg) {
+		flags = reg->value;
 	} else {
 		flags = bq27xxx_read(di, BQ27XXX_REG_FLAGS, single_flags);
 		if (flags < 0) {
@@ -1832,57 +1834,128 @@ static int bq27xxx_battery_current_and_status(
 	return 0;
 }
 
-static void bq27xxx_battery_update_unlocked(struct bq27xxx_device_info *di)
+static int bq27xxx_cached_reg_value_unlocked(struct bq27xxx_device_info *di,
+					     enum bq27xxx_cache_registers item)
 {
-	union power_supply_propval status = di->last_status;
-	struct bq27xxx_reg_cache cache = {0, };
-	bool has_singe_flag = di->opts & BQ27XXX_O_ZERO;
-
-	cache.flags = bq27xxx_read(di, BQ27XXX_REG_FLAGS, has_singe_flag);
-	if ((cache.flags & 0xff) == 0xff)
-		cache.flags = -1; /* read error */
-	if (cache.flags >= 0) {
-		cache.temperature = bq27xxx_battery_read_temperature(di);
+	struct bq27xxx_cache_reg *reg;
+	int tmp = -EINVAL;
+
+	reg = &di->cache[item];
+
+	if (time_is_after_jiffies(reg->last_update + 5 * HZ))
+		return reg->value;
+
+	switch (item) {
+	case CACHE_REG_TEMPERATURE:
+		tmp = bq27xxx_battery_read_temperature(di);
+		break;
+	case CACHE_REG_TIME_TO_EMPTY:
 		if (di->regs[BQ27XXX_REG_TTE] != INVALID_REG_ADDR)
-			cache.time_to_empty = bq27xxx_battery_read_time(di, BQ27XXX_REG_TTE);
+			tmp = bq27xxx_battery_read_time(di, BQ27XXX_REG_TTE);
+		break;
+	case CACHE_REG_TIME_TO_EMPTY_AVG:
 		if (di->regs[BQ27XXX_REG_TTECP] != INVALID_REG_ADDR)
-			cache.time_to_empty_avg = bq27xxx_battery_read_time(di, BQ27XXX_REG_TTECP);
+			tmp = bq27xxx_battery_read_time(di, BQ27XXX_REG_TTECP);
+		break;
+	case CACHE_REG_TIME_TO_FULL:
 		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);
-		di->cache.flags = cache.flags;
-		cache.health = bq27xxx_battery_read_health(di);
+			tmp = bq27xxx_battery_read_time(di, BQ27XXX_REG_TTF);
+		break;
+	case CACHE_REG_CHARGE_FULL:
+		tmp = bq27xxx_battery_read_fcc(di);
+		break;
+	case CACHE_REG_CYCLE_COUNT:
 		if (di->regs[BQ27XXX_REG_CYCT] != INVALID_REG_ADDR)
-			cache.cycle_count = bq27xxx_battery_read_cyct(di);
-
-		/*
-		 * On gauges with signed current reporting the current must be
-		 * checked to detect charging <-> discharging status changes.
-		 */
-		if (!(di->opts & BQ27XXX_O_ZERO))
-			bq27xxx_battery_current_and_status(di, NULL, &status, &cache);
-
-		/* We only have to read charge design full once */
-		if (di->charge_design_full <= 0)
-			di->charge_design_full = bq27xxx_battery_read_dcap(di);
+			tmp = bq27xxx_battery_read_cyct(di);
+		break;
+	case CACHE_REG_CAPACITY:
+		tmp = bq27xxx_battery_read_soc(di);
+		break;
+	case CACHE_REG_ENERGY:
+		if (di->regs[BQ27XXX_REG_AE] != INVALID_REG_ADDR)
+			tmp = bq27xxx_battery_read_energy(di);
+		break;
+	case CACHE_REG_FLAGS:
+		bool has_singe_flag = di->opts & BQ27XXX_O_ZERO;
+
+		tmp = bq27xxx_read(di, BQ27XXX_REG_FLAGS, has_singe_flag);
+		if ((tmp & 0xff) == 0xff)
+			tmp = -1; /* read error */
+		break;
+	default:
+		break;
+	}
+
+	/* only update cache value when successful */
+	if (tmp >= 0) {
+		reg->value = tmp;
+		reg->last_update = jiffies;
 	}
 
-	if ((di->cache.capacity != cache.capacity) ||
-	    (di->cache.flags != cache.flags) ||
+	return tmp;
+}
+
+static int bq27xxx_cached_reg_value(struct bq27xxx_device_info *di,
+				    enum bq27xxx_cache_registers item)
+{
+	int ret;
+
+	mutex_lock(&di->lock);
+	ret = bq27xxx_cached_reg_value_unlocked(di, item);
+	mutex_unlock(&di->lock);
+
+	return ret;
+}
+
+static void bq27xxx_battery_update_unlocked(struct bq27xxx_device_info *di)
+{
+	union power_supply_propval status = di->last_status;
+	int old_flags, flags;
+	int old_capacity, capacity;
+
+	old_capacity = di->cache[CACHE_REG_CAPACITY].value;
+	capacity = old_capacity;
+
+	old_flags = di->cache[CACHE_REG_FLAGS].value;
+	flags = bq27xxx_cached_reg_value_unlocked(di, CACHE_REG_FLAGS);
+
+	if (flags < 0)
+		goto out;
+
+	bq27xxx_cached_reg_value_unlocked(di, CACHE_REG_TEMPERATURE);
+	if (di->regs[BQ27XXX_REG_TTE] != INVALID_REG_ADDR)
+		bq27xxx_cached_reg_value_unlocked(di, CACHE_REG_TIME_TO_EMPTY);
+	if (di->regs[BQ27XXX_REG_TTECP] != INVALID_REG_ADDR)
+		bq27xxx_cached_reg_value_unlocked(di, CACHE_REG_TIME_TO_EMPTY_AVG);
+	if (di->regs[BQ27XXX_REG_TTF] != INVALID_REG_ADDR)
+		bq27xxx_cached_reg_value_unlocked(di, CACHE_REG_TIME_TO_FULL);
+
+	bq27xxx_cached_reg_value_unlocked(di, CACHE_REG_CHARGE_FULL);
+	bq27xxx_cached_reg_value_unlocked(di, CACHE_REG_CAPACITY);
+	if (di->regs[BQ27XXX_REG_AE] != INVALID_REG_ADDR)
+		bq27xxx_cached_reg_value_unlocked(di, CACHE_REG_ENERGY);
+	if (di->regs[BQ27XXX_REG_CYCT] != INVALID_REG_ADDR)
+		bq27xxx_cached_reg_value_unlocked(di, CACHE_REG_CYCLE_COUNT);
+
+	/*
+	 * On gauges with signed current reporting the current must be
+	 * checked to detect charging <-> discharging status changes.
+	 */
+	if (!(di->opts & BQ27XXX_O_ZERO))
+		bq27xxx_battery_current_and_status(di, NULL, &status,
+						   &di->cache[CACHE_REG_FLAGS]);
+
+	/* We only have to read charge design full once */
+	if (di->charge_design_full <= 0)
+		di->charge_design_full = bq27xxx_battery_read_dcap(di);
+
+out:
+	if ((old_capacity != capacity) || (old_flags != flags) ||
 	    (di->last_status.intval != status.intval)) {
 		di->last_status.intval = status.intval;
 		power_supply_changed(di->bat);
 	}
 
-	if (memcmp(&di->cache, &cache, sizeof(cache)) != 0)
-		di->cache = cache;
-
-	di->last_update = jiffies;
-
 	if (!di->removed && poll_interval > 0)
 		mod_delayed_work(system_wq, &di->work, poll_interval * HZ);
 }
@@ -1934,29 +2007,32 @@ static int bq27xxx_battery_capacity_level(struct bq27xxx_device_info *di,
 					  union power_supply_propval *val)
 {
 	int level;
+	int flags;
+
+	flags = di->cache[CACHE_REG_FLAGS].value;
 
 	if (di->opts & BQ27XXX_O_ZERO) {
-		if (di->cache.flags & BQ27000_FLAG_FC)
+		if (flags & BQ27000_FLAG_FC)
 			level = POWER_SUPPLY_CAPACITY_LEVEL_FULL;
-		else if (di->cache.flags & BQ27000_FLAG_EDVF)
+		else if (flags & BQ27000_FLAG_EDVF)
 			level = POWER_SUPPLY_CAPACITY_LEVEL_CRITICAL;
-		else if (di->cache.flags & BQ27000_FLAG_EDV1)
+		else if (flags & BQ27000_FLAG_EDV1)
 			level = POWER_SUPPLY_CAPACITY_LEVEL_LOW;
 		else
 			level = POWER_SUPPLY_CAPACITY_LEVEL_NORMAL;
 	} else if (di->opts & BQ27Z561_O_BITS) {
-		if (di->cache.flags & BQ27Z561_FLAG_FC)
+		if (flags & BQ27Z561_FLAG_FC)
 			level = POWER_SUPPLY_CAPACITY_LEVEL_FULL;
-		else if (di->cache.flags & BQ27Z561_FLAG_FDC)
+		else if (flags & BQ27Z561_FLAG_FDC)
 			level = POWER_SUPPLY_CAPACITY_LEVEL_CRITICAL;
 		else
 			level = POWER_SUPPLY_CAPACITY_LEVEL_NORMAL;
 	} else {
-		if (di->cache.flags & BQ27XXX_FLAG_FC)
+		if (flags & BQ27XXX_FLAG_FC)
 			level = POWER_SUPPLY_CAPACITY_LEVEL_FULL;
-		else if (di->cache.flags & BQ27XXX_FLAG_SOCF)
+		else if (flags & BQ27XXX_FLAG_SOCF)
 			level = POWER_SUPPLY_CAPACITY_LEVEL_CRITICAL;
-		else if (di->cache.flags & BQ27XXX_FLAG_SOC1)
+		else if (flags & BQ27XXX_FLAG_SOC1)
 			level = POWER_SUPPLY_CAPACITY_LEVEL_LOW;
 		else
 			level = POWER_SUPPLY_CAPACITY_LEVEL_NORMAL;
@@ -2004,13 +2080,12 @@ static int bq27xxx_battery_get_property(struct power_supply *psy,
 {
 	int ret = 0;
 	struct bq27xxx_device_info *di = power_supply_get_drvdata(psy);
+	int flags;
+	int cache;
 
-	mutex_lock(&di->lock);
-	if (time_is_before_jiffies(di->last_update + 5 * HZ))
-		bq27xxx_battery_update_unlocked(di);
-	mutex_unlock(&di->lock);
+	flags = bq27xxx_cached_reg_value(di, CACHE_REG_FLAGS);
 
-	if (psp != POWER_SUPPLY_PROP_PRESENT && di->cache.flags < 0)
+	if (psp != POWER_SUPPLY_PROP_PRESENT && flags < 0)
 		return -ENODEV;
 
 	switch (psp) {
@@ -2021,30 +2096,40 @@ static int bq27xxx_battery_get_property(struct power_supply *psy,
 		ret = bq27xxx_battery_voltage(di, val);
 		break;
 	case POWER_SUPPLY_PROP_PRESENT:
-		val->intval = di->cache.flags < 0 ? 0 : 1;
+		val->intval = flags < 0 ? 0 : 1;
 		break;
 	case POWER_SUPPLY_PROP_CURRENT_NOW:
 		ret = bq27xxx_battery_current_and_status(di, val, NULL, NULL);
 		break;
 	case POWER_SUPPLY_PROP_CAPACITY:
-		ret = bq27xxx_simple_value(di->cache.capacity, val);
+		cache = bq27xxx_cached_reg_value(di, CACHE_REG_CAPACITY);
+
+		ret = bq27xxx_simple_value(cache, val);
 		break;
 	case POWER_SUPPLY_PROP_CAPACITY_LEVEL:
 		ret = bq27xxx_battery_capacity_level(di, val);
 		break;
 	case POWER_SUPPLY_PROP_TEMP:
-		ret = bq27xxx_simple_value(di->cache.temperature, val);
+		cache = bq27xxx_cached_reg_value(di, CACHE_REG_TEMPERATURE);
+
+		ret = bq27xxx_simple_value(cache, val);
 		if (ret == 0)
 			val->intval -= 2731; /* convert decidegree k to c */
 		break;
 	case POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW:
-		ret = bq27xxx_simple_value(di->cache.time_to_empty, val);
+		cache = bq27xxx_cached_reg_value(di, CACHE_REG_TIME_TO_EMPTY);
+
+		ret = bq27xxx_simple_value(cache, val);
 		break;
 	case POWER_SUPPLY_PROP_TIME_TO_EMPTY_AVG:
-		ret = bq27xxx_simple_value(di->cache.time_to_empty_avg, val);
+		cache = bq27xxx_cached_reg_value(di, CACHE_REG_TIME_TO_EMPTY_AVG);
+
+		ret = bq27xxx_simple_value(cache, val);
 		break;
 	case POWER_SUPPLY_PROP_TIME_TO_FULL_NOW:
-		ret = bq27xxx_simple_value(di->cache.time_to_full, val);
+		cache = bq27xxx_cached_reg_value(di, CACHE_REG_TIME_TO_FULL);
+
+		ret = bq27xxx_simple_value(cache, val);
 		break;
 	case POWER_SUPPLY_PROP_TECHNOLOGY:
 		if (di->opts & BQ27XXX_O_MUL_CHEM)
@@ -2059,7 +2144,9 @@ static int bq27xxx_battery_get_property(struct power_supply *psy,
 			ret = bq27xxx_simple_value(bq27xxx_battery_read_rc(di), val);
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_FULL:
-		ret = bq27xxx_simple_value(di->cache.charge_full, val);
+		cache = bq27xxx_cached_reg_value(di, CACHE_REG_CHARGE_FULL);
+
+		ret = bq27xxx_simple_value(cache, val);
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
 		ret = bq27xxx_simple_value(di->charge_design_full, val);
@@ -2072,16 +2159,22 @@ static int bq27xxx_battery_get_property(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
 		return -EINVAL;
 	case POWER_SUPPLY_PROP_CYCLE_COUNT:
-		ret = bq27xxx_simple_value(di->cache.cycle_count, val);
+		cache = bq27xxx_cached_reg_value(di, CACHE_REG_CYCLE_COUNT);
+
+		ret = bq27xxx_simple_value(cache, val);
 		break;
 	case POWER_SUPPLY_PROP_ENERGY_NOW:
-		ret = bq27xxx_simple_value(di->cache.energy, val);
+		cache = bq27xxx_cached_reg_value(di, CACHE_REG_ENERGY);
+
+		ret = bq27xxx_simple_value(cache, val);
 		break;
 	case POWER_SUPPLY_PROP_POWER_AVG:
 		ret = bq27xxx_battery_pwr_avg(di, val);
 		break;
 	case POWER_SUPPLY_PROP_HEALTH:
-		ret = bq27xxx_simple_value(di->cache.health, val);
+		cache = bq27xxx_battery_read_health(di);
+
+		ret = bq27xxx_simple_value(cache, val);
 		break;
 	case POWER_SUPPLY_PROP_MANUFACTURER:
 		val->strval = BQ27XXX_MANUFACTURER;
diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
index 7d8025fb74b7..617c8409d80f 100644
--- a/include/linux/power/bq27xxx_battery.h
+++ b/include/linux/power/bq27xxx_battery.h
@@ -46,17 +46,22 @@ struct bq27xxx_access_methods {
 	int (*write_bulk)(struct bq27xxx_device_info *di, u8 reg, u8 *data, int len);
 };
 
-struct bq27xxx_reg_cache {
-	int temperature;
-	int time_to_empty;
-	int time_to_empty_avg;
-	int time_to_full;
-	int charge_full;
-	int cycle_count;
-	int capacity;
-	int energy;
-	int flags;
-	int health;
+struct bq27xxx_cache_reg {
+	int value;
+	unsigned long last_update;
+};
+
+enum bq27xxx_cache_registers {
+	CACHE_REG_TEMPERATURE = 0,
+	CACHE_REG_TIME_TO_EMPTY,
+	CACHE_REG_TIME_TO_EMPTY_AVG,
+	CACHE_REG_TIME_TO_FULL,
+	CACHE_REG_CHARGE_FULL,
+	CACHE_REG_CYCLE_COUNT,
+	CACHE_REG_CAPACITY,
+	CACHE_REG_ENERGY,
+	CACHE_REG_FLAGS,
+	CACHE_REG_MAX,
 };
 
 struct bq27xxx_device_info {
@@ -68,10 +73,9 @@ struct bq27xxx_device_info {
 	struct bq27xxx_dm_reg *dm_regs;
 	u32 unseal_key;
 	struct bq27xxx_access_methods bus;
-	struct bq27xxx_reg_cache cache;
+	struct bq27xxx_cache_reg cache[CACHE_REG_MAX];
 	int charge_design_full;
 	bool removed;
-	unsigned long last_update;
 	union power_supply_propval last_status;
 	struct delayed_work work;
 	struct power_supply *bat;
-- 
2.39.2

Powered by blists - more mailing lists