[PATCH 0/4] driver/power/jz4740-battery: driver cleanup

[PATCH 0/4] driver/power/jz4740-battery: driver cleanup

[Marcos Paulo de Souza]

Hi guys,

This patchset is just a cleanup using devm_* functions. They were
tested in Dingoo A320 for two days, without any problems.

I hope you enjoy :)

Marcos Paulo de Souza (4):
  Use devm_kzalloc instead of kzalloc/kfree
  Use devm_* for reques_mem_region and ioremap
  Use devm_request_irq to avoid some checks
  power/jz4740.c: Simplify exit of function

 drivers/power/jz4740-battery.c |   53 ++++++++++++----------------------------
 1 file changed, 15 insertions(+), 38 deletions(-)

-- 
1.7.9.5

[PATCH 1/4] Use devm_kzalloc instead of kzalloc/kfree

[Marcos Paulo de Souza]

This does not chnange the rationale, just a cleanup

Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>
---
 drivers/power/jz4740-battery.c |   12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/power/jz4740-battery.c b/drivers/power/jz4740-battery.c
index 8dbc7bf..72f6335 100644
--- a/drivers/power/jz4740-battery.c
+++ b/drivers/power/jz4740-battery.c
@@ -252,7 +252,7 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
 		return -ENXIO;
 	}
 
-	jz_battery = kzalloc(sizeof(*jz_battery), GFP_KERNEL);
+	jz_battery = devm_kzalloc(&pdev->dev, sizeof(*jz_battery), GFP_KERNEL);
 	if (!jz_battery) {
 		dev_err(&pdev->dev, "Failed to allocate driver structure\n");
 		return -ENOMEM;
@@ -264,14 +264,14 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
 	if (jz_battery->irq < 0) {
 		ret = jz_battery->irq;
 		dev_err(&pdev->dev, "Failed to get platform irq: %d\n", ret);
-		goto err_free;
+		goto err;
 	}
 
 	jz_battery->mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!jz_battery->mem) {
 		ret = -ENOENT;
 		dev_err(&pdev->dev, "Failed to get platform mmio resource\n");
-		goto err_free;
+		goto err;
 	}
 
 	jz_battery->mem = request_mem_region(jz_battery->mem->start,
@@ -279,7 +279,7 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
 	if (!jz_battery->mem) {
 		ret = -EBUSY;
 		dev_err(&pdev->dev, "Failed to request mmio memory region\n");
-		goto err_free;
+		goto err;
 	}
 
 	jz_battery->base = ioremap_nocache(jz_battery->mem->start,
@@ -373,8 +373,7 @@ err_iounmap:
 	iounmap(jz_battery->base);
 err_release_mem_region:
 	release_mem_region(jz_battery->mem->start, resource_size(jz_battery->mem));
-err_free:
-	kfree(jz_battery);
+err:
 	return ret;
 }
 
@@ -396,7 +395,6 @@ static int __devexit jz_battery_remove(struct platform_device *pdev)
 
 	iounmap(jz_battery->base);
 	release_mem_region(jz_battery->mem->start, resource_size(jz_battery->mem));
-	kfree(jz_battery);
 
 	return 0;
 }
-- 
1.7.9.5

[PATCH 2/4] Use devm_* for request_mem_region and ioremap

[Marcos Paulo de Souza]

This don't change the rationale, just a cleanup.

Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>
---
 drivers/power/jz4740-battery.c |   15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/power/jz4740-battery.c b/drivers/power/jz4740-battery.c
index 72f6335..9d9b2a8 100644
--- a/drivers/power/jz4740-battery.c
+++ b/drivers/power/jz4740-battery.c
@@ -274,7 +274,7 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
 		goto err;
 	}
 
-	jz_battery->mem = request_mem_region(jz_battery->mem->start,
+	jz_battery->mem = devm_request_mem_region(&pdev->dev, jz_battery->mem->start,
 				resource_size(jz_battery->mem),	pdev->name);
 	if (!jz_battery->mem) {
 		ret = -EBUSY;
@@ -282,12 +282,12 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
 		goto err;
 	}
 
-	jz_battery->base = ioremap_nocache(jz_battery->mem->start,
+	jz_battery->base = devm_ioremap_nocache(&pdev->dev, jz_battery->mem->start,
 				resource_size(jz_battery->mem));
 	if (!jz_battery->base) {
 		ret = -EBUSY;
 		dev_err(&pdev->dev, "Failed to ioremap mmio memory\n");
-		goto err_release_mem_region;
+		goto err;
 	}
 
 	battery = &jz_battery->battery;
@@ -311,7 +311,7 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
 			jz_battery);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to request irq %d\n", ret);
-		goto err_iounmap;
+		goto err;
 	}
 	disable_irq(jz_battery->irq);
 
