Re: [PATCH 1/5] power: supply: bq27xxx: Pass of_node along to allow device-tree supply

Re: [PATCH 1/5] power: supply: bq27xxx: Pass of_node along to allow device-tree supply

[Liam Breck]

[dropped some CCs]


On Sun, Apr 30, 2017 at 1:37 PM, Paul Kocialkowski <contact@paulk.fr> wrote:
> This passes the of_node from the bq27xxx i2c battery driver to the
> common code, so that it can be registered and provide external supplies
> linked with device-tree.
>
> Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
> ---
>  drivers/power/supply/bq27xxx_battery.c     | 5 ++++-
>  drivers/power/supply/bq27xxx_battery_i2c.c | 1 +
>  include/linux/power/bq27xxx_battery.h      | 1 +
>  3 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> index 398801a21b86..6ef95442a918 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -1351,7 +1351,10 @@ static void bq27xxx_external_power_changed(struct power_supply *psy)
>  int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>  {
>         struct power_supply_desc *psy_desc;
> -       struct power_supply_config psy_cfg = { .drv_data = di, };
> +       struct power_supply_config psy_cfg = {};
> +
> +       psy_cfg.drv_data = di;
> +       psy_cfg.of_node = di->of_node;

I don't think you need di->of_node, just the following -- we do this
to obtain data from devicetree via power_supply_get_battery_info()
https://patchwork.kernel.org/patch/9692335/

struct power_supply_config psy_cfg = {
  .drv_data = di,
  .of_node = di->dev->of_node,
};

There is some (gcc?) bug on armv7 or omap which causes separate
assignments to corrupt the stack.

>         INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
>         mutex_init(&di->lock);
> diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c
> index c68fbc3fe50a..38a0422a4192 100644
> --- a/drivers/power/supply/bq27xxx_battery_i2c.c
> +++ b/drivers/power/supply/bq27xxx_battery_i2c.c
> @@ -96,6 +96,7 @@ static int bq27xxx_battery_i2c_probe(struct i2c_client *client,
>         di->chip = id->driver_data;
>         di->name = name;
>         di->bus.read = bq27xxx_battery_i2c_read;
> +       di->of_node = client->dev.of_node;
>
>         ret = bq27xxx_battery_setup(di);
>         if (ret)
> diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
> index b312bcef53da..94637b77ecbf 100644
> --- a/include/linux/power/bq27xxx_battery.h
> +++ b/include/linux/power/bq27xxx_battery.h
> @@ -63,6 +63,7 @@ struct bq27xxx_device_info {
>         const char *name;
>         struct bq27xxx_access_methods bus;
>         struct bq27xxx_reg_cache cache;
> +       struct device_node *of_node;
>         int charge_design_full;
>         unsigned long last_update;
>         struct delayed_work work;
> --
> 2.12.2
>

Re: [PATCH 4/5] power: supply: bq27xxx: Look for status change on external power change

[Liam Breck]

Hi Paul,

On Sun, Apr 30, 2017 at 1:38 PM, Paul Kocialkowski <contact@paulk.fr> wrote:
> This introduces a dedicated status change work to look for power
> status change. It is triggered by external power change notifications
> and periodically retries detecting a power status change for 5 seconds.
>
> This is largely inspired by a similar mechanism from the sbs-battery
> driver.

Maybe give an example which needs this? What can issue a notification,
under what circumstances?

Also why should poll_work no longer be rescheduled on notify?

> Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
> ---
>  drivers/power/supply/bq27xxx_battery.c | 38 ++++++++++++++++++++++++++++++++--
>  include/linux/power/bq27xxx_battery.h  |  3 +++
>  2 files changed, 39 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> index 926bd58344d9..cade00df6162 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -1190,6 +1190,11 @@ static int bq27xxx_battery_status(struct bq27xxx_device_info *di,
>                         status = POWER_SUPPLY_STATUS_CHARGING;
>         }
>
> +       if (di->status_retry == 0 && di->status_change_reference != status) {
> +               di->status_change_reference = status;
> +               power_supply_changed(di->bat);
> +       }
> +
>         val->intval = status;
>
>         return 0;
> @@ -1340,12 +1345,38 @@ static int bq27xxx_battery_get_property(struct power_supply *psy,
>         return ret;
>  }
>
> +static void bq27xxx_status_change(struct work_struct *work)
> +{
> +       struct bq27xxx_device_info *di =
> +                       container_of(work, struct bq27xxx_device_info,
> +                                    status_work.work);
> +       union power_supply_propval val;
> +       int ret;
> +
> +       bq27xxx_battery_update(di);
> +
> +       ret = bq27xxx_battery_status(di, &val);
> +       if (ret < 0)
> +               return;
> +
> +       if (di->status_change_reference != val.intval) {
> +               di->status_change_reference = val.intval;
> +               power_supply_changed(di->bat);
> +       }
> +
> +       if (di->status_retry > 0) {
> +               di->status_retry--;
> +               schedule_delayed_work(&di->status_work, HZ);
> +       }
> +}
> +
>  static void bq27xxx_external_power_changed(struct power_supply *psy)
>  {
>         struct bq27xxx_device_info *di = power_supply_get_drvdata(psy);
>
> -       cancel_delayed_work_sync(&di->poll_work);
> -       schedule_delayed_work(&di->poll_work, 0);
> +       cancel_delayed_work_sync(&di->status_work);
> +       schedule_delayed_work(&di->status_work, HZ);
> +       di->status_retry = 5;
>  }
>
>  int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
> @@ -1357,8 +1388,10 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>         psy_cfg.of_node = di->of_node;
>
>         INIT_DELAYED_WORK(&di->poll_work, bq27xxx_battery_poll);
> +       INIT_DELAYED_WORK(&di->status_work, bq27xxx_status_change);
>         mutex_init(&di->lock);
>         di->regs = bq27xxx_regs[di->chip];
> +       di->status_change_reference = POWER_SUPPLY_STATUS_UNKNOWN;
>
>         psy_desc = devm_kzalloc(di->dev, sizeof(*psy_desc), GFP_KERNEL);
>         if (!psy_desc)
> @@ -1400,6 +1433,7 @@ void bq27xxx_battery_teardown(struct bq27xxx_device_info *di)
>         poll_interval = 0;
>
>         cancel_delayed_work_sync(&di->poll_work);
> +       cancel_delayed_work_sync(&di->status_work);
>
>         power_supply_unregister(di->bat);
>
> diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
> index 0a9af513165a..16d604681ace 100644
> --- a/include/linux/power/bq27xxx_battery.h
> +++ b/include/linux/power/bq27xxx_battery.h
> @@ -67,6 +67,9 @@ struct bq27xxx_device_info {
>         int charge_design_full;
>         unsigned long last_update;
>         struct delayed_work poll_work;
> +       struct delayed_work status_work;
> +       int status_retry;
> +       int status_change_reference;
>         struct power_supply *bat;
>         struct list_head list;
>         struct mutex lock;
> --
> 2.12.2
>

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

[Liam Breck]

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

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

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

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

[Paul Kocialkowski]

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

Re: [PATCH 4/5] power: supply: bq27xxx: Look for status change on external power change

[Paul Kocialkowski]

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

Hi,

Le dimanche 30 avril 2017 à 15:13 -0700, Liam Breck a écrit :
> Hi Paul,
> 
> On Sun, Apr 30, 2017 at 1:38 PM, Paul Kocialkowski <contact@paulk.fr> wrote:
> > This introduces a dedicated status change work to look for power
> > status change. It is triggered by external power change notifications
> > and periodically retries detecting a power status change for 5 seconds.
> > 
> > This is largely inspired by a similar mechanism from the sbs-battery
> > driver.
> 
> Maybe give an example which needs this? What can issue a notification,
> under what circumstances?

The external_power_change function is called when a supplier to the battery (in
most cases, an external charger) detected a status change (e.g. plug/unplug)
which will affect the battery (the state should change from charging/full to
discharging).

For the best user experience, we need to detect that change ASAP. Retrieving the
status in the function directly is a bit too early, so a dedicated work is
called a second after the change and it tries to detect a status change ASAP.

With that mechanism, battery status changes due to plug/unplug take a few
seconds to be reflected.

> Also why should poll_work no longer be rescheduled on notify?

poll_work only updates the current charge, it's not in charge of detecting
status changes. The important point here is that the status change detection
work must be called seconds after the external power change (and actually, it
must keep running for a few seconds to detect quick charging->full transitions).

Is that a bit more clear?

> > Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
> > ---
> >  drivers/power/supply/bq27xxx_battery.c | 38
> > ++++++++++++++++++++++++++++++++--
> >  include/linux/power/bq27xxx_battery.h  |  3 +++
> >  2 files changed, 39 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/power/supply/bq27xxx_battery.c
> > b/drivers/power/supply/bq27xxx_battery.c
> > index 926bd58344d9..cade00df6162 100644
> > --- a/drivers/power/supply/bq27xxx_battery.c
> > +++ b/drivers/power/supply/bq27xxx_battery.c
> > @@ -1190,6 +1190,11 @@ static int bq27xxx_battery_status(struct
> > bq27xxx_device_info *di,
> >                         status = POWER_SUPPLY_STATUS_CHARGING;
> >         }
> > 
> > +       if (di->status_retry == 0 && di->status_change_reference != status)
> > {
> > +               di->status_change_reference = status;
> > +               power_supply_changed(di->bat);
> > +       }
> > +
> >         val->intval = status;
> > 
> >         return 0;
> > @@ -1340,12 +1345,38 @@ static int bq27xxx_battery_get_property(struct
> > power_supply *psy,
> >         return ret;
> >  }
> > 
> > +static void bq27xxx_status_change(struct work_struct *work)
> > +{
> > +       struct bq27xxx_device_info *di =
> > +                       container_of(work, struct bq27xxx_device_info,
> > +                                    status_work.work);
> > +       union power_supply_propval val;
> > +       int ret;
> > +
> > +       bq27xxx_battery_update(di);
> > +
> > +       ret = bq27xxx_battery_status(di, &val);
> > +       if (ret < 0)
> > +               return;
> > +
> > +       if (di->status_change_reference != val.intval) {
> > +               di->status_change_reference = val.intval;
> > +               power_supply_changed(di->bat);
> > +       }
> > +
> > +       if (di->status_retry > 0) {
> > +               di->status_retry--;
> > +               schedule_delayed_work(&di->status_work, HZ);
> > +       }
> > +}
> > +
> >  static void bq27xxx_external_power_changed(struct power_supply *psy)
> >  {
> >         struct bq27xxx_device_info *di = power_supply_get_drvdata(psy);
> > 
> > -       cancel_delayed_work_sync(&di->poll_work);
> > -       schedule_delayed_work(&di->poll_work, 0);
> > +       cancel_delayed_work_sync(&di->status_work);
> > +       schedule_delayed_work(&di->status_work, HZ);
> > +       di->status_retry = 5;
> >  }
> > 
> >  int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
> > @@ -1357,8 +1388,10 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info
> > *di)
> >         psy_cfg.of_node = di->of_node;
> > 
> >         INIT_DELAYED_WORK(&di->poll_work, bq27xxx_battery_poll);
> > +       INIT_DELAYED_WORK(&di->status_work, bq27xxx_status_change);
> >         mutex_init(&di->lock);
> >         di->regs = bq27xxx_regs[di->chip];
> > +       di->status_change_reference = POWER_SUPPLY_STATUS_UNKNOWN;
> > 
> >         psy_desc = devm_kzalloc(di->dev, sizeof(*psy_desc), GFP_KERNEL);
> >         if (!psy_desc)
> > @@ -1400,6 +1433,7 @@ void bq27xxx_battery_teardown(struct
> > bq27xxx_device_info *di)
> >         poll_interval = 0;
> > 
> >         cancel_delayed_work_sync(&di->poll_work);
> > +       cancel_delayed_work_sync(&di->status_work);
> > 
> >         power_supply_unregister(di->bat);
> > 
> > diff --git a/include/linux/power/bq27xxx_battery.h
> > b/include/linux/power/bq27xxx_battery.h
> > index 0a9af513165a..16d604681ace 100644
> > --- a/include/linux/power/bq27xxx_battery.h
> > +++ b/include/linux/power/bq27xxx_battery.h
> > @@ -67,6 +67,9 @@ struct bq27xxx_device_info {
> >         int charge_design_full;
> >         unsigned long last_update;
> >         struct delayed_work poll_work;
> > +       struct delayed_work status_work;
> > +       int status_retry;
> > +       int status_change_reference;
> >         struct power_supply *bat;
> >         struct list_head list;
> >         struct mutex lock;
> > --
> > 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 --]

Re: [PATCH 1/5] power: supply: bq27xxx: Pass of_node along to allow device-tree supply

[Paul Kocialkowski]

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

Le dimanche 30 avril 2017 à 15:03 -0700, Liam Breck a écrit :
> [dropped some CCs]
> 
> 
> On Sun, Apr 30, 2017 at 1:37 PM, Paul Kocialkowski <contact@paulk.fr> wrote:
> > This passes the of_node from the bq27xxx i2c battery driver to the
> > common code, so that it can be registered and provide external supplies
> > linked with device-tree.
> > 
> > Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
> > ---
> >  drivers/power/supply/bq27xxx_battery.c     | 5 ++++-
> >  drivers/power/supply/bq27xxx_battery_i2c.c | 1 +
> >  include/linux/power/bq27xxx_battery.h      | 1 +
> >  3 files changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/power/supply/bq27xxx_battery.c
> > b/drivers/power/supply/bq27xxx_battery.c
> > index 398801a21b86..6ef95442a918 100644
> > --- a/drivers/power/supply/bq27xxx_battery.c
> > +++ b/drivers/power/supply/bq27xxx_battery.c
> > @@ -1351,7 +1351,10 @@ static void bq27xxx_external_power_changed(struct
> > power_supply *psy)
> >  int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
> >  {
> >         struct power_supply_desc *psy_desc;
> > -       struct power_supply_config psy_cfg = { .drv_data = di, };
> > +       struct power_supply_config psy_cfg = {};
> > +
> > +       psy_cfg.drv_data = di;
> > +       psy_cfg.of_node = di->of_node;
> 
> I don't think you need di->of_node, just the following -- we do this
> to obtain data from devicetree via power_supply_get_battery_info()
> https://patchwork.kernel.org/patch/9692335/
> 
> struct power_supply_config psy_cfg = {
>   .drv_data = di,
>   .of_node = di->dev->of_node,
> };

That's a good point, thanks! Should I prepare v2 with this or rebase on top of
the series you mentioned previously?

> There is some (gcc?) bug on armv7 or omap which causes separate
> assignments to corrupt the stack.

That's weird, but definitely good to know.

> >         INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
> >         mutex_init(&di->lock);
> > diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c
> > b/drivers/power/supply/bq27xxx_battery_i2c.c
> > index c68fbc3fe50a..38a0422a4192 100644
> > --- a/drivers/power/supply/bq27xxx_battery_i2c.c
> > +++ b/drivers/power/supply/bq27xxx_battery_i2c.c
> > @@ -96,6 +96,7 @@ static int bq27xxx_battery_i2c_probe(struct i2c_client
> > *client,
> >         di->chip = id->driver_data;
> >         di->name = name;
> >         di->bus.read = bq27xxx_battery_i2c_read;
> > +       di->of_node = client->dev.of_node;
> > 
> >         ret = bq27xxx_battery_setup(di);
> >         if (ret)
> > diff --git a/include/linux/power/bq27xxx_battery.h
> > b/include/linux/power/bq27xxx_battery.h
> > index b312bcef53da..94637b77ecbf 100644
> > --- a/include/linux/power/bq27xxx_battery.h
> > +++ b/include/linux/power/bq27xxx_battery.h
> > @@ -63,6 +63,7 @@ struct bq27xxx_device_info {
> >         const char *name;
> >         struct bq27xxx_access_methods bus;
> >         struct bq27xxx_reg_cache cache;
> > +       struct device_node *of_node;
> >         int charge_design_full;
> >         unsigned long last_update;
> >         struct delayed_work work;
> > --
> > 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 --]

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

[Liam Breck]

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?

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

Re: [PATCH 4/5] power: supply: bq27xxx: Look for status change on external power change

[Liam Breck]

On Mon, May 1, 2017 at 3:45 AM, Paul Kocialkowski <contact@paulk.fr> wrote:
> Hi,
>
> Le dimanche 30 avril 2017 à 15:13 -0700, Liam Breck a écrit :
>> Hi Paul,
>>
>> On Sun, Apr 30, 2017 at 1:38 PM, Paul Kocialkowski <contact@paulk.fr> wrote:
>> > This introduces a dedicated status change work to look for power
>> > status change. It is triggered by external power change notifications
>> > and periodically retries detecting a power status change for 5 seconds.
>> >
>> > This is largely inspired by a similar mechanism from the sbs-battery
>> > driver.
>>
>> Maybe give an example which needs this? What can issue a notification,
>> under what circumstances?
>
> The external_power_change function is called when a supplier to the battery (in
> most cases, an external charger) detected a status change (e.g. plug/unplug)
> which will affect the battery (the state should change from charging/full to
> discharging).
>
> For the best user experience, we need to detect that change ASAP. Retrieving the
> status in the function directly is a bit too early, so a dedicated work is
> called a second after the change and it tries to detect a status change ASAP.
>
> With that mechanism, battery status changes due to plug/unplug take a few
> seconds to be reflected.
>
>> Also why should poll_work no longer be rescheduled on notify?
>
> poll_work only updates the current charge, it's not in charge of detecting
> status changes. The important point here is that the status change detection
> work must be called seconds after the external power change (and actually, it
> must keep running for a few seconds to detect quick charging->full transitions).
>
> Is that a bit more clear?

Yes, can you add that to patch description?


>> > Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
>> > ---
>> >  drivers/power/supply/bq27xxx_battery.c | 38
>> > ++++++++++++++++++++++++++++++++--
>> >  include/linux/power/bq27xxx_battery.h  |  3 +++
>> >  2 files changed, 39 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/power/supply/bq27xxx_battery.c
>> > b/drivers/power/supply/bq27xxx_battery.c
>> > index 926bd58344d9..cade00df6162 100644
>> > --- a/drivers/power/supply/bq27xxx_battery.c
>> > +++ b/drivers/power/supply/bq27xxx_battery.c
>> > @@ -1190,6 +1190,11 @@ static int bq27xxx_battery_status(struct
>> > bq27xxx_device_info *di,
>> >                         status = POWER_SUPPLY_STATUS_CHARGING;
>> >         }
>> >
>> > +       if (di->status_retry == 0 && di->status_change_reference != status)
>> > {
>> > +               di->status_change_reference = status;
>> > +               power_supply_changed(di->bat);
>> > +       }
>> > +
>> >         val->intval = status;
>> >
>> >         return 0;
>> > @@ -1340,12 +1345,38 @@ static int bq27xxx_battery_get_property(struct
>> > power_supply *psy,
>> >         return ret;
>> >  }
>> >
>> > +static void bq27xxx_status_change(struct work_struct *work)
>> > +{
>> > +       struct bq27xxx_device_info *di =
>> > +                       container_of(work, struct bq27xxx_device_info,
>> > +                                    status_work.work);
>> > +       union power_supply_propval val;
>> > +       int ret;
>> > +
>> > +       bq27xxx_battery_update(di);
>> > +
>> > +       ret = bq27xxx_battery_status(di, &val);
>> > +       if (ret < 0)
>> > +               return;
>> > +
>> > +       if (di->status_change_reference != val.intval) {
>> > +               di->status_change_reference = val.intval;
>> > +               power_supply_changed(di->bat);
>> > +       }
>> > +
>> > +       if (di->status_retry > 0) {
>> > +               di->status_retry--;
>> > +               schedule_delayed_work(&di->status_work, HZ);
>> > +       }
>> > +}
>> > +
>> >  static void bq27xxx_external_power_changed(struct power_supply *psy)
>> >  {
>> >         struct bq27xxx_device_info *di = power_supply_get_drvdata(psy);
>> >
>> > -       cancel_delayed_work_sync(&di->poll_work);
>> > -       schedule_delayed_work(&di->poll_work, 0);
>> > +       cancel_delayed_work_sync(&di->status_work);
>> > +       schedule_delayed_work(&di->status_work, HZ);
>> > +       di->status_retry = 5;
>> >  }
>> >
>> >  int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>> > @@ -1357,8 +1388,10 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info
>> > *di)
>> >         psy_cfg.of_node = di->of_node;
>> >
>> >         INIT_DELAYED_WORK(&di->poll_work, bq27xxx_battery_poll);
>> > +       INIT_DELAYED_WORK(&di->status_work, bq27xxx_status_change);
>> >         mutex_init(&di->lock);
>> >         di->regs = bq27xxx_regs[di->chip];
>> > +       di->status_change_reference = POWER_SUPPLY_STATUS_UNKNOWN;
>> >
>> >         psy_desc = devm_kzalloc(di->dev, sizeof(*psy_desc), GFP_KERNEL);
>> >         if (!psy_desc)
>> > @@ -1400,6 +1433,7 @@ void bq27xxx_battery_teardown(struct
>> > bq27xxx_device_info *di)
>> >         poll_interval = 0;
>> >
>> >         cancel_delayed_work_sync(&di->poll_work);
>> > +       cancel_delayed_work_sync(&di->status_work);
>> >
>> >         power_supply_unregister(di->bat);
>> >
>> > diff --git a/include/linux/power/bq27xxx_battery.h
>> > b/include/linux/power/bq27xxx_battery.h
>> > index 0a9af513165a..16d604681ace 100644
>> > --- a/include/linux/power/bq27xxx_battery.h
>> > +++ b/include/linux/power/bq27xxx_battery.h
>> > @@ -67,6 +67,9 @@ struct bq27xxx_device_info {
>> >         int charge_design_full;
>> >         unsigned long last_update;
>> >         struct delayed_work poll_work;
>> > +       struct delayed_work status_work;
>> > +       int status_retry;
>> > +       int status_change_reference;
>> >         struct power_supply *bat;
>> >         struct list_head list;
>> >         struct mutex lock;
>> > --
>> > 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/

Re: [PATCH 1/5] power: supply: bq27xxx: Pass of_node along to allow device-tree supply

[Liam Breck]

On Mon, May 1, 2017 at 3:46 AM, Paul Kocialkowski <contact@paulk.fr> wrote:
> Le dimanche 30 avril 2017 à 15:03 -0700, Liam Breck a écrit :
>> [dropped some CCs]
>>
>>
>> On Sun, Apr 30, 2017 at 1:37 PM, Paul Kocialkowski <contact@paulk.fr> wrote:
>> > This passes the of_node from the bq27xxx i2c battery driver to the
>> > common code, so that it can be registered and provide external supplies
>> > linked with device-tree.
>> >
>> > Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
>> > ---
>> >  drivers/power/supply/bq27xxx_battery.c     | 5 ++++-
>> >  drivers/power/supply/bq27xxx_battery_i2c.c | 1 +
>> >  include/linux/power/bq27xxx_battery.h      | 1 +
>> >  3 files changed, 6 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/power/supply/bq27xxx_battery.c
>> > b/drivers/power/supply/bq27xxx_battery.c
>> > index 398801a21b86..6ef95442a918 100644
>> > --- a/drivers/power/supply/bq27xxx_battery.c
>> > +++ b/drivers/power/supply/bq27xxx_battery.c
>> > @@ -1351,7 +1351,10 @@ static void bq27xxx_external_power_changed(struct
>> > power_supply *psy)
>> >  int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>> >  {
>> >         struct power_supply_desc *psy_desc;
>> > -       struct power_supply_config psy_cfg = { .drv_data = di, };
>> > +       struct power_supply_config psy_cfg = {};
>> > +
>> > +       psy_cfg.drv_data = di;
>> > +       psy_cfg.of_node = di->of_node;
>>
>> I don't think you need di->of_node, just the following -- we do this
>> to obtain data from devicetree via power_supply_get_battery_info()
>> https://patchwork.kernel.org/patch/9692335/
>>
>> struct power_supply_config psy_cfg = {
>>   .drv_data = di,
>>   .of_node = di->dev->of_node,
>> };
>
> That's a good point, thanks! Should I prepare v2 with this or rebase on top of
> the series you mentioned previously?

Looks like Sebastian will queue my series this week (actually he just
did, but there's a later rev coming, so wait for that).

Also it would be nice to have docs in the DT binding on linking an
external power supply to fuel gauge. If you write up an example, I can
incorporate it into the docs patch in my series. Or you can file it as
a separate patch...

>> There is some (gcc?) bug on armv7 or omap which causes separate
>> assignments to corrupt the stack.
>
> That's weird, but definitely good to know.
>
>> >         INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
>> >         mutex_init(&di->lock);
>> > diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c
>> > b/drivers/power/supply/bq27xxx_battery_i2c.c
>> > index c68fbc3fe50a..38a0422a4192 100644
>> > --- a/drivers/power/supply/bq27xxx_battery_i2c.c
>> > +++ b/drivers/power/supply/bq27xxx_battery_i2c.c
>> > @@ -96,6 +96,7 @@ static int bq27xxx_battery_i2c_probe(struct i2c_client
>> > *client,
>> >         di->chip = id->driver_data;
>> >         di->name = name;
>> >         di->bus.read = bq27xxx_battery_i2c_read;
>> > +       di->of_node = client->dev.of_node;
>> >
>> >         ret = bq27xxx_battery_setup(di);
>> >         if (ret)
>> > diff --git a/include/linux/power/bq27xxx_battery.h
>> > b/include/linux/power/bq27xxx_battery.h
>> > index b312bcef53da..94637b77ecbf 100644
>> > --- a/include/linux/power/bq27xxx_battery.h
>> > +++ b/include/linux/power/bq27xxx_battery.h
>> > @@ -63,6 +63,7 @@ struct bq27xxx_device_info {
>> >         const char *name;
>> >         struct bq27xxx_access_methods bus;
>> >         struct bq27xxx_reg_cache cache;
>> > +       struct device_node *of_node;
>> >         int charge_design_full;
>> >         unsigned long last_update;
>> >         struct delayed_work work;
>> > --
>> > 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/

Re: [PATCH 4/5] power: supply: bq27xxx: Look for status change on external power change

[Paul Kocialkowski]

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

Hi,

Le lundi 01 mai 2017 à 11:22 -0700, Liam Breck a écrit :
> On Mon, May 1, 2017 at 3:45 AM, Paul Kocialkowski <contact@paulk.fr> wrote:
> > Hi,
> > 
> > Le dimanche 30 avril 2017 à 15:13 -0700, Liam Breck a écrit :
> > > Hi Paul,
> > > 
> > > On Sun, Apr 30, 2017 at 1:38 PM, Paul Kocialkowski <contact@paulk.fr>
> > > wrote:
> > > > This introduces a dedicated status change work to look for power
> > > > status change. It is triggered by external power change notifications
> > > > and periodically retries detecting a power status change for 5 seconds.
> > > > 
> > > > This is largely inspired by a similar mechanism from the sbs-battery
> > > > driver.
> > > 
> > > Maybe give an example which needs this? What can issue a notification,
> > > under what circumstances?
> > 
> > The external_power_change function is called when a supplier to the battery
> > (in
> > most cases, an external charger) detected a status change (e.g. plug/unplug)
> > which will affect the battery (the state should change from charging/full to
> > discharging).
> > 
> > For the best user experience, we need to detect that change ASAP. Retrieving
> > the
> > status in the function directly is a bit too early, so a dedicated work is
> > called a second after the change and it tries to detect a status change
> > ASAP.
> > 
> > With that mechanism, battery status changes due to plug/unplug take a few
> > seconds to be reflected.
> > 
> > > Also why should poll_work no longer be rescheduled on notify?
> > 
> > poll_work only updates the current charge, it's not in charge of detecting
> > status changes. The important point here is that the status change detection
> > work must be called seconds after the external power change (and actually,
> > it
> > must keep running for a few seconds to detect quick charging->full
> > transitions).
> > 
> > Is that a bit more clear?
> 
> Yes, can you add that to patch description?

Sure thing, will do that in v2.

Thanks!

> > > > Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
> > > > ---
> > > >  drivers/power/supply/bq27xxx_battery.c | 38
> > > > ++++++++++++++++++++++++++++++++--
> > > >  include/linux/power/bq27xxx_battery.h  |  3 +++
> > > >  2 files changed, 39 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/power/supply/bq27xxx_battery.c
> > > > b/drivers/power/supply/bq27xxx_battery.c
> > > > index 926bd58344d9..cade00df6162 100644
> > > > --- a/drivers/power/supply/bq27xxx_battery.c
> > > > +++ b/drivers/power/supply/bq27xxx_battery.c
> > > > @@ -1190,6 +1190,11 @@ static int bq27xxx_battery_status(struct
> > > > bq27xxx_device_info *di,
> > > >                         status = POWER_SUPPLY_STATUS_CHARGING;
> > > >         }
> > > > 
> > > > +       if (di->status_retry == 0 && di->status_change_reference !=
> > > > status)
> > > > {
> > > > +               di->status_change_reference = status;
> > > > +               power_supply_changed(di->bat);
> > > > +       }
> > > > +
> > > >         val->intval = status;
> > > > 
> > > >         return 0;
> > > > @@ -1340,12 +1345,38 @@ static int bq27xxx_battery_get_property(struct
> > > > power_supply *psy,
> > > >         return ret;
> > > >  }
> > > > 
> > > > +static void bq27xxx_status_change(struct work_struct *work)
> > > > +{
> > > > +       struct bq27xxx_device_info *di =
> > > > +                       container_of(work, struct bq27xxx_device_info,
> > > > +                                    status_work.work);
> > > > +       union power_supply_propval val;
> > > > +       int ret;
> > > > +
> > > > +       bq27xxx_battery_update(di);
> > > > +
> > > > +       ret = bq27xxx_battery_status(di, &val);
> > > > +       if (ret < 0)
> > > > +               return;
> > > > +
> > > > +       if (di->status_change_reference != val.intval) {
> > > > +               di->status_change_reference = val.intval;
> > > > +               power_supply_changed(di->bat);
> > > > +       }
> > > > +
> > > > +       if (di->status_retry > 0) {
> > > > +               di->status_retry--;
> > > > +               schedule_delayed_work(&di->status_work, HZ);
> > > > +       }
> > > > +}
> > > > +
> > > >  static void bq27xxx_external_power_changed(struct power_supply *psy)
> > > >  {
> > > >         struct bq27xxx_device_info *di = power_supply_get_drvdata(psy);
> > > > 
> > > > -       cancel_delayed_work_sync(&di->poll_work);
> > > > -       schedule_delayed_work(&di->poll_work, 0);
> > > > +       cancel_delayed_work_sync(&di->status_work);
> > > > +       schedule_delayed_work(&di->status_work, HZ);
> > > > +       di->status_retry = 5;
> > > >  }
> > > > 
> > > >  int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
> > > > @@ -1357,8 +1388,10 @@ int bq27xxx_battery_setup(struct
> > > > bq27xxx_device_info
> > > > *di)
> > > >         psy_cfg.of_node = di->of_node;
> > > > 
> > > >         INIT_DELAYED_WORK(&di->poll_work, bq27xxx_battery_poll);
> > > > +       INIT_DELAYED_WORK(&di->status_work, bq27xxx_status_change);
> > > >         mutex_init(&di->lock);
> > > >         di->regs = bq27xxx_regs[di->chip];
> > > > +       di->status_change_reference = POWER_SUPPLY_STATUS_UNKNOWN;
> > > > 
> > > >         psy_desc = devm_kzalloc(di->dev, sizeof(*psy_desc), GFP_KERNEL);
> > > >         if (!psy_desc)
> > > > @@ -1400,6 +1433,7 @@ void bq27xxx_battery_teardown(struct
> > > > bq27xxx_device_info *di)
> > > >         poll_interval = 0;
> > > > 
> > > >         cancel_delayed_work_sync(&di->poll_work);
> > > > +       cancel_delayed_work_sync(&di->status_work);
> > > > 
> > > >         power_supply_unregister(di->bat);
> > > > 
> > > > diff --git a/include/linux/power/bq27xxx_battery.h
> > > > b/include/linux/power/bq27xxx_battery.h
> > > > index 0a9af513165a..16d604681ace 100644
> > > > --- a/include/linux/power/bq27xxx_battery.h
> > > > +++ b/include/linux/power/bq27xxx_battery.h
> > > > @@ -67,6 +67,9 @@ struct bq27xxx_device_info {
> > > >         int charge_design_full;
> > > >         unsigned long last_update;
> > > >         struct delayed_work poll_work;
> > > > +       struct delayed_work status_work;
> > > > +       int status_retry;
> > > > +       int status_change_reference;
> > > >         struct power_supply *bat;
> > > >         struct list_head list;
> > > >         struct mutex lock;
> > > > --
> > > > 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 --]

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

[Paul Kocialkowski]

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

Re: [PATCH 1/5] power: supply: bq27xxx: Pass of_node along to allow device-tree supply

[Paul Kocialkowski]

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

Hi,

Le lundi 01 mai 2017 à 11:30 -0700, Liam Breck a écrit :
> On Mon, May 1, 2017 at 3:46 AM, Paul Kocialkowski <contact@paulk.fr> wrote:
> > Le dimanche 30 avril 2017 à 15:03 -0700, Liam Breck a écrit :
> > > [dropped some CCs]
> > > 
> > > 
> > > On Sun, Apr 30, 2017 at 1:37 PM, Paul Kocialkowski <contact@paulk.fr>
> > > wrote:
> > > > This passes the of_node from the bq27xxx i2c battery driver to the
> > > > common code, so that it can be registered and provide external supplies
> > > > linked with device-tree.
> > > > 
> > > > Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
> > > > ---
> > > >  drivers/power/supply/bq27xxx_battery.c     | 5 ++++-
> > > >  drivers/power/supply/bq27xxx_battery_i2c.c | 1 +
> > > >  include/linux/power/bq27xxx_battery.h      | 1 +
> > > >  3 files changed, 6 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/power/supply/bq27xxx_battery.c
> > > > b/drivers/power/supply/bq27xxx_battery.c
> > > > index 398801a21b86..6ef95442a918 100644
> > > > --- a/drivers/power/supply/bq27xxx_battery.c
> > > > +++ b/drivers/power/supply/bq27xxx_battery.c
> > > > @@ -1351,7 +1351,10 @@ static void bq27xxx_external_power_changed(struct
> > > > power_supply *psy)
> > > >  int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
> > > >  {
> > > >         struct power_supply_desc *psy_desc;
> > > > -       struct power_supply_config psy_cfg = { .drv_data = di, };
> > > > +       struct power_supply_config psy_cfg = {};
> > > > +
> > > > +       psy_cfg.drv_data = di;
> > > > +       psy_cfg.of_node = di->of_node;
> > > 
> > > I don't think you need di->of_node, just the following -- we do this
> > > to obtain data from devicetree via power_supply_get_battery_info()
> > > https://patchwork.kernel.org/patch/9692335/
> > > 
> > > struct power_supply_config psy_cfg = {
> > >   .drv_data = di,
> > >   .of_node = di->dev->of_node,
> > > };
> > 
> > That's a good point, thanks! Should I prepare v2 with this or rebase on top
> > of
> > the series you mentioned previously?
> 
> Looks like Sebastian will queue my series this week (actually he just
> did, but there's a later rev coming, so wait for that).

Good to know!

> Also it would be nice to have docs in the DT binding on linking an
> external power supply to fuel gauge. If you write up an example, I can
> incorporate it into the docs patch in my series. Or you can file it as
> a separate patch...

Hooking up external suppliers is handled by the supply core code and is already
described at: Documentation/devicetree/bindings/power/supply/power_supply.txt

I don't think there's a need to add this in every supply's dt documentation.

> > > There is some (gcc?) bug on armv7 or omap which causes separate
> > > assignments to corrupt the stack.
> > 
> > That's weird, but definitely good to know.
> > 
> > > >         INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
> > > >         mutex_init(&di->lock);
> > > > diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c
> > > > b/drivers/power/supply/bq27xxx_battery_i2c.c
> > > > index c68fbc3fe50a..38a0422a4192 100644
> > > > --- a/drivers/power/supply/bq27xxx_battery_i2c.c
> > > > +++ b/drivers/power/supply/bq27xxx_battery_i2c.c
> > > > @@ -96,6 +96,7 @@ static int bq27xxx_battery_i2c_probe(struct i2c_client
> > > > *client,
> > > >         di->chip = id->driver_data;
> > > >         di->name = name;
> > > >         di->bus.read = bq27xxx_battery_i2c_read;
> > > > +       di->of_node = client->dev.of_node;
> > > > 
> > > >         ret = bq27xxx_battery_setup(di);
> > > >         if (ret)
> > > > diff --git a/include/linux/power/bq27xxx_battery.h
> > > > b/include/linux/power/bq27xxx_battery.h
> > > > index b312bcef53da..94637b77ecbf 100644
> > > > --- a/include/linux/power/bq27xxx_battery.h
> > > > +++ b/include/linux/power/bq27xxx_battery.h
> > > > @@ -63,6 +63,7 @@ struct bq27xxx_device_info {
> > > >         const char *name;
> > > >         struct bq27xxx_access_methods bus;
> > > >         struct bq27xxx_reg_cache cache;
> > > > +       struct device_node *of_node;
> > > >         int charge_design_full;
> > > >         unsigned long last_update;
> > > >         struct delayed_work work;
> > > > --
> > > > 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 --]

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

[Sebastian Reichel]

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

Hi,

On Fri, Jun 09, 2017 at 09:16:01AM +0300, Paul Kocialkowski wrote:
> Le jeudi 08 juin 2017 à 21:27 +0200, Sebastian Reichel a écrit :
> > Hi,
> > 
> > On Thu, Jun 08, 2017 at 01:08:52PM +0300, Paul Kocialkowski wrote:
> > > > > On Wed, 2017-06-07 at 09:52 +0200, Pavel Machek wrote:
> > > > > > > [0]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.
> > > > > > > git/
> > > > > > > comm
> > > > > > > it/?
> > > > > > > h=v4.12-rc4&id=7f93e1fa032bb5ee19b868b9649bc98c82553003
> > > > > > 
> > > > > > Is there some documentation that explains what different power supply
> > > > > > statuses mean? Because without that, we can have long and useless
> > > > > > discussions.
> > > > > 
> > > > > Well, I couldn't really find much except the following from
> > > > > Documentation/
> > > > > (which is not that helpful, and the BATTERY_STATUS_* don't seem to exist
> > > > > anymore):
> > > > > 
> > > > > " STATUS - this attribute represents operating status (charging, full,
> > > > > discharging (i.e. powering a load), etc.). This corresponds to
> > > > > BATTERY_STATUS_* values, as defined in battery.h. "
> > > > > 
> > > > > Generally speaking, I think the question to be asked is what information
> > > > > users
> > > > > will be interested in in each scenario we have to consider.
> > > > 
> > > > Hmm. We really should add some documentation :-(.
> > > 
> > > Maybe we should start a new thread about this to give it more visibility.
> > > That way, PM maintainers could weigh-in and share thoughts.
> > > 
> > > I definitely agree there is a need to clarify what we want to report to
> > > userspace given the various scenarios we've been discussing.
> > 
> > +1 for extension and update of documentation. If its known, that
> > the battery is trickle charged, it should report FULL. No need
> > to annoy people by constantly updating the status. Think of it
> > being mapped directly to a status LED. Of course the CURRENT/ENERGY
> > properties should still be updated, so that anyone interested in
> > the details can see them.
> 
> Agreed. Do ou think there is a need to start a specific discussion about
> various scenarios and how to handle them or do shall we just use common sense
> here?

I suggest to use common sense and if anything is unclear send a
documentation patch, so that we have a basis for discussion.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Paul Kocialkowski]

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

Le jeudi 08 juin 2017 à 21:27 +0200, Sebastian Reichel a écrit :
> Hi,
> 
> On Thu, Jun 08, 2017 at 01:08:52PM +0300, Paul Kocialkowski wrote:
> > > > On Wed, 2017-06-07 at 09:52 +0200, Pavel Machek wrote:
> > > > > > [0]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.
> > > > > > git/
> > > > > > comm
> > > > > > it/?
> > > > > > h=v4.12-rc4&id=7f93e1fa032bb5ee19b868b9649bc98c82553003
> > > > > 
> > > > > Is there some documentation that explains what different power supply
> > > > > statuses mean? Because without that, we can have long and useless
> > > > > discussions.
> > > > 
> > > > Well, I couldn't really find much except the following from
> > > > Documentation/
> > > > (which is not that helpful, and the BATTERY_STATUS_* don't seem to exist
> > > > anymore):
> > > > 
> > > > " STATUS - this attribute represents operating status (charging, full,
> > > > discharging (i.e. powering a load), etc.). This corresponds to
> > > > BATTERY_STATUS_* values, as defined in battery.h. "
> > > > 
> > > > Generally speaking, I think the question to be asked is what information
> > > > users
> > > > will be interested in in each scenario we have to consider.
> > > 
> > > Hmm. We really should add some documentation :-(.
> > 
> > Maybe we should start a new thread about this to give it more visibility.
> > That way, PM maintainers could weigh-in and share thoughts.
> > 
> > I definitely agree there is a need to clarify what we want to report to
> > userspace given the various scenarios we've been discussing.
> 
> +1 for extension and update of documentation. If its known, that
> the battery is trickle charged, it should report FULL. No need
> to annoy people by constantly updating the status. Think of it
> being mapped directly to a status LED. Of course the CURRENT/ENERGY
> properties should still be updated, so that anyone interested in
> the details can see them.

Agreed. Do ou think there is a need to start a specific discussion about
various scenarios and how to handle them or do shall we just use common sense
here?

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

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

[Sebastian Reichel]

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

Hi,

On Thu, Jun 08, 2017 at 01:08:52PM +0300, Paul Kocialkowski wrote:
> > > On Wed, 2017-06-07 at 09:52 +0200, Pavel Machek wrote:
> > > > > [0]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
> > > > > comm
> > > > > it/?
> > > > > h=v4.12-rc4&id=7f93e1fa032bb5ee19b868b9649bc98c82553003
> > > > 
> > > > Is there some documentation that explains what different power supply
> > > > statuses mean? Because without that, we can have long and useless
> > > > discussions.
> > > 
> > > Well, I couldn't really find much except the following from Documentation/
> > > (which is not that helpful, and the BATTERY_STATUS_* don't seem to exist
> > > anymore):
> > > 
> > > " STATUS - this attribute represents operating status (charging, full,
> > > discharging (i.e. powering a load), etc.). This corresponds to
> > > BATTERY_STATUS_* values, as defined in battery.h. "
> > > 
> > > Generally speaking, I think the question to be asked is what information
> > > users
> > > will be interested in in each scenario we have to consider.
> > 
> > Hmm. We really should add some documentation :-(.
> 
> Maybe we should start a new thread about this to give it more visibility.
> That way, PM maintainers could weigh-in and share thoughts.
> 
> I definitely agree there is a need to clarify what we want to report to
> userspace given the various scenarios we've been discussing.

+1 for extension and update of documentation. If its known, that
the battery is trickle charged, it should report FULL. No need
to annoy people by constantly updating the status. Think of it
being mapped directly to a status LED. Of course the CURRENT/ENERGY
properties should still be updated, so that anyone interested in
the details can see them.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Paul Kocialkowski]

Hey,

On Wed, 2017-06-07 at 21:50 +0200, Pavel Machek wrote:
> Hi!
> 
> > On Wed, 2017-06-07 at 09:52 +0200, Pavel Machek wrote:
> > > > [0]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
> > > > comm
> > > > it/?
> > > > h=v4.12-rc4&id=7f93e1fa032bb5ee19b868b9649bc98c82553003
> > > 
> > > Is there some documentation that explains what different power supply
> > > statuses mean? Because without that, we can have long and useless
> > > discussions.
> > 
> > Well, I couldn't really find much except the following from Documentation/
> > (which is not that helpful, and the BATTERY_STATUS_* don't seem to exist
> > anymore):
> > 
> > " STATUS - this attribute represents operating status (charging, full,
> > discharging (i.e. powering a load), etc.). This corresponds to
> > BATTERY_STATUS_* values, as defined in battery.h. "
> > 
> > Generally speaking, I think the question to be asked is what information
> > users
> > will be interested in in each scenario we have to consider.
> 
> Hmm. We really should add some documentation :-(.

Maybe we should start a new thread about this to give it more visibility.
That way, PM maintainers could weigh-in and share thoughts.

I definitely agree there is a need to clarify what we want to report to
userspace given the various scenarios we've been discussing.

> > > If you have 40Wh battery, and you are charging it with 1mW, I don't
> > > believe you should be indicating "charging". That battery is
> > > full. Yes, even full batteries are sometimes charged with very low
> > > currents to keep them full.
> > 
> > That makes sense. Note that this patch was however designed to solve the
> > problem
> > the other way round: my device will report full battery when the PSU was
> > disconnected and that it is, in fact, drawing significant current.
> 
> That is documented / correct behaviour sometimes. Thinkpad batteries
> have thresholds -- lets say 100% and 95%. They charge battery to full
> (as expected), but then they won't start charging battery again unless
> it drops below 95%. So you can have "battery full, charger
> disconnected" state.
> 
> [Design like this prolongs longevity of li-ion batteries.]

That is definitely good to know.

> > > And I'm not sure what this is supposed to do, but its quite strange
> > > code.
> > 
> > Could you comment on what is strange about it? This function corrects the
> > status
> > based on the current flow as explained through this thread.
> > 
> > > +static int sbs_status_correct(struct i2c_client *client, int *intval)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = sbs_read_word_data(client, sbs_data[REG_CURRENT].addr);
> > > +	if (ret < 0)
> > > +	   return ret;
> > > +
> > > +	ret = (s16)ret;
> 
> The last line ... is strange.

The trick here is that sbs_read_word_data will return a negative value (on 32
bits) when an error (say, I/O related) happens, but the current (returned
directly by the call) can also have a legit negative value (current draw).

However, the current value is stored on 16 bytes, so the trick is the use the
upper 2 remaining bytes for error reporting and if there's no error, cast the
value to a signed 16-bit value to get the (legit) signed current value.

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

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

[Pavel Machek]

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

Hi!

> On Wed, 2017-06-07 at 09:52 +0200, Pavel Machek wrote:
> > > [0]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/comm
> > > it/?
> > > h=v4.12-rc4&id=7f93e1fa032bb5ee19b868b9649bc98c82553003
> > 
> > Is there some documentation that explains what different power supply
> > statuses mean? Because without that, we can have long and useless
> > discussions.
> 
> Well, I couldn't really find much except the following from Documentation/
> (which is not that helpful, and the BATTERY_STATUS_* don't seem to exist
> anymore):
> 
> " STATUS - this attribute represents operating status (charging, full,
> discharging (i.e. powering a load), etc.). This corresponds to
> BATTERY_STATUS_* values, as defined in battery.h. "
> 
> Generally speaking, I think the question to be asked is what information users
> will be interested in in each scenario we have to consider.

Hmm. We really should add some documentation :-(.

> > If you have 40Wh battery, and you are charging it with 1mW, I don't
> > believe you should be indicating "charging". That battery is
> > full. Yes, even full batteries are sometimes charged with very low
> > currents to keep them full.
> 
> That makes sense. Note that this patch was however designed to solve the problem
> the other way round: my device will report full battery when the PSU was
> disconnected and that it is, in fact, drawing significant current.

That is documented / correct behaviour sometimes. Thinkpad batteries
have thresholds -- lets say 100% and 95%. They charge battery to full
(as expected), but then they won't start charging battery again unless
it drops below 95%. So you can have "battery full, charger
disconnected" state.

[Design like this prolongs longevity of li-ion batteries.]

> > And I'm not sure what this is supposed to do, but its quite strange
> > code.
> 
> Could you comment on what is strange about it? This function corrects the status
> based on the current flow as explained through this thread.
> 
> > +static int sbs_status_correct(struct i2c_client *client, int *intval)
> > +{
> > +	int ret;
> > +
> > +	ret = sbs_read_word_data(client, sbs_data[REG_CURRENT].addr);
> > +	if (ret < 0)
> > +	   return ret;
> > +
> > +	ret = (s16)ret;

The last line ... is strange.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Paul Kocialkowski]

Hey,

On Wed, 2017-06-07 at 09:52 +0200, Pavel Machek wrote:
> I'd pass to userspace what the controller reports. Yes, I seldom see
> > > > "STATUS_FULL" but that may be a problem we need to track down.
> > > 
> > > The controller is known, from my experience, to not be reliable in that
> > > regard,
> > > so I don't think it makes sense to pass a state that doesn't reflect the
> > > actual
> > > state of charging just because the chip tells us so.
> > > 
> > > Worst case, we could also have a dt property to enable that kind of fixup
> > > workaround and let every device maintainer decide whether it is relevant
> > > for
> > > their device.
> > 
> > Actually, since a similar fix[0] was accepted in sbs-battery, I'd rather not
> > make this optional but rather make it the default and perhaps have a dt prop
> > to
> > disable it.
> > 
> > [0]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/comm
> > it/?
> > h=v4.12-rc4&id=7f93e1fa032bb5ee19b868b9649bc98c82553003
> 
> Is there some documentation that explains what different power supply
> statuses mean? Because without that, we can have long and useless
> discussions.

Well, I couldn't really find much except the following from Documentation/
(which is not that helpful, and the BATTERY_STATUS_* don't seem to exist
anymore):

" STATUS - this attribute represents operating status (charging, full,
discharging (i.e. powering a load), etc.). This corresponds to
BATTERY_STATUS_* values, as defined in battery.h. "

Generally speaking, I think the question to be asked is what information users
will be interested in in each scenario we have to consider.

> If you have 40Wh battery, and you are charging it with 1mW, I don't
> believe you should be indicating "charging". That battery is
> full. Yes, even full batteries are sometimes charged with very low
> currents to keep them full.

That makes sense. Note that this patch was however designed to solve the problem
the other way round: my device will report full battery when the PSU was
disconnected and that it is, in fact, drawing significant current.

> And I'm not sure what this is supposed to do, but its quite strange
> code.

Could you comment on what is strange about it? This function corrects the status
based on the current flow as explained through this thread.

> +static int sbs_status_correct(struct i2c_client *client, int *intval)
> +{
> +	int ret;
> +
> +	ret = sbs_read_word_data(client, sbs_data[REG_CURRENT].addr);
> +	if (ret < 0)
> +	   return ret;
> +
> +	ret = (s16)ret;
> +	
-- 
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/

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

[Pavel Machek]

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

Hi!

> > > I'd pass to userspace what the controller reports. Yes, I seldom see
> > > "STATUS_FULL" but that may be a problem we need to track down.
> > 
> > The controller is known, from my experience, to not be reliable in that
> > regard,
> > so I don't think it makes sense to pass a state that doesn't reflect the
> > actual
> > state of charging just because the chip tells us so.
> > 
> > Worst case, we could also have a dt property to enable that kind of fixup
> > workaround and let every device maintainer decide whether it is relevant for
> > their device.
> 
> Actually, since a similar fix[0] was accepted in sbs-battery, I'd rather not
> make this optional but rather make it the default and perhaps have a dt prop to
> disable it.
> 
> [0]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?
> h=v4.12-rc4&id=7f93e1fa032bb5ee19b868b9649bc98c82553003

Is there some documentation that explains what different power supply
statuses mean? Because without that, we can have long and useless
discussions.

If you have 40Wh battery, and you are charging it with 1mW, I don't
believe you should be indicating "charging". That battery is
full. Yes, even full batteries are sometimes charged with very low
currents to keep them full.

And I'm not sure what this is supposed to do, but its quite strange
code.

+static int sbs_status_correct(struct i2c_client *client, int *intval)
+{
+	int ret;
+
+	ret = sbs_read_word_data(client, sbs_data[REG_CURRENT].addr);
+	if (ret < 0)
+	   return ret;
+
+	ret = (s16)ret;
+

Best regards,
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Paul Kocialkowski]

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

Le mercredi 31 mai 2017 à 21:28 +0200, Paul Kocialkowski a écrit :
> Hi,
> 
> Le mercredi 31 mai 2017 à 19:32 +0200, Pavel Machek a écrit :
> > The status reported directly by the battery controller is not always
> > > > > reliable and should be corrected based on the current draw
> > > > > information.
> > > > > 
> > > > > This implements such a correction with a dedicated function, called
> > > > > when retrieving the supply status.
> > > > > @@ -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;
> > > > 
> > > > Umm.
> > 
> > As in "two casts in one expression -- too ugly to live".
> 
> Oh, I had skipped that comment, sorry about that. Yeah I understand your
> concern. However, this line was mostly inspired by another part of the code,
> below the following comment: /* Other gauges return signed value */
> 
> I think we should fix the first occurence first and then used the fixed syntax
> in v2 of this patch. What do you think?
> 
> > > > > @@ -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;
> > > > > +	}
> > > > 
> > > > Are you sure this works? On N900, we normally see small currents to/from
> > > > "full" battery.
> > > 
> > > In my case, this works perfectly and I am quite surprised of what you're
> > > describing. Is it the case when the battery has a PSU connected?
> > 
> > "PSU"? This is cellphone. It has USB connection and charges from that.
> > 
> > It has been charging for long while now, and current_now fluctuates
> > between 20706 and -2856. USB has limitted current, so I guess "draw
> > current from battery if we need more than USB can provide" is quite common.
> 
> Ah right, I had forgotten about the USB current limitation thing. In this
> case,
> I guess the battery is never actually full and IMO, it should be reported as
> such.
> 
> > pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now
> > 5355
> > pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now
> > 5355
> > pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now
> > -4105
> > pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now
> > -4105
> > pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now
> > -7675
> > pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now
> > -5712
> > pavel@n900:~$ #screen on
> > pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now
> > 4641
> > pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now
> > 4641
> > pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now
> > 37842
> > pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now
> > 16600
> > pavel@n900:~$
> > 
> > > I guess I would consider this a hardware issue (leak currents) and we
> > > could
> > > definitely set some range (in device-tree) to distinguish between full +
> > > leak
> > > currents and bad reporting from the fuel gauge. That would work well in my
> > > case
> > > too.
> > 
> > I'd pass to userspace what the controller reports. Yes, I seldom see
> > "STATUS_FULL" but that may be a problem we need to track down.
> 
> The controller is known, from my experience, to not be reliable in that
> regard,
> so I don't think it makes sense to pass a state that doesn't reflect the
> actual
> state of charging just because the chip tells us so.
> 
> Worst case, we could also have a dt property to enable that kind of fixup
> workaround and let every device maintainer decide whether it is relevant for
> their device.

Actually, since a similar fix[0] was accepted in sbs-battery, I'd rather not
make this optional but rather make it the default and perhaps have a dt prop to
disable it.

> What do you think?

[0]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?
h=v4.12-rc4&id=7f93e1fa032bb5ee19b868b9649bc98c82553003
-- 
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 --]

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

[Paul Kocialkowski]

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

Hi,

Le mercredi 31 mai 2017 à 19:32 +0200, Pavel Machek a écrit :
> The status reported directly by the battery controller is not always
> > > > reliable and should be corrected based on the current draw information.
> > > > 
> > > > This implements such a correction with a dedicated function, called
> > > > when retrieving the supply status.
> > > > @@ -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;
> > > 
> > > Umm.
> 
> As in "two casts in one expression -- too ugly to live".

Oh, I had skipped that comment, sorry about that. Yeah I understand your
concern. However, this line was mostly inspired by another part of the code,
below the following comment: /* Other gauges return signed value */

I think we should fix the first occurence first and then used the fixed syntax
in v2 of this patch. What do you think?

> > > > @@ -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;
> > > > +	}
> > > 
> > > Are you sure this works? On N900, we normally see small currents to/from
> > > "full" battery.
> > 
> > In my case, this works perfectly and I am quite surprised of what you're
> > describing. Is it the case when the battery has a PSU connected?
> 
> "PSU"? This is cellphone. It has USB connection and charges from that.
> 
> It has been charging for long while now, and current_now fluctuates
> between 20706 and -2856. USB has limitted current, so I guess "draw
> current from battery if we need more than USB can provide" is quite common.

Ah right, I had forgotten about the USB current limitation thing. In this case,
I guess the battery is never actually full and IMO, it should be reported as
such.

> pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now
> 5355
> pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now
> 5355
> pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now
> -4105
> pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now
> -4105
> pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now
> -7675
> pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now
> -5712
> pavel@n900:~$ #screen on
> pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now
> 4641
> pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now
> 4641
> pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now
> 37842
> pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now
> 16600
> pavel@n900:~$
> 
> > I guess I would consider this a hardware issue (leak currents) and we could
> > definitely set some range (in device-tree) to distinguish between full +
> > leak
> > currents and bad reporting from the fuel gauge. That would work well in my
> > case
> > too.
> 
> I'd pass to userspace what the controller reports. Yes, I seldom see
> "STATUS_FULL" but that may be a problem we need to track down.

The controller is known, from my experience, to not be reliable in that regard,
so I don't think it makes sense to pass a state that doesn't reflect the actual
state of charging just because the chip tells us so.

Worst case, we could also have a dt property to enable that kind of fixup
workaround and let every device maintainer decide whether it is relevant for
their device.

What do you think?

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

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

[Pavel Machek]

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

Hi!

> > > The status reported directly by the battery controller is not always
> > > reliable and should be corrected based on the current draw information.
> > > 
> > > This implements such a correction with a dedicated function, called
> > > when retrieving the supply status.

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

As in "two casts in one expression -- too ugly to live".

> > > @@ -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;
> > > +	}
> > 
> > Are you sure this works? On N900, we normally see small currents to/from
> > "full" battery.
> 
> In my case, this works perfectly and I am quite surprised of what you're
> describing. Is it the case when the battery has a PSU connected?

"PSU"? This is cellphone. It has USB connection and charges from that.

It has been charging for long while now, and current_now fluctuates
between 20706 and -2856. USB has limitted current, so I guess "draw
current from battery if we need more than USB can provide" is quite common.

pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now
5355
pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now
5355
pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now
-4105
pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now
-4105
pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now
-7675
pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now
-5712
pavel@n900:~$ #screen on
pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now
4641
pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now
4641
pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now
37842
pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now
16600
pavel@n900:~$

> I guess I would consider this a hardware issue (leak currents) and we could
> definitely set some range (in device-tree) to distinguish between full + leak
> currents and bad reporting from the fuel gauge. That would work well in my case
> too.

I'd pass to userspace what the controller reports. Yes, I seldom see
"STATUS_FULL" but that may be a problem we need to track down.



       	  	 	     		 	      	    	    	 Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Paul Kocialkowski]

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

Hi,

Le dimanche 28 mai 2017 à 21:16 +0200, Pavel Machek a écrit :
> Hi!
> 
> > The status reported directly by the battery controller is not always
> > reliable and should be corrected based on the current draw information.
> > 
> > This implements such a correction with a dedicated function, called
> > when retrieving the supply status.
> > 
> > @@ -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;
> 
> Umm.
> 
> > @@ -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;
> > +	}
> 
> Are you sure this works? On N900, we normally see small currents to/from
> "full" battery.

In my case, this works perfectly and I am quite surprised of what you're
describing. Is it the case when the battery has a PSU connected?

I guess I would consider this a hardware issue (leak currents) and we could
definitely set some range (in device-tree) to distinguish between full + leak
currents and bad reporting from the fuel gauge. That would work well in my case
too.

> Should the test be for absolute_value(curr) < something rather than for == 0?
> 
> What hw did you test it on?

I tested this on nyan Chromebooks (Acer Chromebook 13 and HP Chromebook 11) as
well as veyron Chromebooks (Chromebook C201PA) that use a bq27xxx fuel gauge.

Cheers!

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

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

[Pavel Machek]

Hi!

> The status reported directly by the battery controller is not always
> reliable and should be corrected based on the current draw information.
> 
> This implements such a correction with a dedicated function, called
> when retrieving the supply status.
> 

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

Umm.

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

Are you sure this works? On N900, we normally see small currents to/from "full" battery.

Should the test be for absolute_value(curr) < something rather than for == 0?

What hw did you test it on?
										Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[Paul Kocialkowski]

The status reported directly by the battery controller is not always
reliable and should be corrected based on the current draw information.

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;
+	}
 
 	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