From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH 05/12] drm: shmob_drm: Convert to clk_prepare/unprepare Date: Mon, 11 Nov 2013 13:55:16 +0100 Message-ID: <1968627.DN20fBmCvM@avalon> References: <1383000569-8916-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com> <2959885.VdyEnllx0c@avalon> <20131111085523.GC3884@ulmo.nvidia.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1837042502==" Return-path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [95.142.166.194]) by gabe.freedesktop.org (Postfix) with ESMTP id 38BB0FAD4F for ; Mon, 11 Nov 2013 04:54:49 -0800 (PST) In-Reply-To: <20131111085523.GC3884@ulmo.nvidia.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces@lists.freedesktop.org Errors-To: dri-devel-bounces@lists.freedesktop.org To: Thierry Reding Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============1837042502== Content-Type: multipart/signed; boundary="nextPart1623284.rROVLY9Ced"; micalg="pgp-sha1"; protocol="application/pgp-signature" --nextPart1623284.rROVLY9Ced Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" 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 > > > Cc: dri-devel@lists.freedesktop.org > > > Signed-off-by: Laurent Pinchart > > > > > > --- > > > > > > 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 --nextPart1623284.rROVLY9Ced Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part. Content-Transfer-Encoding: 7Bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQEcBAABAgAGBQJSgNO7AAoJEIkPb2GL7hl1/TwH/3vai9woSnoWJcfMLrZLRDsh XXhPvsD4iRcAIzuQMtEIGMKH5jrN8z6Tfn5fSN5ZHkLNnSqjV0R/7HeBBQIgpBSy FaDQixAfzi8073g3LceJDHli55Qs6SYsiHjG1yUz9MRQvw8m0YsPW84dSXfCdDSV 9UTa35PczOLnmutpwoQxIe52RSoeEbCZAdfZVjrVP6p8NCbqAi9n9hCMqqD7anGK ygjamXDujeH336vpHtakHMWLhhDyNoBBduqWaTMlaxTIUfvasvcUkp3ivykmj05D zUBMMgjblY1W3iipXIi9pyjw7BZ6Ys37/dSb2EyUNdcITSPUN6JWBsizcdcVrpI= =QGO7 -----END PGP SIGNATURE----- --nextPart1623284.rROVLY9Ced-- --===============1837042502== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel --===============1837042502==--