When gpio detection is not supplied, presence state transitions depend
on every smbus transfer result from get_property(). This patch tries to
check battery presence again with well supported command before
state transition.
Changes:
v3: check return value of get_presence_and_health()
v2: combine get_presence_and_health functions to reuse
Ikjoon Jang (2):
power: supply: sbs-battery: combine get_presence_and_health
power: supply: sbs-battery: don't assume i2c errors as battery
disconnect
drivers/power/supply/sbs-battery.c | 98 ++++++++++++++++--------------
1 file changed, 53 insertions(+), 45 deletions(-)
--
2.28.0.236.gb10cc79966-goog
This patch enables calling sbs_get_battery_presence_and_health()
without checking its chip type. No functional changes.
Signed-off-by: Ikjoon Jang <[email protected]>
---
drivers/power/supply/sbs-battery.c | 73 +++++++++++++++---------------
1 file changed, 36 insertions(+), 37 deletions(-)
diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
index 83b9924033bd..6acb4ea25d2a 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -389,37 +389,6 @@ static bool sbs_bat_needs_calibration(struct i2c_client *client)
return !!(ret & BIT(7));
}
-static int sbs_get_battery_presence_and_health(
- struct i2c_client *client, enum power_supply_property psp,
- union power_supply_propval *val)
-{
- int ret;
-
- /* Dummy command; if it succeeds, battery is present. */
- ret = sbs_read_word_data(client, sbs_data[REG_STATUS].addr);
-
- if (ret < 0) { /* battery not present*/
- if (psp == POWER_SUPPLY_PROP_PRESENT) {
- val->intval = 0;
- return 0;
- }
- return ret;
- }
-
- if (psp == POWER_SUPPLY_PROP_PRESENT)
- val->intval = 1; /* battery present */
- else { /* POWER_SUPPLY_PROP_HEALTH */
- if (sbs_bat_needs_calibration(client)) {
- val->intval = POWER_SUPPLY_HEALTH_CALIBRATION_REQUIRED;
- } else {
- /* SBS spec doesn't have a general health command. */
- val->intval = POWER_SUPPLY_HEALTH_UNKNOWN;
- }
- }
-
- return 0;
-}
-
static int sbs_get_ti_battery_presence_and_health(
struct i2c_client *client, enum power_supply_property psp,
union power_supply_propval *val)
@@ -478,6 +447,41 @@ static int sbs_get_ti_battery_presence_and_health(
return 0;
}
+static int sbs_get_battery_presence_and_health(
+ struct i2c_client *client, enum power_supply_property psp,
+ union power_supply_propval *val)
+{
+ struct sbs_info *chip = i2c_get_clientdata(client);
+ int ret;
+
+ if (chip->flags & SBS_FLAGS_TI_BQ20ZX5)
+ return sbs_get_ti_battery_presence_and_health(client, psp, val);
+
+ /* Dummy command; if it succeeds, battery is present. */
+ ret = sbs_read_word_data(client, sbs_data[REG_STATUS].addr);
+
+ if (ret < 0) { /* battery not present*/
+ if (psp == POWER_SUPPLY_PROP_PRESENT) {
+ val->intval = 0;
+ return 0;
+ }
+ return ret;
+ }
+
+ if (psp == POWER_SUPPLY_PROP_PRESENT)
+ val->intval = 1; /* battery present */
+ else { /* POWER_SUPPLY_PROP_HEALTH */
+ if (sbs_bat_needs_calibration(client)) {
+ val->intval = POWER_SUPPLY_HEALTH_CALIBRATION_REQUIRED;
+ } else {
+ /* SBS spec doesn't have a general health command. */
+ val->intval = POWER_SUPPLY_HEALTH_UNKNOWN;
+ }
+ }
+
+ return 0;
+}
+
static int sbs_get_battery_property(struct i2c_client *client,
int reg_offset, enum power_supply_property psp,
union power_supply_propval *val)
@@ -780,12 +784,7 @@ static int sbs_get_property(struct power_supply *psy,
switch (psp) {
case POWER_SUPPLY_PROP_PRESENT:
case POWER_SUPPLY_PROP_HEALTH:
- if (chip->flags & SBS_FLAGS_TI_BQ20ZX5)
- ret = sbs_get_ti_battery_presence_and_health(client,
- psp, val);
- else
- ret = sbs_get_battery_presence_and_health(client, psp,
- val);
+ ret = sbs_get_battery_presence_and_health(client, psp, val);
/* this can only be true if no gpio is used */
if (psp == POWER_SUPPLY_PROP_PRESENT)
--
2.28.0.236.gb10cc79966-goog
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 unlimited state changes between present and !present
when one unsupported command was requested and other following commands
were successful, e.g. udev rules tries to read multiple properties.
This patch checks battery presence by reading known good command to
check battery existence.
Signed-off-by: Ikjoon Jang <[email protected]>
---
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 6acb4ea25d2a..2e32fde04628 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -878,10 +878,17 @@ static int sbs_get_property(struct power_supply *psy,
if (!chip->enable_detection)
goto done;
- 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:
@@ -1067,11 +1074,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.236.gb10cc79966-goog
On Thu, Aug 13, 2020 at 1:11 PM Ikjoon Jang <[email protected]> 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 unlimited state changes between present and !present
> when one unsupported command was requested and other following commands
> were successful, e.g. udev rules tries to read multiple properties.
>
> This patch checks battery presence by reading known good command to
> check battery existence.
>
> Signed-off-by: Ikjoon Jang <[email protected]>
Looks good now, AFAICT. Thanks!
Reviewed-by: Nicolas Boichat <[email protected]>
> ---
> 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 6acb4ea25d2a..2e32fde04628 100644
> --- a/drivers/power/supply/sbs-battery.c
> +++ b/drivers/power/supply/sbs-battery.c
> @@ -878,10 +878,17 @@ static int sbs_get_property(struct power_supply *psy,
> if (!chip->enable_detection)
> goto done;
>
> - 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:
> @@ -1067,11 +1074,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.236.gb10cc79966-goog
>
Hi,
On Thu, Aug 13, 2020 at 01:10:07PM +0800, Ikjoon Jang wrote:
> This patch enables calling sbs_get_battery_presence_and_health()
> without checking its chip type. No functional changes.
>
> Signed-off-by: Ikjoon Jang <[email protected]>
> ---
Thanks, queued.
-- Sebastian
> drivers/power/supply/sbs-battery.c | 73 +++++++++++++++---------------
> 1 file changed, 36 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
> index 83b9924033bd..6acb4ea25d2a 100644
> --- a/drivers/power/supply/sbs-battery.c
> +++ b/drivers/power/supply/sbs-battery.c
> @@ -389,37 +389,6 @@ static bool sbs_bat_needs_calibration(struct i2c_client *client)
> return !!(ret & BIT(7));
> }
>
> -static int sbs_get_battery_presence_and_health(
> - struct i2c_client *client, enum power_supply_property psp,
> - union power_supply_propval *val)
> -{
> - int ret;
> -
> - /* Dummy command; if it succeeds, battery is present. */
> - ret = sbs_read_word_data(client, sbs_data[REG_STATUS].addr);
> -
> - if (ret < 0) { /* battery not present*/
> - if (psp == POWER_SUPPLY_PROP_PRESENT) {
> - val->intval = 0;
> - return 0;
> - }
> - return ret;
> - }
> -
> - if (psp == POWER_SUPPLY_PROP_PRESENT)
> - val->intval = 1; /* battery present */
> - else { /* POWER_SUPPLY_PROP_HEALTH */
> - if (sbs_bat_needs_calibration(client)) {
> - val->intval = POWER_SUPPLY_HEALTH_CALIBRATION_REQUIRED;
> - } else {
> - /* SBS spec doesn't have a general health command. */
> - val->intval = POWER_SUPPLY_HEALTH_UNKNOWN;
> - }
> - }
> -
> - return 0;
> -}
> -
> static int sbs_get_ti_battery_presence_and_health(
> struct i2c_client *client, enum power_supply_property psp,
> union power_supply_propval *val)
> @@ -478,6 +447,41 @@ static int sbs_get_ti_battery_presence_and_health(
> return 0;
> }
>
> +static int sbs_get_battery_presence_and_health(
> + struct i2c_client *client, enum power_supply_property psp,
> + union power_supply_propval *val)
> +{
> + struct sbs_info *chip = i2c_get_clientdata(client);
> + int ret;
> +
> + if (chip->flags & SBS_FLAGS_TI_BQ20ZX5)
> + return sbs_get_ti_battery_presence_and_health(client, psp, val);
> +
> + /* Dummy command; if it succeeds, battery is present. */
> + ret = sbs_read_word_data(client, sbs_data[REG_STATUS].addr);
> +
> + if (ret < 0) { /* battery not present*/
> + if (psp == POWER_SUPPLY_PROP_PRESENT) {
> + val->intval = 0;
> + return 0;
> + }
> + return ret;
> + }
> +
> + if (psp == POWER_SUPPLY_PROP_PRESENT)
> + val->intval = 1; /* battery present */
> + else { /* POWER_SUPPLY_PROP_HEALTH */
> + if (sbs_bat_needs_calibration(client)) {
> + val->intval = POWER_SUPPLY_HEALTH_CALIBRATION_REQUIRED;
> + } else {
> + /* SBS spec doesn't have a general health command. */
> + val->intval = POWER_SUPPLY_HEALTH_UNKNOWN;
> + }
> + }
> +
> + return 0;
> +}
> +
> static int sbs_get_battery_property(struct i2c_client *client,
> int reg_offset, enum power_supply_property psp,
> union power_supply_propval *val)
> @@ -780,12 +784,7 @@ static int sbs_get_property(struct power_supply *psy,
> switch (psp) {
> case POWER_SUPPLY_PROP_PRESENT:
> case POWER_SUPPLY_PROP_HEALTH:
> - if (chip->flags & SBS_FLAGS_TI_BQ20ZX5)
> - ret = sbs_get_ti_battery_presence_and_health(client,
> - psp, val);
> - else
> - ret = sbs_get_battery_presence_and_health(client, psp,
> - val);
> + ret = sbs_get_battery_presence_and_health(client, psp, val);
>
> /* this can only be true if no gpio is used */
> if (psp == POWER_SUPPLY_PROP_PRESENT)
> --
> 2.28.0.236.gb10cc79966-goog
>
Hi,
On Thu, Aug 13, 2020 at 01:10:08PM +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 unlimited state changes between present and !present
> when one unsupported command was requested and other following commands
> were successful, e.g. udev rules tries to read multiple properties.
>
> This patch checks battery presence by reading known good command to
> check battery existence.
>
> Signed-off-by: Ikjoon Jang <[email protected]>
> ---
Does not apply, please rebase. Also we probably should add
another compatible value for your specific chip not supporting
some properties, so that they are not exposed?
-- 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 6acb4ea25d2a..2e32fde04628 100644
> --- a/drivers/power/supply/sbs-battery.c
> +++ b/drivers/power/supply/sbs-battery.c
> @@ -878,10 +878,17 @@ static int sbs_get_property(struct power_supply *psy,
> if (!chip->enable_detection)
> goto done;
>
> - 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:
> @@ -1067,11 +1074,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.236.gb10cc79966-goog
>