Messages in this thread | | | Date | Mon, 2 Sep 2019 20:31:55 +0200 | From | Sebastian Reichel <> | Subject | Re: [PATCH v4 1/1] power/supply/sbs-battery: Fix confusing battery status when idle or empty |
| |
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@chromium.org> > > On Mon, Jul 29, 2019 at 8:00 PM 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 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@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 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 > > [unhandled content-type:application/pgp-signature] | |