diff mbox series

[2/3] power: supply: cpcap-battery: Fix battery identification

Message ID 1667647544-12945-3-git-send-email-ivo.g.dimitrov.75@gmail.com
State New
Headers
Series power: supply: cpcap-battery improvements |

Commit Message

Ivaylo Dimitrov Nov. 5, 2022, 11:25 a.m. UTC 
  Use the same logic to identify genuine batteries as Android does.

Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
---
 drivers/power/supply/cpcap-battery.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

Comments

Sebastian Reichel Nov. 10, 2022, 4:05 p.m. UTC | #1 
Hi,

On Sat, Nov 05, 2022 at 01:25:43PM +0200, Ivaylo Dimitrov wrote:
> Use the same logic to identify genuine batteries as Android does.
> 
> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> ---

Why do we care?

-- Sebastian

>  drivers/power/supply/cpcap-battery.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/power/supply/cpcap-battery.c b/drivers/power/supply/cpcap-battery.c
> index 8869067..ca6ee2b 100644
> --- a/drivers/power/supply/cpcap-battery.c
> +++ b/drivers/power/supply/cpcap-battery.c
> @@ -422,7 +422,7 @@ static int cpcap_battery_cc_to_ua(struct cpcap_battery_ddata *ddata,
>  
>  static int cpcap_battery_match_nvmem(struct device *dev, const void *data)
>  {
> -	if (strcmp(dev_name(dev), "89-500029ba0f73") == 0)
> +	if (strncmp(dev_name(dev), "89-500", 6) == 0)
>  		return 1;
>  	else
>  		return 0;
> @@ -439,10 +439,19 @@ static void cpcap_battery_detect_battery_type(struct cpcap_battery_ddata *ddata)
>  	if (IS_ERR_OR_NULL(nvmem)) {
>  		ddata->check_nvmem = true;
>  		dev_info_once(ddata->dev, "Can not find battery nvmem device. Assuming generic lipo battery\n");
> -	} else if (nvmem_device_read(nvmem, 2, 1, &battery_id) < 0) {
> -		battery_id = 0;
> -		ddata->check_nvmem = true;
> -		dev_warn(ddata->dev, "Can not read battery nvmem device. Assuming generic lipo battery\n");
> +	} else {
> +		char buf[24];
> +
> +		if (nvmem_device_read(nvmem, 96, 4, buf) < 0 ||
> +		    strncmp(buf, "COPR", 4) != 0 ||
> +		    nvmem_device_read(nvmem, 104, 24, buf) < 0 ||
> +		    strncmp(buf, "MOTOROLA E.P CHARGE ONLY", 24) != 0 ||
> +		    nvmem_device_read(nvmem, 2, 1, &battery_id) < 0) {
> +			battery_id = 0;
> +			ddata->check_nvmem = true;
> +			dev_warn(ddata->dev, "Can not read battery nvmem device. Assuming generic lipo battery\n");
> +		}
> +
>  	}
>  
>  	switch (battery_id) {
> -- 
> 1.9.1
>
Ivaylo Dimitrov Nov. 10, 2022, 4:50 p.m. UTC | #2 
Hi,

On 10.11.22 г. 18:05 ч., Sebastian Reichel wrote:
> Hi,
> 
> On Sat, Nov 05, 2022 at 01:25:43PM +0200, Ivaylo Dimitrov wrote:
>> Use the same logic to identify genuine batteries as Android does.
>>
>> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
>> ---
> 
> Why do we care?
> 

Because if we know the battery is genuine (or at least pretends to be :) 
), then we can read battery parameters from nvram, see patch 3/3. This 
will allow us to charge HV LiPo batteries to 4.35V, using the full capacity.

Not to say the code currently identifies a specific battery with id of 
"89-500029ba0f73" as genuine one, ignoring all others. So this patch is 
more or less a bugfix too.

Regards,
Ivo


> -- Sebastian
> 
>>   drivers/power/supply/cpcap-battery.c | 19 ++++++++++++++-----
>>   1 file changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/power/supply/cpcap-battery.c b/drivers/power/supply/cpcap-battery.c
>> index 8869067..ca6ee2b 100644
>> --- a/drivers/power/supply/cpcap-battery.c
>> +++ b/drivers/power/supply/cpcap-battery.c
>> @@ -422,7 +422,7 @@ static int cpcap_battery_cc_to_ua(struct cpcap_battery_ddata *ddata,
>>   
>>   static int cpcap_battery_match_nvmem(struct device *dev, const void *data)
>>   {
>> -	if (strcmp(dev_name(dev), "89-500029ba0f73") == 0)
>> +	if (strncmp(dev_name(dev), "89-500", 6) == 0)
>>   		return 1;
>>   	else
>>   		return 0;
>> @@ -439,10 +439,19 @@ static void cpcap_battery_detect_battery_type(struct cpcap_battery_ddata *ddata)
>>   	if (IS_ERR_OR_NULL(nvmem)) {
>>   		ddata->check_nvmem = true;
>>   		dev_info_once(ddata->dev, "Can not find battery nvmem device. Assuming generic lipo battery\n");
>> -	} else if (nvmem_device_read(nvmem, 2, 1, &battery_id) < 0) {
>> -		battery_id = 0;
>> -		ddata->check_nvmem = true;
>> -		dev_warn(ddata->dev, "Can not read battery nvmem device. Assuming generic lipo battery\n");
>> +	} else {
>> +		char buf[24];
>> +
>> +		if (nvmem_device_read(nvmem, 96, 4, buf) < 0 ||
>> +		    strncmp(buf, "COPR", 4) != 0 ||
>> +		    nvmem_device_read(nvmem, 104, 24, buf) < 0 ||
>> +		    strncmp(buf, "MOTOROLA E.P CHARGE ONLY", 24) != 0 ||
>> +		    nvmem_device_read(nvmem, 2, 1, &battery_id) < 0) {
>> +			battery_id = 0;
>> +			ddata->check_nvmem = true;
>> +			dev_warn(ddata->dev, "Can not read battery nvmem device. Assuming generic lipo battery\n");
>> +		}
>> +
>>   	}
>>   
>>   	switch (battery_id) {
>> -- 
>> 1.9.1
>>
Tony Lindgren Nov. 15, 2022, 1:49 p.m. UTC | #3 
Hi,

* Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com> [221110 16:40]:
> On 10.11.22 г. 18:05 ч., Sebastian Reichel wrote:
> > Why do we care?
> >
> Because if we know the battery is genuine (or at least pretends to be :) ),
> then we can read battery parameters from nvram, see patch 3/3. This will
> allow us to charge HV LiPo batteries to 4.35V, using the full capacity.

Let's not enable charge voltages above 4.2V automatically at all unless
the user chooses to set a higher charge voltage via sysfs manually.

We have had reports of bloated batteries if left connected to the charger
at higher voltage than 4.2V. This seems to happen after connected for some
weeks or months. AFAIK this happens both with Android and mainline kernel
at higher voltages.

For more information, please see commit d4ee021c410f ("power: supply:
cpcap-charger: Limit voltage to 4.2V for battery").

No objections for using the NVRAM to detect the battery max voltages
though. That is as long as the default charge voltage does not go above
4.2V.

Regards,

Tony
Ivaylo Dimitrov Nov. 15, 2022, 3:41 p.m. UTC | #4 
Hi,

On 15.11.22 г. 15:49 ч., Tony Lindgren wrote:
> Hi,
> 
> * Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com> [221110 16:40]:
>> On 10.11.22 г. 18:05 ч., Sebastian Reichel wrote:
>>> Why do we care?
>>>
>> Because if we know the battery is genuine (or at least pretends to be :) ),
>> then we can read battery parameters from nvram, see patch 3/3. This will
>> allow us to charge HV LiPo batteries to 4.35V, using the full capacity.
> 
> Let's not enable charge voltages above 4.2V automatically at all unless
> the user chooses to set a higher charge voltage via sysfs manually.
> 
> We have had reports of bloated batteries if left connected to the charger
> at higher voltage than 4.2V. This seems to happen after connected for some
> weeks or months. AFAIK this happens both with Android and mainline kernel
> at higher voltages.
> 

Not that I sent such patch yet, but still, thinking about it, we should 
be able to easily prevent such damage by not restarting the charging 
after battery is full and voltage has dropped by 50mV or so. There can 
be a threshold (lets say 4.25 or 4.2) above which charging shall not be 
re-enabled unless the user reconnects the charger. Even if default stays 
4.2 and it is the user that has enabled 4.35. Just an idea.

> For more information, please see commit d4ee021c410f ("power: supply:
> cpcap-charger: Limit voltage to 4.2V for battery").
> 
> No objections for using the NVRAM to detect the battery max voltages
> though. That is as long as the default charge voltage does not go above
> 4.2V.

Patch 3/3 does just that - reads battery design parameters from NVRAM.

Regards,
Ivo

> 
> Regards,
> 
> Tony
>
Tony Lindgren Nov. 17, 2022, 4:53 a.m. UTC | #5 
* Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com> [221115 15:31]:
> Hi,
> 
> On 15.11.22 г. 15:49 ч., Tony Lindgren wrote:
> > Hi,
> > 
> > * Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com> [221110 16:40]:
> > > On 10.11.22 г. 18:05 ч., Sebastian Reichel wrote:
> > > > Why do we care?
> > > > 
> > > Because if we know the battery is genuine (or at least pretends to be :) ),
> > > then we can read battery parameters from nvram, see patch 3/3. This will
> > > allow us to charge HV LiPo batteries to 4.35V, using the full capacity.
> > 
> > Let's not enable charge voltages above 4.2V automatically at all unless
> > the user chooses to set a higher charge voltage via sysfs manually.
> > 
> > We have had reports of bloated batteries if left connected to the charger
> > at higher voltage than 4.2V. This seems to happen after connected for some
> > weeks or months. AFAIK this happens both with Android and mainline kernel
> > at higher voltages.
> > 
> 
> Not that I sent such patch yet, but still, thinking about it, we should be
> able to easily prevent such damage by not restarting the charging after
> battery is full and voltage has dropped by 50mV or so. There can be a
> threshold (lets say 4.25 or 4.2) above which charging shall not be
> re-enabled unless the user reconnects the charger. Even if default stays 4.2
> and it is the user that has enabled 4.35. Just an idea.

Sure the logic to handle max charge voltage and maintenance charge voltage
could be there. With commit d4ee021c410f we now just wait for the charge
to come down to 4.2V if charged at 4.35V with Android.

We still should not enable higher charge voltages by default though. It
still needs to be enabled by the user via sysfs. It's possible that also
shorter peaks of higher charge voltage accelerate the battery degration.
It just may happen slower than what we've seen earlier. To test this,
multiple devices would need to be left connected to a charger for several
months :)

Regards,

Tony
Carl Klemm Nov. 17, 2022, 8:15 a.m. UTC | #6 
Hi Tony,

We also have a pretty good case having the battery on 4.35V for regular
amounts of time at a time is not damageing:

1. For one thing this corroborated by literature about hvlipos.
2. Personally i used the d4 for manny years with andorid without issue,
giveing the battery manny cycles
3. I think so far we have found very few, if any, devices whos batteris
where replaced by thair previous owners, judgeing by the condition of
the stickers and the battery production dates. Even though maemo leste
has access to manny manny devices.

It is true the hvlipos have a lower cycle lifetime than regular 4.35V
lipos when charged to 4.35 than regular lipos when charged to 4.2V,
however it this effect is not as large as you might think.
It is also true that leaving a Lithium cell of any chemistry on Vmax
for long periods of time siginifcantly accelerates degradion, if this
is sufficant cause to drop the "full" soc a couple of percent is a
debateable and reasonable trade off and would be something we should
then apply to all batteries if chosen, not just hvlipos as it affects
regular lipos just the same.

Regards,

Philipp

On Thu, 2022-11-17 at 06:53 +0200, Tony Lindgren wrote:
> * Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com> [221115 15:31]:
> > Hi,
> > 
> > On 15.11.22 г. 15:49 ч., Tony Lindgren wrote:
> > > Hi,
> > > 
> > > * Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com> [221110 16:40]:
> > > > On 10.11.22 г. 18:05 ч., Sebastian Reichel wrote:
> > > > > Why do we care?
> > > > > 
> > > > Because if we know the battery is genuine (or at least pretends
> > > > to be :) ),
> > > > then we can read battery parameters from nvram, see patch 3/3.
> > > > This will
> > > > allow us to charge HV LiPo batteries to 4.35V, using the full
> > > > capacity.
> > > 
> > > Let's not enable charge voltages above 4.2V automatically at all
> > > unless
> > > the user chooses to set a higher charge voltage via sysfs
> > > manually.
> > > 
> > > We have had reports of bloated batteries if left connected to the
> > > charger
> > > at higher voltage than 4.2V. This seems to happen after connected
> > > for some
> > > weeks or months. AFAIK this happens both with Android and
> > > mainline kernel
> > > at higher voltages.
> > > 
> > 
> > Not that I sent such patch yet, but still, thinking about it, we
> > should be
> > able to easily prevent such damage by not restarting the charging
> > after
> > battery is full and voltage has dropped by 50mV or so. There can be
> > a
> > threshold (lets say 4.25 or 4.2) above which charging shall not be
> > re-enabled unless the user reconnects the charger. Even if default
> > stays 4.2
> > and it is the user that has enabled 4.35. Just an idea.
> 
> Sure the logic to handle max charge voltage and maintenance charge
> voltage
> could be there. With commit d4ee021c410f we now just wait for the
> charge
> to come down to 4.2V if charged at 4.35V with Android.
> 
> We still should not enable higher charge voltages by default though.
> It
> still needs to be enabled by the user via sysfs. It's possible that
> also
> shorter peaks of higher charge voltage accelerate the battery
> degration.
> It just may happen slower than what we've seen earlier. To test this,
> multiple devices would need to be left connected to a charger for
> several
> months :)
> 
> Regards,
> 
> Tony
Tony Lindgren Nov. 22, 2022, 7:15 a.m. UTC | #7 
* Carl Klemm <carl@uvos.xyz> [221117 08:06]:
> 2. Personally i used the d4 for manny years with andorid without issue,
> giveing the battery manny cycles

Many cycles does not seem to be the issue, the issue seems to be leaving
the device connected to the charger for longer periods of time at higher
charge voltages.

No objection to having the capability there. But enabling it by default
is a different story. We need several folks test connected to a charger
for months with proper Tested-by if we want to enable it by default.

Regards,

Tony
diff mbox series

Patch

diff --git a/drivers/power/supply/cpcap-battery.c b/drivers/power/supply/cpcap-battery.c
index 8869067..ca6ee2b 100644
--- a/drivers/power/supply/cpcap-battery.c
+++ b/drivers/power/supply/cpcap-battery.c
@@ -422,7 +422,7 @@  static int cpcap_battery_cc_to_ua(struct cpcap_battery_ddata *ddata,
 
 static int cpcap_battery_match_nvmem(struct device *dev, const void *data)
 {
-	if (strcmp(dev_name(dev), "89-500029ba0f73") == 0)
+	if (strncmp(dev_name(dev), "89-500", 6) == 0)
 		return 1;
 	else
 		return 0;
@@ -439,10 +439,19 @@  static void cpcap_battery_detect_battery_type(struct cpcap_battery_ddata *ddata)
 	if (IS_ERR_OR_NULL(nvmem)) {
 		ddata->check_nvmem = true;
 		dev_info_once(ddata->dev, "Can not find battery nvmem device. Assuming generic lipo battery\n");
-	} else if (nvmem_device_read(nvmem, 2, 1, &battery_id) < 0) {
-		battery_id = 0;
-		ddata->check_nvmem = true;
-		dev_warn(ddata->dev, "Can not read battery nvmem device. Assuming generic lipo battery\n");
+	} else {
+		char buf[24];
+
+		if (nvmem_device_read(nvmem, 96, 4, buf) < 0 ||
+		    strncmp(buf, "COPR", 4) != 0 ||
+		    nvmem_device_read(nvmem, 104, 24, buf) < 0 ||
+		    strncmp(buf, "MOTOROLA E.P CHARGE ONLY", 24) != 0 ||
+		    nvmem_device_read(nvmem, 2, 1, &battery_id) < 0) {
+			battery_id = 0;
+			ddata->check_nvmem = true;
+			dev_warn(ddata->dev, "Can not read battery nvmem device. Assuming generic lipo battery\n");
+		}
+
 	}
 
 	switch (battery_id) {