[PATCH 0/7] Some fixes and improvments for cpcap battery and charger

[PATCH 0/7] Some fixes and improvments for cpcap battery and charger

[Tony Lindgren]

Hi all,

I've been trying to figure out how come the coulomb counter on droid 4 is off
especially for the low power consumption values. Turns out the "coulomb counter"
values correlate better with average power consumption if we divide the value with
number of samples. Otherwise we have a curve instead of flat correlation between
the register values and power consumed. I have some patches coming up eventually to
fix that, but meanwhile I've noticed some minor issues in general that would be
good to have out of the way.

I've only tagged the first one with fixes tag, the other ones can certainly wait
for the merge window considering further changes are needed at least for low power
consumption values.

Regards,

Tony


Tony Lindgren (7):
  power: supply: cpcap-battery: Fix division by zero
  power: supply: cpcap-battery: Fix low battery check
  power: supply: cpcap-battery: Fix signed counter sample register
  power: supply: cpcap-battery: Fix coulomb counter calibration register
    use
  power: supply: cpcap-battery: Use accumulator for current and power
    average
  power: supply: cpcap-battery: Fix pointless EPROBE_DEFER warnings
  power: supply: cpcap-charger: Fix pointless EPROBE_DEFER warnings

 drivers/power/supply/cpcap-battery.c | 45 +++++++++++++++-------------
 drivers/power/supply/cpcap-charger.c |  5 ++--
 2 files changed, 27 insertions(+), 23 deletions(-)

-- 
2.21.0

[PATCH 1/7] power: supply: cpcap-battery: Fix division by zero

[Tony Lindgren]

If called fast enough so samples do not increment, we can get
division by zero in kernel:

__div0
cpcap_battery_cc_raw_div
cpcap_battery_get_property
power_supply_get_property.part.1
power_supply_get_property
power_supply_show_property
power_supply_uevent

