Received-SPF: pass (google.com: domain of linux-kernel+bounces-93711-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Received-SPF: Fail (protection.outlook.com: domain of axis.com does not
 designate 195.60.68.100 as permitted sender) receiver=protection.outlook.com;
 client-ip=195.60.68.100; helo=mail.axis.com;
From: Hermes Zhang <Hermes.Zhang@axis.com>
To: Sebastian Reichel <sre@kernel.org>
CC: <kernel@axis.com>, Hermes Zhang <Hermes.Zhang@axis.com>,
	=?UTF-8?q?Pali=20Roh=C3=A1r?= <pali@kernel.org>, <linux-pm@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
Subject: [PATCH v2] power: supply: bq27xxx: Divide the reg cache to each register
Date: Wed, 6 Mar 2024 18:09:03 +0800
Message-ID: <20240306100904.1914263-1-Hermes.Zhang@axis.com>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Type: text/plain
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Mar 2024 10:09:09.2088
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: fc1fe342-7b12-4f2f-f1ad-08dc3dc576e5
X-MS-Exchange-CrossTenant-Id: 78703d3c-b907-432f-b066-88f7af9ca3af
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=78703d3c-b907-432f-b066-88f7af9ca3af;Ip=[195.60.68.100];Helo=[mail.axis.com]
X-MS-Exchange-CrossTenant-AuthSource:
	AMS1EPF00000045.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB9PR02MB7035

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