@@ -368,11 +368,7 @@ err_free_gpio:
 		gpio_free(jz_battery->pdata->gpio_charge);
 err_free_irq:
 	free_irq(jz_battery->irq, jz_battery);
-err_iounmap:
 	platform_set_drvdata(pdev, NULL);
-	iounmap(jz_battery->base);
-err_release_mem_region:
-	release_mem_region(jz_battery->mem->start, resource_size(jz_battery->mem));
 err:
 	return ret;
 }
@@ -393,9 +389,6 @@ static int __devexit jz_battery_remove(struct platform_device *pdev)
 
 	free_irq(jz_battery->irq, jz_battery);
 
-	iounmap(jz_battery->base);
-	release_mem_region(jz_battery->mem->start, resource_size(jz_battery->mem));
-
 	return 0;
 }
 
-- 
1.7.9.5

[PATCH 3/4] Use devm_request_irq to avoid some checks

[Marcos Paulo de Souza]

With this we can remove a lot of checks.

Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>
---
 drivers/power/jz4740-battery.c |   25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/drivers/power/jz4740-battery.c b/drivers/power/jz4740-battery.c
index 9d9b2a8..b80f7ed 100644
--- a/drivers/power/jz4740-battery.c
+++ b/drivers/power/jz4740-battery.c
@@ -307,8 +307,8 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
 
 	INIT_DELAYED_WORK(&jz_battery->work, jz_battery_work);
 
-	ret = request_irq(jz_battery->irq, jz_battery_irq_handler, 0, pdev->name,
-			jz_battery);
+	ret = devm_request_irq(&pdev->dev, jz_battery->irq, jz_battery_irq_handler, 
+				0, pdev->name, jz_battery);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to request irq %d\n", ret);
 		goto err;
