All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] power: supply: bq27xxx: Introduce parameter to config cache regs
@ 2024-02-19 10:05 Hermes Zhang
  2024-02-21 23:03 ` Sebastian Reichel
  0 siblings, 1 reply; 4+ messages in thread
From: Hermes Zhang @ 2024-02-19 10:05 UTC (permalink / raw)
  To: Sebastian Reichel
  Cc: kernel, Hermes Zhang, Pali Rohár, linux-pm, linux-kernel

Since all of the regs in the bq27xxx_reg_cache are now cached, a simple
property read (such as temperature) will need nine I2C transmissions.
Introduce a new module parameter to enable the reg cache to be configured,
which decrease the amount of unnecessary I2C transmission and preventing
the error -16 (EBUSY) happen when working on an I2C bus that is shared by
many devices.

Signed-off-by: Hermes Zhang <Hermes.Zhang@axis.com>
---
 drivers/power/supply/bq27xxx_battery.c | 65 +++++++++++++++++++-------
 include/linux/power/bq27xxx_battery.h  |  9 ++++
 2 files changed, 58 insertions(+), 16 deletions(-)

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 1c4a9d137744..45fd956ec961 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -1100,6 +1100,11 @@ module_param_cb(poll_interval, &param_ops_poll_interval, &poll_interval, 0644);
 MODULE_PARM_DESC(poll_interval,
 		 "battery poll interval in seconds - 0 disables polling");
 
+static unsigned int bq27xxx_cache_mask = 0xFF;
+module_param(bq27xxx_cache_mask, uint, 0644);
+MODULE_PARM_DESC(bq27xxx_cache_mask,
+		 "mask for bq27xxx reg cache - 0 disables reg cache");
+
 /*
  * Common code for BQ27xxx devices
  */
@@ -1842,21 +1847,29 @@ static void bq27xxx_battery_update_unlocked(struct bq27xxx_device_info *di)
 	if ((cache.flags & 0xff) == 0xff)
 		cache.flags = -1; /* read error */
 	if (cache.flags >= 0) {
-		cache.temperature = bq27xxx_battery_read_temperature(di);
-		if (di->regs[BQ27XXX_REG_TTE] != INVALID_REG_ADDR)
+		if (bq27xxx_cache_mask & BQ27XXX_CACHE_TEMP)
+			cache.temperature = bq27xxx_battery_read_temperature(di);
+		if (di->regs[BQ27XXX_REG_TTE] != INVALID_REG_ADDR &&
+			bq27xxx_cache_mask & BQ27XXX_CACHE_TTE)
 			cache.time_to_empty = bq27xxx_battery_read_time(di, BQ27XXX_REG_TTE);
-		if (di->regs[BQ27XXX_REG_TTECP] != INVALID_REG_ADDR)
+		if (di->regs[BQ27XXX_REG_TTECP] != INVALID_REG_ADDR &&
+			bq27xxx_cache_mask & BQ27XXX_CACHE_TTECP)
 			cache.time_to_empty_avg = bq27xxx_battery_read_time(di, BQ27XXX_REG_TTECP);
-		if (di->regs[BQ27XXX_REG_TTF] != INVALID_REG_ADDR)
+		if (di->regs[BQ27XXX_REG_TTF] != INVALID_REG_ADDR &&
+			bq27xxx_cache_mask & BQ27XXX_CACHE_TTF)
 			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)
+		if (bq27xxx_cache_mask & BQ27XXX_CACHE_CHARGE_FULL)
+			cache.charge_full = bq27xxx_battery_read_fcc(di);
+		if (bq27xxx_cache_mask & BQ27XXX_CACHE_CAPACITY)
+			cache.capacity = bq27xxx_battery_read_soc(di);
+		if (di->regs[BQ27XXX_REG_AE] != INVALID_REG_ADDR &&
+			bq27xxx_cache_mask & BQ27XXX_CACHE_ENERGY)
 			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)
+		if (di->regs[BQ27XXX_REG_CYCT] != INVALID_REG_ADDR &&
+			bq27xxx_cache_mask & BQ27XXX_CACHE_CYCT)
 			cache.cycle_count = bq27xxx_battery_read_cyct(di);
 
 		/*
@@ -2004,6 +2017,7 @@ static int bq27xxx_battery_get_property(struct power_supply *psy,
 {
 	int ret = 0;
 	struct bq27xxx_device_info *di = power_supply_get_drvdata(psy);
+	int tmp;
 
 	mutex_lock(&di->lock);
 	if (time_is_before_jiffies(di->last_update + 5 * HZ))
@@ -2027,24 +2041,37 @@ static int bq27xxx_battery_get_property(struct power_supply *psy,
 		ret = bq27xxx_battery_current_and_status(di, val, NULL, NULL);
 		break;
 	case POWER_SUPPLY_PROP_CAPACITY:
-		ret = bq27xxx_simple_value(di->cache.capacity, val);
+		tmp = bq27xxx_cache_mask & BQ27XXX_CACHE_CAPACITY ?
+				di->cache.capacity : bq27xxx_battery_read_soc(di);
+		ret = bq27xxx_simple_value(tmp, 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);
+		tmp = bq27xxx_cache_mask & BQ27XXX_CACHE_TEMP ?
+				di->cache.temperature : bq27xxx_battery_read_temperature(di);
+		ret = bq27xxx_simple_value(tmp, 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);
+		tmp = bq27xxx_cache_mask & BQ27XXX_CACHE_TTE ?
+				di->cache.time_to_empty :
+				bq27xxx_battery_read_time(di, BQ27XXX_REG_TTE);
+		ret = bq27xxx_simple_value(tmp, val);
 		break;
 	case POWER_SUPPLY_PROP_TIME_TO_EMPTY_AVG:
-		ret = bq27xxx_simple_value(di->cache.time_to_empty_avg, val);
+		tmp = bq27xxx_cache_mask & BQ27XXX_CACHE_TTECP ?
+				di->cache.time_to_empty_avg :
+				bq27xxx_battery_read_time(di, BQ27XXX_REG_TTECP);
+		ret = bq27xxx_simple_value(tmp, val);
 		break;
 	case POWER_SUPPLY_PROP_TIME_TO_FULL_NOW:
-		ret = bq27xxx_simple_value(di->cache.time_to_full, val);
+		tmp = bq27xxx_cache_mask & BQ27XXX_CACHE_TTF ?
+				di->cache.time_to_full :
+				bq27xxx_battery_read_time(di, BQ27XXX_REG_TTF);
+		ret = bq27xxx_simple_value(tmp, val);
 		break;
 	case POWER_SUPPLY_PROP_TECHNOLOGY:
 		if (di->opts & BQ27XXX_O_MUL_CHEM)
@@ -2059,7 +2086,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);
+		tmp = bq27xxx_cache_mask & BQ27XXX_CACHE_CHARGE_FULL ?
+				di->cache.charge_full : bq27xxx_battery_read_fcc(di);
+		ret = bq27xxx_simple_value(tmp, val);
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
 		ret = bq27xxx_simple_value(di->charge_design_full, val);
@@ -2072,10 +2101,14 @@ 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);
+		tmp = bq27xxx_cache_mask & BQ27XXX_CACHE_CYCT ?
+				di->cache.cycle_count : bq27xxx_battery_read_cyct(di);
+		ret = bq27xxx_simple_value(tmp, val);
 		break;
 	case POWER_SUPPLY_PROP_ENERGY_NOW:
-		ret = bq27xxx_simple_value(di->cache.energy, val);
+		tmp = bq27xxx_cache_mask & BQ27XXX_CACHE_ENERGY ?
+				di->cache.energy : bq27xxx_battery_read_energy(di);
+		ret = bq27xxx_simple_value(tmp, val);
 		break;
 	case POWER_SUPPLY_PROP_POWER_AVG:
 		ret = bq27xxx_battery_pwr_avg(di, val);
diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
index 7d8025fb74b7..29d1e7107ee2 100644
--- a/include/linux/power/bq27xxx_battery.h
+++ b/include/linux/power/bq27xxx_battery.h
@@ -4,6 +4,15 @@
 
 #include <linux/power_supply.h>
 
+#define BQ27XXX_CACHE_TEMP        (1 << 0)
+#define BQ27XXX_CACHE_TTE         (1 << 1)
+#define BQ27XXX_CACHE_TTECP       (1 << 2)
+#define BQ27XXX_CACHE_TTF         (1 << 3)
+#define BQ27XXX_CACHE_CHARGE_FULL (1 << 4)
+#define BQ27XXX_CACHE_CYCT        (1 << 5)
+#define BQ27XXX_CACHE_CAPACITY    (1 << 6)
+#define BQ27XXX_CACHE_ENERGY      (1 << 7)
+
 enum bq27xxx_chip {
 	BQ27000 = 1, /* bq27000, bq27200 */
 	BQ27010, /* bq27010, bq27210 */
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] power: supply: bq27xxx: Introduce parameter to config cache regs
  2024-02-19 10:05 [PATCH] power: supply: bq27xxx: Introduce parameter to config cache regs Hermes Zhang
@ 2024-02-21 23:03 ` Sebastian Reichel
  2024-02-23  8:40   ` Hermes Zhang
  0 siblings, 1 reply; 4+ messages in thread
From: Sebastian Reichel @ 2024-02-21 23:03 UTC (permalink / raw)
  To: Hermes Zhang; +Cc: kernel, Pali Rohár, linux-pm, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 8078 bytes --]

Hi,

On Mon, Feb 19, 2024 at 06:05:40PM +0800, Hermes Zhang wrote:
> Since all of the regs in the bq27xxx_reg_cache are now cached, a simple
> property read (such as temperature) will need nine I2C transmissions.
> Introduce a new module parameter to enable the reg cache to be configured,
> which decrease the amount of unnecessary I2C transmission and preventing
> the error -16 (EBUSY) happen when working on an I2C bus that is shared by
> many devices.

So the problem is not the caching, but the grouping. So instead
of adding this hack, please change the code to do the caching
per register. That way you can just keep the caching enabled and
don't need any custom module parameters.

-- Sebastian

> Signed-off-by: Hermes Zhang <Hermes.Zhang@axis.com>
> ---
>  drivers/power/supply/bq27xxx_battery.c | 65 +++++++++++++++++++-------
>  include/linux/power/bq27xxx_battery.h  |  9 ++++
>  2 files changed, 58 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> index 1c4a9d137744..45fd956ec961 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -1100,6 +1100,11 @@ module_param_cb(poll_interval, &param_ops_poll_interval, &poll_interval, 0644);
>  MODULE_PARM_DESC(poll_interval,
>  		 "battery poll interval in seconds - 0 disables polling");
>  
> +static unsigned int bq27xxx_cache_mask = 0xFF;
> +module_param(bq27xxx_cache_mask, uint, 0644);
> +MODULE_PARM_DESC(bq27xxx_cache_mask,
> +		 "mask for bq27xxx reg cache - 0 disables reg cache");
> +
>  /*
>   * Common code for BQ27xxx devices
>   */
> @@ -1842,21 +1847,29 @@ static void bq27xxx_battery_update_unlocked(struct bq27xxx_device_info *di)
>  	if ((cache.flags & 0xff) == 0xff)
>  		cache.flags = -1; /* read error */
>  	if (cache.flags >= 0) {
> -		cache.temperature = bq27xxx_battery_read_temperature(di);
> -		if (di->regs[BQ27XXX_REG_TTE] != INVALID_REG_ADDR)
> +		if (bq27xxx_cache_mask & BQ27XXX_CACHE_TEMP)
> +			cache.temperature = bq27xxx_battery_read_temperature(di);
> +		if (di->regs[BQ27XXX_REG_TTE] != INVALID_REG_ADDR &&
> +			bq27xxx_cache_mask & BQ27XXX_CACHE_TTE)
>  			cache.time_to_empty = bq27xxx_battery_read_time(di, BQ27XXX_REG_TTE);
> -		if (di->regs[BQ27XXX_REG_TTECP] != INVALID_REG_ADDR)
> +		if (di->regs[BQ27XXX_REG_TTECP] != INVALID_REG_ADDR &&
> +			bq27xxx_cache_mask & BQ27XXX_CACHE_TTECP)
>  			cache.time_to_empty_avg = bq27xxx_battery_read_time(di, BQ27XXX_REG_TTECP);
> -		if (di->regs[BQ27XXX_REG_TTF] != INVALID_REG_ADDR)
> +		if (di->regs[BQ27XXX_REG_TTF] != INVALID_REG_ADDR &&
> +			bq27xxx_cache_mask & BQ27XXX_CACHE_TTF)
>  			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)
> +		if (bq27xxx_cache_mask & BQ27XXX_CACHE_CHARGE_FULL)
> +			cache.charge_full = bq27xxx_battery_read_fcc(di);
> +		if (bq27xxx_cache_mask & BQ27XXX_CACHE_CAPACITY)
> +			cache.capacity = bq27xxx_battery_read_soc(di);
> +		if (di->regs[BQ27XXX_REG_AE] != INVALID_REG_ADDR &&
> +			bq27xxx_cache_mask & BQ27XXX_CACHE_ENERGY)
>  			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)
> +		if (di->regs[BQ27XXX_REG_CYCT] != INVALID_REG_ADDR &&
> +			bq27xxx_cache_mask & BQ27XXX_CACHE_CYCT)
>  			cache.cycle_count = bq27xxx_battery_read_cyct(di);
>  
>  		/*
> @@ -2004,6 +2017,7 @@ static int bq27xxx_battery_get_property(struct power_supply *psy,
>  {
>  	int ret = 0;
>  	struct bq27xxx_device_info *di = power_supply_get_drvdata(psy);
> +	int tmp;
>  
>  	mutex_lock(&di->lock);
>  	if (time_is_before_jiffies(di->last_update + 5 * HZ))
> @@ -2027,24 +2041,37 @@ static int bq27xxx_battery_get_property(struct power_supply *psy,
>  		ret = bq27xxx_battery_current_and_status(di, val, NULL, NULL);
>  		break;
>  	case POWER_SUPPLY_PROP_CAPACITY:
> -		ret = bq27xxx_simple_value(di->cache.capacity, val);
> +		tmp = bq27xxx_cache_mask & BQ27XXX_CACHE_CAPACITY ?
> +				di->cache.capacity : bq27xxx_battery_read_soc(di);
> +		ret = bq27xxx_simple_value(tmp, 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);
> +		tmp = bq27xxx_cache_mask & BQ27XXX_CACHE_TEMP ?
> +				di->cache.temperature : bq27xxx_battery_read_temperature(di);
> +		ret = bq27xxx_simple_value(tmp, 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);
> +		tmp = bq27xxx_cache_mask & BQ27XXX_CACHE_TTE ?
> +				di->cache.time_to_empty :
> +				bq27xxx_battery_read_time(di, BQ27XXX_REG_TTE);
> +		ret = bq27xxx_simple_value(tmp, val);
>  		break;
>  	case POWER_SUPPLY_PROP_TIME_TO_EMPTY_AVG:
> -		ret = bq27xxx_simple_value(di->cache.time_to_empty_avg, val);
> +		tmp = bq27xxx_cache_mask & BQ27XXX_CACHE_TTECP ?
> +				di->cache.time_to_empty_avg :
> +				bq27xxx_battery_read_time(di, BQ27XXX_REG_TTECP);
> +		ret = bq27xxx_simple_value(tmp, val);
>  		break;
>  	case POWER_SUPPLY_PROP_TIME_TO_FULL_NOW:
> -		ret = bq27xxx_simple_value(di->cache.time_to_full, val);
> +		tmp = bq27xxx_cache_mask & BQ27XXX_CACHE_TTF ?
> +				di->cache.time_to_full :
> +				bq27xxx_battery_read_time(di, BQ27XXX_REG_TTF);
> +		ret = bq27xxx_simple_value(tmp, val);
>  		break;
>  	case POWER_SUPPLY_PROP_TECHNOLOGY:
>  		if (di->opts & BQ27XXX_O_MUL_CHEM)
> @@ -2059,7 +2086,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);
> +		tmp = bq27xxx_cache_mask & BQ27XXX_CACHE_CHARGE_FULL ?
> +				di->cache.charge_full : bq27xxx_battery_read_fcc(di);
> +		ret = bq27xxx_simple_value(tmp, val);
>  		break;
>  	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
>  		ret = bq27xxx_simple_value(di->charge_design_full, val);
> @@ -2072,10 +2101,14 @@ 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);
> +		tmp = bq27xxx_cache_mask & BQ27XXX_CACHE_CYCT ?
> +				di->cache.cycle_count : bq27xxx_battery_read_cyct(di);
> +		ret = bq27xxx_simple_value(tmp, val);
>  		break;
>  	case POWER_SUPPLY_PROP_ENERGY_NOW:
> -		ret = bq27xxx_simple_value(di->cache.energy, val);
> +		tmp = bq27xxx_cache_mask & BQ27XXX_CACHE_ENERGY ?
> +				di->cache.energy : bq27xxx_battery_read_energy(di);
> +		ret = bq27xxx_simple_value(tmp, val);
>  		break;
>  	case POWER_SUPPLY_PROP_POWER_AVG:
>  		ret = bq27xxx_battery_pwr_avg(di, val);
> diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
> index 7d8025fb74b7..29d1e7107ee2 100644
> --- a/include/linux/power/bq27xxx_battery.h
> +++ b/include/linux/power/bq27xxx_battery.h
> @@ -4,6 +4,15 @@
>  
>  #include <linux/power_supply.h>
>  
> +#define BQ27XXX_CACHE_TEMP        (1 << 0)
> +#define BQ27XXX_CACHE_TTE         (1 << 1)
> +#define BQ27XXX_CACHE_TTECP       (1 << 2)
> +#define BQ27XXX_CACHE_TTF         (1 << 3)
> +#define BQ27XXX_CACHE_CHARGE_FULL (1 << 4)
> +#define BQ27XXX_CACHE_CYCT        (1 << 5)
> +#define BQ27XXX_CACHE_CAPACITY    (1 << 6)
> +#define BQ27XXX_CACHE_ENERGY      (1 << 7)
> +
>  enum bq27xxx_chip {
>  	BQ27000 = 1, /* bq27000, bq27200 */
>  	BQ27010, /* bq27010, bq27210 */
> -- 
> 2.39.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] power: supply: bq27xxx: Introduce parameter to config cache regs
  2024-02-21 23:03 ` Sebastian Reichel
