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

[Vikas Sajjan <vikas.sajjan@linaro.org>]

[-- Attachment #1.1: Type: text/plain, Size: 4733 bytes --]

Hi Mr Dae,

On 22 April 2013 11:19, Inki Dae <inki.dae@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@linaro.org]
> > Sent: Monday, April 01, 2013 5:51 PM
> > To: Vikas Sajjan
> > Cc: dri-devel@lists.freedesktop.org; linux-samsung-soc@vger.kernel.org;
> > jy0922.shim@samsung.com; inki.dae@samsung.com; kgene.kim@samsung.com;
> > linaro-kernel@lists.linaro.org; linux-media@vger.kernel.org
> > Subject: Re: [PATCH v3] drm/exynos: enable FIMD clocks
> >
> > 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.
> >
>

  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@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);
>
> 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?

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

[-- Attachment #1.2: Type: text/html, Size: 7010 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel