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>
