linux-pm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/1] power/supply/sbs-battery: Fix confusing battery status when idle or empty
@ 2019-07-25  8:25 Richard Tresidder
  2019-07-25 13:39 ` Guenter Roeck
  0 siblings, 1 reply; 3+ messages in thread
From: Richard Tresidder @ 2019-07-25  8:25 UTC (permalink / raw)
  To: sre, enric.balletbo, ncrews, andrew.smirnov, groeck, rtresidd,
	david, 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 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;
 		else if (ret < 0)
 			*intval = POWER_SUPPLY_STATUS_DISCHARGING;
 	}
@@ -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


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH 1/1] power/supply/sbs-battery: Fix confusing battery status when idle or empty
  2019-07-25  8:25 [PATCH 1/1] power/supply/sbs-battery: Fix confusing battery status when idle or empty Richard Tresidder
@ 2019-07-25 13:39 ` Guenter Roeck
  2019-07-26  2:46   ` Richard Tresidder
  0 siblings, 1 reply; 3+ messages in thread
From: Guenter Roeck @ 2019-07-25 13:39 UTC (permalink / raw)
  To: Richard Tresidder
  Cc: Sebastian Reichel, Enric Balletbo i Serra, Nick Crews,
	andrew.smirnov, Guenter Roeck, david, Thomas Gleixner,
	Kate Stewart, Greg Kroah-Hartman, rfontana, allison, baolin.wang,
	Linux PM list, linux-kernel

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
>

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH 1/1] power/supply/sbs-battery: Fix confusing battery status when idle or empty
  2019-07-25 13:39 ` Guenter Roeck
@ 2019-07-26  2:46   ` Richard Tresidder
  0 siblings, 0 replies; 3+ messages in thread
From: Richard Tresidder @ 2019-07-26  2:46 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Sebastian Reichel, Enric Balletbo i Serra, Nick Crews,
	andrew.smirnov, Guenter Roeck, david, Thomas Gleixner,
	Kate Stewart, Greg Kroah-Hartman, rfontana, allison, baolin.wang,
	Linux PM list, linux-kernel

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


^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-07-26  2:46 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-25  8:25 [PATCH 1/1] power/supply/sbs-battery: Fix confusing battery status when idle or empty Richard Tresidder
2019-07-25 13:39 ` Guenter Roeck
2019-07-26  2:46   ` Richard Tresidder

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).