Multiple applications may access the battery gauge at the same time, so
the gauge may be busy and EBUSY will be returned. The driver will set a
flag to record the EBUSY state, and this flag will be kept until the next
periodic update. When this flag is set, bq27xxx_battery_get_property()
will just return ENODEV until the flag is updated.

Even if the gauge was busy during the last accessing attempt, returning
ENODEV is not ideal, and can cause confusion in the applications layer.

Instead, retry accessing the gauge to update the flag is as expected, for
the gauge typically recovers from busy state within a few milliseconds.
If still failed to access the gauge, the real error code would be returned
instead of ENODEV (as suggested by Pali Rohár).

Signed-off-by: Jerry Lv <Jerry.Lv@axis.com>
---
When the battery gauge is busy, retry to access 10 miliseconds later,
retry up to 3 times. When failed to access the gauge, return the real
error code.

Differences related to previous versions:
v2 (as suggested by Pali Rohár):
- retry up to 3 times when gauge is busy.
- return the real error code when fail to access the device.

v1:
- initial version for review.
---
 drivers/power/supply/bq27xxx_battery.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 750fda543308..9c40bbc292c1 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -1871,11 +1871,19 @@ static int bq27xxx_battery_current_and_status(
 
 static void bq27xxx_battery_update_unlocked(struct bq27xxx_device_info *di)
 {
+#define MAX_RETRY 3
+	int retry = 0, sleep = 10;
 	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);
+	do {
+		cache.flags = bq27xxx_read(di, BQ27XXX_REG_FLAGS, has_singe_flag);
+		if (cache.flags == -EBUSY && retry < MAX_RETRY) {
+			retry++;
+			BQ27XXX_MSLEEP(sleep);		/* sleep 10 miliseconds when busy */
+		}
+	} while (cache.flags == -EBUSY && retry < MAX_RETRY);
 	if ((cache.flags & 0xff) == 0xff)
 		cache.flags = -1; /* read error */
 	if (cache.flags >= 0) {
@@ -2030,7 +2038,7 @@ static int bq27xxx_battery_get_property(struct power_supply *psy,
 	mutex_unlock(&di->lock);
 
 	if (psp != POWER_SUPPLY_PROP_PRESENT && di->cache.flags < 0)
-		return -ENODEV;
+		return di->cache.flags;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_STATUS:

---
base-commit: 42f7652d3eb527d03665b09edac47f85fb600924
change-id: 20241008-foo-fix-b2244cbe6dce

Best regards,
-- 
Jerry Lv <Jerry.Lv@axis.com>

On Tuesday 29 October 2024 11:35:00 Jerry Lv wrote:
> Multiple applications may access the battery gauge at the same time, so
> the gauge may be busy and EBUSY will be returned. The driver will set a
> flag to record the EBUSY state, and this flag will be kept until the next
> periodic update. When this flag is set, bq27xxx_battery_get_property()
> will just return ENODEV until the flag is updated.
> 
> Even if the gauge was busy during the last accessing attempt, returning
> ENODEV is not ideal, and can cause confusion in the applications layer.
> 
> Instead, retry accessing the gauge to update the flag is as expected, for
> the gauge typically recovers from busy state within a few milliseconds.
> If still failed to access the gauge, the real error code would be returned
> instead of ENODEV (as suggested by Pali Rohár).
> 
> Signed-off-by: Jerry Lv <Jerry.Lv@axis.com>
> ---
> When the battery gauge is busy, retry to access 10 miliseconds later,
> retry up to 3 times. When failed to access the gauge, return the real
> error code.
> 
> Differences related to previous versions:
> v2 (as suggested by Pali Rohár):
> - retry up to 3 times when gauge is busy.
> - return the real error code when fail to access the device.
> 
> v1:
> - initial version for review.
> ---
>  drivers/power/supply/bq27xxx_battery.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> index 750fda543308..9c40bbc292c1 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -1871,11 +1871,19 @@ static int bq27xxx_battery_current_and_status(
>  
>  static void bq27xxx_battery_update_unlocked(struct bq27xxx_device_info *di)
>  {
> +#define MAX_RETRY 3
> +	int retry = 0, sleep = 10;
>  	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);
> +	do {
> +		cache.flags = bq27xxx_read(di, BQ27XXX_REG_FLAGS, has_singe_flag);
> +		if (cache.flags == -EBUSY && retry < MAX_RETRY) {
> +			retry++;
> +			BQ27XXX_MSLEEP(sleep);		/* sleep 10 miliseconds when busy */
> +		}
> +	} while (cache.flags == -EBUSY && retry < MAX_RETRY);

Hello, this is for sure nice improvement.

Anyway, I think that I mentioned it in previous email, this problem
which you describe does not affect only bq27xxx_battery_update_unlocked()
but also any other function which calls bq27xxx_read().

What about rather moving this -EBUSY retry logic into the bq27xxx_read()
function itself? Or even better, directly inside bq27xxx_battery_i2c_read()
function? This would fix this problem on all places.

>  	if ((cache.flags & 0xff) == 0xff)
>  		cache.flags = -1; /* read error */
>  	if (cache.flags >= 0) {
> @@ -2030,7 +2038,7 @@ static int bq27xxx_battery_get_property(struct power_supply *psy,
>  	mutex_unlock(&di->lock);
>  
>  	if (psp != POWER_SUPPLY_PROP_PRESENT && di->cache.flags < 0)
> -		return -ENODEV;
> +		return di->cache.flags;
>  
>  	switch (psp) {
>  	case POWER_SUPPLY_PROP_STATUS:
> 
> ---
> base-commit: 42f7652d3eb527d03665b09edac47f85fb600924
> change-id: 20241008-foo-fix-b2244cbe6dce
> 
> Best regards,
> -- 
> Jerry Lv <Jerry.Lv@axis.com>
>