diff mbox

[04/10] drm/exynos: Use devm_clk_get in exynos_drm_fimc.c

Message ID 1356338031-23674-5-git-send-email-sachin.kamat@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Sachin Kamat Dec. 24, 2012, 8:33 a.m. UTC
This eliminates the need for explicit clk_put and makes the
cleanup and exit path code simpler.

Cc: Eunchul Kim <chulspro.kim@samsung.com>
Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
---
 drivers/gpu/drm/exynos/exynos_drm_fimc.c |   46 ++++++-----------------------
 1 files changed, 10 insertions(+), 36 deletions(-)

Comments

Inki Dae Dec. 26, 2012, 11:04 a.m. UTC | #1
2012/12/24 Sachin Kamat <sachin.kamat@linaro.org>

> This eliminates the need for explicit clk_put and makes the
> cleanup and exit path code simpler.
>
> Cc: Eunchul Kim <chulspro.kim@samsung.com>
> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_fimc.c |   46
> ++++++-----------------------
>  1 files changed, 10 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimc.c
> b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
> index 972aa70..c4006b8 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimc.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
> @@ -1739,64 +1739,50 @@ static int __devinit fimc_probe(struct
> platform_device *pdev)
>                 platform_get_device_id(pdev)->driver_data;
>
>         /* clock control */
> -       ctx->sclk_fimc_clk = clk_get(dev, "sclk_fimc");
> +       ctx->sclk_fimc_clk = devm_clk_get(dev, "sclk_fimc");
>         if (IS_ERR(ctx->sclk_fimc_clk)) {
>                 dev_err(dev, "failed to get src fimc clock.\n");
>                 return PTR_ERR(ctx->sclk_fimc_clk);
>         }
>         clk_enable(ctx->sclk_fimc_clk);
>
> -       ctx->fimc_clk = clk_get(dev, "fimc");
> +       ctx->fimc_clk = devm_clk_get(dev, "fimc");
>         if (IS_ERR(ctx->fimc_clk)) {
>                 dev_err(dev, "failed to get fimc clock.\n");
>                 clk_disable(ctx->sclk_fimc_clk);
> -               clk_put(ctx->sclk_fimc_clk);
>                 return PTR_ERR(ctx->fimc_clk);
>         }
>
> -       ctx->wb_clk = clk_get(dev, "pxl_async0");
> +       ctx->wb_clk = devm_clk_get(dev, "pxl_async0");
>         if (IS_ERR(ctx->wb_clk)) {
>                 dev_err(dev, "failed to get writeback a clock.\n");
>                 clk_disable(ctx->sclk_fimc_clk);
> -               clk_put(ctx->sclk_fimc_clk);
> -               clk_put(ctx->fimc_clk);
>                 return PTR_ERR(ctx->wb_clk);
>         }
>
> -       ctx->wb_b_clk = clk_get(dev, "pxl_async1");
> +       ctx->wb_b_clk = devm_clk_get(dev, "pxl_async1");
>         if (IS_ERR(ctx->wb_b_clk)) {
>                 dev_err(dev, "failed to get writeback b clock.\n");
>                 clk_disable(ctx->sclk_fimc_clk);
> -               clk_put(ctx->sclk_fimc_clk);
> -               clk_put(ctx->fimc_clk);
> -               clk_put(ctx->wb_clk);
>                 return PTR_ERR(ctx->wb_b_clk);
>         }
>
> -       parent_clk = clk_get(dev, ddata->parent_clk);
> +       parent_clk = devm_clk_get(dev, ddata->parent_clk);
>
>         if (IS_ERR(parent_clk)) {
>                 dev_err(dev, "failed to get parent clock.\n");
>                 clk_disable(ctx->sclk_fimc_clk);
> -               clk_put(ctx->sclk_fimc_clk);
> -               clk_put(ctx->fimc_clk);
> -               clk_put(ctx->wb_clk);
> -               clk_put(ctx->wb_b_clk);
>                 return PTR_ERR(parent_clk);
>         }
>
>         if (clk_set_parent(ctx->sclk_fimc_clk, parent_clk)) {
>                 dev_err(dev, "failed to set parent.\n");
> -               clk_put(parent_clk);
> +               devm_clk_put(dev, parent_clk);
>

remove the above call. is there any reason that devm_clk_put should be
called here?


>                 clk_disable(ctx->sclk_fimc_clk);
> -               clk_put(ctx->sclk_fimc_clk);
> -               clk_put(ctx->fimc_clk);
> -               clk_put(ctx->wb_clk);
> -               clk_put(ctx->wb_b_clk);
>                 return -EINVAL;
>         }
>
> -       clk_put(parent_clk);
> +       devm_clk_put(dev, parent_clk);
>

ditto.


