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

[Paul Kocialkowski <contact@paulk.fr>]

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