From: Libo Chen <[email protected]>
currently, when clk_get(NULL,"usb-device") fail, it does not
disable && put usb_bus_clock. It is incorrect.
this patch use new interface devm_xxx instead of xxx then
we no need to care about cleanup resource in err case that is boring
and reduce code size.
Signed-off-by: Libo Chen <[email protected]>
---
drivers/usb/gadget/s3c2410_udc.c | 85
++++++++++++----------------------------
1 file changed, 25 insertions(+), 60 deletions(-)
diff --git a/drivers/usb/gadget/s3c2410_udc.c
b/drivers/usb/gadget/s3c2410_udc.c
old mode 100644
new mode 100755
index d0e75e1..0c573a8
--- a/drivers/usb/gadget/s3c2410_udc.c
+++ b/drivers/usb/gadget/s3c2410_udc.c
@@ -1780,7 +1780,7 @@ static int s3c2410_udc_probe(struct
platform_device *pdev)
dev_dbg(dev, "%s()\n", __func__);
- usb_bus_clock = clk_get(NULL, "usb-bus-gadget");
+ usb_bus_clock = devm_clk_get(NULL, "usb-bus-gadget");
if (IS_ERR(usb_bus_clock)) {
dev_err(dev, "failed to get usb bus clock source\n");
return PTR_ERR(usb_bus_clock);
@@ -1788,8 +1788,9 @@ static int s3c2410_udc_probe(struct
platform_device *pdev)
clk_enable(usb_bus_clock);
- udc_clock = clk_get(NULL, "usb-device");
+ udc_clock = devm_clk_get(NULL, "usb-device");
if (IS_ERR(udc_clock)) {
+ clk_disable(usb_bus_clock);
dev_err(dev, "failed to get udc clock source\n");
return PTR_ERR(udc_clock);
}
@@ -1814,13 +1815,15 @@ static int s3c2410_udc_probe(struct
platform_device *pdev)
rsrc_start = S3C2410_PA_USBDEV;
rsrc_len = S3C24XX_SZ_USBDEV;
- if (!request_mem_region(rsrc_start, rsrc_len, gadget_name))
- return -EBUSY;
+ if (!devm_request_mem_region(rsrc_start, rsrc_len, gadget_name)) {
+ retval = -EBUSY;
+ goto out;
+ }
- base_addr = ioremap(rsrc_start, rsrc_len);
+ base_addr = devm_ioremap(rsrc_start, rsrc_len);
if (!base_addr) {
retval = -ENOMEM;
- goto err_mem;
+ goto out;
}
the_controller = udc;
@@ -1830,31 +1833,31 @@ static int s3c2410_udc_probe(struct
platform_device *pdev)
s3c2410_udc_reinit(udc);
/* irq setup after old hardware state is cleaned up */
- retval = request_irq(IRQ_USBD, s3c2410_udc_irq,
+ retval = devm_request_irq(IRQ_USBD, s3c2410_udc_irq,
0, gadget_name, udc);
if (retval != 0) {
dev_err(dev, "cannot get irq %i, err %d\n", IRQ_USBD, retval);
retval = -EBUSY;
- goto err_map;
+ goto out;
}
dev_dbg(dev, "got irq %i\n", IRQ_USBD);
if (udc_info && udc_info->vbus_pin > 0) {
- retval = gpio_request(udc_info->vbus_pin, "udc vbus");
+ retval = devm_gpio_request(udc_info->vbus_pin, "udc vbus");
if (retval < 0) {
dev_err(dev, "cannot claim vbus pin\n");
- goto err_int;
+ goto out;
}
irq = gpio_to_irq(udc_info->vbus_pin);
if (irq < 0) {
dev_err(dev, "no irq for gpio vbus pin\n");
- goto err_gpio_claim;
+ goto out;
}
- retval = request_irq(irq, s3c2410_udc_vbus_irq,
+ retval = devm_request_irq(irq, s3c2410_udc_vbus_irq,
IRQF_TRIGGER_RISING
| IRQF_TRIGGER_FALLING | IRQF_SHARED,
gadget_name, udc);
@@ -1863,7 +1866,7 @@ static int s3c2410_udc_probe(struct
platform_device *pdev)
dev_err(dev, "can't get vbus irq %d, err %d\n",
irq, retval);
retval = -EBUSY;
- goto err_gpio_claim;
+ goto out;
}
dev_dbg(dev, "got irq %i\n", irq);
@@ -1874,17 +1877,17 @@ static int s3c2410_udc_probe(struct
platform_device *pdev)
if (udc_info && !udc_info->udc_command &&
gpio_is_valid(udc_info->pullup_pin)) {
- retval = gpio_request_one(udc_info->pullup_pin,
+ retval = devm_gpio_request_one(udc_info->pullup_pin,
udc_info->vbus_pin_inverted ?
GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
"udc pullup");
if (retval)
- goto err_vbus_irq;
+ goto out;
}
retval = usb_add_gadget_udc(&pdev->dev, &udc->gadget);
if (retval)
- goto err_add_udc;
+ goto out;
if (s3c2410_udc_debugfs_root) {
udc->regs_info = debugfs_create_file("registers", S_IRUGO,
@@ -1898,22 +1901,9 @@ static int s3c2410_udc_probe(struct
platform_device *pdev)
return 0;
-err_add_udc:
- if (udc_info && !udc_info->udc_command &&
- gpio_is_valid(udc_info->pullup_pin))
- gpio_free(udc_info->pullup_pin);
-err_vbus_irq:
- if (udc_info && udc_info->vbus_pin > 0)
- free_irq(gpio_to_irq(udc_info->vbus_pin), udc);
-err_gpio_claim:
- if (udc_info && udc_info->vbus_pin > 0)
- gpio_free(udc_info->vbus_pin);
-err_int:
- free_irq(IRQ_USBD, udc);
-err_map:
- iounmap(base_addr);
-err_mem:
- release_mem_region(rsrc_start, rsrc_len);
+out:
+ clk_disable(udc_clock);
+ clk_disable(usb_bus_clock);
return retval;
}
@@ -1931,36 +1921,11 @@ static int s3c2410_udc_remove(struct
platform_device *pdev)
if (udc->driver)
return -EBUSY;
- usb_del_gadget_udc(&udc->gadget);
debugfs_remove(udc->regs_info);
+ usb_del_gadget_udc(&udc->gadget);
- if (udc_info && !udc_info->udc_command &&
- gpio_is_valid(udc_info->pullup_pin))
- gpio_free(udc_info->pullup_pin);
-
- if (udc_info && udc_info->vbus_pin > 0) {
- irq = gpio_to_irq(udc_info->vbus_pin);
- free_irq(irq, udc);
- }
-
- free_irq(IRQ_USBD, udc);
-
- iounmap(base_addr);
- release_mem_region(rsrc_start, rsrc_len);
-
- platform_set_drvdata(pdev, NULL);
-
- if (!IS_ERR(udc_clock) && udc_clock != NULL) {
- clk_disable(udc_clock);
- clk_put(udc_clock);
- udc_clock = NULL;
- }
-
- if (!IS_ERR(usb_bus_clock) && usb_bus_clock != NULL) {
- clk_disable(usb_bus_clock);
- clk_put(usb_bus_clock);
- usb_bus_clock = NULL;
- }
+ clk_disable(udc_clock);
+ clk_disable(usb_bus_clock);
dev_dbg(&pdev->dev, "%s: remove ok\n", __func__);
return 0;
--
1.8.1.2
Hi Libo,
On Saturday 11 of May 2013 14:28:15 Libo Chen wrote:
> From: Libo Chen <[email protected]>
The patch subject is slightly misleading. It suggests that the patch only
changes clock handling, but in fact the biggest part of the patch is
conversion to devm_ helpers.
I think following subject would suit this patch better:
[PATCH] usb: gadget: s3c2410: Convert to managed resource allocation
What do you think?
Also please see my comments inline.
> currently, when clk_get(NULL,"usb-device") fail, it does not
> disable && put usb_bus_clock. It is incorrect.
>
> this patch use new interface devm_xxx instead of xxx then
> we no need to care about cleanup resource in err case that is boring
> and reduce code size.
>
> Signed-off-by: Libo Chen <[email protected]>
> ---
> drivers/usb/gadget/s3c2410_udc.c | 85
> ++++++++++++----------------------------
> 1 file changed, 25 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/usb/gadget/s3c2410_udc.c
> b/drivers/usb/gadget/s3c2410_udc.c
> old mode 100644
> new mode 100755
> index d0e75e1..0c573a8
> --- a/drivers/usb/gadget/s3c2410_udc.c
> +++ b/drivers/usb/gadget/s3c2410_udc.c
> @@ -1780,7 +1780,7 @@ static int s3c2410_udc_probe(struct
> platform_device *pdev)
>
> dev_dbg(dev, "%s()\n", __func__);
>
> - usb_bus_clock = clk_get(NULL, "usb-bus-gadget");
> + usb_bus_clock = devm_clk_get(NULL, "usb-bus-gadget");
Passing NULL as the dev parameter of devm_ functions is a BUG. You need to
pass a valid pointer to struct device to them, otherwise it doesn't make
sense - the list of allocated resources is a part of struct device.
> if (IS_ERR(usb_bus_clock)) {
> dev_err(dev, "failed to get usb bus clock source\n");
> return PTR_ERR(usb_bus_clock);
> @@ -1788,8 +1788,9 @@ static int s3c2410_udc_probe(struct
> platform_device *pdev)
>
> clk_enable(usb_bus_clock);
If you enable this clock after allocation of all resources instead, you
won't have to disable it manually like in following chunk:
>
> - udc_clock = clk_get(NULL, "usb-device");
> + udc_clock = devm_clk_get(NULL, "usb-device");
> if (IS_ERR(udc_clock)) {
> + clk_disable(usb_bus_clock);
> dev_err(dev, "failed to get udc clock source\n");
> return PTR_ERR(udc_clock);
> }
> @@ -1814,13 +1815,15 @@ static int s3c2410_udc_probe(struct
> platform_device *pdev)
> rsrc_start = S3C2410_PA_USBDEV;
> rsrc_len = S3C24XX_SZ_USBDEV;
>
> - if (!request_mem_region(rsrc_start, rsrc_len, gadget_name))
> - return -EBUSY;
> + if (!devm_request_mem_region(rsrc_start, rsrc_len, gadget_name)) {
> + retval = -EBUSY;
> + goto out;
> + }
>
> - base_addr = ioremap(rsrc_start, rsrc_len);
> + base_addr = devm_ioremap(rsrc_start, rsrc_len);
This won't even compile... Was this patch tested at all?
See the definition of devm_ioremap:
http://lxr.free-electrons.com/source/lib/devres.c#L25
In addition, a better function is available, devm_request_and_ioremap.
(http://lxr.free-electrons.com/source/lib/devres.c#L143)
You can use it instead of calling request_mem_region and ioremap
separately.
> if (!base_addr) {
> retval = -ENOMEM;
> - goto err_mem;
> + goto out;
> }
>
> the_controller = udc;
> @@ -1830,31 +1833,31 @@ static int s3c2410_udc_probe(struct
> platform_device *pdev)
> s3c2410_udc_reinit(udc);
>
> /* irq setup after old hardware state is cleaned up */
> - retval = request_irq(IRQ_USBD, s3c2410_udc_irq,
> + retval = devm_request_irq(IRQ_USBD, s3c2410_udc_irq,
> 0, gadget_name, udc);
This won't compile too.
> if (retval != 0) {
> dev_err(dev, "cannot get irq %i, err %d\n", IRQ_USBD,
retval);
> retval = -EBUSY;
> - goto err_map;
> + goto out;
> }
>
> dev_dbg(dev, "got irq %i\n", IRQ_USBD);
>
> if (udc_info && udc_info->vbus_pin > 0) {
> - retval = gpio_request(udc_info->vbus_pin, "udc vbus");
> + retval = devm_gpio_request(udc_info->vbus_pin, "udc
vbus");
Neither this will.
> if (retval < 0) {
> dev_err(dev, "cannot claim vbus pin\n");
> - goto err_int;
> + goto out;
> }
>
> irq = gpio_to_irq(udc_info->vbus_pin);
> if (irq < 0) {
> dev_err(dev, "no irq for gpio vbus pin\n");
> - goto err_gpio_claim;
> + goto out;
> }
>
> - retval = request_irq(irq, s3c2410_udc_vbus_irq,
> + retval = devm_request_irq(irq, s3c2410_udc_vbus_irq,
> IRQF_TRIGGER_RISING
Neither this will.
>
> | IRQF_TRIGGER_FALLING | IRQF_SHARED,
>
> gadget_name, udc);
> @@ -1863,7 +1866,7 @@ static int s3c2410_udc_probe(struct
> platform_device *pdev)
> dev_err(dev, "can't get vbus irq %d, err %d\n",
> irq, retval);
> retval = -EBUSY;
> - goto err_gpio_claim;
> + goto out;
> }
>
> dev_dbg(dev, "got irq %i\n", irq);
> @@ -1874,17 +1877,17 @@ static int s3c2410_udc_probe(struct
> platform_device *pdev)
> if (udc_info && !udc_info->udc_command &&
> gpio_is_valid(udc_info->pullup_pin)) {
>
> - retval = gpio_request_one(udc_info->pullup_pin,
> + retval = devm_gpio_request_one(udc_info->pullup_pin,
Neither this will.
Well, this patch is a good idea, but a very poor implementation.
Please _test_ (compile and boot) your patch next time.
Best regards,
Tomasz
δΊ 2013-5-11 18:15, Tomasz Figa ει:
> Hi Libo,
>
> On Saturday 11 of May 2013 14:28:15 Libo Chen wrote:
>> >From: Libo Chen<[email protected]>
> The patch subject is slightly misleading. It suggests that the patch only
> changes clock handling, but in fact the biggest part of the patch is
> conversion to devm_ helpers.
>
> I think following subject would suit this patch better:
>
> [PATCH] usb: gadget: s3c2410: Convert to managed resource allocation
>
> What do you think?
>
> Also please see my comments inline.
>
Hi Tomasz,
Thank you for your remind. Sorry, I forgeted to add RFC.
"Convert to managed resource allocation" is more suitable.
Your advice is very helpfull for me to understand those interfaces.
I will be careful in the future patches.
Thanks again