lkml.org 
[lkml]   [2019]   [Jul]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v3 1/1] power/supply/sbs-battery: Fix confusing battery status when idle or empty
On Mon, Jul 29, 2019 at 2:07 AM 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.
>
> drivers/power/supply/power_supply_sysfs.c | 2 +-
> drivers/power/supply/sbs-battery.c | 44 +++++++++++--------------------
> include/linux/power_supply.h | 1 +
> 3 files changed, 18 insertions(+), 29 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..b28402d 100644
> --- a/drivers/power/supply/sbs-battery.c
> +++ b/drivers/power/supply/sbs-battery.c
> @@ -293,16 +293,18 @@ static int sbs_status_correct(struct i2c_client *client, int *intval)

sbs_status_correct() sounds like it is checking for a condition. Change
to sbs_correct_status() or sbs_correct_battery_status() to imply that it
is performing an action. Also, change "intval" to "status"?

>
> 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)
> + *intval = POWER_SUPPLY_STATUS_CHARGING;
> + else if (ret < 0)
> + *intval = POWER_SUPPLY_STATUS_DISCHARGING;
> + else {
> + /* Current is 0, so how full is the battery? */
> + if (*intval & BATTERY_FULL_CHARGED)
> + *intval = POWER_SUPPLY_STATUS_FULL;
> + else if (*intval & BATTERY_FULL_DISCHARGED)
> + *intval = POWER_SUPPLY_STATUS_EMPTY;
> + else
> + *intval = POWER_SUPPLY_STATUS_NOT_CHARGING;

This will cause some behavior changes for users. Can we get the opinion
of someone familiar with the users of this driver as to whether this is OK?

> }
>
> 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_status_correct(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_status_correct(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
>

\
 
 \ /
  Last update: 2019-07-29 19:24    [W:0.061 / U:0.136 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site