@@ -319,7 +319,7 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
 		ret = gpio_request(pdata->gpio_charge, dev_name(&pdev->dev));
 		if (ret) {
 			dev_err(&pdev->dev, "charger state gpio request failed.\n");
-			goto err_free_irq;
+			goto err;
 		}
 		ret = gpio_direction_input(pdata->gpio_charge);
 		if (ret) {
@@ -330,7 +330,8 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
 		jz_battery->charge_irq = gpio_to_irq(pdata->gpio_charge);
 
 		if (jz_battery->charge_irq >= 0) {
-			ret = request_irq(jz_battery->charge_irq,
+			ret = devm_request_irq(&pdev->dev,
+				    jz_battery->charge_irq,
 				    jz_battery_charge_irq,
 				    IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
 				    dev_name(&pdev->dev), jz_battery);
@@ -352,7 +353,7 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
 	ret = power_supply_register(&pdev->dev, &jz_battery->battery);
 	if (ret) {
 		dev_err(&pdev->dev, "power supply battery register failed.\n");
-		goto err_free_charge_irq;
+		goto err_free_gpio;
 	}
 
 	platform_set_drvdata(pdev, jz_battery);
@@ -360,16 +361,11 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
 
 	return 0;
 
-err_free_charge_irq:
-	if (jz_battery->charge_irq >= 0)
-		free_irq(jz_battery->charge_irq, jz_battery);
 err_free_gpio:
 	if (gpio_is_valid(pdata->gpio_charge))
 		gpio_free(jz_battery->pdata->gpio_charge);
-err_free_irq:
-	free_irq(jz_battery->irq, jz_battery);
-	platform_set_drvdata(pdev, NULL);
 err:
+	platform_set_drvdata(pdev, NULL);
 	return ret;
 }
 
@@ -379,16 +375,11 @@ static int __devexit jz_battery_remove(struct platform_device *pdev)
 
 	cancel_delayed_work_sync(&jz_battery->work);
 
-	if (gpio_is_valid(jz_battery->pdata->gpio_charge)) {
-		if (jz_battery->charge_irq >= 0)
-			free_irq(jz_battery->charge_irq, jz_battery);
+	if (gpio_is_valid(jz_battery->pdata->gpio_charge))
 		gpio_free(jz_battery->pdata->gpio_charge);
-	}
 
 	power_supply_unregister(&jz_battery->battery);
 
-	free_irq(jz_battery->irq, jz_battery);
-
 	return 0;
 }
 
-- 
1.7.9.5

[PATCH 4/4] power/jz4740.c: Simplify exit of function

[Marcos Paulo de Souza]

This commit simplifies the exit of probe function, by returning
directly when it don't need to release any resources.

Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>
---
 drivers/power/jz4740-battery.c |   17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/power/jz4740-battery.c b/drivers/power/jz4740-battery.c
index b80f7ed..0d6c9e4 100644
--- a/drivers/power/jz4740-battery.c
+++ b/drivers/power/jz4740-battery.c
@@ -262,32 +262,28 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
 
 	jz_battery->irq = platform_get_irq(pdev, 0);
 	if (jz_battery->irq < 0) {
-		ret = jz_battery->irq;
 		dev_err(&pdev->dev, "Failed to get platform irq: %d\n", ret);
-		goto err;
+		return jz_battery->irq;
 	}
 
 	jz_battery->mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!jz_battery->mem) {
-		ret = -ENOENT;
 		dev_err(&pdev->dev, "Failed to get platform mmio resource\n");
-		goto err;
+		return -ENOENT;
 	}
 
 	jz_battery->mem = devm_request_mem_region(&pdev->dev, jz_battery->mem->start,
 				resource_size(jz_battery->mem),	pdev->name);
 	if (!jz_battery->mem) {
-		ret = -EBUSY;
 		dev_err(&pdev->dev, "Failed to request mmio memory region\n");
-		goto err;
+		return -EBUSY;
 	}
 
 	jz_battery->base = devm_ioremap_nocache(&pdev->dev, jz_battery->mem->start,
 				resource_size(jz_battery->mem));
 	if (!jz_battery->base) {
-		ret = -EBUSY;
 		dev_err(&pdev->dev, "Failed to ioremap mmio memory\n");
-		goto err;
+		return -EBUSY;
 	}
 
 	battery = &jz_battery->battery;
@@ -311,7 +307,7 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
 				0, pdev->name, jz_battery);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to request irq %d\n", ret);
-		goto err;
+		return ret;
 	}
 	disable_irq(jz_battery->irq);
 
@@ -319,7 +315,7 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
 		ret = gpio_request(pdata->gpio_charge, dev_name(&pdev->dev));
 		if (ret) {
 			dev_err(&pdev->dev, "charger state gpio request failed.\n");
-			goto err;
+			return ret;
 		}
 		ret = gpio_direction_input(pdata->gpio_charge);
 		if (ret) {
@@ -364,7 +360,6 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
 err_free_gpio:
 	if (gpio_is_valid(pdata->gpio_charge))
 		gpio_free(jz_battery->pdata->gpio_charge);
-err:
 	platform_set_drvdata(pdev, NULL);
 	return ret;
 }
-- 
1.7.9.5

Re: [PATCH 1/4] Use devm_kzalloc instead of kzalloc/kfree

[Lars-Peter Clausen]

On 10/19/2012 01:41 AM, Marcos Paulo de Souza wrote:
> This does not chnange the rationale, just a cleanup
> 
> Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>

Thanks,

Acked-by: Lars-Peter Clausen <lars@metafoo.de?

