[PATCH v3] drm/exynos: enable FIMD clocks

[Inki Dae]

2013/4/22 Vikas Sajjan <vikas.sajjan at linaro.org>

>
> Hi Mr Dae,
>
> On 22 April 2013 11:19, Inki Dae <inki.dae at samsung.com> wrote:
>
>> Hi, Mr. Vikas
>>
>> Please fix the below typos Viresh pointed out and my comments.
>>
>> > -----Original Message-----
>> > From: Viresh Kumar [mailto:viresh.kumar at linaro.org]
>> > Sent: Monday, April 01, 2013 5:51 PM
>> > To: Vikas Sajjan
>> > Cc: dri-devel at lists.freedesktop.org; linux-samsung-soc at vger.kernel.org;
>> > jy0922.shim at samsung.com; inki.dae at samsung.com; kgene.kim at samsung.com;
>> > linaro-kernel at lists.linaro.org; linux-media at vger.kernel.org
>> > Subject: Re: [PATCH v3] drm/exynos: enable FIMD clocks
>> >
>> > On 1 April 2013 14:13, Vikas Sajjan <vikas.sajjan at 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.
>> >
>>
>
>   I shall modify the commit message as below as suggested by tomaz figa,
>
>   " Common Clock Framework introduced the need to prepare clocks before
> enabling them, otherwise clk_enable() fails. This patch adds clk_prepare
> calls to the driver. This patch also removes clk_disable() from
> fimd_remove() as it will be done by pm_runtime_put_sync".
>
>
>>  > > Signed-off-by: Vikas Sajjan <vikas.sajjan at 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 at 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);
>>
>> Just remove the above codes. It seems that clk_disable(also
>> clk_disable_unprepare) isn't needed because it will be done by
>> pm_runtime_put_sync and please re-post it(probably patch v5??)
>>
>>    So you mean I should work on v3 version, and keep  the
> clk_prepare_enable() code as is in fimd_clock() and just remove the
> clk_disable_unprepare() and also clk_disable() from fimd_remove(), right?
>
>
Right, please post it. And adding clk_enable() to probe() is another issue
so this could be discussed later. As is this patch is needed for CCF
support.



> Viresh and Tomaz Figa, let me know if these changes are fine with you guys.
>
> Thanks,
>> Inki Dae
>>
>> >
>> > 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
>>
>>
>
>
> --
> Thanks and Regards
>  Vikas Sajjan
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20130422/3d07d998/attachment-0001.html>