Hi Thierry,

On Monday 11 November 2013 09:55:24 Thierry Reding wrote:
> On Sat, Nov 09, 2013 at 01:51:04PM +0100, Laurent Pinchart wrote:
> > Hi Dave,
> > 
> > Could you please pick this patch up ?
> > 
> > On Monday 28 October 2013 23:49:22 Laurent Pinchart wrote:
> > > Turn clk_enable() and clk_disable() calls into clk_prepare_enable() and
> > > clk_disable_unprepare() to get ready for the migration to the common
> > > clock framework.
> > > 
> > > Cc: David Airlie <airlied@linux.ie>
> > > Cc: dri-devel@lists.freedesktop.org
> > > Signed-off-by: Laurent Pinchart
> > > <laurent.pinchart+renesas@ideasonboard.com>
> > > ---
> > > 
> > >  drivers/gpu/drm/shmobile/shmob_drm_crtc.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
> > > b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c index 54bad98..562f9a4
> > > 100644
> > > --- a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
> > > +++ b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
> > > @@ -40,7 +40,7 @@
> > >  static void shmob_drm_clk_on(struct shmob_drm_device *sdev)
> > >  {
> > >  	if (sdev->clock)
> > > -		clk_enable(sdev->clock);
> > > +		clk_prepare_enable(sdev->clock);
> 
> Sorry for jumping in so late, but shouldn't this be split into two
> separate calls, clk_prepare() in .probe() and clk_enable() here?

The clock prepare and enable operations are split to allow clock 
implementations to sleep. Clocks should be kept disable whenever possible, the 
clk_enable() and clk_disable() calls should be as close as possible to the 
time range during which the clock needs to be enabled. This means that those 
calls might happen in a context where sleeping isn't allowed. If a clock 
implementation needs to sleep to enable the clock (by performing an I2C access 
for instance), that operation should be performed at prepare time.

>From a clock user point of view, both clk_prepare() and clk_enable() should be 
called as late as possible. If clk_enable() needs to be called in an atomic 
context clk_prepare() must be called earlier, in a non-atomic context(). 
Otherwise there'e no point in splitting the two calls.

> Also note that both clk_prepare() and clk_enable() (and therefore
> clk_prepare_enable() as well) can fail, so you should really check
> the return values here.

Yes, that's a good point. I'd like to fix that in a separate patch in order to 
avoid delaying this one.

> > >  #if 0
> > >  	if (sdev->meram_dev && sdev->meram_dev->pdev)
> > >  		pm_runtime_get_sync(&sdev->meram_dev->pdev->dev);
> > > @@ -54,7 +54,7 @@ static void shmob_drm_clk_off(struct shmob_drm_device
> > > *sdev)
> > > 	pm_runtime_put_sync(&sdev->meram_dev->pdev->dev);
> > >  #endif
> > >  	if (sdev->clock)
> > > -		clk_disable(sdev->clock);
> > > +		clk_disable_unprepare(sdev->clock);
> 
> Similarily I'd expect this to be clk_disable() only, with the
> clk_unprepare() in .remove(). Or perhaps there's a very good reason to
> do both here?

-- 
Regards,

Laurent Pinchart