Fixes: 874b2adbed12 ("power: supply: cpcap-battery: Add a battery driver")
Cc: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/power/supply/cpcap-battery.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/power/supply/cpcap-battery.c b/drivers/power/supply/cpcap-battery.c
--- a/drivers/power/supply/cpcap-battery.c
+++ b/drivers/power/supply/cpcap-battery.c
@@ -221,6 +221,9 @@ static int cpcap_battery_cc_raw_div(struct cpcap_battery_ddata *ddata,
 	int avg_current;
 	u32 cc_lsb;
 
+	if (!divider)
+		return 0;
+
 	sample &= 0xffffff;		/* 24-bits, unsigned */
 	offset &= 0x7ff;		/* 10-bits, signed */
 
-- 
2.21.0

Re: [PATCH 1/7] power: supply: cpcap-battery: Fix division by zero

[Pavel Machek]

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

On Sun 2019-04-07 11:12:48, Tony Lindgren wrote:
> If called fast enough so samples do not increment, we can get
> division by zero in kernel:
> 
> __div0
> cpcap_battery_cc_raw_div
> cpcap_battery_get_property
> power_supply_get_property.part.1
> power_supply_get_property
> power_supply_show_property
> power_supply_uevent
> 
> Fixes: 874b2adbed12 ("power: supply: cpcap-battery: Add a battery driver")
> Cc: Pavel Machek <pavel@ucw.cz>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Acked-by: Pavel Machek <pavel@ucw.cz>

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

[PATCH 2/7] power: supply: cpcap-battery: Fix low battery check

[Tony Lindgren]

We need to check current instead of the charge counter to see if
a charger is connected. The charge counter shows the cumulated value
instead of the current charge current and can be negative or positive.

Cc: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/power/supply/cpcap-battery.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/power/supply/cpcap-battery.c b/drivers/power/supply/cpcap-battery.c
--- a/drivers/power/supply/cpcap-battery.c
+++ b/drivers/power/supply/cpcap-battery.c
@@ -562,11 +562,11 @@ static irqreturn_t cpcap_battery_irq_thread(int irq, void *data)
 
 	switch (d->action) {
 	case CPCAP_BATTERY_IRQ_ACTION_BATTERY_LOW:
-		if (latest->counter_uah >= 0)
+		if (latest->current_ua >= 0)
 			dev_warn(ddata->dev, "Battery low at 3.3V!\n");
 		break;
 	case CPCAP_BATTERY_IRQ_ACTION_POWEROFF:
-		if (latest->counter_uah >= 0) {
+		if (latest->current_ua >= 0) {
 			dev_emerg(ddata->dev,
 				  "Battery empty at 3.1V, powering off\n");
 			orderly_poweroff(true);
-- 
2.21.0

Re: [PATCH 2/7] power: supply: cpcap-battery: Fix low battery check

[Pavel Machek]

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

On Sun 2019-04-07 11:12:49, Tony Lindgren wrote:
> We need to check current instead of the charge counter to see if
> a charger is connected. The charge counter shows the cumulated value
> instead of the current charge current and can be negative or positive.
> 

Acked-by: Pavel Machek <pavel@ucw.cz>

> Signed-off-by: Tony Lindgren <tony@atomide.com>


> ---
>  drivers/power/supply/cpcap-battery.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/power/supply/cpcap-battery.c b/drivers/power/supply/cpcap-battery.c
> --- a/drivers/power/supply/cpcap-battery.c
> +++ b/drivers/power/supply/cpcap-battery.c
> @@ -562,11 +562,11 @@ static irqreturn_t cpcap_battery_irq_thread(int irq, void *data)
>  
>  	switch (d->action) {
>  	case CPCAP_BATTERY_IRQ_ACTION_BATTERY_LOW:
> -		if (latest->counter_uah >= 0)
> +		if (latest->current_ua >= 0)
>  			dev_warn(ddata->dev, "Battery low at 3.3V!\n");
>  		break;
>  	case CPCAP_BATTERY_IRQ_ACTION_POWEROFF:
> -		if (latest->counter_uah >= 0) {
> +		if (latest->current_ua >= 0) {
>  			dev_emerg(ddata->dev,
>  				  "Battery empty at 3.1V, powering off\n");
>  			orderly_poweroff(true);

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

[PATCH 3/7] power: supply: cpcap-battery: Fix signed counter sample register

[Tony Lindgren]

The accumulator sample register is signed 32-bits wide register on
droid 4. And only the earlier version of cpcap has a signed 24-bits
wide register. We're currently passing it around as unsigned, so
let's fix that and use sign_extend32() for the earlier revision.

Cc: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/power/supply/cpcap-battery.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/power/supply/cpcap-battery.c b/drivers/power/supply/cpcap-battery.c
--- a/drivers/power/supply/cpcap-battery.c
+++ b/drivers/power/supply/cpcap-battery.c
@@ -82,7 +82,7 @@ struct cpcap_battery_config {
 };
 
 struct cpcap_coulomb_counter_data {
-	s32 sample;		/* 24-bits */
+	s32 sample;		/* 24 or 32 bits */
 	s32 accumulator;
 	s16 offset;		/* 10-bits */
 };
@@ -213,7 +213,7 @@ static int cpcap_battery_get_current(struct cpcap_battery_ddata *ddata)
  * TI or ST coulomb counter in the PMIC.
  */
 static int cpcap_battery_cc_raw_div(struct cpcap_battery_ddata *ddata,
-				    u32 sample, s32 accumulator,
+				    s32 sample, s32 accumulator,
 				    s16 offset, u32 divider)
 {
 	s64 acc;
@@ -224,7 +224,6 @@ static int cpcap_battery_cc_raw_div(struct cpcap_battery_ddata *ddata,
 	if (!divider)
 		return 0;
 
-	sample &= 0xffffff;		/* 24-bits, unsigned */
 	offset &= 0x7ff;		/* 10-bits, signed */
 
 	switch (ddata->vendor) {
@@ -259,7 +258,7 @@ static int cpcap_battery_cc_raw_div(struct cpcap_battery_ddata *ddata,
 
 /* 3600000μAms = 1μAh */
 static int cpcap_battery_cc_to_uah(struct cpcap_battery_ddata *ddata,
-				   u32 sample, s32 accumulator,
+				   s32 sample, s32 accumulator,
 				   s16 offset)
 {
 	return cpcap_battery_cc_raw_div(ddata, sample,
@@ -268,7 +267,7 @@ static int cpcap_battery_cc_to_uah(struct cpcap_battery_ddata *ddata,
 }
 
 static int cpcap_battery_cc_to_ua(struct cpcap_battery_ddata *ddata,
-				  u32 sample, s32 accumulator,
+				  s32 sample, s32 accumulator,
 				  s16 offset)
 {
 	return cpcap_battery_cc_raw_div(ddata, sample,
@@ -312,6 +311,8 @@ cpcap_battery_read_accumulated(struct cpcap_battery_ddata *ddata,
 	/* Sample value CPCAP_REG_CCS1 & 2 */
 	ccd->sample = (buf[1] & 0x0fff) << 16;
 	ccd->sample |= buf[0];
+	if (ddata->vendor == CPCAP_VENDOR_TI)
+		ccd->sample = sign_extend32(24, ccd->sample);
 
 	/* Accumulator value CPCAP_REG_CCA1 & 2 */
 	ccd->accumulator = ((s16)buf[3]) << 16;
-- 
2.21.0

Re: [PATCH 3/7] power: supply: cpcap-battery: Fix signed counter sample register

[Pavel Machek]

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

On Sun 2019-04-07 11:12:50, Tony Lindgren wrote:
> The accumulator sample register is signed 32-bits wide register on
> droid 4. And only the earlier version of cpcap has a signed 24-bits
> wide register. We're currently passing it around as unsigned, so
> let's fix that and use sign_extend32() for the earlier revision.
> 
> Cc: Pavel Machek <pavel@ucw.cz>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Acked-by: Pavel Machek <pavel@ucw.cz>

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

[PATCH 4/7] power: supply: cpcap-battery: Fix coulomb counter calibration register use

[Tony Lindgren]

The coulomb counter calibration is not CCO, it's CCM. And the CCM is
nine bits wide signed register, so let's use sign_extend32() for it.

Cc: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/power/supply/cpcap-battery.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/power/supply/cpcap-battery.c b/drivers/power/supply/cpcap-battery.c
--- a/drivers/power/supply/cpcap-battery.c
+++ b/drivers/power/supply/cpcap-battery.c
@@ -84,7 +84,7 @@ struct cpcap_battery_config {
 struct cpcap_coulomb_counter_data {
 	s32 sample;		/* 24 or 32 bits */
 	s32 accumulator;
-	s16 offset;		/* 10-bits */
+	s16 offset;		/* 9 bits */
 };
 
 enum cpcap_battery_state {
@@ -224,8 +224,6 @@ static int cpcap_battery_cc_raw_div(struct cpcap_battery_ddata *ddata,
 	if (!divider)
 		return 0;
 
-	offset &= 0x7ff;		/* 10-bits, signed */
-
 	switch (ddata->vendor) {
 	case CPCAP_VENDOR_ST:
 		cc_lsb = 95374;		/* μAms per LSB */
@@ -318,12 +316,12 @@ cpcap_battery_read_accumulated(struct cpcap_battery_ddata *ddata,
 	ccd->accumulator = ((s16)buf[3]) << 16;
 	ccd->accumulator |= buf[2];
 
-	/* Offset value CPCAP_REG_CCO */
-	ccd->offset = buf[5];
-
-	/* Adjust offset based on mode value CPCAP_REG_CCM? */
-	if (buf[4] >= 0x200)
-		ccd->offset |= 0xfc00;
+	/*
+	 * Coulomb counter calibration offset is CPCAP_REG_CCM,
+	 * REG_CCO seems unused
+	 */
+	ccd->offset = buf[4];
+	ccd->offset = sign_extend32(ccd->offset, 9);
 
 	return cpcap_battery_cc_to_uah(ddata,
 				       ccd->sample,
-- 
2.21.0

Re: [PATCH 4/7] power: supply: cpcap-battery: Fix coulomb counter calibration register use

[Pavel Machek]

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

On Sun 2019-04-07 11:12:51, Tony Lindgren wrote:
> The coulomb counter calibration is not CCO, it's CCM. And the CCM is
> nine bits wide signed register, so let's use sign_extend32() for it.
> 
> Cc: Pavel Machek <pavel@ucw.cz>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Acked-by: Pavel Machek <pavel@ucw.cz>

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

[PATCH 5/7] power: supply: cpcap-battery: Use accumulator for current and power average

[Tony Lindgren]

We should not use measured current value for average since we have proper
coulomb counter values available. Using measured current value should
be only used when the value is queried at a higher rate than the 250 ms
rate the coulomb counter is configured to run at.

Cc: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/power/supply/cpcap-battery.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/power/supply/cpcap-battery.c b/drivers/power/supply/cpcap-battery.c
--- a/drivers/power/supply/cpcap-battery.c
+++ b/drivers/power/supply/cpcap-battery.c
@@ -476,11 +476,11 @@ static int cpcap_battery_get_property(struct power_supply *psy,
 		val->intval = ddata->config.info.voltage_min_design;
 		break;
 	case POWER_SUPPLY_PROP_CURRENT_AVG:
-		if (cached) {
+		sample = latest->cc.sample - previous->cc.sample;
+		if (!sample) {
 			val->intval = cpcap_battery_cc_get_avg_current(ddata);
 			break;
 		}
-		sample = latest->cc.sample - previous->cc.sample;
 		accumulator = latest->cc.accumulator - previous->cc.accumulator;
 		val->intval = cpcap_battery_cc_to_ua(ddata, sample,
 						     accumulator,
@@ -497,13 +497,13 @@ static int cpcap_battery_get_property(struct power_supply *psy,
 		val->intval = div64_s64(tmp, 100);
 		break;
 	case POWER_SUPPLY_PROP_POWER_AVG:
-		if (cached) {
+		sample = latest->cc.sample - previous->cc.sample;
+		if (!sample) {
 			tmp = cpcap_battery_cc_get_avg_current(ddata);
 			tmp *= (latest->voltage / 10000);
 			val->intval = div64_s64(tmp, 100);
 			break;
 		}
-		sample = latest->cc.sample - previous->cc.sample;
 		accumulator = latest->cc.accumulator - previous->cc.accumulator;
 		tmp = cpcap_battery_cc_to_ua(ddata, sample, accumulator,
 					     latest->cc.offset);
-- 
2.21.0

[PATCH 6/7] power: supply: cpcap-battery: Fix pointless EPROBE_DEFER warnings

[Tony Lindgren]

With loadable modules we may get the following during init:

could not initialize VBUS or ID IIO: -517

Let's not print any pointless error messages for deferred probe.

Cc: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/power/supply/cpcap-battery.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/power/supply/cpcap-battery.c b/drivers/power/supply/cpcap-battery.c
--- a/drivers/power/supply/cpcap-battery.c
+++ b/drivers/power/supply/cpcap-battery.c
@@ -669,8 +669,9 @@ static int cpcap_battery_init_iio(struct cpcap_battery_ddata *ddata)
 	return 0;
 
 out_err:
-	dev_err(ddata->dev, "could not initialize VBUS or ID IIO: %i\n",
-		error);
+	if (error != -EPROBE_DEFER)
+		dev_err(ddata->dev, "could not initialize VBUS or ID IIO: %i\n",
+			error);
 
 	return error;
 }
-- 
2.21.0

Re: [PATCH 6/7] power: supply: cpcap-battery: Fix pointless EPROBE_DEFER warnings

[Pavel Machek]

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

On Sun 2019-04-07 11:12:53, Tony Lindgren wrote:
> With loadable modules we may get the following during init:
> 
> could not initialize VBUS or ID IIO: -517
> 
> Let's not print any pointless error messages for deferred probe.
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>

6,7: Acked-by: Pavel Machek <pavel@ucw.cz>

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

[PATCH 7/7] power: supply: cpcap-charger: Fix pointless EPROBE_DEFER warnings

[Tony Lindgren]

With loadable modules we may get the following during init:

could not initialize VBUS or ID IIO: -517

Let's not print any pointless error messages for deferred probe.

Cc: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/power/supply/cpcap-charger.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/power/supply/cpcap-charger.c b/drivers/power/supply/cpcap-charger.c
--- a/drivers/power/supply/cpcap-charger.c
+++ b/drivers/power/supply/cpcap-charger.c
@@ -574,8 +574,9 @@ static int cpcap_charger_init_iio(struct cpcap_charger_ddata *ddata)
 	return 0;
 
 out_err:
-	dev_err(ddata->dev, "could not initialize VBUS or ID IIO: %i\n",
-		error);
+	if (error != -EPROBE_DEFER)
+		dev_err(ddata->dev, "could not initialize VBUS or ID IIO: %i\n",
+			error);
 
 	return error;
 }
-- 
2.21.0

Re: [PATCH 0/7] Some fixes and improvments for cpcap battery and charger

[Sebastian Reichel]

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

Hi,

On Sun, Apr 07, 2019 at 11:12:47AM -0700, Tony Lindgren wrote:
> I've been trying to figure out how come the coulomb counter on droid 4 is off
> especially for the low power consumption values. Turns out the "coulomb counter"
> values correlate better with average power consumption if we divide the value with
> number of samples. Otherwise we have a curve instead of flat correlation between
> the register values and power consumed. I have some patches coming up eventually to
> fix that, but meanwhile I've noticed some minor issues in general that would be
> good to have out of the way.
> 
> I've only tagged the first one with fixes tag, the other ones can certainly wait
> for the merge window considering further changes are needed at least for low power
> consumption values.

Thanks, all queued.

-- Sebastian

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

Re: [PATCH 0/7] Some fixes and improvments for cpcap battery and charger

[Tony Lindgren]

Hi all,

Just to follow-up on the information below.

* Tony Lindgren <tony@atomide.com> [190407 11:13]:
> I've been trying to figure out how come the coulomb counter on droid 4 is off
> especially for the low power consumption values. Turns out the "coulomb counter"
> values correlate better with average power consumption if we divide the value with
> number of samples. Otherwise we have a curve instead of flat correlation between
> the register values and power consumed. I have some patches coming up eventually to
> fix that, but meanwhile I've noticed some minor issues in general that would be
> good to have out of the way.

So I've compared measurements from my power supply to measurements from
Baylibre ACME ina226, custom ina226 setup and ARM Energy Probe, and turns
out it's my power supply that has started wrong values and propably needs
calibration :)

So apologies for bashing the cpcap coulomb counter, it seems it's already
quite accurate showing average few mW less over 3 minute sample period
compared to my other measurements.

Regards,

Tony