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

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

[Ivaylo Dimitrov]

This patch series implements few improvememts to cpcap-battery driver

[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 | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/power/supply/cpcap-battery.c b/drivers/power/supply/cpcap-battery.c
index 4676560..2659df7 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 timer_list low_timer;
 	u32 cc_lsb;		/* μAms per LSB */
 	atomic_t active;
 	int charge_full;
@@ -914,9 +915,14 @@ 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 &&
+		    !timer_pending(&ddata->low_timer)) {
 			dev_warn(ddata->dev, "Battery low at %imV!\n",
 				latest->voltage / 1000);
+			mod_timer(&ddata->low_timer,
+				  jiffies + msecs_to_jiffies(30000));
+			disable_irq_nosync(d->irq);
+		}
 		break;
 	case CPCAP_BATTERY_IRQ_ACTION_POWEROFF:
 		if (latest->current_ua >= 0 && latest->voltage <= 3200000) {
@@ -1087,6 +1093,19 @@ static int cpcap_battery_calibrate(struct cpcap_battery_ddata *ddata)
 	return error;
 }
 
+static void cpcap_battery_lowbph_enable(struct timer_list *t)
+{
+	struct cpcap_battery_ddata *ddata = from_timer(ddata, t, low_timer);
+	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 +1137,8 @@ static int cpcap_battery_probe(struct platform_device *pdev)
 	if (!ddata)
 		return -ENOMEM;
 
+	timer_setup(&ddata->low_timer, cpcap_battery_lowbph_enable, 0);
+
 	cpcap_battery_detect_battery_type(ddata);
 
 	INIT_LIST_HEAD(&ddata->irq_list);
@@ -1185,6 +1206,8 @@ static int cpcap_battery_remove(struct platform_device *pdev)
 	if (error)
 		dev_err(&pdev->dev, "could not disable: %i\n", error);
 
+	del_timer_sync(&ddata->low_timer);
+
 	return 0;
 }
 
-- 
1.9.1

[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 2659df7..4cfc325 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

[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 4cfc325..5c51685 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 1/3] power: cpcap-battery: Do not issue low signal too frequently

[Ivaylo Dimitrov]

This patch has issues (calling enable_irq() from timer function), will 
send v2 that uses delayed_work.

On 1.11.22 г. 21:53 ч., 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>
> ---
>   drivers/power/supply/cpcap-battery.c | 25 ++++++++++++++++++++++++-
>   1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/power/supply/cpcap-battery.c b/drivers/power/supply/cpcap-battery.c
> index 4676560..2659df7 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 timer_list low_timer;
>   	u32 cc_lsb;		/* μAms per LSB */
>   	atomic_t active;
>   	int charge_full;
> @@ -914,9 +915,14 @@ 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 &&
> +		    !timer_pending(&ddata->low_timer)) {
>   			dev_warn(ddata->dev, "Battery low at %imV!\n",
>   				latest->voltage / 1000);
> +			mod_timer(&ddata->low_timer,
> +				  jiffies + msecs_to_jiffies(30000));
> +			disable_irq_nosync(d->irq);
> +		}
>   		break;
>   	case CPCAP_BATTERY_IRQ_ACTION_POWEROFF:
>   		if (latest->current_ua >= 0 && latest->voltage <= 3200000) {
> @@ -1087,6 +1093,19 @@ static int cpcap_battery_calibrate(struct cpcap_battery_ddata *ddata)
>   	return error;
>   }
>   
> +static void cpcap_battery_lowbph_enable(struct timer_list *t)
> +{
> +	struct cpcap_battery_ddata *ddata = from_timer(ddata, t, low_timer);
> +	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 +1137,8 @@ static int cpcap_battery_probe(struct platform_device *pdev)
>   	if (!ddata)
>   		return -ENOMEM;
>   
> +	timer_setup(&ddata->low_timer, cpcap_battery_lowbph_enable, 0);
> +
>   	cpcap_battery_detect_battery_type(ddata);
>   
>   	INIT_LIST_HEAD(&ddata->irq_list);
> @@ -1185,6 +1206,8 @@ static int cpcap_battery_remove(struct platform_device *pdev)
>   	if (error)
>   		dev_err(&pdev->dev, "could not disable: %i\n", error);
>   
> +	del_timer_sync(&ddata->low_timer);
> +
>   	return 0;
>   }
>   
> 

[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