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

[Paul Kocialkowski <contact@paulk.fr>]

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

Le lundi 01 mai 2017 à 11:18 -0700, Liam Breck a écrit :
> Hi Paul,
> 
> On Mon, May 1, 2017 at 3:39 AM, Paul Kocialkowski <contact@paulk.fr> wrote:
> > 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.
> 
> It's Andrew's call, but to my eye a single block of code for current
> calc is more readable.
> 
> > 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;
> > > > +
> 
> Why truncate to s16?

Precisely to get the sign right. Basically, we get curr as an int, for which
negative values indicate an error, *not* a negative current.

The current information is only in the first 16 bits, so an explicit cast to s16
allows retrieving the sign information and extending it to 32 bits. This way, we
can use the sign information later on.

The same thing is done in the regular current calculation function, so it's
nothing new introduced by this change.

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