[PATCH v4] power: supply: sbs-battery: don't assume i2c errors as battery disconnect

[PATCH v4] power: supply: sbs-battery: don't assume i2c errors as battery disconnect

[Ikjoon Jang]

Current sbs-battery considers all smbus errors as disconnection events
when battery-detect pin isn't supplied, and restored to present state back
when any successful transaction is made.

This can lead to unwanted state changes between present and !present
when there's one i2c error and other following commands were successful.

This patch provides a unified way of checking presence by calling
sbs_get_battery_presence_and_health() when detect pin is not used.

Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
---
v4: rebase from merge conflict, amend commit messages
v3: check return value of get_presence_and_health()
v2: combine get_presence_and_health functions to reuse
---

 drivers/power/supply/sbs-battery.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
index 6273211cd673..dacc4bc1c013 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -959,10 +959,17 @@ static int sbs_get_property(struct power_supply *psy,
 		return -EINVAL;
 	}
 
-	if (!chip->gpio_detect &&
-		chip->is_present != (ret >= 0)) {
-		sbs_update_presence(chip, (ret >= 0));
-		power_supply_changed(chip->power_supply);
+	if (!chip->gpio_detect && chip->is_present != (ret >= 0)) {
+		bool old_present = chip->is_present;
+		union power_supply_propval val;
+
+		ret = sbs_get_battery_presence_and_health(
+				client, POWER_SUPPLY_PROP_PRESENT, &val);
+
+		sbs_update_presence(chip, !ret && val.intval);
+
+		if (old_present != chip->is_present)
+			power_supply_changed(chip->power_supply);
 	}
 
 done:
@@ -1147,11 +1154,13 @@ static int sbs_probe(struct i2c_client *client)
 	 * to the battery.
 	 */
 	if (!(force_load || chip->gpio_detect)) {
-		rc = sbs_read_word_data(client, sbs_data[REG_STATUS].addr);
+		union power_supply_propval val;
 
-		if (rc < 0) {
-			dev_err(&client->dev, "%s: Failed to get device status\n",
-				__func__);
+		rc = sbs_get_battery_presence_and_health(
+				client, POWER_SUPPLY_PROP_PRESENT, &val);
+		if (rc < 0 || !val.intval) {
+			dev_err(&client->dev, "Failed to get present status\n");
+			rc = -ENODEV;
 			goto exit_psupply;
 		}
 	}
-- 
2.28.0.402.g5ffc5be6b7-goog

Re: [PATCH v4] power: supply: sbs-battery: don't assume i2c errors as battery disconnect

[Sebastian Reichel]

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

Hi,

On Fri, Aug 28, 2020 at 12:36:26PM +0800, Ikjoon Jang wrote:
> Current sbs-battery considers all smbus errors as disconnection events
> when battery-detect pin isn't supplied, and restored to present state back
> when any successful transaction is made.
> 
> This can lead to unwanted state changes between present and !present
> when there's one i2c error and other following commands were successful.
> 
> This patch provides a unified way of checking presence by calling
> sbs_get_battery_presence_and_health() when detect pin is not used.
> 
> Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
> ---
> v4: rebase from merge conflict, amend commit messages
> v3: check return value of get_presence_and_health()
> v2: combine get_presence_and_health functions to reuse
> ---

Thanks, queued.

-- Sebastian

> 
>  drivers/power/supply/sbs-battery.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
> index 6273211cd673..dacc4bc1c013 100644
> --- a/drivers/power/supply/sbs-battery.c
> +++ b/drivers/power/supply/sbs-battery.c
> @@ -959,10 +959,17 @@ static int sbs_get_property(struct power_supply *psy,
>  		return -EINVAL;
>  	}
>  
> -	if (!chip->gpio_detect &&
> -		chip->is_present != (ret >= 0)) {
> -		sbs_update_presence(chip, (ret >= 0));
> -		power_supply_changed(chip->power_supply);
> +	if (!chip->gpio_detect && chip->is_present != (ret >= 0)) {
> +		bool old_present = chip->is_present;
> +		union power_supply_propval val;
> +
> +		ret = sbs_get_battery_presence_and_health(
> +				client, POWER_SUPPLY_PROP_PRESENT, &val);
> +
> +		sbs_update_presence(chip, !ret && val.intval);
> +
> +		if (old_present != chip->is_present)
> +			power_supply_changed(chip->power_supply);
>  	}
>  
>  done:
> @@ -1147,11 +1154,13 @@ static int sbs_probe(struct i2c_client *client)
>  	 * to the battery.
>  	 */
>  	if (!(force_load || chip->gpio_detect)) {
> -		rc = sbs_read_word_data(client, sbs_data[REG_STATUS].addr);
> +		union power_supply_propval val;
>  
> -		if (rc < 0) {
> -			dev_err(&client->dev, "%s: Failed to get device status\n",
> -				__func__);
> +		rc = sbs_get_battery_presence_and_health(
> +				client, POWER_SUPPLY_PROP_PRESENT, &val);
> +		if (rc < 0 || !val.intval) {
> +			dev_err(&client->dev, "Failed to get present status\n");
> +			rc = -ENODEV;
>  			goto exit_psupply;
>  		}
>  	}
> -- 
> 2.28.0.402.g5ffc5be6b7-goog
> 

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

[PATCH] power: supply: sbs-battery: keep error code when get_property() fails

[Ikjoon Jang]

Commit c4f382930145 (power: supply: sbs-battery: don't assume
i2c errors as battery disconnect) overwrites the original error code
returned from internal functions. On such a sporadic i2c error,
a user will get a wrong value without errors.

Fixes: c4f382930145 (power: supply: sbs-battery: don't assume i2c errors as battery disconnect)

Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
---
Sorry, I missed an case with present state unchanged.
Sebastian, if you're okay with this patch,
I think this could be squashed into original commit c4f382930145 in your
branch.
---
 drivers/power/supply/sbs-battery.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
index dacc4bc1c013..13192cbcce71 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -962,11 +962,10 @@ static int sbs_get_property(struct power_supply *psy,
 	if (!chip->gpio_detect && chip->is_present != (ret >= 0)) {
 		bool old_present = chip->is_present;
 		union power_supply_propval val;
-
-		ret = sbs_get_battery_presence_and_health(
+		int err = sbs_get_battery_presence_and_health(
 				client, POWER_SUPPLY_PROP_PRESENT, &val);
 
-		sbs_update_presence(chip, !ret && val.intval);
+		sbs_update_presence(chip, !err && val.intval);
 
 		if (old_present != chip->is_present)
 			power_supply_changed(chip->power_supply);
@@ -976,19 +975,14 @@ static int sbs_get_property(struct power_supply *psy,
 	if (!ret) {
 		/* Convert units to match requirements for power supply class */
 		sbs_unit_adjustment(client, psp, val);
+		dev_dbg(&client->dev,
+			"%s: property = %d, value = %x\n", __func__,
+			psp, val->intval);
+	} else if (!chip->is_present)  {
+		/* battery not present, so return NODATA for properties */
+		ret = -ENODATA;
 	}
-
-	dev_dbg(&client->dev,
-		"%s: property = %d, value = %x\n", __func__, psp, val->intval);
-
-	if (ret && chip->is_present)
-		return ret;
-
-	/* battery not present, so return NODATA for properties */
-	if (ret)
-		return -ENODATA;
-
-	return 0;
+	return ret;
 }
 
 static void sbs_supply_changed(struct sbs_info *chip)
-- 
2.28.0.402.g5ffc5be6b7-goog