All of lore.kernel.org
 help / color / mirror / Atom feed
* [RESEND v2 1/1] power/supply/sbs-battery: Fix confusing battery status when idle or empty
@ 2019-07-25  8:55 Richard Tresidder
  2019-07-26 17:47 ` Nick Crews
  2019-07-26 20:31 ` kbuild test robot
  0 siblings, 2 replies; 4+ messages in thread
From: Richard Tresidder @ 2019-07-25  8:55 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.
    
    v2: missed a later merge that should have been included in original patch

 drivers/power/supply/power_supply_sysfs.c |  3 ++-
 drivers/power/supply/sbs-battery.c        | 32 +++++++++++++++----------------
 include/linux/power_supply.h              |  2 ++
 3 files changed, 20 insertions(+), 17 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..664c317 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -294,16 +294,12 @@ 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 ((*intval == POWER_SUPPLY_STATUS_DISCHARGING && (ret == 0)) {
+		/* Charging indicator not set in battery */
+		*intval = POWER_SUPPLY_STATUS_IDLE;
+	} else if ((*intval == POWER_SUPPLY_STATUS_FULL) && (ret < 0)) {
+		/* Full Flag set but we are discharging */
+		*intval = POWER_SUPPLY_STATUS_DISCHARGING;
 	}
 
 	return 0;
@@ -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] 4+ messages in thread
* Re: [RESEND v2 1/1] power/supply/sbs-battery: Fix confusing battery status when idle or empty
  2019-07-25  8:55 [RESEND v2 1/1] power/supply/sbs-battery: Fix confusing battery status when idle or empty Richard Tresidder
@ 2019-07-26 17:47 ` Nick Crews
  2019-07-29  4:25   ` Richard Tresidder
  2019-07-26 20:31 ` kbuild test robot
  1 sibling, 1 reply; 4+ messages in thread
From: Nick Crews @ 2019-07-26 17:47 UTC (permalink / raw)
  To: Richard Tresidder
  Cc: Sebastian Reichel, Enric Balletbo i Serra, andrew.smirnov,
	Guenter Roeck, david, Thomas Gleixner, Kate Stewart,
	Greg Kroah-Hartman, rfontana, allison, baolin.wang, linux-pm,
	linux-kernel

Hi Richard!

Thanks for the patch. I'm not familiar with these batteries, but I have
a few thoughts. For others, the SBS battery spec is at
http://sbs-forum.org/specs/sbdat110.pdf, and section 5.1.21 at page 28
is useful.

On Thu, Jul 25, 2019 at 2:55 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.
>
>     v2: missed a later merge that should have been included in original patch
>
>  drivers/power/supply/power_supply_sysfs.c |  3 ++-
>  drivers/power/supply/sbs-battery.c        | 32 +++++++++++++++----------------
>  include/linux/power_supply.h              |  2 ++
>  3 files changed, 20 insertions(+), 17 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"

How is "Idle" different" from "Not charging"? To me they both mean that the
battery is at an intermediate state of charge and that there is no significant
current going into or out of the battery. Can you just use "Not
charging" instead
of adding a new status type? Idle seems like a better name to me, but
unfortunately we can't change that at this point.

Adding "Empty" seems quite reasonable.

>  };
>
>  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..664c317 100644
> --- a/drivers/power/supply/sbs-battery.c
> +++ b/drivers/power/supply/sbs-battery.c
> @@ -294,16 +294,12 @@ 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 ((*intval == POWER_SUPPLY_STATUS_DISCHARGING && (ret == 0)) {
> +               /* Charging indicator not set in battery */
> +               *intval = POWER_SUPPLY_STATUS_IDLE;
> +       } else if ((*intval == POWER_SUPPLY_STATUS_FULL) && (ret < 0)) {
> +               /* Full Flag set but we are discharging */
> +               *intval = POWER_SUPPLY_STATUS_DISCHARGING;

In any of these cases do we really care about intval? Wouldn't the
current be the
ultimate ground truth of what is happening? e.g.
-(ret == 0) means "Idle"/"Not charging"
-(ret > 0) mean charging
-(ret < 0) means discharging

At the least, how about a check for
else if ((*intval == POWER_SUPPLY_STATUS_EMPTY) && (ret > 0)) {
               /* Empty flag set but current is positive. */
               *intval = POWER_SUPPLY_STATUS_CHARGING;
}

>         }
>
>         return 0;
> @@ -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);

Unrelated, but sbs_status_correct() can return a negative error code.
We should check for that. Here pass it up the call stack, in the other
call site probably do something else.

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

This last case is somewhat misleading, when the DISCHARGING flag is set all
it means that it the battery is not charging (what a dumb name in the
spec, but whatever).
Thus the battery could actually be idle when we say here that it is
discharging. We correct
this later by reading the current, but at least a comment explaining
as such would be nice.
Otherwise this pattern of "if not A, then B, else A" as opposed to the
more standard
"if A, then A, else B" looks really weird.

I'm being an armchair coder here, but is there anything keeping you from moving
all of this code into sbs_status_correct()? That would de-duplicate it
and solve the
problem mentioned above. What about:

/*
 * @intval: On input is the result of sbs_read_word_data(REG_STATUS),
 *               on output is a POWER_SUPPLY_STATUS_* value after
 *               correcting for the current.
 */
static int sbs_status_correct(struct i2c_client *client, int *intval)
{
        int ret;
        s16 current;

        ret = sbs_read_word_data(client, sbs_data[REG_CURRENT].addr);
        if (ret < 0)
                return ret;

        current = (s16)ret;

        if (current > 0)
                *intval = POWER_SUPPLY_STATUS_CHARGING;
        else if (current < 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;
        }

        return 0;
}

and then you would call it without doing any of the original parsing?

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

Cheers,
Nick

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [RESEND v2 1/1] power/supply/sbs-battery: Fix confusing battery status when idle or empty
  2019-07-25  8:55 [RESEND v2 1/1] power/supply/sbs-battery: Fix confusing battery status when idle or empty Richard Tresidder
  2019-07-26 17:47 ` Nick Crews
