Re: [PATCH v3] drm/exynos: enable FIMD clocks

[Viresh Kumar <viresh.kumar@linaro.org>]

On 1 April 2013 14:13, Vikas Sajjan <vikas.sajjan@linaro.org> wrote:
> While migrating to common clock framework (CCF), found that the FIMD clocks

s/found/we found/

> were pulled down by the CCF.
> If CCF finds any clock(s) which has NOT been claimed by any of the
> drivers, then such clock(s) are PULLed low by CCF.
>
> By calling clk_prepare_enable() for FIMD clocks fixes the issue.

s/By calling/Calling/

and

s/the/this

> this patch also replaces clk_disable() with clk_disable_unprepare()

s/this/This

> during exit.

Sorry but your log doesn't say what you are doing. You are just adding
relevant calls to clk_prepare/unprepare() before calling clk_enable/disable.

> Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
> ---
> Changes since v2:
>         - moved clk_prepare_enable() and clk_disable_unprepare() from
>         fimd_probe() to fimd_clock() as suggested by Inki Dae <inki.dae@samsung.com>
> Changes since v1:
>         - added error checking for clk_prepare_enable() and also replaced
>         clk_disable() with clk_disable_unprepare() during exit.
> ---
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c |   14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> index 9537761..f2400c8 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> @@ -799,18 +799,18 @@ static int fimd_clock(struct fimd_context *ctx, bool enable)
>         if (enable) {
>                 int ret;
>
> -               ret = clk_enable(ctx->bus_clk);
> +               ret = clk_prepare_enable(ctx->bus_clk);
>                 if (ret < 0)
>                         return ret;
>
> -               ret = clk_enable(ctx->lcd_clk);
> +               ret = clk_prepare_enable(ctx->lcd_clk);
>                 if  (ret < 0) {
> -                       clk_disable(ctx->bus_clk);
> +                       clk_disable_unprepare(ctx->bus_clk);
>                         return ret;
>                 }
>         } else {
> -               clk_disable(ctx->lcd_clk);
> -               clk_disable(ctx->bus_clk);
> +               clk_disable_unprepare(ctx->lcd_clk);
> +               clk_disable_unprepare(ctx->bus_clk);
>         }
>
>         return 0;
> @@ -981,8 +981,8 @@ static int fimd_remove(struct platform_device *pdev)
>         if (ctx->suspended)
>                 goto out;
>
> -       clk_disable(ctx->lcd_clk);
> -       clk_disable(ctx->bus_clk);
> +       clk_disable_unprepare(ctx->lcd_clk);
> +       clk_disable_unprepare(ctx->bus_clk);

You are doing things at the right place but i have a suggestion. Are you doing
anything in your clk_prepare() atleast for this device? Probably not.

If not, then its better to call clk_prepare/unprepare only once at probe/remove
and keep clk_enable/disable calls as is.

--
viresh