* [PATCH v3 0/2] power: supply: sbs-battery: fix presence check
[not found] <0200811065307.2094930-1-ikjn@chromium.org>
@ 2020-08-13 5:10 ` Ikjoon Jang
2020-08-13 5:10 ` [PATCH v3 1/2] power: supply: sbs-battery: combine get_presence_and_health Ikjoon Jang
2020-08-13 5:10 ` [PATCH v3 2/2] power: supply: sbs-battery: don't assume i2c errors as battery disconnect Ikjoon Jang
0 siblings, 2 replies; 6+ messages in thread
From: Ikjoon Jang @ 2020-08-13 5:10 UTC (permalink / raw)
To: Sebastian Reichel, linux-pm; +Cc: linux-kernel, drinkcat, Ikjoon Jang
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 1/2] power: supply: sbs-battery: combine get_presence_and_health
2020-08-13 5:10 ` [PATCH v3 0/2] power: supply: sbs-battery: fix presence check Ikjoon Jang
@ 2020-08-13 5:10 ` Ikjoon Jang
2020-08-27 21:58 ` Sebastian Reichel
2020-08-13 5:10 ` [PATCH v3 2/2] power: supply: sbs-battery: don't assume i2c errors as battery disconnect Ikjoon Jang
1 sibling, 1 reply; 6+ messages in thread
From: Ikjoon Jang @ 2020-08-13 5:10 UTC (permalink / raw)
To: Sebastian Reichel, linux-pm; +Cc: linux-kernel, drinkcat, Ikjoon Jang
This patch enables calling sbs_get_battery_presence_and_health()
without checking its chip type. No functional changes.
Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v3 2/2] power: supply: sbs-battery: don't assume i2c errors as battery disconnect
2020-08-13 5:10 ` [PATCH v3 0/2] power: supply: sbs-battery: fix presence check Ikjoon Jang
2020-08-13 5:10 ` [PATCH v3 1/2] power: supply: sbs-battery: combine get_presence_and_health Ikjoon Jang
@ 2020-08-13 5:10 ` Ikjoon Jang
2020-08-13 5:26 ` Nicolas Boichat
2020-08-27 22:00 ` Sebastian Reichel
1 sibling, 2 replies; 6+ messages in thread
From: Ikjoon Jang @ 2020-08-13 5:10 UTC (permalink / raw)
To: Sebastian Reichel, linux-pm; +Cc: linux-kernel, drinkcat, 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 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 <ikjn@chromium.org>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] power: supply: sbs-battery: don't assume i2c errors as battery disconnect
2020-08-13 5:10 ` [PATCH v3 2/2] power: supply: sbs-battery: don't assume i2c errors as battery disconnect Ikjoon Jang
@ 2020-08-13 5:26 ` Nicolas Boichat
2020-08-27 22:00 ` Sebastian Reichel
1 sibling, 0 replies; 6+ messages in thread
From: Nicolas Boichat @ 2020-08-13 5:26 UTC (permalink / raw)
To: Ikjoon Jang; +Cc: Sebastian Reichel, open list:THERMAL, lkml
On Thu, Aug 13, 2020 at 1:11 PM Ikjoon Jang <ikjn@chromium.org> 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 <ikjn@chromium.org>
Looks good now, AFAICT. Thanks!
Reviewed-by: Nicolas Boichat <drinkcat@chromium.org>
> ---
> 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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] power: supply: sbs-battery: combine get_presence_and_health
2020-08-13 5:10 ` [PATCH v3 1/2] power: supply: sbs-battery: combine get_presence_and_health Ikjoon Jang
@ 2020-08-27 21:58 ` Sebastian Reichel
0 siblings, 0 replies; 6+ messages in thread
From: Sebastian Reichel @ 2020-08-27 21:58 UTC (permalink / raw)
To: Ikjoon Jang; +Cc: linux-pm, linux-kernel, drinkcat
[-- Attachment #1: Type: text/plain, Size: 3768 bytes --]
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 <ikjn@chromium.org>
> ---
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
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] power: supply: sbs-battery: don't assume i2c errors as battery disconnect
2020-08-13 5:10 ` [PATCH v3 2/2] power: supply: sbs-battery: don't assume i2c errors as battery disconnect Ikjoon Jang
2020-08-13 5:26 ` Nicolas Boichat
@ 2020-08-27 22:00 ` Sebastian Reichel
1 sibling, 0 replies; 6+ messages in thread
From: Sebastian Reichel @ 2020-08-27 22:00 UTC (permalink / raw)
To: Ikjoon Jang; +Cc: linux-pm, linux-kernel, drinkcat
[-- Attachment #1: Type: text/plain, Size: 2586 bytes --]
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 <ikjn@chromium.org>
> ---
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
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-08-27 22:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <0200811065307.2094930-1-ikjn@chromium.org>
2020-08-13 5:10 ` [PATCH v3 0/2] power: supply: sbs-battery: fix presence check Ikjoon Jang
2020-08-13 5:10 ` [PATCH v3 1/2] power: supply: sbs-battery: combine get_presence_and_health Ikjoon Jang
2020-08-27 21:58 ` Sebastian Reichel
2020-08-13 5:10 ` [PATCH v3 2/2] power: supply: sbs-battery: don't assume i2c errors as battery disconnect Ikjoon Jang
2020-08-13 5:26 ` Nicolas Boichat
2020-08-27 22:00 ` Sebastian Reichel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).