>         clk_set_rate(ctx->sclk_fimc_clk, pdata->clk_rate);
>
>         /* resource memory */
> @@ -1804,16 +1790,14 @@ static int __devinit fimc_probe(struct
> platform_device *pdev)
>         ctx->regs = devm_request_and_ioremap(dev, ctx->regs_res);
>         if (!ctx->regs) {
>                 dev_err(dev, "failed to map registers.\n");
> -               ret = -ENXIO;
> -               goto err_clk;
> +               return -ENXIO;
>         }
>
>         /* resource irq */
>         res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>         if (!res) {
>                 dev_err(dev, "failed to request irq resource.\n");
> -               ret = -ENOENT;
> -               goto err_clk;
> +               return -ENOENT;
>         }
>
>         ctx->irq = res->start;
> @@ -1821,7 +1805,7 @@ static int __devinit fimc_probe(struct
> platform_device *pdev)
>                 IRQF_ONESHOT, "drm_fimc", ctx);
>         if (ret < 0) {
>                 dev_err(dev, "failed to request irq.\n");
> -               goto err_clk;
> +               return ret;
>         }
>
>         /* context initailization */
> @@ -1867,11 +1851,6 @@ err_ippdrv_register:
>         pm_runtime_disable(dev);
>  err_get_irq:
>         free_irq(ctx->irq, ctx);
> -err_clk:
> -       clk_put(ctx->sclk_fimc_clk);
> -       clk_put(ctx->fimc_clk);
> -       clk_put(ctx->wb_clk);
> -       clk_put(ctx->wb_b_clk);
>
>         return ret;
>  }
> @@ -1891,11 +1870,6 @@ static int __devexit fimc_remove(struct
> platform_device *pdev)
>
>         free_irq(ctx->irq, ctx);
>
> -       clk_put(ctx->sclk_fimc_clk);
> -       clk_put(ctx->fimc_clk);
> -       clk_put(ctx->wb_clk);
> -       clk_put(ctx->wb_b_clk);
> -
>         return 0;
>  }
>
> --
> 1.7.4.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
Sachin Kamat Dec. 26, 2012, 1:19 p.m. UTC | #2
On Wednesday, 26 December 2012, Inki Dae <inki.dae@samsung.com> wrote:
>
>
> 2012/12/24 Sachin Kamat <sachin.kamat@linaro.org>
>>
>> This eliminates the need for explicit clk_put and makes the
>> cleanup and exit path code simpler.
>>
>> Cc: Eunchul Kim <chulspro.kim@samsung.com>
>> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
>> ---
>>  drivers/gpu/drm/exynos/exynos_drm_fimc.c |   46
++++++-----------------------
>>  1 files changed, 10 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimc.c
b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
>> index 972aa70..c4006b8 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_fimc.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
>> @@ -1739,64 +1739,50 @@ static int __devinit fimc_probe(struct
platform_device *pdev)
>>                 platform_get_device_id(pdev)->driver_data;
>>
>>         /* clock control */
>> -       ctx->sclk_fimc_clk = clk_get(dev, "sclk_fimc");
>> +       ctx->sclk_fimc_clk = devm_clk_get(dev, "sclk_fimc");
>>         if (IS_ERR(ctx->sclk_fimc_clk)) {
>>                 dev_err(dev, "failed to get src fimc clock.\n");
>>                 return PTR_ERR(ctx->sclk_fimc_clk);
>>         }
>>         clk_enable(ctx->sclk_fimc_clk);
>>
>> -       ctx->fimc_clk = clk_get(dev, "fimc");
>> +       ctx->fimc_clk = devm_clk_get(dev, "fimc");
>>         if (IS_ERR(ctx->fimc_clk)) {
>>                 dev_err(dev, "failed to get fimc clock.\n");
>>                 clk_disable(ctx->sclk_fimc_clk);
>> -               clk_put(ctx->sclk_fimc_clk);
>>                 return PTR_ERR(ctx->fimc_clk);
>>         }
>>
>> -       ctx->wb_clk = clk_get(dev, "pxl_async0");
>> +       ctx->wb_clk = devm_clk_get(dev, "pxl_async0");
>>         if (IS_ERR(ctx->wb_clk)) {
>>                 dev_err(dev, "failed to get writeback a clock.\n");
>>                 clk_disable(ctx->sclk_fimc_clk);
>> -               clk_put(ctx->sclk_fimc_clk);
>> -               clk_put(ctx->fimc_clk);
>>                 return PTR_ERR(ctx->wb_clk);
>>         }
>>
>> -       ctx->wb_b_clk = clk_get(dev, "pxl_async1");
>> +       ctx->wb_b_clk = devm_clk_get(dev, "pxl_async1");
>>         if (IS_ERR(ctx->wb_b_clk)) {
>>                 dev_err(dev, "failed to get writeback b clock.\n");
>>                 clk_disable(ctx->sclk_fimc_clk);
>> -               clk_put(ctx->sclk_fimc_clk);
>> -               clk_put(ctx->fimc_clk);
>> -               clk_put(ctx->wb_clk);
>>                 return PTR_ERR(ctx->wb_b_clk);
>>         }
>>
>> -       parent_clk = clk_get(dev, ddata->parent_clk);
>> +       parent_clk = devm_clk_get(dev, ddata->parent_clk);
>>
>>         if (IS_ERR(parent_clk)) {
>>                 dev_err(dev, "failed to get parent clock.\n");
>>                 clk_disable(ctx->sclk_fimc_clk);
>> -               clk_put(ctx->sclk_fimc_clk);
>> -               clk_put(ctx->fimc_clk);
>> -               clk_put(ctx->wb_clk);
>> -               clk_put(ctx->wb_b_clk);
>>                 return PTR_ERR(parent_clk);
>>         }
>>
>>         if (clk_set_parent(ctx->sclk_fimc_clk, parent_clk)) {
>>                 dev_err(dev, "failed to set parent.\n");
>> -               clk_put(parent_clk);
>> +               devm_clk_put(dev, parent_clk);
>
> remove the above call. is there any reason that devm_clk_put should be
called here?

Devm resources are freed/released automatically only when the device
detaches. In the above case the clock was released explicitly (for whatever
reasons) earlier. Hence i have used devm call to keep the code logic same
as i do not know the behavior if this clock is 'put' when the device
detaches instead of here.

>
>>
>>                 clk_disable(ctx->sclk_fimc_clk);
>> -               clk_put(ctx->sclk_fimc_clk);
>> -               clk_put(ctx->fimc_clk);
>> -               clk_put(ctx->wb_clk);
>> -               clk_put(ctx->wb_b_clk);
>>                 return -EINVAL;
>>         }
>>
>> -       clk_put(parent_clk);
>> +       devm_clk_put(dev, parent_clk);
>
> ditto.
>
>>
>>         clk_set_rate(ctx->sclk_fimc_clk, pdata->clk_rate);
>>
>>         /* resource memory */
>> @@ -1804,16 +1790,14 @@ static int __devinit fimc_probe(struct
platform_device *pdev)
>>         ctx->regs = devm_request_and_ioremap(dev, ctx->regs_res);
>>         if (!ctx->regs) {
>>                 dev_err(dev, "failed to map registers.\n");
>> -               ret = -ENXIO;
>> -               goto err_clk;
>> +               return -ENXIO;
>>         }
>>
>>         /* resource irq */
>>         res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>>         if (!res) {
>>                 dev_err(dev, "failed to request irq resource.\n");
>> -               ret = -ENOENT;
>> -               goto err_clk;
>> +               return -ENOENT;
>>         }
>>
>>         ctx->irq = res->start;
>> @@ -1821,7 +1805,7 @@ static int __devinit fimc_probe(struct
platform_device *pdev)
>>                 IRQF_ONESHOT, "drm_fimc", ctx);
>>         if (ret < 0) {
>>                 dev_err(dev, "failed to request irq.\n");
>> -               goto err_clk;
>> +               return ret;
>>         }
>>
>>         /* context initailization */
>> @@ -1867,11 +1851,6 @@ err_ippdrv_register:
>>         pm_runtime_disable(dev);
>>  err_get_irq:
>>         free_irq(ctx->irq, ctx);
>> -err_clk:
>> -       clk_put(ctx->sclk_fimc_clk);
>> -       clk_put(ctx->fimc_clk);
>> -       clk_put(ctx->wb_clk);
>> -       clk_put(ctx->wb_b_clk);
>>
>>         return ret;
>>  }
>> @@ -1891,11 +1870,6 @@ static int __devexit fimc_remove(struct
platform_device *pdev)
>>
>>         free_irq(ctx->irq, ctx);
>>
>> -       clk_put(ctx->sclk_fimc_clk);
>> -       clk_put(ctx->fimc_clk);
>> -       clk_put(ctx->wb_clk);
>> -       clk_put(ctx->wb_b_clk);
>> -
>>         return 0;
>>  }
>>
>> --
>> 1.7.4.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
Sachin Kamat Dec. 26, 2012, 1:22 p.m. UTC | #3
On Wednesday, 26 December 2012, Inki Dae <inki.dae@samsung.com> wrote:
>
>
> 2012/12/24 Sachin Kamat <sachin.kamat@linaro.org>
>>
>> This eliminates the need for explicit clk_put and makes the
>> cleanup and exit path code simpler.
>>
>> Cc: Eunchul Kim <chulspro.kim@samsung.com>
>> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
>> ---
>>  drivers/gpu/drm/exynos/exynos_drm_fimc.c |   46
++++++-----------------------
>>  1 files changed, 10 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimc.c
b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
>> index 972aa70..c4006b8 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_fimc.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
>> @@ -1739,64 +1739,50 @@ static int __devinit fimc_probe(struct
platform_device *pdev)
>>                 platform_get_device_id(pdev)->driver_data;
>>
>>         /* clock control */
>> -       ctx->sclk_fimc_clk = clk_get(dev, "sclk_fimc");
>> +       ctx->sclk_fimc_clk = devm_clk_get(dev, "sclk_fimc");
>>         if (IS_ERR(ctx->sclk_fimc_clk)) {
>>                 dev_err(dev, "failed to get src fimc clock.\n");
>>                 return PTR_ERR(ctx->sclk_fimc_clk);
>>         }
>>         clk_enable(ctx->sclk_fimc_clk);
>>
>> -       ctx->fimc_clk = clk_get(dev, "fimc");
>> +       ctx->fimc_clk = devm_clk_get(dev, "fimc");
>>         if (IS_ERR(ctx->fimc_clk)) {
>>                 dev_err(dev, "failed to get fimc clock.\n");
>>                 clk_disable(ctx->sclk_fimc_clk);
>> -               clk_put(ctx->sclk_fimc_clk);
>>                 return PTR_ERR(ctx->fimc_clk);
>>         }
>>
>> -       ctx->wb_clk = clk_get(dev, "pxl_async0");
>> +       ctx->wb_clk = devm_clk_get(dev, "pxl_async0");
>>         if (IS_ERR(ctx->wb_clk)) {
>>                 dev_err(dev, "failed to get writeback a clock.\n");
>>                 clk_disable(ctx->sclk_fimc_clk);
>> -               clk_put(ctx->sclk_fimc_clk);
>> -               clk_put(ctx->fimc_clk);
>>                 return PTR_ERR(ctx->wb_clk);
>>         }
>>
>> -       ctx->wb_b_clk = clk_get(dev, "pxl_async1");
>> +       ctx->wb_b_clk = devm_clk_get(dev, "pxl_async1");
>>         if (IS_ERR(ctx->wb_b_clk)) {
>>                 dev_err(dev, "failed to get writeback b clock.\n");
>>                 clk_disable(ctx->sclk_fimc_clk);
>> -               clk_put(ctx->sclk_fimc_clk);
>> -               clk_put(ctx->fimc_clk);
>> -               clk_put(ctx->wb_clk);
>>                 return PTR_ERR(ctx->wb_b_clk);
>>         }
>>
>> -       parent_clk = clk_get(dev, ddata->parent_clk);
>> +       parent_clk = devm_clk_get(dev, ddata->parent_clk);
>>
>>         if (IS_ERR(parent_clk)) {
>>                 dev_err(dev, "failed to get parent clock.\n");
>>                 clk_disable(ctx->sclk_fimc_clk);
>> -               clk_put(ctx->sclk_fimc_clk);
>> -               clk_put(ctx->fimc_clk);
>> -               clk_put(ctx->wb_clk);
>> -               clk_put(ctx->wb_b_clk);
>>                 return PTR_ERR(parent_clk);
>>         }
>>
>>         if (clk_set_parent(ctx->sclk_fimc_clk, parent_clk)) {
>>                 dev_err(dev, "failed to set parent.\n");
>> -               clk_put(parent_clk);
>> +               devm_clk_put(dev, parent_clk);
>
> remove the above call. is there any reason that devm_clk_put should be
called here?

Devm resources are freed/released automatically only when the device
detaches. In the above case the clock was released explicitly (for whatever
reasons) earlier. Hence i have used devm call to keep the code logic same
as i do not know the behavior if this clock is 'put' when the device
detaches instead of here.

>
>>
>>                 clk_disable(ctx->sclk_fimc_clk);
>> -               clk_put(ctx->sclk_fimc_clk);
>> -               clk_put(ctx->fimc_clk);
>> -               clk_put(ctx->wb_clk);
>> -               clk_put(ctx->wb_b_clk);
>>                 return -EINVAL;
>>         }
>>
>> -       clk_put(parent_clk);
>> +       devm_clk_put(dev, parent_clk);
>
> ditto.
>
>>
>>         clk_set_rate(ctx->sclk_fimc_clk, pdata->clk_rate);
>>
>>         /* resource memory */
>> @@ -1804,16 +1790,14 @@ static int __devinit fimc_probe(struct
platform_device *pdev)
>>         ctx->regs = devm_request_and_ioremap(dev, ctx->regs_res);
>>         if (!ctx->regs) {
>>                 dev_err(dev, "failed to map registers.\n");
>> -               ret = -ENXIO;
>> -               goto err_clk;
>> +               return -ENXIO;
>>         }
>>
>>         /* resource irq */
>>         res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>>         if (!res) {
>>                 dev_err(dev, "failed to request irq resource.\n");
>> -               ret = -ENOENT;
>> -               goto err_clk;
>> +               return -ENOENT;
>>         }
>>
>>         ctx->irq = res->start;
>> @@ -1821,7 +1805,7 @@ static int __devinit fimc_probe(struct
platform_device *pdev)
>>                 IRQF_ONESHOT, "drm_fimc", ctx);
>>         if (ret < 0) {
>>                 dev_err(dev, "failed to request irq.\n");
>> -               goto err_clk;
>> +               return ret;
>>         }
>>
>>         /* context initailization */
>> @@ -1867,11 +1851,6 @@ err_ippdrv_register:
>>         pm_runtime_disable(dev);
>>  err_get_irq:
>>         free_irq(ctx->irq, ctx);
>> -err_clk:
>> -       clk_put(ctx->sclk_fimc_clk);
>> -       clk_put(ctx->fimc_clk);
>> -       clk_put(ctx->wb_clk);
>> -       clk_put(ctx->wb_b_clk);
>>
>>         return ret;
>>  }
>> @@ -1891,11 +1870,6 @@ static int __devexit fimc_remove(struct
platform_device *pdev)
>>
>>         free_irq(ctx->irq, ctx);
>>
>> -       clk_put(ctx->sclk_fimc_clk);
>> -       clk_put(ctx->fimc_clk);
>> -       clk_put(ctx->wb_clk);
>> -       clk_put(ctx->wb_b_clk);
>> -
>>         return 0;
>>  }
>>
>> --
>> 1.7.4.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
Inki Dae Dec. 27, 2012, 10:13 a.m. UTC | #4
2012/12/26 Sachin Kamat <sachin.kamat@linaro.org>