> ---
>  drivers/power/jz4740-battery.c |   12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/power/jz4740-battery.c b/drivers/power/jz4740-battery.c
> index 8dbc7bf..72f6335 100644
> --- a/drivers/power/jz4740-battery.c
> +++ b/drivers/power/jz4740-battery.c
> @@ -252,7 +252,7 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
>  		return -ENXIO;
>  	}
>  
> -	jz_battery = kzalloc(sizeof(*jz_battery), GFP_KERNEL);
> +	jz_battery = devm_kzalloc(&pdev->dev, sizeof(*jz_battery), GFP_KERNEL);
>  	if (!jz_battery) {
>  		dev_err(&pdev->dev, "Failed to allocate driver structure\n");
>  		return -ENOMEM;
> @@ -264,14 +264,14 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
>  	if (jz_battery->irq < 0) {
>  		ret = jz_battery->irq;
>  		dev_err(&pdev->dev, "Failed to get platform irq: %d\n", ret);
> -		goto err_free;
> +		goto err;
>  	}
>  
>  	jz_battery->mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	if (!jz_battery->mem) {
>  		ret = -ENOENT;
>  		dev_err(&pdev->dev, "Failed to get platform mmio resource\n");
> -		goto err_free;
> +		goto err;
>  	}
>  
>  	jz_battery->mem = request_mem_region(jz_battery->mem->start,
> @@ -279,7 +279,7 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
>  	if (!jz_battery->mem) {
>  		ret = -EBUSY;
>  		dev_err(&pdev->dev, "Failed to request mmio memory region\n");
> -		goto err_free;
> +		goto err;
>  	}
>  
>  	jz_battery->base = ioremap_nocache(jz_battery->mem->start,
> @@ -373,8 +373,7 @@ err_iounmap:
>  	iounmap(jz_battery->base);
>  err_release_mem_region:
>  	release_mem_region(jz_battery->mem->start, resource_size(jz_battery->mem));
> -err_free:
> -	kfree(jz_battery);
> +err:
>  	return ret;
>  }
>  
> @@ -396,7 +395,6 @@ static int __devexit jz_battery_remove(struct platform_device *pdev)
>  
>  	iounmap(jz_battery->base);
>  	release_mem_region(jz_battery->mem->start, resource_size(jz_battery->mem));
> -	kfree(jz_battery);
>  
>  	return 0;
>  }

Re: [PATCH 2/4] Use devm_* for request_mem_region and ioremap

[Lars-Peter Clausen]

On 10/19/2012 01:41 AM, Marcos Paulo de Souza wrote:
> This don't change the rationale, just a cleanup.
> 
> Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>

Patch is ok in general, but it would be even better if you'd use
devm_request_and_ioremap. It does both the devm_request_mem_region and
devm_ioremap_nocache. It also checks whether the passed in memory region is
NULL and will print error messages on it's own.

Care to resend the patch with this change?

Thanks,
- Lars

> ---
>  drivers/power/jz4740-battery.c |   15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/power/jz4740-battery.c b/drivers/power/jz4740-battery.c
> index 72f6335..9d9b2a8 100644
> --- a/drivers/power/jz4740-battery.c
> +++ b/drivers/power/jz4740-battery.c
> @@ -274,7 +274,7 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
>  		goto err;
>  	}
>  
> -	jz_battery->mem = request_mem_region(jz_battery->mem->start,
> +	jz_battery->mem = devm_request_mem_region(&pdev->dev, jz_battery->mem->start,
>  				resource_size(jz_battery->mem),	pdev->name);
>  	if (!jz_battery->mem) {
>  		ret = -EBUSY;
> @@ -282,12 +282,12 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
>  		goto err;
>  	}
>  
> -	jz_battery->base = ioremap_nocache(jz_battery->mem->start,
> +	jz_battery->base = devm_ioremap_nocache(&pdev->dev, jz_battery->mem->start,
>  				resource_size(jz_battery->mem));
>  	if (!jz_battery->base) {
>  		ret = -EBUSY;
>  		dev_err(&pdev->dev, "Failed to ioremap mmio memory\n");
> -		goto err_release_mem_region;
> +		goto err;
>  	}
>  
>  	battery = &jz_battery->battery;
> @@ -311,7 +311,7 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
>  			jz_battery);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Failed to request irq %d\n", ret);
> -		goto err_iounmap;
> +		goto err;
>  	}
>  	disable_irq(jz_battery->irq);
>  
> @@ -368,11 +368,7 @@ err_free_gpio:
>  		gpio_free(jz_battery->pdata->gpio_charge);
>  err_free_irq:
>  	free_irq(jz_battery->irq, jz_battery);
> -err_iounmap:
>  	platform_set_drvdata(pdev, NULL);
> -	iounmap(jz_battery->base);
> -err_release_mem_region:
> -	release_mem_region(jz_battery->mem->start, resource_size(jz_battery->mem));
>  err:
>  	return ret;
>  }
> @@ -393,9 +389,6 @@ static int __devexit jz_battery_remove(struct platform_device *pdev)
>  
>  	free_irq(jz_battery->irq, jz_battery);
>  
> -	iounmap(jz_battery->base);
> -	release_mem_region(jz_battery->mem->start, resource_size(jz_battery->mem));
> -
>  	return 0;
>  }
>  