@ 2019-07-26 20:31 ` kbuild test robot
  1 sibling, 0 replies; 4+ messages in thread
From: kbuild test robot @ 2019-07-26 20:31 UTC (permalink / raw)
  To: Richard Tresidder
  Cc: kbuild-all, sre, enric.balletbo, ncrews, andrew.smirnov, groeck,
	rtresidd, david, tglx, kstewart, gregkh, rfontana, allison,
	baolin.wang, linux-pm, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 2303 bytes --]

Hi Richard,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.3-rc1 next-20190726]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Richard-Tresidder/power-supply-sbs-battery-Fix-confusing-battery-status-when-idle-or-empty/20190727-031454
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=arm 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

   drivers/power//supply/sbs-battery.c: In function 'sbs_status_correct':
>> drivers/power//supply/sbs-battery.c:296:65: error: expected ')' before '{' token
     if ((*intval == POWER_SUPPLY_STATUS_DISCHARGING && (ret == 0)) {
                                                                    ^
>> drivers/power//supply/sbs-battery.c:305:1: error: expected expression before '}' token
    }
    ^
>> drivers/power//supply/sbs-battery.c:305:1: warning: control reaches end of non-void function [-Wreturn-type]
    }
    ^

vim +296 drivers/power//supply/sbs-battery.c

   285	
   286	static int sbs_status_correct(struct i2c_client *client, int *intval)
   287	{
   288		int ret;
   289	
   290		ret = sbs_read_word_data(client, sbs_data[REG_CURRENT].addr);
   291		if (ret < 0)
   292			return ret;
   293	
   294		ret = (s16)ret;
   295	
 > 296		if ((*intval == POWER_SUPPLY_STATUS_DISCHARGING && (ret == 0)) {
   297			/* Charging indicator not set in battery */
   298			*intval = POWER_SUPPLY_STATUS_IDLE;
   299		} else if ((*intval == POWER_SUPPLY_STATUS_FULL) && (ret < 0)) {
   300			/* Full Flag set but we are discharging */
   301			*intval = POWER_SUPPLY_STATUS_DISCHARGING;
   302		}
   303	
   304		return 0;
 > 305	}
   306	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 71381 bytes --]

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

Hi Nick
    Cheers yep have the spec docs. We're using Inspired Energy packs 
most of the time.
I appreciate your review and I'm more than happy to do a more complete 
change as your armchair coding provided :)
See below for additional comments.

Cheers
    Richard Tresidder
On 27/07/2019 1:47 am, Nick Crews wrote:
> Hi Richard!
>
> Thanks for the patch. I'm not familiar with these batteries, but I have
> a few thoughts. For others, the SBS battery spec is at
> http://sbs-forum.org/specs/sbdat110.pdf, and section 5.1.21 at page 28
> is useful.
>
> On Thu, Jul 25, 2019 at 2:55 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.
>>
>>      v2: missed a later merge that should have been included in original patch
>>
>>   drivers/power/supply/power_supply_sysfs.c |  3 ++-
>>   drivers/power/supply/sbs-battery.c        | 32 +++++++++++++++----------------
>>   include/linux/power_supply.h              |  2 ++
>>   3 files changed, 20 insertions(+), 17 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"
> How is "Idle" different" from "Not charging"? To me they both mean that the
> battery is at an intermediate state of charge and that there is no significant
> current going into or out of the battery. Can you just use "Not
> charging" instead
> of adding a new status type? Idle seems like a better name to me, but
> unfortunately we can't change that at this point.
>
> Adding "Empty" seems quite reasonable.
I can change to us "Not Charging" it just didn't sound right as a state 
for a battery sitting doing nothing.
I'd kind of though that something else would set this "Not Charging" 
state due to external supply or something.
Maybe due to being chained through a Smart Battery Manager that was 
charging one battery at a time for example.
But perhaps that is beyond this patch at the moment.
We have some other systems that have 4 batteries in them managed by a 
pair of LTC1760's
In that system for example we could determine if a battery was meant to 
be in a "Not Charging" state.
The one I'm integrating at the moment just has the batteries in parallel 
via some Ideal Diodes..

>
>>   };
>>
>>   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..664c317 100644
>> --- a/drivers/power/supply/sbs-battery.c
>> +++ b/drivers/power/supply/sbs-battery.c
>> @@ -294,16 +294,12 @@ 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 ((*intval == POWER_SUPPLY_STATUS_DISCHARGING && (ret == 0)) {
>> +               /* Charging indicator not set in battery */
>> +               *intval = POWER_SUPPLY_STATUS_IDLE;
>> +       } else if ((*intval == POWER_SUPPLY_STATUS_FULL) && (ret < 0)) {
>> +               /* Full Flag set but we are discharging */
>> +               *intval = POWER_SUPPLY_STATUS_DISCHARGING;
> In any of these cases do we really care about intval? Wouldn't the
> current be the
> ultimate ground truth of what is happening? e.g.
> -(ret == 0) means "Idle"/"Not charging"
> -(ret > 0) mean charging
> -(ret < 0) means discharging
>
> At the least, how about a check for
> else if ((*intval == POWER_SUPPLY_STATUS_EMPTY) && (ret > 0)) {
>                 /* Empty flag set but current is positive. */
>                 *intval = POWER_SUPPLY_STATUS_CHARGING;
> }
Yep I missed that possibility thanks
>>          }
>>
>>          return 0;
>> @@ -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);
> Unrelated, but sbs_status_correct() can return a negative error code.
> We should check for that. Here pass it up the call stack, in the other
> call site probably do something else.
Yep I'll take a look at this.
>> @@ -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;
> This last case is somewhat misleading, when the DISCHARGING flag is set all
> it means that it the battery is not charging (what a dumb name in the
> spec, but whatever).
> Thus the battery could actually be idle when we say here that it is
> discharging. We correct
> this later by reading the current, but at least a comment explaining
> as such would be nice.
> Otherwise this pattern of "if not A, then B, else A" as opposed to the
> more standard
> "if A, then A, else B" looks really weird.
Yes I think my logic there was wrong, I was thinking that the flag would 
set when discharging.
Not as you've correctly pointed out and after I re-read the spec doc it 
is cleared when charging is detected.
Rather backwards logic on my part.. can't remember why I'd done it that 
way..
>
> I'm being an armchair coder here, but is there anything keeping you from moving
> all of this code into sbs_status_correct()? That would de-duplicate it
> and solve the
> problem mentioned above. What about:
>
> /*
>   * @intval: On input is the result of sbs_read_word_data(REG_STATUS),
>   *               on output is a POWER_SUPPLY_STATUS_* value after
>   *               correcting for the current.
>   */
> static int sbs_status_correct(struct i2c_client *client, int *intval)
> {
>          int ret;
>          s16 current;
>
>          ret = sbs_read_word_data(client, sbs_data[REG_CURRENT].addr);
>          if (ret < 0)
>                  return ret;
>
>          current = (s16)ret;
>
>          if (current > 0)
>                  *intval = POWER_SUPPLY_STATUS_CHARGING;
>          else if (current < 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;
>          }
>
>          return 0;
> }
>
> and then you would call it without doing any of the original parsing?
>
>>          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
>>
> Cheers,
> Nick
>
>


^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-07-29  4:25 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-25  8:55 [RESEND v2 1/1] power/supply/sbs-battery: Fix confusing battery status when idle or empty Richard Tresidder
2019-07-26 17:47 ` Nick Crews
2019-07-29  4:25   ` Richard Tresidder
2019-07-26 20:31 ` kbuild test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.