[01/15] power: supply: cpcap-battery: Fix battery full status reporting
diff mbox series

Message ID 20200315151206.30909-1-spinal.by@gmail.com
State New
Headers show
Series
  • [01/15] power: supply: cpcap-battery: Fix battery full status reporting
Related show

Commit Message

Arthur D. March 15, 2020, 3:11 p.m. UTC
Don't report that the battery is fully charged if the charging current
exceeds 100 mA.

Signed-off-by: Arthur Demchenkov <spinal.by@gmail.com>
---
 drivers/power/supply/cpcap-battery.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Pavel Machek March 15, 2020, 6:58 p.m. UTC | #1
Hi!

> Don't report that the battery is fully charged if the charging current
> exceeds 100 mA.

Could you merge patches together for possibility of reasonable review?

> @@ -408,7 +408,8 @@ static bool cpcap_battery_full(struct cpcap_battery_ddata *ddata)
>  	struct cpcap_battery_state_data *state = cpcap_battery_latest(ddata);
>  
>  	if (state->voltage >=
> -	    (ddata->config.bat.constant_charge_voltage_max_uv - 18000))
> +	    (ddata->config.bat.constant_charge_voltage_max_uv - 18000) &&
> +		state->current_ua > -100000)

It seems that this 100mA threshold is changed about 3 times in the
series :-(.

Plus, it might be better to place booleans into struct, rather than
using "static bool" inside a function.

Could we get some kind of explanations for the whole series? 100mA is
rather high current for end of charge. If you stop updating
full capacity when "your" end of charge is met, you'll have battery
charged to more than 100%. I... don't think that makes sense.

Best regards,
								Pavel
Tony Lindgren March 15, 2020, 7:19 p.m. UTC | #2
* Pavel Machek <pavel@ucw.cz> [200315 18:59]:
> Hi!
> 
> > Don't report that the battery is fully charged if the charging current
> > exceeds 100 mA.
> 
> Could you merge patches together for possibility of reasonable review?
> 
> > @@ -408,7 +408,8 @@ static bool cpcap_battery_full(struct cpcap_battery_ddata *ddata)
> >  	struct cpcap_battery_state_data *state = cpcap_battery_latest(ddata);
> >  
> >  	if (state->voltage >=
> > -	    (ddata->config.bat.constant_charge_voltage_max_uv - 18000))
> > +	    (ddata->config.bat.constant_charge_voltage_max_uv - 18000) &&
> > +		state->current_ua > -100000)
> 
> It seems that this 100mA threshold is changed about 3 times in the
> series :-(.
> 
> Plus, it might be better to place booleans into struct, rather than
> using "static bool" inside a function.
> 
> Could we get some kind of explanations for the whole series? 100mA is
> rather high current for end of charge. If you stop updating
> full capacity when "your" end of charge is met, you'll have battery
> charged to more than 100%. I... don't think that makes sense.

Yeh please compress out the unncessary parts of my RFC patches :)

For reference only, below is a diff of all the changes for
cpcap-battery.c against v5.6-rc5.

I applied this series on top of a droid4-pending-v5.6 based branch
that has the earlier RFC changes applied.

Regards,

Tony

