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

[Paul Kocialkowski]

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@xxxxxxxx>
> > ---
> > Â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: signature.asc
Description: This is a digitally signed message part