@ 2024-02-23  8:40   ` Hermes Zhang
  2024-02-25 21:10     ` Sebastian Reichel
  0 siblings, 1 reply; 4+ messages in thread
From: Hermes Zhang @ 2024-02-23  8:40 UTC (permalink / raw)
  To: Sebastian Reichel, Hermes Zhang
  Cc: kernel, Pali Rohár, linux-pm, linux-kernel

Hi,

On 2024/2/22 7:03, Sebastian Reichel wrote:
> Hi,
>
> On Mon, Feb 19, 2024 at 06:05:40PM +0800, Hermes Zhang wrote:
>> Since all of the regs in the bq27xxx_reg_cache are now cached, a simple
>> property read (such as temperature) will need nine I2C transmissions.
>> Introduce a new module parameter to enable the reg cache to be configured,
>> which decrease the amount of unnecessary I2C transmission and preventing
>> the error -16 (EBUSY) happen when working on an I2C bus that is shared by
>> many devices.
> So the problem is not the caching, but the grouping. So instead
> of adding this hack, please change the code to do the caching
> per register. That way you can just keep the caching enabled and
> don't need any custom module parameters.

Thanks for the reply. Yes, the key is the grouping. So do you suggest to 
drop the bq27xxx_reg_cache struct totally and handle the cache for each 
register in e.g. bq27xxx_battery_get_property()? Then it will require an 
extra time info for each register, will that be a big cost? Or am I 
misunderstanding?