8< -----------------
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
@@ -110,6 +110,8 @@ struct cpcap_coulomb_counter_data {
 enum cpcap_battery_state {
 	CPCAP_BATTERY_STATE_PREVIOUS,
 	CPCAP_BATTERY_STATE_LATEST,
+	CPCAP_BATTERY_STATE_EMPTY,
+	CPCAP_BATTERY_STATE_FULL,
 	CPCAP_BATTERY_STATE_NR,
 };
 
@@ -132,10 +134,32 @@ struct cpcap_battery_ddata {
 	struct cpcap_battery_state_data state[CPCAP_BATTERY_STATE_NR];
 	u32 cc_lsb;		/* ╬╝Ams per LSB */
 	atomic_t active;
+	int charge_full;
 	int status;
 	u16 vendor;
 };
 
+struct cpcap_battery_capacity {
+	int capacity;
+	int voltage;
+};
+
+#define CPCAP_CAP(l, v)				\
+{						\
+	.capacity = (l),			\
+	.voltage = (v),				\
+},
+
+/* Pessimistic battery capacity mapping before high or low value is seen */
+static const struct cpcap_battery_capacity cpcap_battery_cap[] = {
+	CPCAP_CAP(POWER_SUPPLY_CAPACITY_LEVEL_UNKNOWN,        0)
+	CPCAP_CAP(POWER_SUPPLY_CAPACITY_LEVEL_CRITICAL, 3100000)
+	CPCAP_CAP(POWER_SUPPLY_CAPACITY_LEVEL_LOW,      3300000)
+	CPCAP_CAP(POWER_SUPPLY_CAPACITY_LEVEL_NORMAL,   3700000)
+	CPCAP_CAP(POWER_SUPPLY_CAPACITY_LEVEL_HIGH,     4000000)
+	CPCAP_CAP(POWER_SUPPLY_CAPACITY_LEVEL_FULL,     4200000 - 18000)
+};
+
 #define CPCAP_NO_BATTERY	-400
 
 static struct cpcap_battery_state_data *
@@ -160,6 +184,18 @@ cpcap_battery_previous(struct cpcap_battery_ddata *ddata)
 	return cpcap_battery_get_state(ddata, CPCAP_BATTERY_STATE_PREVIOUS);
 }
 
