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 4/5] power: supply: bq27xxx: Look for status change on external power change

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 6040 bytes --]

On Sunday 07 May 2017 19:37:41 Paul Kocialkowski wrote:
> Hi,
> 
> Le vendredi 05 mai 2017 à 10:04 +0200, Pali Rohár a écrit :
> > On Sunday 30 April 2017 20:27:26 Paul Kocialkowski 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.
> > 
> > What is reason/motivation for such change? It should be written in
> > commit message (to help understand; not only for me), because
> > really I do not know why such patch is needed.
> 
> Well, I really don't understand how things can work out without this
> patch.
> 
> How exactly are external power notifications supposed to be handled
> with the current state of the driver?
> 
> From my tests, changes on external status change notifications were
> unreliable as they did not look for a status change at all and
> instead scheduled a poll work, which would not at any point call
> power_supply_changed, but instead update the status for later reads.
> 
> This is *not* the expected behavior. When an external power supply is
> connected/disconnected, the driver should detect the status change
> and report it as soon as detected. This is precisely what this patch
> does (as is done already with the sbs battery driver).
> 
> Note that it is up to *the driver* to report that status change, it
> must not wait for userspace to query whether something changed.
> 
> Does that clarify why this is needed?

Yea, now I see what is the reason... Problem is that bq27xxx does not 
provide any notification mechanism and we can only poll for changes.

Question is if timeout 5 seconds is always enough and what happen if 
not... Similarly if such additional wakeups does not increase 
cpu/battery/power usage on small embedded devices...

On Nokia N900 we have other HW with drivers which detect charger 
connection/disconnection and doing periodical polling for monitoring 
charge current and temperature.

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

-- 
Pali Rohár
pali.rohar@gmail.com

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

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

[Paul Kocialkowski]

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

Hi,

Le vendredi 05 mai 2017 à 10:04 +0200, Pali Rohár a écrit :
> On Sunday 30 April 2017 20:27:26 Paul Kocialkowski 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.
> 
> What is reason/motivation for such change? It should be written in
> commit message (to help understand; not only for me), because really I
> do not know why such patch is needed.

Well, I really don't understand how things can work out without this patch.

How exactly are external power notifications supposed to be handled with the
current state of the driver?

From my tests, changes on external status change notifications were unreliable
as they did not look for a status change at all and instead scheduled a poll
work, which would not at any point call power_supply_changed, but instead update
the status for later reads.

This is *not* the expected behavior. When an external power supply is
connected/disconnected, the driver should detect the status change and report it
as soon as detected. This is precisely what this patch does (as is done already
with the sbs battery driver).

Note that it is up to *the driver* to report that status change, it must not
wait for userspace to query whether something changed.

Does that clarify why this is needed?

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

[Pali Rohár]

On Sunday 30 April 2017 20:27:26 Paul Kocialkowski 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.

What is reason/motivation for such change? It should be written in
commit message (to help understand; not only for me), because really I
do not know why such patch is needed.

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

-- 
Pali Rohár
pali.rohar@gmail.com

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

[Paul Kocialkowski]

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.

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