[PATCH v2 0/3] power: supply: cpcap-battery improvements

[PATCH v2 0/3] power: supply: cpcap-battery improvements

[Ivaylo Dimitrov]

This patch series implements few improvememts in cpcap-battery driver

Changes compared to v1:
[PATCH 1/3] power: cpcap-battery: Do not issue low signal too

use delayed_work instead of timer as enable_irq() should not be called in
atomic context (timer function).

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

no changes

[PATCH 3/3] power: supply: cpcap_battery: Read battery parameters

no changes

[PATCH 1/3] power: cpcap-battery: Do not issue low signal too frequently

[Ivaylo Dimitrov]

It seems that low battery irq may be generated tens of times per second,
leading to userspace being flooded with unnecessary events.

Fix that by preventing such events being generated more than once every 30
seconds.

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

diff --git a/drivers/power/supply/cpcap-battery.c b/drivers/power/supply/cpcap-battery.c
index 4676560..8869067 100644
--- a/drivers/power/supply/cpcap-battery.c
+++ b/drivers/power/supply/cpcap-battery.c
@@ -137,6 +137,7 @@ struct cpcap_battery_ddata {
 	struct power_supply *psy;
 	struct cpcap_battery_config config;
 	struct cpcap_battery_state_data state[CPCAP_BATTERY_STATE_NR];
+	struct delayed_work low_irq_work;
 	u32 cc_lsb;		/* μAms per LSB */
 	atomic_t active;
 	int charge_full;
@@ -914,9 +915,13 @@ static irqreturn_t cpcap_battery_irq_thread(int irq, void *data)
 		dev_info(ddata->dev, "Coulomb counter calibration done\n");
 		break;
 	case CPCAP_BATTERY_IRQ_ACTION_BATTERY_LOW:
-		if (latest->current_ua >= 0)
+		if (latest->current_ua >= 0 &&
+		    !delayed_work_pending((&ddata->low_irq_work))) {
 			dev_warn(ddata->dev, "Battery low at %imV!\n",
 				latest->voltage / 1000);
+			schedule_delayed_work(&ddata->low_irq_work, 30 * HZ);
+			disable_irq_nosync(d->irq);
+		}
 		break;
 	case CPCAP_BATTERY_IRQ_ACTION_POWEROFF:
 		if (latest->current_ua >= 0 && latest->voltage <= 3200000) {
@@ -1087,6 +1092,21 @@ static int cpcap_battery_calibrate(struct cpcap_battery_ddata *ddata)
 	return error;
 }
 
+static void cpcap_battery_lowbph_enable(struct work_struct *work)
+{
+	struct delayed_work *d_work = to_delayed_work(work);
+	struct cpcap_battery_ddata *ddata = container_of(d_work,
+			struct cpcap_battery_ddata, low_irq_work);
+	struct cpcap_interrupt_desc *d;
+
+	list_for_each_entry(d, &ddata->irq_list, node) {
+		if (d->action == CPCAP_BATTERY_IRQ_ACTION_BATTERY_LOW)
+			break;
+	}
+
+	enable_irq(d->irq);
+}
+
 #ifdef CONFIG_OF
 static const struct of_device_id cpcap_battery_id_table[] = {
 	{
@@ -1118,6 +1138,8 @@ static int cpcap_battery_probe(struct platform_device *pdev)
 	if (!ddata)
 		return -ENOMEM;
 
+	INIT_DELAYED_WORK(&ddata->low_irq_work, cpcap_battery_lowbph_enable);
+
 	cpcap_battery_detect_battery_type(ddata);
 
 	INIT_LIST_HEAD(&ddata->irq_list);
@@ -1185,6 +1207,9 @@ static int cpcap_battery_remove(struct platform_device *pdev)
 	if (error)
 		dev_err(&pdev->dev, "could not disable: %i\n", error);
 
+	/* make sure to call enable_irq() if needed */
+	flush_delayed_work(&ddata->low_irq_work);
+
 	return 0;
 }
 
-- 
1.9.1

Re: [PATCH 1/3] power: cpcap-battery: Do not issue low signal too frequently

[Sebastian Reichel]

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

Hi,

On Sat, Nov 05, 2022 at 01:25:42PM +0200, Ivaylo Dimitrov wrote:
> It seems that low battery irq may be generated tens of times per second,
> leading to userspace being flooded with unnecessary events.
> 
> Fix that by preventing such events being generated more than once every 30
> seconds.
> 
> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> ---

Concept looks ok to me, but the code is slightly racy, since the
thread is flushed before the IRQ is disabled in the remove routine.

>  drivers/power/supply/cpcap-battery.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/power/supply/cpcap-battery.c b/drivers/power/supply/cpcap-battery.c
> index 4676560..8869067 100644
> --- a/drivers/power/supply/cpcap-battery.c
> +++ b/drivers/power/supply/cpcap-battery.c
> @@ -137,6 +137,7 @@ struct cpcap_battery_ddata {
>  	struct power_supply *psy;
>  	struct cpcap_battery_config config;
>  	struct cpcap_battery_state_data state[CPCAP_BATTERY_STATE_NR];
> +	struct delayed_work low_irq_work;
>  	u32 cc_lsb;		/* μAms per LSB */
>  	atomic_t active;
>  	int charge_full;
> @@ -914,9 +915,13 @@ static irqreturn_t cpcap_battery_irq_thread(int irq, void *data)
>  		dev_info(ddata->dev, "Coulomb counter calibration done\n");
>  		break;
>  	case CPCAP_BATTERY_IRQ_ACTION_BATTERY_LOW:
> -		if (latest->current_ua >= 0)
> +		if (latest->current_ua >= 0 &&
> +		    !delayed_work_pending((&ddata->low_irq_work))) {
>  			dev_warn(ddata->dev, "Battery low at %imV!\n",
>  				latest->voltage / 1000);
> +			schedule_delayed_work(&ddata->low_irq_work, 30 * HZ);
> +			disable_irq_nosync(d->irq);
> +		}
>  		break;
>  	case CPCAP_BATTERY_IRQ_ACTION_POWEROFF:
>  		if (latest->current_ua >= 0 && latest->voltage <= 3200000) {
> @@ -1087,6 +1092,21 @@ static int cpcap_battery_calibrate(struct cpcap_battery_ddata *ddata)
>  	return error;
>  }
>  
> +static void cpcap_battery_lowbph_enable(struct work_struct *work)
> +{
> +	struct delayed_work *d_work = to_delayed_work(work);
> +	struct cpcap_battery_ddata *ddata = container_of(d_work,
> +			struct cpcap_battery_ddata, low_irq_work);
> +	struct cpcap_interrupt_desc *d;
> +
> +	list_for_each_entry(d, &ddata->irq_list, node) {
> +		if (d->action == CPCAP_BATTERY_IRQ_ACTION_BATTERY_LOW)
> +			break;
> +	}
> +
> +	enable_irq(d->irq);
> +}
> +
>  #ifdef CONFIG_OF
>  static const struct of_device_id cpcap_battery_id_table[] = {
>  	{
> @@ -1118,6 +1138,8 @@ static int cpcap_battery_probe(struct platform_device *pdev)
>  	if (!ddata)
>  		return -ENOMEM;
>  
> +	INIT_DELAYED_WORK(&ddata->low_irq_work, cpcap_battery_lowbph_enable);

use devm_delayed_work_autocancel() and put it directly before
cpcap_battery_init_interrupts().

>  	cpcap_battery_detect_battery_type(ddata);
>  
>  	INIT_LIST_HEAD(&ddata->irq_list);
> @@ -1185,6 +1207,9 @@ static int cpcap_battery_remove(struct platform_device *pdev)
>  	if (error)
>  		dev_err(&pdev->dev, "could not disable: %i\n", error);
>  
> +	/* make sure to call enable_irq() if needed */
> +	flush_delayed_work(&ddata->low_irq_work);

and this can be dropped afterwards.

> +
>  	return 0;
>  }

Thanks,

-- Sebastian

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

Re: [PATCH 1/3] power: cpcap-battery: Do not issue low signal too frequently

[Ivaylo Dimitrov]

Hi,

On 10.11.22 г. 17:55 ч., Sebastian Reichel wrote:
> Hi,
> 
> On Sat, Nov 05, 2022 at 01:25:42PM +0200, Ivaylo Dimitrov wrote:
>> It seems that low battery irq may be generated tens of times per second,
>> leading to userspace being flooded with unnecessary events.
>>
>> Fix that by preventing such events being generated more than once every 30
>> seconds.
>>
>> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
>> ---
> 
> Concept looks ok to me, but the code is slightly racy, since the
> thread is flushed before the IRQ is disabled in the remove routine.
> 

I did that on purpose, to have matching disable_irq()/enable_irq() 
calls. Maybe I over-engineered it, but my understanding is:

When remove() is called, if we have delayed work pending, that means 
that lowbph IRQ was disabled in cpcap_battery_irq_thread() and we have 
to re-enable it. If delayed_work is not pending, flush_delayed_work() 
will do nothing, IIUC.

Maybe I shall protect schedule_delayed_work() and disable_irq_nosync() 
calls with a mutex and wait for it before calling flush_delayed_work() 
in remove? That way there will be guarantee that if delayed_work is 
pending, IRQ is disabled too, while now maybe there is a small time 
window remove() to be called between schedule_delayed_work() and 
disable_irq_nosync().

>>   drivers/power/supply/cpcap-battery.c | 27 ++++++++++++++++++++++++++-
>>   1 file changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/power/supply/cpcap-battery.c b/drivers/power/supply/cpcap-battery.c
>> index 4676560..8869067 100644
>> --- a/drivers/power/supply/cpcap-battery.c
>> +++ b/drivers/power/supply/cpcap-battery.c
>> @@ -137,6 +137,7 @@ struct cpcap_battery_ddata {
>>   	struct power_supply *psy;
>>   	struct cpcap_battery_config config;
>>   	struct cpcap_battery_state_data state[CPCAP_BATTERY_STATE_NR];
>> +	struct delayed_work low_irq_work;
>>   	u32 cc_lsb;		/* μAms per LSB */
>>   	atomic_t active;
>>   	int charge_full;
>> @@ -914,9 +915,13 @@ static irqreturn_t cpcap_battery_irq_thread(int irq, void *data)
>>   		dev_info(ddata->dev, "Coulomb counter calibration done\n");
>>   		break;
>>   	case CPCAP_BATTERY_IRQ_ACTION_BATTERY_LOW:
>> -		if (latest->current_ua >= 0)
>> +		if (latest->current_ua >= 0 &&
>> +		    !delayed_work_pending((&ddata->low_irq_work))) {
>>   			dev_warn(ddata->dev, "Battery low at %imV!\n",
>>   				latest->voltage / 1000);
>> +			schedule_delayed_work(&ddata->low_irq_work, 30 * HZ);
>> +			disable_irq_nosync(d->irq);
>> +		}
>>   		break;
>>   	case CPCAP_BATTERY_IRQ_ACTION_POWEROFF:
>>   		if (latest->current_ua >= 0 && latest->voltage <= 3200000) {
>> @@ -1087,6 +1092,21 @@ static int cpcap_battery_calibrate(struct cpcap_battery_ddata *ddata)
>>   	return error;
>>   }
>>   
>> +static void cpcap_battery_lowbph_enable(struct work_struct *work)
>> +{
>> +	struct delayed_work *d_work = to_delayed_work(work);
>> +	struct cpcap_battery_ddata *ddata = container_of(d_work,
>> +			struct cpcap_battery_ddata, low_irq_work);
>> +	struct cpcap_interrupt_desc *d;
>> +
>> +	list_for_each_entry(d, &ddata->irq_list, node) {
>> +		if (d->action == CPCAP_BATTERY_IRQ_ACTION_BATTERY_LOW)
>> +			break;
>> +	}
>> +
>> +	enable_irq(d->irq);
>> +}
>> +
>>   #ifdef CONFIG_OF
>>   static const struct of_device_id cpcap_battery_id_table[] = {
>>   	{
>> @@ -1118,6 +1138,8 @@ static int cpcap_battery_probe(struct platform_device *pdev)
>>   	if (!ddata)
>>   		return -ENOMEM;
>>   
>> +	INIT_DELAYED_WORK(&ddata->low_irq_work, cpcap_battery_lowbph_enable);
> 
> use devm_delayed_work_autocancel() and put it directly before
> cpcap_battery_init_interrupts().
> 
>>   	cpcap_battery_detect_battery_type(ddata);
>>   
>>   	INIT_LIST_HEAD(&ddata->irq_list);
>> @@ -1185,6 +1207,9 @@ static int cpcap_battery_remove(struct platform_device *pdev)
>>   	if (error)
>>   		dev_err(&pdev->dev, "could not disable: %i\n", error);
>>   
>> +	/* make sure to call enable_irq() if needed */
>> +	flush_delayed_work(&ddata->low_irq_work);
> 
> and this can be dropped afterwards.
> 

Ok, but what will happen if we have lowbph IRQ already disabled? 
Wouldn't that cause issues, because of unbalanced 
disable_irq()/enable_irq() calls?

Thanks,
Ivo

>> +
>>   	return 0;
>>   }
> 
> Thanks,
> 
> -- Sebastian
> 

Re: [PATCH 1/3] power: cpcap-battery: Do not issue low signal too frequently

[Dan Carpenter]

Hi Ivaylo,

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ivaylo-Dimitrov/power-supply-cpcap-battery-improvements/20221105-192758
base:   https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git for-next
patch link:    https://lore.kernel.org/r/1667647544-12945-2-git-send-email-ivo.g.dimitrov.75%40gmail.com
patch subject: [PATCH 1/3] power: cpcap-battery: Do not issue low signal too frequently
config: m68k-randconfig-m041-20221110
compiler: m68k-linux-gcc (GCC) 12.1.0

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>

smatch warnings:
drivers/power/supply/cpcap-battery.c:1084 cpcap_battery_lowbph_enable() warn: iterator used outside loop: 'd'

vim +/d +1084 drivers/power/supply/cpcap-battery.c

ce48d112324af4 Ivaylo Dimitrov 2022-11-05  1072  static void cpcap_battery_lowbph_enable(struct work_struct *work)
ce48d112324af4 Ivaylo Dimitrov 2022-11-05  1073  {
ce48d112324af4 Ivaylo Dimitrov 2022-11-05  1074  	struct delayed_work *d_work = to_delayed_work(work);
ce48d112324af4 Ivaylo Dimitrov 2022-11-05  1075  	struct cpcap_battery_ddata *ddata = container_of(d_work,
ce48d112324af4 Ivaylo Dimitrov 2022-11-05  1076  			struct cpcap_battery_ddata, low_irq_work);
ce48d112324af4 Ivaylo Dimitrov 2022-11-05  1077  	struct cpcap_interrupt_desc *d;
ce48d112324af4 Ivaylo Dimitrov 2022-11-05  1078  
ce48d112324af4 Ivaylo Dimitrov 2022-11-05  1079  	list_for_each_entry(d, &ddata->irq_list, node) {
ce48d112324af4 Ivaylo Dimitrov 2022-11-05  1080  		if (d->action == CPCAP_BATTERY_IRQ_ACTION_BATTERY_LOW)
ce48d112324af4 Ivaylo Dimitrov 2022-11-05  1081  			break;
ce48d112324af4 Ivaylo Dimitrov 2022-11-05  1082  	}
ce48d112324af4 Ivaylo Dimitrov 2022-11-05  1083  
ce48d112324af4 Ivaylo Dimitrov 2022-11-05 @1084  	enable_irq(d->irq);

If we exit the loop without hitting a break then "d" is not a valid
pointer and "enable_irq(d->irq);" will do something bad.

ce48d112324af4 Ivaylo Dimitrov 2022-11-05  1085  }

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

[Ivaylo Dimitrov]

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

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

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

[Sebastian Reichel]

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

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
> 

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

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

[Ivaylo Dimitrov]

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

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

[Tony Lindgren]

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

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

[Ivaylo Dimitrov]

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
> 

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

[Tony Lindgren]

* 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

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

[Carl Klemm]

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

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

[Tony Lindgren]

* 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

[PATCH 3/3] power: supply: cpcap_battery: Read battery parameters from nvram

[Ivaylo Dimitrov]

Formulas were taken from android blob

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

diff --git a/drivers/power/supply/cpcap-battery.c b/drivers/power/supply/cpcap-battery.c
index ca6ee2b..92aa66c 100644
--- a/drivers/power/supply/cpcap-battery.c
+++ b/drivers/power/supply/cpcap-battery.c
@@ -74,9 +74,6 @@
 
 #define CPCAP_BATTERY_CC_SAMPLE_PERIOD_MS	250
 
-#define CPCAP_BATTERY_EB41_HW4X_ID 0x9E
-#define CPCAP_BATTERY_BW8X_ID 0x98
-
 enum {
 	CPCAP_BATTERY_IIO_BATTDET,
 	CPCAP_BATTERY_IIO_VOLTAGE,
@@ -388,22 +385,9 @@ static int cpcap_battery_cc_to_ua(struct cpcap_battery_ddata *ddata,
  * kernel on droid 4, full is 4351000 and software initiates shutdown
  * at 3078000. The device will die around 2743000.
  */
-static const struct cpcap_battery_config cpcap_battery_eb41_data = {
-	.cd_factor = 0x3cc,
-	.info.technology = POWER_SUPPLY_TECHNOLOGY_LION,
-	.info.voltage_max_design = 4351000,
-	.info.voltage_min_design = 3100000,
-	.info.charge_full_design = 1740000,
-	.bat.constant_charge_voltage_max_uv = 4200000,
-};
-
-/* Values for the extended Droid Bionic battery bw8x. */
-static const struct cpcap_battery_config cpcap_battery_bw8x_data = {
+static struct cpcap_battery_config cpcap_battery_mot_data = {
 	.cd_factor = 0x3cc,
 	.info.technology = POWER_SUPPLY_TECHNOLOGY_LION,
-	.info.voltage_max_design = 4200000,
-	.info.voltage_min_design = 3200000,
-	.info.charge_full_design = 2760000,
 	.bat.constant_charge_voltage_max_uv = 4200000,
 };
 
@@ -431,39 +415,63 @@ static int cpcap_battery_match_nvmem(struct device *dev, const void *data)
 static void cpcap_battery_detect_battery_type(struct cpcap_battery_ddata *ddata)
 {
 	struct nvmem_device *nvmem;
-	u8 battery_id = 0;
+	char buf[24];
+	u8 capacity;
+	u8 mul_idx;
+	u8 charge_voltage;
+	u32 v;
+	static const u32 multipliers[] = {20, 10, 10, 10, 10, 40, 10, 20, 40};
 
 	ddata->check_nvmem = false;
 
 	nvmem = nvmem_device_find(NULL, &cpcap_battery_match_nvmem);
 	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 {
-		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");
-		}
+		goto unknown;
+	}
 
+	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) {
+		dev_warn(ddata->dev, "Unknown battery nvmem device. Assuming generic lipo battery\n");
+		goto unknown;
 	}
 
-	switch (battery_id) {
-	case CPCAP_BATTERY_EB41_HW4X_ID:
-		ddata->config = cpcap_battery_eb41_data;
-		break;
-	case CPCAP_BATTERY_BW8X_ID:
-		ddata->config = cpcap_battery_bw8x_data;
-		break;
-	default:
-		ddata->config = cpcap_battery_unkown_data;
+	if (nvmem_device_read(nvmem, 49, 1, &mul_idx) < 0 ||
+	    nvmem_device_read(nvmem, 34, 1, &capacity) < 0 ||
+	    nvmem_device_read(nvmem, 65, 1, &charge_voltage) < 0) {
+		dev_warn(ddata->dev, "Can not read battery nvmem device. Assuming generic lipo battery\n");
+		goto unknown;
 	}
+
+	/* design capacity */
+	mul_idx -= 2;
+
+	if (mul_idx < ARRAY_SIZE(multipliers))
+		v = multipliers[mul_idx];
+	else
+		v = 10;
+
+	cpcap_battery_mot_data.info.charge_full_design = 1000 * v * capacity;
+
+	/* design max voltage */
+	v = 1000 * ((16702 * charge_voltage) / 1000 + 1260);
+	cpcap_battery_mot_data.info.voltage_max_design = v;
+
+	/* design min voltage */
+	if (v > 4200000)
+		cpcap_battery_mot_data.info.voltage_min_design = 3100000;
+	else
+		cpcap_battery_mot_data.info.voltage_min_design = 3200000;
+
+	ddata->config = cpcap_battery_mot_data;
+
+	return;
+
+unknown:
+	ddata->check_nvmem = true;
+	ddata->config = cpcap_battery_unkown_data;
 }
 
 /**
-- 
1.9.1

Re: [PATCH 3/3] power: supply: cpcap_battery: Read battery parameters from nvram

[Ivaylo Dimitrov]

ping

On 5.11.22 г. 13:25 ч., Ivaylo Dimitrov wrote:
> Formulas were taken from android blob
> 
> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> ---
>   drivers/power/supply/cpcap-battery.c | 88 ++++++++++++++++++++----------------
>   1 file changed, 48 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/power/supply/cpcap-battery.c b/drivers/power/supply/cpcap-battery.c
> index ca6ee2b..92aa66c 100644
> --- a/drivers/power/supply/cpcap-battery.c
> +++ b/drivers/power/supply/cpcap-battery.c
> @@ -74,9 +74,6 @@
>   
>   #define CPCAP_BATTERY_CC_SAMPLE_PERIOD_MS	250
>   
> -#define CPCAP_BATTERY_EB41_HW4X_ID 0x9E
> -#define CPCAP_BATTERY_BW8X_ID 0x98
> -
>   enum {
>   	CPCAP_BATTERY_IIO_BATTDET,
>   	CPCAP_BATTERY_IIO_VOLTAGE,
> @@ -388,22 +385,9 @@ static int cpcap_battery_cc_to_ua(struct cpcap_battery_ddata *ddata,
>    * kernel on droid 4, full is 4351000 and software initiates shutdown
>    * at 3078000. The device will die around 2743000.
>    */
> -static const struct cpcap_battery_config cpcap_battery_eb41_data = {
> -	.cd_factor = 0x3cc,
> -	.info.technology = POWER_SUPPLY_TECHNOLOGY_LION,
> -	.info.voltage_max_design = 4351000,
> -	.info.voltage_min_design = 3100000,
> -	.info.charge_full_design = 1740000,
> -	.bat.constant_charge_voltage_max_uv = 4200000,
> -};
> -
> -/* Values for the extended Droid Bionic battery bw8x. */
> -static const struct cpcap_battery_config cpcap_battery_bw8x_data = {
> +static struct cpcap_battery_config cpcap_battery_mot_data = {
>   	.cd_factor = 0x3cc,
>   	.info.technology = POWER_SUPPLY_TECHNOLOGY_LION,
> -	.info.voltage_max_design = 4200000,
> -	.info.voltage_min_design = 3200000,
> -	.info.charge_full_design = 2760000,
>   	.bat.constant_charge_voltage_max_uv = 4200000,
>   };
>   
> @@ -431,39 +415,63 @@ static int cpcap_battery_match_nvmem(struct device *dev, const void *data)
>   static void cpcap_battery_detect_battery_type(struct cpcap_battery_ddata *ddata)
>   {
>   	struct nvmem_device *nvmem;
> -	u8 battery_id = 0;
> +	char buf[24];
> +	u8 capacity;
> +	u8 mul_idx;
> +	u8 charge_voltage;
> +	u32 v;
> +	static const u32 multipliers[] = {20, 10, 10, 10, 10, 40, 10, 20, 40};
>   
>   	ddata->check_nvmem = false;
>   
>   	nvmem = nvmem_device_find(NULL, &cpcap_battery_match_nvmem);
>   	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 {
> -		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");
> -		}
> +		goto unknown;
> +	}
>   
> +	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) {
> +		dev_warn(ddata->dev, "Unknown battery nvmem device. Assuming generic lipo battery\n");
> +		goto unknown;
>   	}
>   
> -	switch (battery_id) {
> -	case CPCAP_BATTERY_EB41_HW4X_ID:
> -		ddata->config = cpcap_battery_eb41_data;
> -		break;
> -	case CPCAP_BATTERY_BW8X_ID:
> -		ddata->config = cpcap_battery_bw8x_data;
> -		break;
> -	default:
> -		ddata->config = cpcap_battery_unkown_data;
> +	if (nvmem_device_read(nvmem, 49, 1, &mul_idx) < 0 ||
> +	    nvmem_device_read(nvmem, 34, 1, &capacity) < 0 ||
> +	    nvmem_device_read(nvmem, 65, 1, &charge_voltage) < 0) {
> +		dev_warn(ddata->dev, "Can not read battery nvmem device. Assuming generic lipo battery\n");
> +		goto unknown;
>   	}
> +
> +	/* design capacity */
> +	mul_idx -= 2;
> +
> +	if (mul_idx < ARRAY_SIZE(multipliers))
> +		v = multipliers[mul_idx];
> +	else
> +		v = 10;
> +
> +	cpcap_battery_mot_data.info.charge_full_design = 1000 * v * capacity;
> +
> +	/* design max voltage */
> +	v = 1000 * ((16702 * charge_voltage) / 1000 + 1260);
> +	cpcap_battery_mot_data.info.voltage_max_design = v;
> +
> +	/* design min voltage */
> +	if (v > 4200000)
> +		cpcap_battery_mot_data.info.voltage_min_design = 3100000;
> +	else
> +		cpcap_battery_mot_data.info.voltage_min_design = 3200000;
> +
> +	ddata->config = cpcap_battery_mot_data;
> +
> +	return;
> +
> +unknown:
> +	ddata->check_nvmem = true;
> +	ddata->config = cpcap_battery_unkown_data;
>   }
>   
>   /**
>