+static struct cpcap_battery_state_data *
+cpcap_battery_get_empty(struct cpcap_battery_ddata *ddata)
+{
+	return cpcap_battery_get_state(ddata, CPCAP_BATTERY_STATE_EMPTY);
+}
+
+static struct cpcap_battery_state_data *
+cpcap_battery_get_full(struct cpcap_battery_ddata *ddata)
+{
+	return cpcap_battery_get_state(ddata, CPCAP_BATTERY_STATE_FULL);
+}
+
 static int cpcap_charger_battery_temperature(struct cpcap_battery_ddata *ddata,
 					     int *value)
 {
@@ -369,17 +405,39 @@ static int cpcap_battery_cc_get_avg_current(struct cpcap_battery_ddata *ddata)
 static bool cpcap_battery_full(struct cpcap_battery_ddata *ddata)
 {
 	struct cpcap_battery_state_data *state = cpcap_battery_latest(ddata);
+	static bool is_full;
+
+	if (state->voltage <
+	    (ddata->config.bat.constant_charge_voltage_max_uv - 18000)) {
+		is_full = false;
+	} else if (is_full) {
+		if (state->current_ua < -170000)
+			is_full = false;
+	} else if (state->current_ua >= -170000 &&
+		   cpcap_battery_cc_get_avg_current(ddata) >= -112000) {
+		is_full = true;
+	}
+
+	return is_full;
+}
 
-	if (state->voltage >=
-	    (ddata->config.bat.constant_charge_voltage_max_uv - 18000))
-		return true;
+static bool cpcap_battery_low(struct cpcap_battery_ddata *ddata)
+{
+	struct cpcap_battery_state_data *state = cpcap_battery_latest(ddata);
+	static bool is_low;
 
-	return false;
+	if (state->current_ua > 0 && (state->voltage <= 3350000 || is_low))
+		is_low = true;
+	else
+		is_low = false;
+
+	return is_low;
 }
 
 static int cpcap_battery_update_status(struct cpcap_battery_ddata *ddata)
 {
-	struct cpcap_battery_state_data state, *latest, *previous;
+	struct cpcap_battery_state_data state, *latest, *previous,
+					*empty, *full;
 	ktime_t now;
 	int error;
 
@@ -408,9 +466,56 @@ static int cpcap_battery_update_status(struct cpcap_battery_ddata *ddata)
 	memcpy(previous, latest, sizeof(*previous));
 	memcpy(latest, &state, sizeof(*latest));
 
+	if (cpcap_battery_full(ddata)) {
+		full = cpcap_battery_get_full(ddata);
+		memcpy(full, latest, sizeof(*full));
+
+		empty = cpcap_battery_get_empty(ddata);
+		if (empty->voltage && empty->voltage != -1) {
+			empty->voltage = -1;
+			ddata->charge_full =
+				empty->counter_uah - full->counter_uah;
+		} else if (ddata->charge_full) {
+			empty->voltage = -1;
+			empty->counter_uah =
+				full->counter_uah + ddata->charge_full;
+		}
+	} else if (cpcap_battery_low(ddata)) {
+		empty = cpcap_battery_get_empty(ddata);
+		memcpy(empty, latest, sizeof(*empty));
+
+		full = cpcap_battery_get_full(ddata);
+		if (full->voltage) {
+			full->voltage = 0;
+			ddata->charge_full =
+				empty->counter_uah - full->counter_uah;
+		}
+	}
+
 	return 0;
 }
 
+static int cpcap_battery_get_capacity_level(struct cpcap_battery_ddata *ddata)
+{
+	struct cpcap_battery_state_data *latest;
+	const struct cpcap_battery_capacity *cap = NULL;
+	int voltage, i;
+
+	latest = cpcap_battery_latest(ddata);
+	voltage = latest->voltage;
+
+	for (i = ARRAY_SIZE(cpcap_battery_cap) - 1; i >=0; i--) {
+		cap = &cpcap_battery_cap[i];
+		if (voltage >= cap->voltage)
+			break;
+	}
+
+	if (!cap)
+		return 0;
+
+	return cap->capacity;
+}
+
 static enum power_supply_property cpcap_battery_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_PRESENT,
@@ -421,10 +526,13 @@ static enum power_supply_property cpcap_battery_props[] = {
 	POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE,
 	POWER_SUPPLY_PROP_CURRENT_AVG,
 	POWER_SUPPLY_PROP_CURRENT_NOW,
+	POWER_SUPPLY_PROP_CHARGE_FULL,
+	POWER_SUPPLY_PROP_CHARGE_NOW,
 	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
 	POWER_SUPPLY_PROP_CHARGE_COUNTER,
 	POWER_SUPPLY_PROP_POWER_NOW,
 	POWER_SUPPLY_PROP_POWER_AVG,
+	POWER_SUPPLY_PROP_CAPACITY,
 	POWER_SUPPLY_PROP_CAPACITY_LEVEL,
 	POWER_SUPPLY_PROP_SCOPE,
 	POWER_SUPPLY_PROP_TEMP,
@@ -435,7 +543,7 @@ static int cpcap_battery_get_property(struct power_supply *psy,
 				      union power_supply_propval *val)
 {
 	struct cpcap_battery_ddata *ddata = power_supply_get_drvdata(psy);
-	struct cpcap_battery_state_data *latest, *previous;
+	struct cpcap_battery_state_data *latest, *previous, *empty;
 	u32 sample;
 	s32 accumulator;
 	int cached;
@@ -515,19 +623,33 @@ static int cpcap_battery_get_property(struct power_supply *psy,
 		tmp *= ((latest->voltage + previous->voltage) / 20000);
 		val->intval = div64_s64(tmp, 100);
 		break;
+	case POWER_SUPPLY_PROP_CAPACITY:
+		empty = cpcap_battery_get_empty(ddata);
+		if (!empty->voltage || !ddata->charge_full)
+			return -ENODATA;
+		/* (ddata->charge_full / 200) is needed for rounding */
+		val->intval = empty->counter_uah - latest->counter_uah +
+			ddata->charge_full / 200;
+		val->intval = clamp(val->intval, 0, ddata->charge_full);
+		val->intval = val->intval * 100 / ddata->charge_full;
+		break;
 	case POWER_SUPPLY_PROP_CAPACITY_LEVEL:
-		if (cpcap_battery_full(ddata))
-			val->intval = POWER_SUPPLY_CAPACITY_LEVEL_FULL;
-		else if (latest->voltage >= 3750000)
-			val->intval = POWER_SUPPLY_CAPACITY_LEVEL_HIGH;
-		else if (latest->voltage >= 3300000)
-			val->intval = POWER_SUPPLY_CAPACITY_LEVEL_NORMAL;
-		else if (latest->voltage > 3100000)
-			val->intval = POWER_SUPPLY_CAPACITY_LEVEL_LOW;
-		else if (latest->voltage <= 3100000)
-			val->intval = POWER_SUPPLY_CAPACITY_LEVEL_CRITICAL;
-		else
-			val->intval = POWER_SUPPLY_CAPACITY_LEVEL_UNKNOWN;
+		val->intval = cpcap_battery_get_capacity_level(ddata);
+		break;
+	case POWER_SUPPLY_PROP_CHARGE_NOW:
+		empty = cpcap_battery_get_empty(ddata);
+		if (!empty->voltage)
+			return -ENODATA;
+		val->intval = empty->counter_uah - latest->counter_uah;
+		if (val->intval < 0)
+			val->intval = 0;
+		else if (ddata->charge_full && ddata->charge_full < val->intval)
+			val->intval = ddata->charge_full;
+		break;
+	case POWER_SUPPLY_PROP_CHARGE_FULL:
+		if (!ddata->charge_full)
+			return -ENODATA;
+		val->intval = ddata->charge_full;
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
 		val->intval = ddata->config.info.charge_full_design;
@@ -590,6 +712,15 @@ static int cpcap_battery_set_property(struct power_supply *psy,
 		ddata->config.bat.constant_charge_voltage_max_uv = val->intval;
 
 		return cpcap_battery_update_charger(ddata, val->intval);
+	case POWER_SUPPLY_PROP_CHARGE_FULL:
+		if (val->intval < 0)
+			return -EINVAL;
+		if (val->intval > ddata->config.info.charge_full_design)
+			return -EINVAL;
+
+		ddata->charge_full = val->intval;
+
+		return 0;
 	default:
 		return -EINVAL;
 	}
@@ -602,6 +733,7 @@ static int cpcap_battery_property_is_writeable(struct power_supply *psy,
 {
 	switch (psp) {
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
+	case POWER_SUPPLY_PROP_CHARGE_FULL:
 		return 1;
 	default:
 		return 0;
Arthur D. March 15, 2020, 8:51 p.m. UTC | #3
> Could we get some kind of explanations for the whole series? 100mA is
> rather high current for end of charge. If you stop updating
> full capacity when "your" end of charge is met, you'll have battery
> charged to more than 100%. I... don't think that makes sense.

The aim was to allow userspace to see accurate values for charge_now,
charge_full and capacity (percentage) of the battery. It will allow
the user to estimate how long the device can work on the battery
with current power consumption.

For this the user will need to do a battery calibration. I.e.
he will need to fully charge and then discharge the battery.
Or vice versa: discharge -> charge. Once the user completes the
calibration cycle, he will be able to see pretty accurate values
for charge_now, charge_full and capacity. This is similar to how
bq27200 IC from Nokia N900 works.

Also this patchset allows the userspace to restore the calibration
value after reboot. By the calibration value I mean charge_full.

We can't rely on restoring charge_now value, because the user may
have multiple operating systems installed and if he works in another
OS for a while the charge_now value will become invalid.

So, after a reboot the user may want to restore the charge_full
value, so the kernel will be able to estimate the percentage
and capacity values without forcing the user to do a full calib-
ration cycle again. The only thing the user will have to do
is to fully charge OR fully discharge the battery at least once.
And he will get all values set.

Now about the chosen limits. For some reason the charging is
interrupted (and restarted after a while) when the following
conditions are met:
1) the charging current is < 112 mA
2) the display backlight is off

This behaviour was observed in Maemo Leste with hildon-desktop
running. I tested these patches for several days, so I picked up
the parameters for optimal (from my point of view) work in practice
taking into account the current "features" of Droid 4 drivers.

If we could somehow fix this behaviour (charging interruption),
I'd reconsider the end of charge current value to be 50 mA.

Making it lower than 50 mA doesn't seem to make much sence
because of the extended charging time visibility without giving
significant improvement in charge_full accuracy.

> If you stop updating full capacity when "your" end of charge is met,
> you'll have battery charged to more than 100%.

No worries. With the implemented algorithm, the user will not notice
that the battery is more than 100% charged (which is just a convention
here). And this situation gives an advantage in that it has a slightly
pessimistic charge_full value, which in practice turns out to be good:
the user will be warned about low battery a little ahead of time.

--
Best regards, Spinal
Tony Lindgren March 15, 2020, 9:59 p.m. UTC | #4
* Arthur D. <spinal.by@gmail.com> [200315 20:51]:
> Now about the chosen limits. For some reason the charging is
> interrupted (and restarted after a while) when the following
> conditions are met:
> 1) the charging current is < 112 mA
> 2) the display backlight is off
> 
> This behaviour was observed in Maemo Leste with hildon-desktop
> running. I tested these patches for several days, so I picked up
> the parameters for optimal (from my point of view) work in practice
> taking into account the current "features" of Droid 4 drivers.
> 
> If we could somehow fix this behaviour (charging interruption),
> I'd reconsider the end of charge current value to be 50 mA.

Hmm well we do have two chargers, the usb charger and the
unknown inductive charger for the pins on the back.

It would be best to keep cpcap-battery.c independent of the
chargers to avoid depndencies as the chargers do usually start
charging briefly always when connected.

Maybe just adding something like below would be enough of a check:

static int
cpcap_battery_get_counter_rate(struct cpcap_battery_ddata *ddata,
			       int poll_time_ms);

And then based on the value being negative or positive you
would know if it's charging or not. I guess we could then
use this also for POWER_SUPPLY_PROP_CHARGE_NOW with poll_time_ms
value of 0. I think the charge counter is configure to poll
at 250 ms right now.

Regards,

Tony
Arthur D. March 16, 2020, 1:30 a.m. UTC | #5
Hi, Tony.

It seems like a misunderstanding here. There's no problem in detecting
if the charging is in progress. The green led is switched off and
the battery current sign is changed from "-" to "+" (which means
that the battery is being discharged). So there's no need in additional
checks. For cpcap-battery this situation seems like a battery stopped
charging. And it doesn't matter if that was a user who disconnected
the charger or it was done somewhere in a driver/firmware/hardware.

The problem is that the charging current cant get to the point <100 mA,
not talking about <50 mA. And that's why I set the value of 112 mA for
the end of charge current: to help the kernel to detect this plateau and
to stop the calibration cycle, so the userspace can get all the battery
parameters I mentioned in the previous mail.

Please note, that the behaviour I mentioned was observed only when the
conditions written in my last mail were met. The important one was:
> 2) the display backlight is off

Because when I unlocked the display the charging current was able
to go below 112 mA. Of course I couldn't rely on something like this:
the user should stay with backlight on to have the battery calibrated.
Think about it: waiting for the charging current to drop from 100 mA
to 50 mA can take dozens of minutes (it depends on the age of battery -
the older the battery the longer it will take), and the user should
force somehow the device to not switch off the display hightlight
until the battery is calibrated.

Of course it's unacceptable, so I decided to set the end of charge
current limit to 112 mA. Which allows the user to just put the device
on a table and to wait until it's fully charged without a need
to interfere the charging process with some action from the user.

--
Best regards, Spinal


>> Now about the chosen limits. For some reason the charging is
>> interrupted (and restarted after a while) when the following
>> conditions are met:
>> 1) the charging current is < 112 mA
>> 2) the display backlight is off
>>
>> This behaviour was observed in Maemo Leste with hildon-desktop
>> running. I tested these patches for several days, so I picked up
>> the parameters for optimal (from my point of view) work in practice
>> taking into account the current "features" of Droid 4 drivers.
>>
>> If we could somehow fix this behaviour (charging interruption),
>> I'd reconsider the end of charge current value to be 50 mA.
>
> Hmm well we do have two chargers, the usb charger and the
> unknown inductive charger for the pins on the back.
>
> It would be best to keep cpcap-battery.c independent of the
> chargers to avoid depndencies as the chargers do usually start
> charging briefly always when connected.
>
> Maybe just adding something like below would be enough of a check:
>
> static int
> cpcap_battery_get_counter_rate(struct cpcap_battery_ddata *ddata,
> 			       int poll_time_ms);
>
> And then based on the value being negative or positive you
> would know if it's charging or not. I guess we could then
> use this also for POWER_SUPPLY_PROP_CHARGE_NOW with poll_time_ms
> value of 0. I think the charge counter is configure to poll
> at 250 ms right now.
Tony Lindgren March 16, 2020, 4:02 p.m. UTC | #6
* Arthur D. <spinal.by@gmail.com> [200316 01:31]:
> Hi, Tony.
> 
> It seems like a misunderstanding here. There's no problem in detecting
> if the charging is in progress. The green led is switched off and
> the battery current sign is changed from "-" to "+" (which means
> that the battery is being discharged). So there's no need in additional
> checks. For cpcap-battery this situation seems like a battery stopped
> charging. And it doesn't matter if that was a user who disconnected
> the charger or it was done somewhere in a driver/firmware/hardware.
> 
> The problem is that the charging current cant get to the point <100 mA,
> not talking about <50 mA. And that's why I set the value of 112 mA for
> the end of charge current: to help the kernel to detect this plateau and
> to stop the calibration cycle, so the userspace can get all the battery
> parameters I mentioned in the previous mail.

