* [PATCH v4 1/1] power/supply/sbs-battery: Fix confusing battery status when idle or empty
@ 2019-07-30 2:00 Richard Tresidder
2019-08-01 18:34 ` Nick Crews
0 siblings, 1 reply; 3+ messages in thread
From: Richard Tresidder @ 2019-07-30 2:00 UTC (permalink / raw)
To: sre, enric.balletbo, ncrews, andrew.smirnov, groeck, david,
rtresidd, tglx, kstewart, gregkh, rfontana, allison, baolin.wang,
linux-pm, linux-kernel
When a battery or batteries in a system are in parallel then one or more
may not be providing any current to the system.
This fixes an incorrect status indication of FULL for the battery simply
because it wasn't discharging at that point in time.
The battery will now be flagged as NOT CHARGING.
Have also added the additional check for the battery FULL DISCHARGED flag
which will now flag a status of EMPTY.
Signed-off-by: Richard Tresidder <rtresidd@electromag.com.au>
---
Notes:
power/supply/sbs-battery: Fix confusing battery status when idle or empty
When a battery or batteries in a system are in parallel then one or more
may not be providing any current to the system.
This fixes an incorrect status indication of FULL for the battery simply
because it wasn't discharging at that point in time.
The battery will now be flagged as NOT CHARGING.
Have also added the additional check for the battery FULL DISCHARGED flag
which will now flag a status of EMPTY.
v2: Missed a later merge that should have been included in original patch
v3: Refactor the sbs_status_correct function to capture all the states for
normal operation rather than being spread across multile functions.
v4: Remove unnecessary brackets, rename sbs_status_correct to
sbs_correct_battery_status
drivers/power/supply/power_supply_sysfs.c | 2 +-
drivers/power/supply/sbs-battery.c | 46 ++++++++++++-------------------
include/linux/power_supply.h | 1 +
3 files changed, 19 insertions(+), 30 deletions(-)
diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index f37ad4e..305e833 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -51,7 +51,7 @@
};
static const char * const power_supply_status_text[] = {
- "Unknown", "Charging", "Discharging", "Not charging", "Full"
+ "Unknown", "Charging", "Discharging", "Not charging", "Full", "Empty"
};
static const char * const power_supply_charge_type_text[] = {
diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
index 048d205..3ed70d4 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -283,7 +283,7 @@ static int sbs_write_word_data(struct i2c_client *client, u8 address,
return 0;
}
-static int sbs_status_correct(struct i2c_client *client, int *intval)
+static int sbs_correct_battery_status(struct i2c_client *client, int *status)
{
int ret;
@@ -293,16 +293,18 @@ static int sbs_status_correct(struct i2c_client *client, int *intval)
ret = (s16)ret;
- /* Not drawing current means full (cannot be not charging) */
- if (ret == 0)
- *intval = POWER_SUPPLY_STATUS_FULL;
-
- if (*intval == POWER_SUPPLY_STATUS_FULL) {
- /* Drawing or providing current when full */
- if (ret > 0)
- *intval = POWER_SUPPLY_STATUS_CHARGING;
- else if (ret < 0)
- *intval = POWER_SUPPLY_STATUS_DISCHARGING;
+ if (ret > 0)
+ *status = POWER_SUPPLY_STATUS_CHARGING;
+ else if (ret < 0)
+ *status = POWER_SUPPLY_STATUS_DISCHARGING;
+ else {
+ /* Current is 0, so how full is the battery? */
+ if (*status & BATTERY_FULL_CHARGED)
+ *status = POWER_SUPPLY_STATUS_FULL;
+ else if (*status & BATTERY_FULL_DISCHARGED)
+ *status = POWER_SUPPLY_STATUS_EMPTY;
+ else
+ *status = POWER_SUPPLY_STATUS_NOT_CHARGING;
}
return 0;
@@ -421,14 +423,9 @@ static int sbs_get_battery_property(struct i2c_client *client,
return 0;
}
- if (ret & BATTERY_FULL_CHARGED)
- val->intval = POWER_SUPPLY_STATUS_FULL;
- else if (ret & BATTERY_DISCHARGING)
- val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
- else
- val->intval = POWER_SUPPLY_STATUS_CHARGING;
-
- sbs_status_correct(client, &val->intval);
+ ret = sbs_correct_battery_status(client, &val->intval);
+ if (ret < 0)
+ return ret;
if (chip->poll_time == 0)
chip->last_state = val->intval;
@@ -773,20 +770,11 @@ static void sbs_delayed_work(struct work_struct *work)
ret = sbs_read_word_data(chip->client, sbs_data[REG_STATUS].addr);
/* if the read failed, give up on this work */
- if (ret < 0) {
+ if (ret < 0 || sbs_correct_battery_status(chip->client, &ret) < 0) {
chip->poll_time = 0;
return;
}
- if (ret & BATTERY_FULL_CHARGED)
- ret = POWER_SUPPLY_STATUS_FULL;
- else if (ret & BATTERY_DISCHARGING)
- ret = POWER_SUPPLY_STATUS_DISCHARGING;
- else
- ret = POWER_SUPPLY_STATUS_CHARGING;
-
- sbs_status_correct(chip->client, &ret);
-
if (chip->last_state != ret) {
chip->poll_time = 0;
power_supply_changed(chip->power_supply);
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 28413f7..8fb10ec 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -37,6 +37,7 @@ enum {
POWER_SUPPLY_STATUS_DISCHARGING,
POWER_SUPPLY_STATUS_NOT_CHARGING,
POWER_SUPPLY_STATUS_FULL,
+ POWER_SUPPLY_STATUS_EMPTY,
};
/* What algorithm is the charger using? */
--
1.8.3.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v4 1/1] power/supply/sbs-battery: Fix confusing battery status when idle or empty
2019-07-30 2:00 [PATCH v4 1/1] power/supply/sbs-battery: Fix confusing battery status when idle or empty Richard Tresidder
@ 2019-08-01 18:34 ` Nick Crews
2019-09-02 18:31 ` Sebastian Reichel
0 siblings, 1 reply; 3+ messages in thread
From: Nick Crews @ 2019-08-01 18:34 UTC (permalink / raw)
To: Richard Tresidder
Cc: Sebastian Reichel, Enric Balletbo i Serra, andrew.smirnov,
Guenter Roeck, david, Thomas Gleixner, Kate Stewart,
Greg Kroah-Hartman, rfontana, allison, Baolin Wang, linux-pm,
linux-kernel
Thanks Richard, I still would like some more opinions
on this changing the userspace experience, but LGTM
otherwise.
Reviewed-by: Nick Crews <ncrews@chromium.org>
On Mon, Jul 29, 2019 at 8:00 PM Richard Tresidder
<rtresidd@electromag.com.au> wrote:
>
> When a battery or batteries in a system are in parallel then one or more
> may not be providing any current to the system.
> This fixes an incorrect status indication of FULL for the battery simply
> because it wasn't discharging at that point in time.
> The battery will now be flagged as NOT CHARGING.
> Have also added the additional check for the battery FULL DISCHARGED flag
> which will now flag a status of EMPTY.
>
> Signed-off-by: Richard Tresidder <rtresidd@electromag.com.au>
> ---
>
> Notes:
> power/supply/sbs-battery: Fix confusing battery status when idle or empty
>
> When a battery or batteries in a system are in parallel then one or more
> may not be providing any current to the system.
> This fixes an incorrect status indication of FULL for the battery simply
> because it wasn't discharging at that point in time.
> The battery will now be flagged as NOT CHARGING.
> Have also added the additional check for the battery FULL DISCHARGED flag
> which will now flag a status of EMPTY.
>
> v2: Missed a later merge that should have been included in original patch
> v3: Refactor the sbs_status_correct function to capture all the states for
> normal operation rather than being spread across multile functions.
> v4: Remove unnecessary brackets, rename sbs_status_correct to
> sbs_correct_battery_status
>
> drivers/power/supply/power_supply_sysfs.c | 2 +-
> drivers/power/supply/sbs-battery.c | 46 ++++++++++++-------------------
> include/linux/power_supply.h | 1 +
> 3 files changed, 19 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index f37ad4e..305e833 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -51,7 +51,7 @@
> };
>
> static const char * const power_supply_status_text[] = {
> - "Unknown", "Charging", "Discharging", "Not charging", "Full"
> + "Unknown", "Charging", "Discharging", "Not charging", "Full", "Empty"
> };
>
> static const char * const power_supply_charge_type_text[] = {
> diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
> index 048d205..3ed70d4 100644
> --- a/drivers/power/supply/sbs-battery.c
> +++ b/drivers/power/supply/sbs-battery.c
> @@ -283,7 +283,7 @@ static int sbs_write_word_data(struct i2c_client *client, u8 address,
> return 0;
> }
>
> -static int sbs_status_correct(struct i2c_client *client, int *intval)
> +static int sbs_correct_battery_status(struct i2c_client *client, int *status)
> {
> int ret;
>
> @@ -293,16 +293,18 @@ static int sbs_status_correct(struct i2c_client *client, int *intval)
>
> ret = (s16)ret;
>
> - /* Not drawing current means full (cannot be not charging) */
> - if (ret == 0)
> - *intval = POWER_SUPPLY_STATUS_FULL;
> -
> - if (*intval == POWER_SUPPLY_STATUS_FULL) {
> - /* Drawing or providing current when full */
> - if (ret > 0)
> - *intval = POWER_SUPPLY_STATUS_CHARGING;
> - else if (ret < 0)
> - *intval = POWER_SUPPLY_STATUS_DISCHARGING;
> + if (ret > 0)
> + *status = POWER_SUPPLY_STATUS_CHARGING;
> + else if (ret < 0)
> + *status = POWER_SUPPLY_STATUS_DISCHARGING;
> + else {
> + /* Current is 0, so how full is the battery? */
> + if (*status & BATTERY_FULL_CHARGED)
> + *status = POWER_SUPPLY_STATUS_FULL;
> + else if (*status & BATTERY_FULL_DISCHARGED)
> + *status = POWER_SUPPLY_STATUS_EMPTY;
> + else
> + *status = POWER_SUPPLY_STATUS_NOT_CHARGING;
> }
>
> return 0;
> @@ -421,14 +423,9 @@ static int sbs_get_battery_property(struct i2c_client *client,
> return 0;
> }
>
> - if (ret & BATTERY_FULL_CHARGED)
> - val->intval = POWER_SUPPLY_STATUS_FULL;
> - else if (ret & BATTERY_DISCHARGING)
> - val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
> - else
> - val->intval = POWER_SUPPLY_STATUS_CHARGING;
> -
> - sbs_status_correct(client, &val->intval);
> + ret = sbs_correct_battery_status(client, &val->intval);
> + if (ret < 0)
> + return ret;
>
> if (chip->poll_time == 0)
> chip->last_state = val->intval;
> @@ -773,20 +770,11 @@ static void sbs_delayed_work(struct work_struct *work)
>
> ret = sbs_read_word_data(chip->client, sbs_data[REG_STATUS].addr);
> /* if the read failed, give up on this work */
> - if (ret < 0) {
> + if (ret < 0 || sbs_correct_battery_status(chip->client, &ret) < 0) {
> chip->poll_time = 0;
> return;
> }
>
> - if (ret & BATTERY_FULL_CHARGED)
> - ret = POWER_SUPPLY_STATUS_FULL;
> - else if (ret & BATTERY_DISCHARGING)
> - ret = POWER_SUPPLY_STATUS_DISCHARGING;
> - else
> - ret = POWER_SUPPLY_STATUS_CHARGING;
> -
> - sbs_status_correct(chip->client, &ret);
> -
> if (chip->last_state != ret) {
> chip->poll_time = 0;
> power_supply_changed(chip->power_supply);
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 28413f7..8fb10ec 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -37,6 +37,7 @@ enum {
> POWER_SUPPLY_STATUS_DISCHARGING,
> POWER_SUPPLY_STATUS_NOT_CHARGING,
> POWER_SUPPLY_STATUS_FULL,
> + POWER_SUPPLY_STATUS_EMPTY,
> };
>
> /* What algorithm is the charger using? */
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v4 1/1] power/supply/sbs-battery: Fix confusing battery status when idle or empty
2019-08-01 18:34 ` Nick Crews
@ 2019-09-02 18:31 ` Sebastian Reichel
0 siblings, 0 replies; 3+ messages in thread
From: Sebastian Reichel @ 2019-09-02 18:31 UTC (permalink / raw)
To: Nick Crews
Cc: Richard Tresidder, Enric Balletbo i Serra, andrew.smirnov,
Guenter Roeck, david, Thomas Gleixner, Kate Stewart,
Greg Kroah-Hartman, rfontana, allison, Baolin Wang, linux-pm,
linux-kernel
[-- Attachment #1: Type: text/plain, Size: 6921 bytes --]
Hi,
Looks good to me, but needs to be split (one patch adding STATUS_EMPTY
to the power-supply core and one patch fixing sbs-battery driver).
-- Sebastian
On Thu, Aug 01, 2019 at 12:34:18PM -0600, Nick Crews wrote:
> Thanks Richard, I still would like some more opinions
> on this changing the userspace experience, but LGTM
> otherwise.
>
> Reviewed-by: Nick Crews <ncrews@chromium.org>
>
> On Mon, Jul 29, 2019 at 8:00 PM Richard Tresidder
> <rtresidd@electromag.com.au> wrote:
> >
> > When a battery or batteries in a system are in parallel then one or more
> > may not be providing any current to the system.
> > This fixes an incorrect status indication of FULL for the battery simply
> > because it wasn't discharging at that point in time.
> > The battery will now be flagged as NOT CHARGING.
> > Have also added the additional check for the battery FULL DISCHARGED flag
> > which will now flag a status of EMPTY.
> >
> > Signed-off-by: Richard Tresidder <rtresidd@electromag.com.au>
> > ---
> >
> > Notes:
> > power/supply/sbs-battery: Fix confusing battery status when idle or empty
> >
> > When a battery or batteries in a system are in parallel then one or more
> > may not be providing any current to the system.
> > This fixes an incorrect status indication of FULL for the battery simply
> > because it wasn't discharging at that point in time.
> > The battery will now be flagged as NOT CHARGING.
> > Have also added the additional check for the battery FULL DISCHARGED flag
> > which will now flag a status of EMPTY.
> >
> > v2: Missed a later merge that should have been included in original patch
> > v3: Refactor the sbs_status_correct function to capture all the states for
> > normal operation rather than being spread across multile functions.
> > v4: Remove unnecessary brackets, rename sbs_status_correct to
> > sbs_correct_battery_status
> >
> > drivers/power/supply/power_supply_sysfs.c | 2 +-
> > drivers/power/supply/sbs-battery.c | 46 ++++++++++++-------------------
> > include/linux/power_supply.h | 1 +
> > 3 files changed, 19 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> > index f37ad4e..305e833 100644
> > --- a/drivers/power/supply/power_supply_sysfs.c
> > +++ b/drivers/power/supply/power_supply_sysfs.c
> > @@ -51,7 +51,7 @@
> > };
> >
> > static const char * const power_supply_status_text[] = {
> > - "Unknown", "Charging", "Discharging", "Not charging", "Full"
> > + "Unknown", "Charging", "Discharging", "Not charging", "Full", "Empty"
> > };
> >
> > static const char * const power_supply_charge_type_text[] = {
> > diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
> > index 048d205..3ed70d4 100644
> > --- a/drivers/power/supply/sbs-battery.c
> > +++ b/drivers/power/supply/sbs-battery.c
> > @@ -283,7 +283,7 @@ static int sbs_write_word_data(struct i2c_client *client, u8 address,
> > return 0;
> > }
> >
> > -static int sbs_status_correct(struct i2c_client *client, int *intval)
> > +static int sbs_correct_battery_status(struct i2c_client *client, int *status)
> > {
> > int ret;
> >
> > @@ -293,16 +293,18 @@ static int sbs_status_correct(struct i2c_client *client, int *intval)
> >
> > ret = (s16)ret;
> >
> > - /* Not drawing current means full (cannot be not charging) */
> > - if (ret == 0)
> > - *intval = POWER_SUPPLY_STATUS_FULL;
> > -
> > - if (*intval == POWER_SUPPLY_STATUS_FULL) {
> > - /* Drawing or providing current when full */
> > - if (ret > 0)
> > - *intval = POWER_SUPPLY_STATUS_CHARGING;
> > - else if (ret < 0)
> > - *intval = POWER_SUPPLY_STATUS_DISCHARGING;
> > + if (ret > 0)
> > + *status = POWER_SUPPLY_STATUS_CHARGING;
> > + else if (ret < 0)
> > + *status = POWER_SUPPLY_STATUS_DISCHARGING;
> > + else {
> > + /* Current is 0, so how full is the battery? */
> > + if (*status & BATTERY_FULL_CHARGED)
> > + *status = POWER_SUPPLY_STATUS_FULL;
> > + else if (*status & BATTERY_FULL_DISCHARGED)
> > + *status = POWER_SUPPLY_STATUS_EMPTY;
> > + else
> > + *status = POWER_SUPPLY_STATUS_NOT_CHARGING;
> > }
> >
> > return 0;
> > @@ -421,14 +423,9 @@ static int sbs_get_battery_property(struct i2c_client *client,
> > return 0;
> > }
> >
> > - if (ret & BATTERY_FULL_CHARGED)
> > - val->intval = POWER_SUPPLY_STATUS_FULL;
> > - else if (ret & BATTERY_DISCHARGING)
> > - val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
> > - else
> > - val->intval = POWER_SUPPLY_STATUS_CHARGING;
> > -
> > - sbs_status_correct(client, &val->intval);
> > + ret = sbs_correct_battery_status(client, &val->intval);
> > + if (ret < 0)
> > + return ret;
> >
> > if (chip->poll_time == 0)
> > chip->last_state = val->intval;
> > @@ -773,20 +770,11 @@ static void sbs_delayed_work(struct work_struct *work)
> >
> > ret = sbs_read_word_data(chip->client, sbs_data[REG_STATUS].addr);
> > /* if the read failed, give up on this work */
> > - if (ret < 0) {
> > + if (ret < 0 || sbs_correct_battery_status(chip->client, &ret) < 0) {
> > chip->poll_time = 0;
> > return;
> > }
> >
> > - if (ret & BATTERY_FULL_CHARGED)
> > - ret = POWER_SUPPLY_STATUS_FULL;
> > - else if (ret & BATTERY_DISCHARGING)
> > - ret = POWER_SUPPLY_STATUS_DISCHARGING;
> > - else
> > - ret = POWER_SUPPLY_STATUS_CHARGING;
> > -
> > - sbs_status_correct(chip->client, &ret);
> > -
> > if (chip->last_state != ret) {
> > chip->poll_time = 0;
> > power_supply_changed(chip->power_supply);
> > diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> > index 28413f7..8fb10ec 100644
> > --- a/include/linux/power_supply.h
> > +++ b/include/linux/power_supply.h
> > @@ -37,6 +37,7 @@ enum {
> > POWER_SUPPLY_STATUS_DISCHARGING,
> > POWER_SUPPLY_STATUS_NOT_CHARGING,
> > POWER_SUPPLY_STATUS_FULL,
> > + POWER_SUPPLY_STATUS_EMPTY,
> > };
> >
> > /* What algorithm is the charger using? */
> > --
> > 1.8.3.1
> >
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-09-02 18:32 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-30 2:00 [PATCH v4 1/1] power/supply/sbs-battery: Fix confusing battery status when idle or empty Richard Tresidder
2019-08-01 18:34 ` Nick Crews
2019-09-02 18:31 ` 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).