Re: [PATCH 3/4] Use devm_request_irq to avoid some checks

[Lars-Peter Clausen]

On 10/19/2012 01:41 AM, Marcos Paulo de Souza wrote:
> With this we can remove a lot of checks.
> 
> Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>

This one is a bit more tricky and the driver currently gets it (partially
wrong). The issue is that since the power supply is referenced in the
interrupt handler, the IRQs should be requested after the power supply has
been registered and be freed before the power supply is unregistered.

So devm can't really be used here. But as I said the current driver already
gets it wrong.

- Lars

> ---
>  drivers/power/jz4740-battery.c |   25 ++++++++-----------------
>  1 file changed, 8 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/power/jz4740-battery.c b/drivers/power/jz4740-battery.c
> index 9d9b2a8..b80f7ed 100644
> --- a/drivers/power/jz4740-battery.c
> +++ b/drivers/power/jz4740-battery.c
> @@ -307,8 +307,8 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
>  
>  	INIT_DELAYED_WORK(&jz_battery->work, jz_battery_work);
>  
> -	ret = request_irq(jz_battery->irq, jz_battery_irq_handler, 0, pdev->name,
> -			jz_battery);
> +	ret = devm_request_irq(&pdev->dev, jz_battery->irq, jz_battery_irq_handler, 
> +				0, pdev->name, jz_battery);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Failed to request irq %d\n", ret);
>  		goto err;
> @@ -319,7 +319,7 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
>  		ret = gpio_request(pdata->gpio_charge, dev_name(&pdev->dev));
>  		if (ret) {
>  			dev_err(&pdev->dev, "charger state gpio request failed.\n");
> -			goto err_free_irq;
> +			goto err;
>  		}
>  		ret = gpio_direction_input(pdata->gpio_charge);
>  		if (ret) {
> @@ -330,7 +330,8 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
>  		jz_battery->charge_irq = gpio_to_irq(pdata->gpio_charge);
>  
>  		if (jz_battery->charge_irq >= 0) {
> -			ret = request_irq(jz_battery->charge_irq,
> +			ret = devm_request_irq(&pdev->dev,
> +				    jz_battery->charge_irq,
>  				    jz_battery_charge_irq,
>  				    IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
>  				    dev_name(&pdev->dev), jz_battery);
> @@ -352,7 +353,7 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
>  	ret = power_supply_register(&pdev->dev, &jz_battery->battery);
>  	if (ret) {
>  		dev_err(&pdev->dev, "power supply battery register failed.\n");
> -		goto err_free_charge_irq;
> +		goto err_free_gpio;
>  	}
>  
>  	platform_set_drvdata(pdev, jz_battery);
> @@ -360,16 +361,11 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
>  
>  	return 0;
>  
> -err_free_charge_irq:
> -	if (jz_battery->charge_irq >= 0)
> -		free_irq(jz_battery->charge_irq, jz_battery);
>  err_free_gpio:
>  	if (gpio_is_valid(pdata->gpio_charge))
>  		gpio_free(jz_battery->pdata->gpio_charge);
> -err_free_irq:
> -	free_irq(jz_battery->irq, jz_battery);
> -	platform_set_drvdata(pdev, NULL);
>  err:
> +	platform_set_drvdata(pdev, NULL);
>  	return ret;
>  }
>  
> @@ -379,16 +375,11 @@ static int __devexit jz_battery_remove(struct platform_device *pdev)
>  
>  	cancel_delayed_work_sync(&jz_battery->work);
>  
> -	if (gpio_is_valid(jz_battery->pdata->gpio_charge)) {
> -		if (jz_battery->charge_irq >= 0)
> -			free_irq(jz_battery->charge_irq, jz_battery);
> +	if (gpio_is_valid(jz_battery->pdata->gpio_charge))
>  		gpio_free(jz_battery->pdata->gpio_charge);
> -	}
>  
>  	power_supply_unregister(&jz_battery->battery);
>  
> -	free_irq(jz_battery->irq, jz_battery);
> -
>  	return 0;
>  }
>  

Re: [PATCH 4/4] power/jz4740.c: Simplify exit of function

[Lars-Peter Clausen]

On 10/19/2012 01:41 AM, Marcos Paulo de Souza wrote:
> This commit simplifies the exit of probe function, by returning
> directly when it don't need to release any resources.
> 
> Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>

Looks good, but I'd just fold the relevant parts in the previous patches.

> ---
>  drivers/power/jz4740-battery.c |   17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/power/jz4740-battery.c b/drivers/power/jz4740-battery.c
> index b80f7ed..0d6c9e4 100644
> --- a/drivers/power/jz4740-battery.c
> +++ b/drivers/power/jz4740-battery.c
> @@ -262,32 +262,28 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
>  
>  	jz_battery->irq = platform_get_irq(pdev, 0);
>  	if (jz_battery->irq < 0) {
> -		ret = jz_battery->irq;
>  		dev_err(&pdev->dev, "Failed to get platform irq: %d\n", ret);
> -		goto err;
> +		return jz_battery->irq;
>  	}
>  
>  	jz_battery->mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	if (!jz_battery->mem) {
> -		ret = -ENOENT;
>  		dev_err(&pdev->dev, "Failed to get platform mmio resource\n");
> -		goto err;
> +		return -ENOENT;
>  	}
>  
>  	jz_battery->mem = devm_request_mem_region(&pdev->dev, jz_battery->mem->start,
>  				resource_size(jz_battery->mem),	pdev->name);
>  	if (!jz_battery->mem) {
> -		ret = -EBUSY;
>  		dev_err(&pdev->dev, "Failed to request mmio memory region\n");
> -		goto err;
> +		return -EBUSY;
>  	}
>  
>  	jz_battery->base = devm_ioremap_nocache(&pdev->dev, jz_battery->mem->start,
>  				resource_size(jz_battery->mem));
>  	if (!jz_battery->base) {
> -		ret = -EBUSY;
>  		dev_err(&pdev->dev, "Failed to ioremap mmio memory\n");
> -		goto err;
> +		return -EBUSY;
>  	}
>  
>  	battery = &jz_battery->battery;
> @@ -311,7 +307,7 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
>  				0, pdev->name, jz_battery);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Failed to request irq %d\n", ret);
> -		goto err;
> +		return ret;
>  	}
>  	disable_irq(jz_battery->irq);
>  
> @@ -319,7 +315,7 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
>  		ret = gpio_request(pdata->gpio_charge, dev_name(&pdev->dev));
>  		if (ret) {
>  			dev_err(&pdev->dev, "charger state gpio request failed.\n");
> -			goto err;
> +			return ret;
>  		}
>  		ret = gpio_direction_input(pdata->gpio_charge);
>  		if (ret) {
> @@ -364,7 +360,6 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
>  err_free_gpio:
>  	if (gpio_is_valid(pdata->gpio_charge))
>  		gpio_free(jz_battery->pdata->gpio_charge);
> -err:
>  	platform_set_drvdata(pdev, NULL);
>  	return ret;
>  }