>
>
> On Wednesday, 26 December 2012, Inki Dae <inki.dae@samsung.com> wrote:
> >
> >
> > 2012/12/24 Sachin Kamat <sachin.kamat@linaro.org>
> >>
> >> This eliminates the need for explicit clk_put and makes the
> >> cleanup and exit path code simpler.
> >>
> >> Cc: Eunchul Kim <chulspro.kim@samsung.com>
> >> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> >> ---
> >>  drivers/gpu/drm/exynos/exynos_drm_fimc.c |   46
> ++++++-----------------------
> >>  1 files changed, 10 insertions(+), 36 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimc.c
> b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
> >> index 972aa70..c4006b8 100644
> >> --- a/drivers/gpu/drm/exynos/exynos_drm_fimc.c
> >> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
> >> @@ -1739,64 +1739,50 @@ static int __devinit fimc_probe(struct
> platform_device *pdev)
> >>                 platform_get_device_id(pdev)->driver_data;
> >>
> >>         /* clock control */
> >> -       ctx->sclk_fimc_clk = clk_get(dev, "sclk_fimc");
> >> +       ctx->sclk_fimc_clk = devm_clk_get(dev, "sclk_fimc");
> >>         if (IS_ERR(ctx->sclk_fimc_clk)) {
> >>                 dev_err(dev, "failed to get src fimc clock.\n");
> >>                 return PTR_ERR(ctx->sclk_fimc_clk);
> >>         }
> >>         clk_enable(ctx->sclk_fimc_clk);
> >>
> >> -       ctx->fimc_clk = clk_get(dev, "fimc");
> >> +       ctx->fimc_clk = devm_clk_get(dev, "fimc");
> >>         if (IS_ERR(ctx->fimc_clk)) {
> >>                 dev_err(dev, "failed to get fimc clock.\n");
> >>                 clk_disable(ctx->sclk_fimc_clk);
> >> -               clk_put(ctx->sclk_fimc_clk);
> >>                 return PTR_ERR(ctx->fimc_clk);
> >>         }
> >>
> >> -       ctx->wb_clk = clk_get(dev, "pxl_async0");
> >> +       ctx->wb_clk = devm_clk_get(dev, "pxl_async0");
> >>         if (IS_ERR(ctx->wb_clk)) {
> >>                 dev_err(dev, "failed to get writeback a clock.\n");
> >>                 clk_disable(ctx->sclk_fimc_clk);
> >> -               clk_put(ctx->sclk_fimc_clk);
> >> -               clk_put(ctx->fimc_clk);
> >>                 return PTR_ERR(ctx->wb_clk);
> >>         }
> >>
> >> -       ctx->wb_b_clk = clk_get(dev, "pxl_async1");
> >> +       ctx->wb_b_clk = devm_clk_get(dev, "pxl_async1");
> >>         if (IS_ERR(ctx->wb_b_clk)) {
> >>                 dev_err(dev, "failed to get writeback b clock.\n");
> >>                 clk_disable(ctx->sclk_fimc_clk);
> >> -               clk_put(ctx->sclk_fimc_clk);
> >> -               clk_put(ctx->fimc_clk);
> >> -               clk_put(ctx->wb_clk);
> >>                 return PTR_ERR(ctx->wb_b_clk);
> >>         }
> >>
> >> -       parent_clk = clk_get(dev, ddata->parent_clk);
> >> +       parent_clk = devm_clk_get(dev, ddata->parent_clk);
> >>
> >>         if (IS_ERR(parent_clk)) {
> >>                 dev_err(dev, "failed to get parent clock.\n");
> >>                 clk_disable(ctx->sclk_fimc_clk);
> >> -               clk_put(ctx->sclk_fimc_clk);
> >> -               clk_put(ctx->fimc_clk);
> >> -               clk_put(ctx->wb_clk);
> >> -               clk_put(ctx->wb_b_clk);
> >>                 return PTR_ERR(parent_clk);
> >>         }
> >>
> >>         if (clk_set_parent(ctx->sclk_fimc_clk, parent_clk)) {
> >>                 dev_err(dev, "failed to set parent.\n");
> >> -               clk_put(parent_clk);
> >> +               devm_clk_put(dev, parent_clk);
> >
> > remove the above call. is there any reason that devm_clk_put should be
> called here?
>
> Devm resources are freed/released automatically only when the device
> detaches. In the above case the clock was released explicitly (for whatever
> reasons) earlier. Hence i have used devm call to keep the code logic same
> as i do not know the behavior if this clock is 'put' when the device
> detaches instead of here.
>
>
If probe is failed, devm resources are released by devres_release_all(). So
that is unnecessary call. Remove it.


>
> >
> >>
> >>                 clk_disable(ctx->sclk_fimc_clk);
> >> -               clk_put(ctx->sclk_fimc_clk);
> >> -               clk_put(ctx->fimc_clk);
> >> -               clk_put(ctx->wb_clk);
> >> -               clk_put(ctx->wb_b_clk);
> >>                 return -EINVAL;
> >>         }
> >>
> >> -       clk_put(parent_clk);
> >> +       devm_clk_put(dev, parent_clk);
> >
> > ditto.
> >
> >>
> >>         clk_set_rate(ctx->sclk_fimc_clk, pdata->clk_rate);
> >>
> >>         /* resource memory */
> >> @@ -1804,16 +1790,14 @@ static int __devinit fimc_probe(struct
> platform_device *pdev)
> >>         ctx->regs = devm_request_and_ioremap(dev, ctx->regs_res);
> >>         if (!ctx->regs) {
> >>                 dev_err(dev, "failed to map registers.\n");
> >> -               ret = -ENXIO;
> >> -               goto err_clk;
> >> +               return -ENXIO;
> >>         }
> >>
> >>         /* resource irq */
> >>         res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> >>         if (!res) {
> >>                 dev_err(dev, "failed to request irq resource.\n");
> >> -               ret = -ENOENT;
> >> -               goto err_clk;
> >> +               return -ENOENT;
> >>         }
> >>
> >>         ctx->irq = res->start;
> >> @@ -1821,7 +1805,7 @@ static int __devinit fimc_probe(struct
> platform_device *pdev)
> >>                 IRQF_ONESHOT, "drm_fimc", ctx);
> >>         if (ret < 0) {
> >>                 dev_err(dev, "failed to request irq.\n");
> >> -               goto err_clk;
> >> +               return ret;
> >>         }
> >>
> >>         /* context initailization */
> >> @@ -1867,11 +1851,6 @@ err_ippdrv_register:
> >>         pm_runtime_disable(dev);
> >>  err_get_irq:
> >>         free_irq(ctx->irq, ctx);
> >> -err_clk:
> >> -       clk_put(ctx->sclk_fimc_clk);
> >> -       clk_put(ctx->fimc_clk);
> >> -       clk_put(ctx->wb_clk);
> >> -       clk_put(ctx->wb_b_clk);
> >>
> >>         return ret;
> >>  }
> >> @@ -1891,11 +1870,6 @@ static int __devexit fimc_remove(struct
> platform_device *pdev)
> >>
> >>         free_irq(ctx->irq, ctx);
> >>
> >> -       clk_put(ctx->sclk_fimc_clk);
> >> -       clk_put(ctx->fimc_clk);
> >> -       clk_put(ctx->wb_clk);
> >> -       clk_put(ctx->wb_b_clk);
> >> -
> >>         return 0;
> >>  }
> >>
> >> --
> >> 1.7.4.1
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> >
>
> --
> With warm regards,
> Sachin
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
Sachin Kamat Dec. 27, 2012, 11:04 a.m. UTC | #5
On 27 December 2012 15:43, Inki Dae <inki.dae@samsung.com> wrote:
>
>
> 2012/12/26 Sachin Kamat <sachin.kamat@linaro.org>
>>
>>
>>
>> On Wednesday, 26 December 2012, Inki Dae <inki.dae@samsung.com> wrote:
>> >
>> >
>> > 2012/12/24 Sachin Kamat <sachin.kamat@linaro.org>
>> >>
>> >> This eliminates the need for explicit clk_put and makes the
>> >> cleanup and exit path code simpler.
>> >>
>> >> Cc: Eunchul Kim <chulspro.kim@samsung.com>
>> >> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
>> >> ---
>> >>  drivers/gpu/drm/exynos/exynos_drm_fimc.c |   46
>> >> ++++++-----------------------
>> >>  1 files changed, 10 insertions(+), 36 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimc.c
>> >> b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
>> >> index 972aa70..c4006b8 100644
>> >> --- a/drivers/gpu/drm/exynos/exynos_drm_fimc.c
>> >> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
>> >> @@ -1739,64 +1739,50 @@ static int __devinit fimc_probe(struct
>> >> platform_device *pdev)
>> >>                 platform_get_device_id(pdev)->driver_data;
>> >>
>> >>         /* clock control */
>> >> -       ctx->sclk_fimc_clk = clk_get(dev, "sclk_fimc");
>> >> +       ctx->sclk_fimc_clk = devm_clk_get(dev, "sclk_fimc");
>> >>         if (IS_ERR(ctx->sclk_fimc_clk)) {
>> >>                 dev_err(dev, "failed to get src fimc clock.\n");
>> >>                 return PTR_ERR(ctx->sclk_fimc_clk);
>> >>         }
>> >>         clk_enable(ctx->sclk_fimc_clk);
>> >>
>> >> -       ctx->fimc_clk = clk_get(dev, "fimc");
>> >> +       ctx->fimc_clk = devm_clk_get(dev, "fimc");
>> >>         if (IS_ERR(ctx->fimc_clk)) {
>> >>                 dev_err(dev, "failed to get fimc clock.\n");
>> >>                 clk_disable(ctx->sclk_fimc_clk);
>> >> -               clk_put(ctx->sclk_fimc_clk);
>> >>                 return PTR_ERR(ctx->fimc_clk);
>> >>         }
>> >>
>> >> -       ctx->wb_clk = clk_get(dev, "pxl_async0");
>> >> +       ctx->wb_clk = devm_clk_get(dev, "pxl_async0");
>> >>         if (IS_ERR(ctx->wb_clk)) {
>> >>                 dev_err(dev, "failed to get writeback a clock.\n");
>> >>                 clk_disable(ctx->sclk_fimc_clk);
>> >> -               clk_put(ctx->sclk_fimc_clk);
>> >> -               clk_put(ctx->fimc_clk);
>> >>                 return PTR_ERR(ctx->wb_clk);
>> >>         }
>> >>
>> >> -       ctx->wb_b_clk = clk_get(dev, "pxl_async1");
>> >> +       ctx->wb_b_clk = devm_clk_get(dev, "pxl_async1");
>> >>         if (IS_ERR(ctx->wb_b_clk)) {
>> >>                 dev_err(dev, "failed to get writeback b clock.\n");
>> >>                 clk_disable(ctx->sclk_fimc_clk);
>> >> -               clk_put(ctx->sclk_fimc_clk);
>> >> -               clk_put(ctx->fimc_clk);
>> >> -               clk_put(ctx->wb_clk);
>> >>                 return PTR_ERR(ctx->wb_b_clk);
>> >>         }
>> >>
>> >> -       parent_clk = clk_get(dev, ddata->parent_clk);
>> >> +       parent_clk = devm_clk_get(dev, ddata->parent_clk);
>> >>
>> >>         if (IS_ERR(parent_clk)) {
>> >>                 dev_err(dev, "failed to get parent clock.\n");
>> >>                 clk_disable(ctx->sclk_fimc_clk);
>> >> -               clk_put(ctx->sclk_fimc_clk);
>> >> -               clk_put(ctx->fimc_clk);
>> >> -               clk_put(ctx->wb_clk);
>> >> -               clk_put(ctx->wb_b_clk);
>> >>                 return PTR_ERR(parent_clk);
>> >>         }
>> >>
>> >>         if (clk_set_parent(ctx->sclk_fimc_clk, parent_clk)) {
>> >>                 dev_err(dev, "failed to set parent.\n");
>> >> -               clk_put(parent_clk);
>> >> +               devm_clk_put(dev, parent_clk);
>> >
>> > remove the above call. is there any reason that devm_clk_put should be
>> > called here?
>>
>> Devm resources are freed/released automatically only when the device
>> detaches. In the above case the clock was released explicitly (for whatever
>> reasons) earlier. Hence i have used devm call to keep the code logic same as
>> i do not know the behavior if this clock is 'put' when the device detaches
>> instead of here.
>>
>
> If probe is failed, devm resources are released by devres_release_all(). So
> that is unnecessary call. Remove it.

In this case you are right. It is not required. I will remove it.
But in the below case it is not part of cleanup, so I will keep it.
What is your opinion about it?

>
>>
>>
>> >
>> >>
>> >>                 clk_disable(ctx->sclk_fimc_clk);
>> >> -               clk_put(ctx->sclk_fimc_clk);
>> >> -               clk_put(ctx->fimc_clk);
>> >> -               clk_put(ctx->wb_clk);
>> >> -               clk_put(ctx->wb_b_clk);
>> >>                 return -EINVAL;
>> >>         }
>> >>
>> >> -       clk_put(parent_clk);
>> >> +       devm_clk_put(dev, parent_clk);
>> >
>> > ditto.
>> >
>> >>
>> >>         clk_set_rate(ctx->sclk_fimc_clk, pdata->clk_rate);
>> >>
>> >>         /* resource memory */
>> >> @@ -1804,16 +1790,14 @@ static int __devinit fimc_probe(struct
>> >> platform_device *pdev)
>> >>         ctx->regs = devm_request_and_ioremap(dev, ctx->regs_res);
>> >>         if (!ctx->regs) {
>> >>                 dev_err(dev, "failed to map registers.\n");
>> >> -               ret = -ENXIO;
>> >> -               goto err_clk;
>> >> +               return -ENXIO;
>> >>         }
>> >>
>> >>         /* resource irq */
>> >>         res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>> >>         if (!res) {
>> >>                 dev_err(dev, "failed to request irq resource.\n");
>> >> -               ret = -ENOENT;
>> >> -               goto err_clk;
>> >> +               return -ENOENT;
>> >>         }
>> >>
>> >>         ctx->irq = res->start;
>> >> @@ -1821,7 +1805,7 @@ static int __devinit fimc_probe(struct
>> >> platform_device *pdev)
>> >>                 IRQF_ONESHOT, "drm_fimc", ctx);
>> >>         if (ret < 0) {
>> >>                 dev_err(dev, "failed to request irq.\n");
>> >> -               goto err_clk;
>> >> +               return ret;
>> >>         }
>> >>
>> >>         /* context initailization */
>> >> @@ -1867,11 +1851,6 @@ err_ippdrv_register:
>> >>         pm_runtime_disable(dev);
>> >>  err_get_irq:
>> >>         free_irq(ctx->irq, ctx);
>> >> -err_clk:
>> >> -       clk_put(ctx->sclk_fimc_clk);
>> >> -       clk_put(ctx->fimc_clk);
>> >> -       clk_put(ctx->wb_clk);
>> >> -       clk_put(ctx->wb_b_clk);
>> >>
>> >>         return ret;
>> >>  }
>> >> @@ -1891,11 +1870,6 @@ static int __devexit fimc_remove(struct
>> >> platform_device *pdev)
>> >>
>> >>         free_irq(ctx->irq, ctx);
>> >>
>> >> -       clk_put(ctx->sclk_fimc_clk);
>> >> -       clk_put(ctx->fimc_clk);
>> >> -       clk_put(ctx->wb_clk);
>> >> -       clk_put(ctx->wb_b_clk);
>> >> -
>> >>         return 0;
>> >>  }
>> >>
>> >> --
>> >> 1.7.4.1
>> >>
>> >> _______________________________________________
>> >> dri-devel mailing list
>> >> dri-devel@lists.freedesktop.org
>> >> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>> >
>> >
>>
>> --
>> With warm regards,
>> Sachin
>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>
diff mbox

