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 <[email protected]>
---
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
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
<[email protected]> 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 <[email protected]>
> ---
>
> 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
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 <[email protected]>
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