<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">2013/4/22 Vikas Sajjan <span dir="ltr"><<a href="mailto:vikas.sajjan@linaro.org" target="_blank">vikas.sajjan@linaro.org</a>></span><br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><br><div class="gmail_extra">Hi Mr Dae,<br></div><div class="gmail_extra"><br><div class="gmail_quote"><div><div class="h5">On 22 April 2013 11:19, Inki Dae <span dir="ltr"><<a href="mailto:inki.dae@samsung.com" target="_blank">inki.dae@samsung.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi, Mr. Vikas<br>
<br>
Please fix the below typos Viresh pointed out and my comments.<br>
<div><div><br>
> -----Original Message-----<br>
> From: Viresh Kumar [mailto:<a href="mailto:viresh.kumar@linaro.org" target="_blank">viresh.kumar@linaro.org</a>]<br>
> Sent: Monday, April 01, 2013 5:51 PM<br>
> To: Vikas Sajjan<br>
> Cc: <a href="mailto:dri-devel@lists.freedesktop.org" target="_blank">dri-devel@lists.freedesktop.org</a>; <a href="mailto:linux-samsung-soc@vger.kernel.org" target="_blank">linux-samsung-soc@vger.kernel.org</a>;<br>
> <a href="mailto:jy0922.shim@samsung.com" target="_blank">jy0922.shim@samsung.com</a>; <a href="mailto:inki.dae@samsung.com" target="_blank">inki.dae@samsung.com</a>; <a href="mailto:kgene.kim@samsung.com" target="_blank">kgene.kim@samsung.com</a>;<br>
> <a href="mailto:linaro-kernel@lists.linaro.org" target="_blank">linaro-kernel@lists.linaro.org</a>; <a href="mailto:linux-media@vger.kernel.org" target="_blank">linux-media@vger.kernel.org</a><br>
> Subject: Re: [PATCH v3] drm/exynos: enable FIMD clocks<br>
><br>
> On 1 April 2013 14:13, Vikas Sajjan <<a href="mailto:vikas.sajjan@linaro.org" target="_blank">vikas.sajjan@linaro.org</a>> wrote:<br>
> > While migrating to common clock framework (CCF), found that the FIMD<br>
> clocks<br>
><br>
> s/found/we found/<br>
><br>
> > were pulled down by the CCF.<br>
> > If CCF finds any clock(s) which has NOT been claimed by any of the<br>
> > drivers, then such clock(s) are PULLed low by CCF.<br>
> ><br>
> > By calling clk_prepare_enable() for FIMD clocks fixes the issue.<br>
><br>
> s/By calling/Calling/<br>
><br>
> and<br>
><br>
> s/the/this<br>
><br>
> > this patch also replaces clk_disable() with clk_disable_unprepare()<br>
><br>
> s/this/This<br>
><br>
> > during exit.<br>
><br>
> Sorry but your log doesn't say what you are doing. You are just adding<br>
> relevant calls to clk_prepare/unprepare() before calling<br>
> clk_enable/disable.<br>
><br></div></div></blockquote><div><br></div></div></div><div> I shall modify the commit message as below as suggested by tomaz figa,<br><br> " Common Clock Framework introduced the need to prepare clocks before<br>
enabling them, otherwise clk_enable() fails. This patch adds clk_prepare<br>
calls to the driver. This patch also removes clk_disable() from fimd_remove() as it will be done by pm_runtime_put_sync".<br> <br></div><div><div class="h5"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><div>
> > Signed-off-by: Vikas Sajjan <<a href="mailto:vikas.sajjan@linaro.org" target="_blank">vikas.sajjan@linaro.org</a>><br>
> > ---<br>
> > Changes since v2:<br>
> > - moved clk_prepare_enable() and clk_disable_unprepare() from<br>
> > fimd_probe() to fimd_clock() as suggested by Inki Dae<br>
> <<a href="mailto:inki.dae@samsung.com" target="_blank">inki.dae@samsung.com</a>><br>
> > Changes since v1:<br>
> > - added error checking for clk_prepare_enable() and also<br>
replaced<br>
> > clk_disable() with clk_disable_unprepare() during exit.<br>
> > ---<br>
> > drivers/gpu/drm/exynos/exynos_drm_fimd.c | 14 +++++++-------<br>
> > 1 file changed, 7 insertions(+), 7 deletions(-)<br>
> ><br>
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c<br>
> b/drivers/gpu/drm/exynos/exynos_drm_fimd.c<br>
> > index 9537761..f2400c8 100644<br>
> > --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c<br>
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c<br>
> > @@ -799,18 +799,18 @@ static int fimd_clock(struct fimd_context *ctx,<br>
> bool enable)<br>
> > if (enable) {<br>
> > int ret;<br>
> ><br>
> > - ret = clk_enable(ctx->bus_clk);<br>
> > + ret = clk_prepare_enable(ctx->bus_clk);<br>
> > if (ret < 0)<br>
> > return ret;<br>
> ><br>
> > - ret = clk_enable(ctx->lcd_clk);<br>
> > + ret = clk_prepare_enable(ctx->lcd_clk);<br>
> > if (ret < 0) {<br>
> > - clk_disable(ctx->bus_clk);<br>
> > + clk_disable_unprepare(ctx->bus_clk);<br>
> > return ret;<br>
> > }<br>
> > } else {<br>
> > - clk_disable(ctx->lcd_clk);<br>
> > - clk_disable(ctx->bus_clk);<br>
> > + clk_disable_unprepare(ctx->lcd_clk);<br>
> > + clk_disable_unprepare(ctx->bus_clk);<br>
> > }<br>
> ><br>
> > return 0;<br>
> > @@ -981,8 +981,8 @@ static int fimd_remove(struct platform_device *pdev)<br>
> > if (ctx->suspended)<br>
> > goto out;<br>
> ><br>
> > - clk_disable(ctx->lcd_clk);<br>
> > - clk_disable(ctx->bus_clk);<br>
> > + clk_disable_unprepare(ctx->lcd_clk);<br>
> > + clk_disable_unprepare(ctx->bus_clk);<br>
<br>
</div></div>Just remove the above codes. It seems that clk_disable(also<br>
clk_disable_unprepare) isn't needed because it will be done by<br>
pm_runtime_put_sync and please re-post it(probably patch v5??)<br>
<br></blockquote></div></div><div> 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? <br>
</div><div><br></div></div></div></div></blockquote><div><br></div><div>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.<br>
<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Viresh and Tomaz Figa, let me know if these changes are fine with you guys.<br>
</div><div class="im"><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Thanks,<br>
Inki Dae<br>
<div><div><br>
><br>
> You are doing things at the right place but i have a suggestion. Are you<br>
> doing<br>
> anything in your clk_prepare() atleast for this device? Probably not.<br>
><br>
> If not, then its better to call clk_prepare/unprepare only once at<br>
> probe/remove<br>
> and keep clk_enable/disable calls as is.<br>
><br>
> --<br>
> viresh<br>
<br>
</div></div></blockquote></div></div><span class="HOEnZb"><font color="#888888"><br><br clear="all"><br>-- <br>Thanks and Regards<div> Vikas Sajjan</div>
</font></span></div></div>
<br>_______________________________________________<br>
dri-devel mailing list<br>
<a href="mailto:dri-devel@lists.freedesktop.org">dri-devel@lists.freedesktop.org</a><br>
<a href="http://lists.freedesktop.org/mailman/listinfo/dri-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/dri-devel</a><br>
<br></blockquote></div><br></div></div>