Patch

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimc.c b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
index 972aa70..c4006b8 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
@@ -1739,64 +1739,50 @@  static int __devinit fimc_probe(struct platform_device *pdev)
 		platform_get_device_id(pdev)->driver_data;
 
 	/* clock control */
-	ctx->sclk_fimc_clk = clk_get(dev, "sclk_fimc");
+	ctx->sclk_fimc_clk = devm_clk_get(dev, "sclk_fimc");
 	if (IS_ERR(ctx->sclk_fimc_clk)) {
 		dev_err(dev, "failed to get src fimc clock.\n");
 		return PTR_ERR(ctx->sclk_fimc_clk);
 	}
 	clk_enable(ctx->sclk_fimc_clk);
 
-	ctx->fimc_clk = clk_get(dev, "fimc");
+	ctx->fimc_clk = devm_clk_get(dev, "fimc");
 	if (IS_ERR(ctx->fimc_clk)) {
 		dev_err(dev, "failed to get fimc clock.\n");
 		clk_disable(ctx->sclk_fimc_clk);
-		clk_put(ctx->sclk_fimc_clk);
 		return PTR_ERR(ctx->fimc_clk);
 	}
 
-	ctx->wb_clk = clk_get(dev, "pxl_async0");
+	ctx->wb_clk = devm_clk_get(dev, "pxl_async0");
 	if (IS_ERR(ctx->wb_clk)) {
 		dev_err(dev, "failed to get writeback a clock.\n");
 		clk_disable(ctx->sclk_fimc_clk);
-		clk_put(ctx->sclk_fimc_clk);
-		clk_put(ctx->fimc_clk);
 		return PTR_ERR(ctx->wb_clk);
 	}
 
