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

[Sebastian Reichel]

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@xxxxxxxxxxxx>
> 
> On Mon, Jul 29, 2019 at 8:00 PM 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 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@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 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: signature.asc
Description: PGP signature