OK I guess that's easy to change if we figure out something better :)
Maybe add some define for it like CPCAP_BATTERY_FULL_CHARGE_CURRENT or
similar?

> Please note, that the behaviour I mentioned was observed only when the
> conditions written in my last mail were met. The important one was:
> > 2) the display backlight is off
> 
> Because when I unlocked the display the charging current was able
> to go below 112 mA. Of course I couldn't rely on something like this:
> the user should stay with backlight on to have the battery calibrated.
> Think about it: waiting for the charging current to drop from 100 mA
> to 50 mA can take dozens of minutes (it depends on the age of battery -
> the older the battery the longer it will take), and the user should
> force somehow the device to not switch off the display hightlight
> until the battery is calibrated.
> 
> Of course it's unacceptable, so I decided to set the end of charge
> current limit to 112 mA. Which allows the user to just put the device
> on a table and to wait until it's fully charged without a need
> to interfere the charging process with some action from the user.

Yeah OK thanks.

Tony

Patch
diff mbox series

diff --git a/drivers/power/supply/cpcap-battery.c b/drivers/power/supply/cpcap-battery.c
index d7b3234ec264..34a9dbcd1a23 100644
--- a/drivers/power/supply/cpcap-battery.c
+++ b/drivers/power/supply/cpcap-battery.c
@@ -408,7 +408,8 @@  static bool cpcap_battery_full(struct cpcap_battery_ddata *ddata)
 	struct cpcap_battery_state_data *state = cpcap_battery_latest(ddata);
 
 	if (state->voltage >=
-	    (ddata->config.bat.constant_charge_voltage_max_uv - 18000))
+	    (ddata->config.bat.constant_charge_voltage_max_uv - 18000) &&
+		state->current_ua > -100000)
 		return true;
 
 	return false;