-	ctx->wb_b_clk = clk_get(dev, "pxl_async1");
+	ctx->wb_b_clk = devm_clk_get(dev, "pxl_async1");
 	if (IS_ERR(ctx->wb_b_clk)) {
 		dev_err(dev, "failed to get writeback b clock.\n");
 		clk_disable(ctx->sclk_fimc_clk);
-		clk_put(ctx->sclk_fimc_clk);
-		clk_put(ctx->fimc_clk);
-		clk_put(ctx->wb_clk);
 		return PTR_ERR(ctx->wb_b_clk);
 	}
 
-	parent_clk = clk_get(dev, ddata->parent_clk);
+	parent_clk = devm_clk_get(dev, ddata->parent_clk);
 
 	if (IS_ERR(parent_clk)) {
 		dev_err(dev, "failed to get parent clock.\n");
 		clk_disable(ctx->sclk_fimc_clk);
-		clk_put(ctx->sclk_fimc_clk);
-		clk_put(ctx->fimc_clk);
-		clk_put(ctx->wb_clk);
-		clk_put(ctx->wb_b_clk);
 		return PTR_ERR(parent_clk);
 	}
 
 	if (clk_set_parent(ctx->sclk_fimc_clk, parent_clk)) {
 		dev_err(dev, "failed to set parent.\n");
-		clk_put(parent_clk);
+		devm_clk_put(dev, parent_clk);
 		clk_disable(ctx->sclk_fimc_clk);
-		clk_put(ctx->sclk_fimc_clk);
-		clk_put(ctx->fimc_clk);
-		clk_put(ctx->wb_clk);
-		clk_put(ctx->wb_b_clk);
 		return -EINVAL;
 	}
 