Best Regards,

Hermes


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] power: supply: bq27xxx: Introduce parameter to config cache regs
  2024-02-23  8:40   ` Hermes Zhang
@ 2024-02-25 21:10     ` Sebastian Reichel
  0 siblings, 0 replies; 4+ messages in thread
From: Sebastian Reichel @ 2024-02-25 21:10 UTC (permalink / raw)
  To: Hermes Zhang
  Cc: Hermes Zhang, kernel, Pali Rohár, linux-pm, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1394 bytes --]

Hi,

On Fri, Feb 23, 2024 at 04:40:18PM +0800, Hermes Zhang wrote:
> On 2024/2/22 7:03, Sebastian Reichel wrote:
> > On Mon, Feb 19, 2024 at 06:05:40PM +0800, Hermes Zhang wrote:
> > > Since all of the regs in the bq27xxx_reg_cache are now cached, a simple
> > > property read (such as temperature) will need nine I2C transmissions.
> > > Introduce a new module parameter to enable the reg cache to be configured,
> > > which decrease the amount of unnecessary I2C transmission and preventing
> > > the error -16 (EBUSY) happen when working on an I2C bus that is shared by
> > > many devices.
> > So the problem is not the caching, but the grouping. So instead
> > of adding this hack, please change the code to do the caching
> > per register. That way you can just keep the caching enabled and
> > don't need any custom module parameters.
> 
> Thanks for the reply. Yes, the key is the grouping. So do you suggest to
> drop the bq27xxx_reg_cache struct totally and handle the cache for each
> register in e.g. bq27xxx_battery_get_property()? Then it will require an
> extra time info for each register, will that be a big cost? Or am I
> misunderstanding?

Yes, this requires time info for each cached register. I don't think
the added memory is a big deal. There usually is only a single
battery and we are caching 10 timestamps. So that's 80 bytes.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2024-02-25 21:10 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-19 10:05 [PATCH] power: supply: bq27xxx: Introduce parameter to config cache regs Hermes Zhang
2024-02-21 23:03 ` Sebastian Reichel
2024-02-23  8:40   ` Hermes Zhang
2024-02-25 21:10     ` Sebastian Reichel

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.