* [PATCH 1/4] Use devm_kzalloc instead of kzalloc/kfree
2012-10-18 23:41 [PATCH 0/4] driver/power/jz4740-battery: driver cleanup Marcos Paulo de Souza
@ 2012-10-18 23:41 ` Marcos Paulo de Souza
2012-10-19 9:21 ` Lars-Peter Clausen
2012-10-18 23:41 ` [PATCH 2/4] Use devm_* for request_mem_region and ioremap Marcos Paulo de Souza
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Marcos Paulo de Souza @ 2012-10-18 23:41 UTC (permalink / raw)
To: lars, cbou, dwmw2, linux-kernel; +Cc: 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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 1/4] Use devm_kzalloc instead of kzalloc/kfree
2012-10-18 23:41 ` [PATCH 1/4] Use devm_kzalloc instead of kzalloc/kfree Marcos Paulo de Souza
@ 2012-10-19 9:21 ` Lars-Peter Clausen
0 siblings, 0 replies; 11+ messages in thread
From: Lars-Peter Clausen @ 2012-10-19 9:21 UTC (permalink / raw)
To: Marcos Paulo de Souza; +Cc: cbou, dwmw2, linux-kernel
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;
> }
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/4] Use devm_* for request_mem_region and ioremap
2012-10-18 23:41 [PATCH 0/4] driver/power/jz4740-battery: driver cleanup Marcos Paulo de Souza
2012-10-18 23:41 ` [PATCH 1/4] Use devm_kzalloc instead of kzalloc/kfree Marcos Paulo de Souza
@ 2012-10-18 23:41 ` Marcos Paulo de Souza
2012-10-19 9:25 ` Lars-Peter Clausen
2012-10-18 23:41 ` [PATCH 3/4] Use devm_request_irq to avoid some checks Marcos Paulo de Souza
2012-10-18 23:41 ` [PATCH 4/4] power/jz4740.c: Simplify exit of function Marcos Paulo de Souza
3 siblings, 1 reply; 11+ messages in thread
From: Marcos Paulo de Souza @ 2012-10-18 23:41 UTC (permalink / raw)
To: lars, cbou, dwmw2, linux-kernel; +Cc: 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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 2/4] Use devm_* for request_mem_region and ioremap
2012-10-18 23:41 ` [PATCH 2/4] Use devm_* for request_mem_region and ioremap Marcos Paulo de Souza
@ 2012-10-19 9:25 ` Lars-Peter Clausen
0 siblings, 0 replies; 11+ messages in thread
From: Lars-Peter Clausen @ 2012-10-19 9:25 UTC (permalink / raw)
To: Marcos Paulo de Souza; +Cc: cbou, dwmw2, linux-kernel
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;
> }
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/4] Use devm_request_irq to avoid some checks
2012-10-18 23:41 [PATCH 0/4] driver/power/jz4740-battery: driver cleanup Marcos Paulo de Souza
2012-10-18 23:41 ` [PATCH 1/4] Use devm_kzalloc instead of kzalloc/kfree Marcos Paulo de Souza
2012-10-18 23:41 ` [PATCH 2/4] Use devm_* for request_mem_region and ioremap Marcos Paulo de Souza
@ 2012-10-18 23:41 ` Marcos Paulo de Souza
2012-10-19 9:44 ` Lars-Peter Clausen
2012-10-18 23:41 ` [PATCH 4/4] power/jz4740.c: Simplify exit of function Marcos Paulo de Souza
3 siblings, 1 reply; 11+ messages in thread
From: Marcos Paulo de Souza @ 2012-10-18 23:41 UTC (permalink / raw)
To: lars, cbou, dwmw2, linux-kernel; +Cc: 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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 3/4] Use devm_request_irq to avoid some checks
2012-10-18 23:41 ` [PATCH 3/4] Use devm_request_irq to avoid some checks Marcos Paulo de Souza
@ 2012-10-19 9:44 ` Lars-Peter Clausen
2012-10-19 14:44 ` Marcos Souza
0 siblings, 1 reply; 11+ messages in thread
From: Lars-Peter Clausen @ 2012-10-19 9:44 UTC (permalink / raw)
To: Marcos Paulo de Souza; +Cc: cbou, dwmw2, linux-kernel
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;
> }
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 3/4] Use devm_request_irq to avoid some checks
2012-10-19 9:44 ` Lars-Peter Clausen
@ 2012-10-19 14:44 ` Marcos Souza
2012-10-19 16:05 ` Lars-Peter Clausen
0 siblings, 1 reply; 11+ messages in thread
From: Marcos Souza @ 2012-10-19 14:44 UTC (permalink / raw)
To: Lars-Peter Clausen; +Cc: cbou, dwmw2, linux-kernel
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"
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 3/4] Use devm_request_irq to avoid some checks
2012-10-19 14:44 ` Marcos Souza
@ 2012-10-19 16:05 ` Lars-Peter Clausen
0 siblings, 0 replies; 11+ messages in thread
From: Lars-Peter Clausen @ 2012-10-19 16:05 UTC (permalink / raw)
To: Marcos Souza; +Cc: cbou, dwmw2, linux-kernel
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;
>>> }
>>>
>>
>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 4/4] power/jz4740.c: Simplify exit of function
2012-10-18 23:41 [PATCH 0/4] driver/power/jz4740-battery: driver cleanup Marcos Paulo de Souza
` (2 preceding siblings ...)
2012-10-18 23:41 ` [PATCH 3/4] Use devm_request_irq to avoid some checks Marcos Paulo de Souza
@ 2012-10-18 23:41 ` Marcos Paulo de Souza
2012-10-19 12:15 ` Lars-Peter Clausen
3 siblings, 1 reply; 11+ messages in thread
From: Marcos Paulo de Souza @ 2012-10-18 23:41 UTC (permalink / raw)
To: lars, cbou, dwmw2, linux-kernel; +Cc: 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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 4/4] power/jz4740.c: Simplify exit of function
2012-10-18 23:41 ` [PATCH 4/4] power/jz4740.c: Simplify exit of function Marcos Paulo de Souza
@ 2012-10-19 12:15 ` Lars-Peter Clausen
0 siblings, 0 replies; 11+ messages in thread
From: Lars-Peter Clausen @ 2012-10-19 12:15 UTC (permalink / raw)
To: Marcos Paulo de Souza; +Cc: cbou, dwmw2, linux-kernel
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;
> }
^ permalink raw reply [flat|nested] 11+ messages in thread