-	clk_put(parent_clk);
+	devm_clk_put(dev, parent_clk);
 	clk_set_rate(ctx->sclk_fimc_clk, pdata->clk_rate);
 
 	/* resource memory */
@@ -1804,16 +1790,14 @@  static int __devinit fimc_probe(struct platform_device *pdev)
 	ctx->regs = devm_request_and_ioremap(dev, ctx->regs_res);
 	if (!ctx->regs) {
 		dev_err(dev, "failed to map registers.\n");
-		ret = -ENXIO;
-		goto err_clk;
+		return -ENXIO;
 	}
 
 	/* resource irq */
 	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 	if (!res) {
 		dev_err(dev, "failed to request irq resource.\n");
-		ret = -ENOENT;
-		goto err_clk;
+		return -ENOENT;
 	}
 
 	ctx->irq = res->start;
@@ -1821,7 +1805,7 @@  static int __devinit fimc_probe(struct platform_device *pdev)
 		IRQF_ONESHOT, "drm_fimc", ctx);
 	if (ret < 0) {
 		dev_err(dev, "failed to request irq.\n");
-		goto err_clk;
+		return ret;
 	}
 
 	/* context initailization */
@@ -1867,11 +1851,6 @@  err_ippdrv_register:
 	pm_runtime_disable(dev);
 err_get_irq:
 	free_irq(ctx->irq, ctx);
-err_clk:
-	clk_put(ctx->sclk_fimc_clk);
-	clk_put(ctx->fimc_clk);
-	clk_put(ctx->wb_clk);
-	clk_put(ctx->wb_b_clk);
 
 	return ret;
 }
@@ -1891,11 +1870,6 @@  static int __devexit fimc_remove(struct platform_device *pdev)
 
 	free_irq(ctx->irq, ctx);
 
-	clk_put(ctx->sclk_fimc_clk);
-	clk_put(ctx->fimc_clk);
-	clk_put(ctx->wb_clk);
-	clk_put(ctx->wb_b_clk);
-
 	return 0;
 }