| Message ID | 1383000569-8916-6-git-send-email-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); > #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); > } > > /* > ---------------------------------------------------------------------------
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? 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. > > #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? Thierry
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?
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); #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); } /* -----------------------------------------------------------------------------
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(-)