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?