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

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

[Paul Kocialkowski]

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

[PATCH 2/5] power: supply: bq27xxx: Register power supply with devm

[Paul Kocialkowski]

This uses the managed devices resources version of the
power_supply_register_no_ws function to register the power supply.

Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
---
 drivers/power/supply/bq27xxx_battery.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 6ef95442a918..be476e0bc85d 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -1371,7 +1371,7 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
 	psy_desc->get_property = bq27xxx_battery_get_property;
 	psy_desc->external_power_changed = bq27xxx_external_power_changed;
 
-	di->bat = power_supply_register_no_ws(di->dev, psy_desc, &psy_cfg);
+	di->bat = devm_power_supply_register_no_ws(di->dev, psy_desc, &psy_cfg);
 	if (IS_ERR(di->bat)) {
 		dev_err(di->dev, "failed to register battery\n");
 		return PTR_ERR(di->bat);
-- 
2.12.2

Re: [PATCH 2/5] power: supply: bq27xxx: Register power supply with devm

[Sebastian Reichel]

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

Hi,

On Sun, Apr 30, 2017 at 08:27:24PM +0200, Paul Kocialkowski wrote:
> This uses the managed devices resources version of the
> power_supply_register_no_ws function to register the power supply.
> 
> Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
> ---
>  drivers/power/supply/bq27xxx_battery.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> index 6ef95442a918..be476e0bc85d 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -1371,7 +1371,7 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>  	psy_desc->get_property = bq27xxx_battery_get_property;
>  	psy_desc->external_power_changed = bq27xxx_external_power_changed;
>  
> -	di->bat = power_supply_register_no_ws(di->dev, psy_desc, &psy_cfg);
> +	di->bat = devm_power_supply_register_no_ws(di->dev, psy_desc, &psy_cfg);
>  	if (IS_ERR(di->bat)) {
>  		dev_err(di->dev, "failed to register battery\n");
>  		return PTR_ERR(di->bat);

That does not make sense if bq27xxx_battery_teardown() still calls
power_supply_unregister().

-- Sebastian

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

Re: [PATCH 2/5] power: supply: bq27xxx: Register power supply with devm

[Paul Kocialkowski]

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

Le lundi 01 mai 2017 à 12:55 +0200, Sebastian Reichel a écrit :
> Hi,
> 
> On Sun, Apr 30, 2017 at 08:27:24PM +0200, Paul Kocialkowski wrote:
> > This uses the managed devices resources version of the
> > power_supply_register_no_ws function to register the power supply.
> > 
> > Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
> > ---
> >  drivers/power/supply/bq27xxx_battery.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/power/supply/bq27xxx_battery.c
> > b/drivers/power/supply/bq27xxx_battery.c
> > index 6ef95442a918..be476e0bc85d 100644
> > --- a/drivers/power/supply/bq27xxx_battery.c
> > +++ b/drivers/power/supply/bq27xxx_battery.c
> > @@ -1371,7 +1371,7 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info
> > *di)
> >  	psy_desc->get_property = bq27xxx_battery_get_property;
> >  	psy_desc->external_power_changed = bq27xxx_external_power_changed;
> >  
> > -	di->bat = power_supply_register_no_ws(di->dev, psy_desc, &psy_cfg);
> > +	di->bat = devm_power_supply_register_no_ws(di->dev, psy_desc,
> > &psy_cfg);
> >  	if (IS_ERR(di->bat)) {
> >  		dev_err(di->dev, "failed to register battery\n");
> >  		return PTR_ERR(di->bat);
> 
> That does not make sense if bq27xxx_battery_teardown() still calls
> power_supply_unregister().

That's a very good point, thanks! Will fix in v2.

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

[PATCH 3/5] power: supply: bq27xxx: Rename work structure member to poll_work

[Paul Kocialkowski]

This renames the work structure member to poll_work, in anticipation
of the introduction of a status_work member used to detect status
changes.

Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
---
 drivers/power/supply/bq27xxx_battery.c     | 20 ++++++++++----------
 drivers/power/supply/bq27xxx_battery_i2c.c |  2 +-
 include/linux/power/bq27xxx_battery.h      |  2 +-
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index be476e0bc85d..926bd58344d9 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -769,8 +769,8 @@ static int poll_interval_param_set(const char *val, const struct kernel_param *k
 
 	mutex_lock(&bq27xxx_list_lock);
 	list_for_each_entry(di, &bq27xxx_battery_devices, list) {
-		cancel_delayed_work_sync(&di->work);
-		schedule_delayed_work(&di->work, 0);
+		cancel_delayed_work_sync(&di->poll_work);
+		schedule_delayed_work(&di->poll_work, 0);
 	}
 	mutex_unlock(&bq27xxx_list_lock);
 
@@ -1126,12 +1126,12 @@ static void bq27xxx_battery_poll(struct work_struct *work)
 {
 	struct bq27xxx_device_info *di =
 			container_of(work, struct bq27xxx_device_info,
-				     work.work);
+				     poll_work.work);
 
 	bq27xxx_battery_update(di);
 
 	if (poll_interval > 0)
-		schedule_delayed_work(&di->work, poll_interval * HZ);
+		schedule_delayed_work(&di->poll_work, poll_interval * HZ);
 }
 
 /*
@@ -1265,8 +1265,8 @@ static int bq27xxx_battery_get_property(struct power_supply *psy,
 
 	mutex_lock(&di->lock);
 	if (time_is_before_jiffies(di->last_update + 5 * HZ)) {
-		cancel_delayed_work_sync(&di->work);
-		bq27xxx_battery_poll(&di->work.work);
+		cancel_delayed_work_sync(&di->poll_work);
+		bq27xxx_battery_poll(&di->poll_work.work);
 	}
 	mutex_unlock(&di->lock);
 
@@ -1344,8 +1344,8 @@ 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->work);
-	schedule_delayed_work(&di->work, 0);
+	cancel_delayed_work_sync(&di->poll_work);
+	schedule_delayed_work(&di->poll_work, 0);
 }
 
 int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
@@ -1356,7 +1356,7 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
 	psy_cfg.drv_data = di;
 	psy_cfg.of_node = di->of_node;
 
-	INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
+	INIT_DELAYED_WORK(&di->poll_work, bq27xxx_battery_poll);
 	mutex_init(&di->lock);
 	di->regs = bq27xxx_regs[di->chip];
 
@@ -1399,7 +1399,7 @@ void bq27xxx_battery_teardown(struct bq27xxx_device_info *di)
 	 */
 	poll_interval = 0;
 
-	cancel_delayed_work_sync(&di->work);
+	cancel_delayed_work_sync(&di->poll_work);
 
 	power_supply_unregister(di->bat);
 
diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c
index 38a0422a4192..1b2ad22190ae 100644
--- a/drivers/power/supply/bq27xxx_battery_i2c.c
+++ b/drivers/power/supply/bq27xxx_battery_i2c.c
@@ -103,7 +103,7 @@ static int bq27xxx_battery_i2c_probe(struct i2c_client *client,
 		goto err_failed;
 
 	/* Schedule a polling after about 1 min */
-	schedule_delayed_work(&di->work, 60 * HZ);
+	schedule_delayed_work(&di->poll_work, 60 * HZ);
 
 	i2c_set_clientdata(client, di);
 
diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
index 94637b77ecbf..0a9af513165a 100644
--- a/include/linux/power/bq27xxx_battery.h
+++ b/include/linux/power/bq27xxx_battery.h
@@ -66,7 +66,7 @@ struct bq27xxx_device_info {
 	struct device_node *of_node;
 	int charge_design_full;
 	unsigned long last_update;
-	struct delayed_work work;
+	struct delayed_work poll_work;
 	struct power_supply *bat;
 	struct list_head list;
 	struct mutex lock;
-- 
2.12.2

[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

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

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]

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

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

[Paul Kocialkowski]

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

This implements such a correction with a dedicated function, called
when retrieving the supply status.

Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
---
 drivers/power/supply/bq27xxx_battery.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index cade00df6162..f7694e775e68 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -1171,8 +1171,22 @@ static int bq27xxx_battery_status(struct bq27xxx_device_info *di,
 				  union power_supply_propval *val)
 {
 	int status;
+	int curr;
+	int flags;
+
+	curr = bq27xxx_read(di, BQ27XXX_REG_AI, false);
+	if (curr < 0) {
+		dev_err(di->dev, "error reading current\n");
+		return curr;
+	}
 
 	if (di->chip == BQ27000 || di->chip == BQ27010) {
+		flags = bq27xxx_read(di, BQ27XXX_REG_FLAGS, true);
+		if (flags & BQ27000_FLAG_CHGS) {
+			dev_dbg(di->dev, "negative current!\n");
+			curr = -curr;
+		}
+
 		if (di->cache.flags & BQ27000_FLAG_FC)
 			status = POWER_SUPPLY_STATUS_FULL;
 		else if (di->cache.flags & BQ27000_FLAG_CHGS)
@@ -1182,6 +1196,8 @@ static int bq27xxx_battery_status(struct bq27xxx_device_info *di,
 		else
 			status = POWER_SUPPLY_STATUS_DISCHARGING;
 	} else {
+		curr = (int)((s16)curr) * 1000;
+
 		if (di->cache.flags & BQ27XXX_FLAG_FC)
 			status = POWER_SUPPLY_STATUS_FULL;
 		else if (di->cache.flags & BQ27XXX_FLAG_DSC)
@@ -1190,6 +1206,18 @@ static int bq27xxx_battery_status(struct bq27xxx_device_info *di,
 			status = POWER_SUPPLY_STATUS_CHARGING;
 	}
 
+
+	if (curr == 0 && status != POWER_SUPPLY_STATUS_NOT_CHARGING)
+		status = POWER_SUPPLY_STATUS_FULL;
+
+	if (status == POWER_SUPPLY_STATUS_FULL) {
+		/* Drawing or providing current when full */
+		if (curr > 0)
+			status = POWER_SUPPLY_STATUS_CHARGING;
+		else if (curr < 0)
+			status = POWER_SUPPLY_STATUS_DISCHARGING;
+	}
+
 	if (di->status_retry == 0 && di->status_change_reference != status) {
 		di->status_change_reference = status;
 		power_supply_changed(di->bat);
-- 
2.12.2

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

[Pavel Machek]

Hi!

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

> @@ -1182,6 +1196,8 @@ static int bq27xxx_battery_status(struct bq27xxx_device_info *di,
>  		else
>  			status = POWER_SUPPLY_STATUS_DISCHARGING;
>  	} else {
> +		curr = (int)((s16)curr) * 1000;

Umm.

> @@ -1190,6 +1206,18 @@ static int bq27xxx_battery_status(struct bq27xxx_device_info *di,
>  			status = POWER_SUPPLY_STATUS_CHARGING;
>  	}
>  
> +
> +	if (curr == 0 && status != POWER_SUPPLY_STATUS_NOT_CHARGING)
> +		status = POWER_SUPPLY_STATUS_FULL;
> +
> +	if (status == POWER_SUPPLY_STATUS_FULL) {
> +		/* Drawing or providing current when full */
> +		if (curr > 0)
> +			status = POWER_SUPPLY_STATUS_CHARGING;
> +		else if (curr < 0)
> +			status = POWER_SUPPLY_STATUS_DISCHARGING;
> +	}

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

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

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

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

[Paul Kocialkowski]

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

Hi,

Le dimanche 28 mai 2017 à 21:16 +0200, Pavel Machek a écrit :
> Hi!
> 
> > The status reported directly by the battery controller is not always
> > reliable and should be corrected based on the current draw information.
> > 
> > This implements such a correction with a dedicated function, called
> > when retrieving the supply status.
> > 
> > @@ -1182,6 +1196,8 @@ static int bq27xxx_battery_status(struct
> > bq27xxx_device_info *di,
> >  		else
> >  			status = POWER_SUPPLY_STATUS_DISCHARGING;
> >  	} else {
> > +		curr = (int)((s16)curr) * 1000;
> 
> Umm.
> 
> > @@ -1190,6 +1206,18 @@ static int bq27xxx_battery_status(struct
> > bq27xxx_device_info *di,
> >  			status = POWER_SUPPLY_STATUS_CHARGING;
> >  	}
> >  
> > +
> > +	if (curr == 0 && status != POWER_SUPPLY_STATUS_NOT_CHARGING)
> > +		status = POWER_SUPPLY_STATUS_FULL;
> > +
> > +	if (status == POWER_SUPPLY_STATUS_FULL) {
> > +		/* Drawing or providing current when full */
> > +		if (curr > 0)
> > +			status = POWER_SUPPLY_STATUS_CHARGING;
> > +		else if (curr < 0)
> > +			status = POWER_SUPPLY_STATUS_DISCHARGING;
> > +	}
> 
> Are you sure this works? On N900, we normally see small currents to/from
> "full" battery.

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

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

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

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

Cheers!

-- 
Paul Kocialkowski, developer of free digital technology and hardware support

Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/

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

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

[Pavel Machek]

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

Hi!

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

> > > @@ -1182,6 +1196,8 @@ static int bq27xxx_battery_status(struct
> > > bq27xxx_device_info *di,
> > >  		else
> > >  			status = POWER_SUPPLY_STATUS_DISCHARGING;
> > >  	} else {
> > > +		curr = (int)((s16)curr) * 1000;
> > 
> > Umm.

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

> > > @@ -1190,6 +1206,18 @@ static int bq27xxx_battery_status(struct
> > > bq27xxx_device_info *di,
> > >  			status = POWER_SUPPLY_STATUS_CHARGING;
> > >  	}
> > >  
> > > +
> > > +	if (curr == 0 && status != POWER_SUPPLY_STATUS_NOT_CHARGING)
> > > +		status = POWER_SUPPLY_STATUS_FULL;
> > > +
> > > +	if (status == POWER_SUPPLY_STATUS_FULL) {
> > > +		/* Drawing or providing current when full */
> > > +		if (curr > 0)
> > > +			status = POWER_SUPPLY_STATUS_CHARGING;
> > > +		else if (curr < 0)
> > > +			status = POWER_SUPPLY_STATUS_DISCHARGING;
> > > +	}
> > 
> > Are you sure this works? On N900, we normally see small currents to/from
> > "full" battery.
> 
> In my case, this works perfectly and I am quite surprised of what you're
> describing. Is it the case when the battery has a PSU connected?

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

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

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

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

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



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

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

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

[Paul Kocialkowski]

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

Hi,

Le mercredi 31 mai 2017 à 19:32 +0200, Pavel Machek a écrit :
> The status reported directly by the battery controller is not always
> > > > reliable and should be corrected based on the current draw information.
> > > > 
> > > > This implements such a correction with a dedicated function, called
> > > > when retrieving the supply status.
> > > > @@ -1182,6 +1196,8 @@ static int bq27xxx_battery_status(struct
> > > > bq27xxx_device_info *di,
> > > >  		else
> > > >  			status = POWER_SUPPLY_STATUS_DISCHARGING;
> > > >  	} else {
> > > > +		curr = (int)((s16)curr) * 1000;
> > > 
> > > Umm.
> 
> As in "two casts in one expression -- too ugly to live".

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

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

> > > > @@ -1190,6 +1206,18 @@ static int bq27xxx_battery_status(struct
> > > > bq27xxx_device_info *di,
> > > >  			status = POWER_SUPPLY_STATUS_CHARGING;
> > > >  	}
> > > >  
> > > > +
> > > > +	if (curr == 0 && status != POWER_SUPPLY_STATUS_NOT_CHARGING)
> > > > +		status = POWER_SUPPLY_STATUS_FULL;
> > > > +
> > > > +	if (status == POWER_SUPPLY_STATUS_FULL) {
> > > > +		/* Drawing or providing current when full */
> > > > +		if (curr > 0)
> > > > +			status = POWER_SUPPLY_STATUS_CHARGING;
> > > > +		else if (curr < 0)
> > > > +			status = POWER_SUPPLY_STATUS_DISCHARGING;
> > > > +	}
> > > 
> > > Are you sure this works? On N900, we normally see small currents to/from
> > > "full" battery.
> > 
> > In my case, this works perfectly and I am quite surprised of what you're
> > describing. Is it the case when the battery has a PSU connected?
> 
> "PSU"? This is cellphone. It has USB connection and charges from that.
> 
> It has been charging for long while now, and current_now fluctuates
> between 20706 and -2856. USB has limitted current, so I guess "draw
> current from battery if we need more than USB can provide" is quite common.

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

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

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

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

What do you think?

-- 
Paul Kocialkowski, developer of free digital technology and hardware support

Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/

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

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

[Paul Kocialkowski]

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

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

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

> What do you think?

[0]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?
h=v4.12-rc4&id=7f93e1fa032bb5ee19b868b9649bc98c82553003
-- 
Paul Kocialkowski, developer of free digital technology and hardware support

Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/

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

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

[Pavel Machek]

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

Hi!

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

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

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

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

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

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

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

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

[Paul Kocialkowski]

Hey,

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

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

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

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

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

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

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

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

> +static int sbs_status_correct(struct i2c_client *client, int *intval)
> +{
> +	int ret;
> +
> +	ret = sbs_read_word_data(client, sbs_data[REG_CURRENT].addr);
> +	if (ret < 0)
> +	   return ret;
> +
> +	ret = (s16)ret;
> +	
-- 
Paul Kocialkowski, developer of free digital technology and hardware support

Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/

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

[Pavel Machek]

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

Hi!

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

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

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

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

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

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

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

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

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

[Paul Kocialkowski]

Hey,

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

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

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

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

That is definitely good to know.

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

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

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

-- 
Paul Kocialkowski, developer of free digital technology and hardware support

Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/

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

[Sebastian Reichel]

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

Hi,

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

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

-- Sebastian

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

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

[Paul Kocialkowski]

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

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

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

-- 
Paul Kocialkowski, developer of free digital technology and hardware support

Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/

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

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

[Sebastian Reichel]

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

Hi,

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

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

-- Sebastian

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