Re: [PATCH 3/4] Use devm_request_irq to avoid some checks

[Marcos Souza]

Hi Lars,

2012/10/19 Lars-Peter Clausen <lars@metafoo.de>:
> On 10/19/2012 01:41 AM, Marcos Paulo de Souza wrote:
>> With this we can remove a lot of checks.
>>
>> Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>
>
> This one is a bit more tricky and the driver currently gets it (partially
> wrong). The issue is that since the power supply is referenced in the
> interrupt handler, the IRQs should be requested after the power supply has
> been registered and be freed before the power supply is unregistered.
>
> So devm can't really be used here. But as I said the current driver already
> gets it wrong.

Tanks for all the reviews! I will resend these patches will your comments.

But in this case, there is a way that we can fix this issue about
register/unregister IRQ?

Maybe another approach?

If you give some tips for me, my next patches will have this issue fixed too.

Thanks for all!

> - Lars
>
>> ---
>>  drivers/power/jz4740-battery.c |   25 ++++++++-----------------
>>  1 file changed, 8 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/power/jz4740-battery.c b/drivers/power/jz4740-battery.c
>> index 9d9b2a8..b80f7ed 100644
>> --- a/drivers/power/jz4740-battery.c
>> +++ b/drivers/power/jz4740-battery.c
>> @@ -307,8 +307,8 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
>>
>>       INIT_DELAYED_WORK(&jz_battery->work, jz_battery_work);
>>
>> -     ret = request_irq(jz_battery->irq, jz_battery_irq_handler, 0, pdev->name,
>> -                     jz_battery);
>> +     ret = devm_request_irq(&pdev->dev, jz_battery->irq, jz_battery_irq_handler,
>> +                             0, pdev->name, jz_battery);
>>       if (ret) {
>>               dev_err(&pdev->dev, "Failed to request irq %d\n", ret);
>>               goto err;
>> @@ -319,7 +319,7 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
>>               ret = gpio_request(pdata->gpio_charge, dev_name(&pdev->dev));
>>               if (ret) {
>>                       dev_err(&pdev->dev, "charger state gpio request failed.\n");
>> -                     goto err_free_irq;
>> +                     goto err;
>>               }
>>               ret = gpio_direction_input(pdata->gpio_charge);
>>               if (ret) {
>> @@ -330,7 +330,8 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
>>               jz_battery->charge_irq = gpio_to_irq(pdata->gpio_charge);
>>
>>               if (jz_battery->charge_irq >= 0) {
>> -                     ret = request_irq(jz_battery->charge_irq,
>> +                     ret = devm_request_irq(&pdev->dev,
>> +                                 jz_battery->charge_irq,
>>                                   jz_battery_charge_irq,
>>                                   IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
>>                                   dev_name(&pdev->dev), jz_battery);
>> @@ -352,7 +353,7 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
>>       ret = power_supply_register(&pdev->dev, &jz_battery->battery);
>>       if (ret) {
>>               dev_err(&pdev->dev, "power supply battery register failed.\n");
>> -             goto err_free_charge_irq;
>> +             goto err_free_gpio;
>>       }
>>
>>       platform_set_drvdata(pdev, jz_battery);
>> @@ -360,16 +361,11 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
>>
>>       return 0;
>>
>> -err_free_charge_irq:
>> -     if (jz_battery->charge_irq >= 0)
>> -             free_irq(jz_battery->charge_irq, jz_battery);
>>  err_free_gpio:
>>       if (gpio_is_valid(pdata->gpio_charge))
>>               gpio_free(jz_battery->pdata->gpio_charge);
>> -err_free_irq:
>> -     free_irq(jz_battery->irq, jz_battery);
>> -     platform_set_drvdata(pdev, NULL);
>>  err:
>> +     platform_set_drvdata(pdev, NULL);
>>       return ret;
>>  }
>>
>> @@ -379,16 +375,11 @@ static int __devexit jz_battery_remove(struct platform_device *pdev)
>>
>>       cancel_delayed_work_sync(&jz_battery->work);
>>
>> -     if (gpio_is_valid(jz_battery->pdata->gpio_charge)) {
>> -             if (jz_battery->charge_irq >= 0)
>> -                     free_irq(jz_battery->charge_irq, jz_battery);
>> +     if (gpio_is_valid(jz_battery->pdata->gpio_charge))
>>               gpio_free(jz_battery->pdata->gpio_charge);
>> -     }
>>
>>       power_supply_unregister(&jz_battery->battery);
>>
>> -     free_irq(jz_battery->irq, jz_battery);
>> -
>>       return 0;
>>  }
>>
>



-- 
Att,

Marcos Paulo de Souza
Acadêmico de Ciencia da Computação - FURB - SC
Github: https://github.com/marcosps/
"Uma vida sem desafios é uma vida sem razão"
"A life without challenges, is a non reason life"

Re: [PATCH 3/4] Use devm_request_irq to avoid some checks

[Lars-Peter Clausen]

On 10/19/2012 04:44 PM, Marcos Souza wrote:
> Hi Lars,
> 
> 2012/10/19 Lars-Peter Clausen <lars@metafoo.de>:
>> On 10/19/2012 01:41 AM, Marcos Paulo de Souza wrote:
>>> With this we can remove a lot of checks.
>>>
>>> Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>
>>
>> This one is a bit more tricky and the driver currently gets it (partially
>> wrong). The issue is that since the power supply is referenced in the
>> interrupt handler, the IRQs should be requested after the power supply has
>> been registered and be freed before the power supply is unregistered.
>>
>> So devm can't really be used here. But as I said the current driver already
>> gets it wrong.
> 
> Tanks for all the reviews! I will resend these patches will your comments.
> 
> But in this case, there is a way that we can fix this issue about
> register/unregister IRQ?
> 
> Maybe another approach?
> 
> If you give some tips for me, my next patches will have this issue fixed too.
> 
> Thanks for all!
> 


To fix the potential race condition issue which the driver currently has, it is
necessary to request both IRQs only after the power supply has been registered.
This also means the power supply needs to be unregistered if requesting one of
the IRQs fails. Similar for the remove path the IRQs need to be freed before
the power supply is unregistered. And since the IRQs need to be freed before
the power supply is removed we can't use devm_request_irq.

- Lars

>>> ---
>>>  drivers/power/jz4740-battery.c |   25 ++++++++-----------------
>>>  1 file changed, 8 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/power/jz4740-battery.c b/drivers/power/jz4740-battery.c
>>> index 9d9b2a8..b80f7ed 100644
>>> --- a/drivers/power/jz4740-battery.c
>>> +++ b/drivers/power/jz4740-battery.c
>>> @@ -307,8 +307,8 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
>>>
>>>       INIT_DELAYED_WORK(&jz_battery->work, jz_battery_work);
>>>
>>> -     ret = request_irq(jz_battery->irq, jz_battery_irq_handler, 0, pdev->name,
>>> -                     jz_battery);
>>> +     ret = devm_request_irq(&pdev->dev, jz_battery->irq, jz_battery_irq_handler,
>>> +                             0, pdev->name, jz_battery);
>>>       if (ret) {
>>>               dev_err(&pdev->dev, "Failed to request irq %d\n", ret);
>>>               goto err;
>>> @@ -319,7 +319,7 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
>>>               ret = gpio_request(pdata->gpio_charge, dev_name(&pdev->dev));
>>>               if (ret) {
>>>                       dev_err(&pdev->dev, "charger state gpio request failed.\n");
>>> -                     goto err_free_irq;
>>> +                     goto err;
>>>               }
>>>               ret = gpio_direction_input(pdata->gpio_charge);
>>>               if (ret) {
>>> @@ -330,7 +330,8 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
>>>               jz_battery->charge_irq = gpio_to_irq(pdata->gpio_charge);
>>>
>>>               if (jz_battery->charge_irq >= 0) {
>>> -                     ret = request_irq(jz_battery->charge_irq,
>>> +                     ret = devm_request_irq(&pdev->dev,
>>> +                                 jz_battery->charge_irq,
>>>                                   jz_battery_charge_irq,
>>>                                   IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
>>>                                   dev_name(&pdev->dev), jz_battery);
>>> @@ -352,7 +353,7 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
>>>       ret = power_supply_register(&pdev->dev, &jz_battery->battery);
>>>       if (ret) {
>>>               dev_err(&pdev->dev, "power supply battery register failed.\n");
>>> -             goto err_free_charge_irq;
>>> +             goto err_free_gpio;
>>>       }
>>>
>>>       platform_set_drvdata(pdev, jz_battery);
>>> @@ -360,16 +361,11 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
>>>
>>>       return 0;
>>>
>>> -err_free_charge_irq:
>>> -     if (jz_battery->charge_irq >= 0)
>>> -             free_irq(jz_battery->charge_irq, jz_battery);
>>>  err_free_gpio:
>>>       if (gpio_is_valid(pdata->gpio_charge))
>>>               gpio_free(jz_battery->pdata->gpio_charge);
>>> -err_free_irq:
>>> -     free_irq(jz_battery->irq, jz_battery);
>>> -     platform_set_drvdata(pdev, NULL);
>>>  err:
>>> +     platform_set_drvdata(pdev, NULL);
>>>       return ret;
>>>  }
>>>
>>> @@ -379,16 +375,11 @@ static int __devexit jz_battery_remove(struct platform_device *pdev)
>>>
>>>       cancel_delayed_work_sync(&jz_battery->work);
>>>
>>> -     if (gpio_is_valid(jz_battery->pdata->gpio_charge)) {
>>> -             if (jz_battery->charge_irq >= 0)
>>> -                     free_irq(jz_battery->charge_irq, jz_battery);
>>> +     if (gpio_is_valid(jz_battery->pdata->gpio_charge))
>>>               gpio_free(jz_battery->pdata->gpio_charge);
>>> -     }
>>>
>>>       power_supply_unregister(&jz_battery->battery);
>>>
>>> -     free_irq(jz_battery->irq, jz_battery);
>>> -
>>>       return 0;
>>>  }
>>>
>>
> 
> 
>