Re: [PATCH 1/1] power/supply/sbs-battery: Fix confusing battery status when idle or empty

[Richard Tresidder]

Hi Guenter
ÂÂ Yep sorry there was a merge that I missed during that initial send 
of the patch.
I sent a version 2 shortly after.

Regards
ÂÂ Richard Tresidder

On 25/07/2019 9:39 pm, Guenter Roeck wrote:
> On Thu, Jul 25, 2019 at 1:25 AM Richard Tresidder
> <rtresidd@xxxxxxxxxxxxxxxxx> 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 IDLE.
> > 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@xxxxxxxxxxxxxxxxx>
> > ---
> >
> > 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 IDLE.
> >      Have also added the additional check for the battery FULL DISCHARGED flag
> >      which will now flag a status of EMPTY.
> >
> >   drivers/power/supply/power_supply_sysfs.c |  3 ++-
> >   drivers/power/supply/sbs-battery.c        | 28 ++++++++++++++--------------
> >   include/linux/power_supply.h              |  2 ++
> >   3 files changed, 18 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> > index ce6671c..68ec49d 100644
> > --- a/drivers/power/supply/power_supply_sysfs.c
> > +++ b/drivers/power/supply/power_supply_sysfs.c
> > @@ -51,7 +51,8 @@
> >   };
> >
> >   static const char * const power_supply_status_text[] = {
> > -       "Unknown", "Charging", "Discharging", "Not charging", "Full"
> > +       "Unknown", "Charging", "Discharging", "Not charging", "Full",
> > +       "Empty", "Idle"
> >   };
> >
> >   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 ea8ba3e..e6c636c 100644
> > --- a/drivers/power/supply/sbs-battery.c
> > +++ b/drivers/power/supply/sbs-battery.c
> > @@ -294,14 +294,10 @@ 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;
> > +       if (*intval == POWER_SUPPLY_STATUS_DISCHARGING) {
> > +               /* Charging indicator not set in battery */
> > +               if (ret == 0)
> > +                       *intval = POWER_SUPPLY_STATUS_IDLE;
> But doesn't the above already indicate that it _is_ discharging ?
>
> >                  else if (ret < 0)
> >                          *intval = POWER_SUPPLY_STATUS_DISCHARGING;
> This doesn't make sense. *intval is already set to
> POWER_SUPPLY_STATUS_DISCHARGING
> in this situation.
>
> >          }
> > @@ -424,10 +420,12 @@ static int sbs_get_battery_property(struct i2c_client *client,
> >
> >                  if (ret & BATTERY_FULL_CHARGED)
> >                          val->intval = POWER_SUPPLY_STATUS_FULL;
> > -               else if (ret & BATTERY_DISCHARGING)
> > -                       val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
> > -               else
> > +               else if (ret & BATTERY_FULL_DISCHARGED)
> > +                       val->intval = POWER_SUPPLY_STATUS_EMPTY;
> > +               else if (!(ret & BATTERY_DISCHARGING))
> >                          val->intval = POWER_SUPPLY_STATUS_CHARGING;
> > +               else
> > +                       val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
> >
> >                  sbs_status_correct(client, &val->intval);
> >
> > @@ -781,10 +779,12 @@ static void sbs_delayed_work(struct work_struct *work)
> >
> >          if (ret & BATTERY_FULL_CHARGED)
> >                  ret = POWER_SUPPLY_STATUS_FULL;
> > -       else if (ret & BATTERY_DISCHARGING)
> > -               ret = POWER_SUPPLY_STATUS_DISCHARGING;
> > -       else
> > +       else if (ret & BATTERY_FULL_DISCHARGED)
> > +               ret = POWER_SUPPLY_STATUS_EMPTY;
> > +       else if (!(ret & BATTERY_DISCHARGING))
> >                  ret = POWER_SUPPLY_STATUS_CHARGING;
> > +       else
> > +               ret = POWER_SUPPLY_STATUS_DISCHARGING;
> >
> >          sbs_status_correct(chip->client, &ret);
> >
> > diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> > index 28413f7..c9f3347 100644
> > --- a/include/linux/power_supply.h
> > +++ b/include/linux/power_supply.h
> > @@ -37,6 +37,8 @@ enum {
> >          POWER_SUPPLY_STATUS_DISCHARGING,
> >          POWER_SUPPLY_STATUS_NOT_CHARGING,
> >          POWER_SUPPLY_STATUS_FULL,
> > +       POWER_SUPPLY_STATUS_EMPTY,
> > +       POWER_SUPPLY_STATUS_IDLE,
> >   };
> >
> >   /* What algorithm is the charger using? */
> > --
> > 1.8.3.1