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

[Richard Tresidder <rtresidd@electromag.com.au>]

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@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 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@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 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
>>
>