Re: [PATCH 5/5] power: supply: bq27xxx: Correct supply status with current draw

[Paul Kocialkowski <contact@paulk.fr>]

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

Hi,

Le dimanche 30 avril 2017 à 15:35 -0700, Liam Breck a écrit :
> Hi Paul,
> 
> On Sun, Apr 30, 2017 at 1:38 PM, Paul Kocialkowski <contact@paulk.fr> wrote:
> > The status reported directly by the battery controller is not always
> > reliable and should be corrected based on the current draw information.
> 
> I have noticed incorrect status on bq27425. On which chip/s did you
> find this? It might not be a flaw across the whole family...

I got this with a BQ27541 on a veyron_minnie (Chromebook Flip C100PA).

From my experience with batteries in general, it seems that flags are not
updated as often and are often not as accurate as what the current info tells.
So I think it's best to rely on the current info rather than the chip-provided
flags.

This makes a big difference during actual use: current can tell almost instantly
 that a supply was plugged-in/unplugged while the charging/full flags take a lot
longer to be updated. Also, the full flag is never set in my case (perhaps it
still expects the initial full charge value) even though it should be.

I don't think that will cause a problem for other chips of the family anyway:
they should only benefit from this!

> > This implements such a correction with a dedicated function, called
> > when retrieving the supply status.
> > 
> > Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
> > ---
> >  drivers/power/supply/bq27xxx_battery.c | 28 ++++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> > 
> > diff --git a/drivers/power/supply/bq27xxx_battery.c
> > b/drivers/power/supply/bq27xxx_battery.c
> > index cade00df6162..f7694e775e68 100644
> > --- a/drivers/power/supply/bq27xxx_battery.c
> > +++ b/drivers/power/supply/bq27xxx_battery.c
> > @@ -1171,8 +1171,22 @@ static int bq27xxx_battery_status(struct
> > bq27xxx_device_info *di,
> >                                   union power_supply_propval *val)
> >  {
> >         int status;
> > +       int curr;
> > +       int flags;
> > +
> > +       curr = bq27xxx_read(di, BQ27XXX_REG_AI, false);
> > +       if (curr < 0) {
> > +               dev_err(di->dev, "error reading current\n");
> > +               return curr;
> > +       }
> 
> Maybe read current (then sign-flip, *= 1000) above the status fix?

From what I understood of the rest of the driver (see bq27xxx_battery_current),
sign flip is done differently in bq27000/bq27010 and others, so since we already
have specific instruction blocks, I think it's best to put the current
calculation in each.

Also, since we only test the current against 0 I thought it didn't matter that
the scale is not the same for both values. I could add the following for the
bq27000/bq27010: * BQ27XXX_CURRENT_CONSTANT / BQ27XXX_RS
but it won't change a thing.

> >         if (di->chip == BQ27000 || di->chip == BQ27010) {
> > +               flags = bq27xxx_read(di, BQ27XXX_REG_FLAGS, true);
> > +               if (flags & BQ27000_FLAG_CHGS) {
> > +                       dev_dbg(di->dev, "negative current!\n");
> > +                       curr = -curr;
> > +               }
> > +
> >                 if (di->cache.flags & BQ27000_FLAG_FC)
> >                         status = POWER_SUPPLY_STATUS_FULL;
> >                 else if (di->cache.flags & BQ27000_FLAG_CHGS)
> > @@ -1182,6 +1196,8 @@ static int bq27xxx_battery_status(struct
> > bq27xxx_device_info *di,
> >                 else
> >                         status = POWER_SUPPLY_STATUS_DISCHARGING;
> >         } else {
> > +               curr = (int)((s16)curr) * 1000;
> > +
> >                 if (di->cache.flags & BQ27XXX_FLAG_FC)
> >                         status = POWER_SUPPLY_STATUS_FULL;
> >                 else if (di->cache.flags & BQ27XXX_FLAG_DSC)
> > @@ -1190,6 +1206,18 @@ static int bq27xxx_battery_status(struct
> > bq27xxx_device_info *di,
> >                         status = POWER_SUPPLY_STATUS_CHARGING;
> >         }
> > 
> > +
> > +       if (curr == 0 && status != POWER_SUPPLY_STATUS_NOT_CHARGING)
> > +               status = POWER_SUPPLY_STATUS_FULL;
> > +
> > +       if (status == POWER_SUPPLY_STATUS_FULL) {
> > +               /* Drawing or providing current when full */
> > +               if (curr > 0)
> > +                       status = POWER_SUPPLY_STATUS_CHARGING;
> > +               else if (curr < 0)
> > +                       status = POWER_SUPPLY_STATUS_DISCHARGING;
> > +       }
> > +
> >         if (di->status_retry == 0 && di->status_change_reference != status)
> > {
> >                 di->status_change_reference = status;
> >                 power_supply_changed(di->bat);
> > --
> > 2.12.2
> > 
-- 
Paul Kocialkowski, developer of free digital technology and hardware support

Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]