| Message ID | 20170430182727.24412-4-contact@paulk.fr (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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;
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; > >
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;
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;
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(-)