The clk_prepare_enable() call can fail. Check it's return value. We can't propagate it all the way to the user as the KMS operations in which the clock is enabled return a void.
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com --- drivers/gpu/drm/shmobile/shmob_drm_crtc.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c index 562f9a4..0428076 100644 --- a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c +++ b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c @@ -37,14 +37,21 @@ * Clock management */
-static void shmob_drm_clk_on(struct shmob_drm_device *sdev) +static int shmob_drm_clk_on(struct shmob_drm_device *sdev) { - if (sdev->clock) - clk_prepare_enable(sdev->clock); + int ret; + + if (sdev->clock) { + ret = clk_prepare_enable(sdev->clock); + if (ret < 0) + return ret; + } #if 0 if (sdev->meram_dev && sdev->meram_dev->pdev) pm_runtime_get_sync(&sdev->meram_dev->pdev->dev); #endif + + return 0; }
static void shmob_drm_clk_off(struct shmob_drm_device *sdev) @@ -161,6 +168,7 @@ static void shmob_drm_crtc_start(struct shmob_drm_crtc *scrtc) struct drm_device *dev = sdev->ddev; struct drm_plane *plane; u32 value; + int ret;
if (scrtc->started) return; @@ -170,7 +178,9 @@ static void shmob_drm_crtc_start(struct shmob_drm_crtc *scrtc) return;
/* Enable clocks before accessing the hardware. */ - shmob_drm_clk_on(sdev); + ret = shmob_drm_clk_on(sdev); + if (ret < 0) + return;
/* Reset and enable the LCDC. */ lcdc_write(sdev, LDCNT2R, lcdc_read(sdev, LDCNT2R) | LDCNT2R_BR);
On Mon, Dec 02, 2013 at 01:52:20AM +0100, Laurent Pinchart wrote: [...]
diff --git a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
[...]
@@ -170,7 +178,9 @@ static void shmob_drm_crtc_start(struct shmob_drm_crtc *scrtc) return;
/* Enable clocks before accessing the hardware. */
- shmob_drm_clk_on(sdev);
- ret = shmob_drm_clk_on(sdev);
- if (ret < 0)
return;
Perhaps this should be printing an error or using WARN_ON()? Otherwise it might be very difficult to diagnose what's going wrong.
Thierry
Hi Thierry,
On Tuesday 03 December 2013 10:59:24 Thierry Reding wrote:
On Mon, Dec 02, 2013 at 01:52:20AM +0100, Laurent Pinchart wrote: [...]
diff --git a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
[...]
@@ -170,7 +178,9 @@ static void shmob_drm_crtc_start(struct shmob_drm_crtc *scrtc) return;
/* Enable clocks before accessing the hardware. */
- shmob_drm_clk_on(sdev);
- ret = shmob_drm_clk_on(sdev);
- if (ret < 0)
return;Perhaps this should be printing an error or using WARN_ON()? Otherwise it might be very difficult to diagnose what's going wrong.
That's a good point. In practice, as the driver always uses a Renesas SoC MSTP clock, the clock code will print a message to the kernel log though, so I'm not sure whether we should duplicate that here. If you think we should I'll submit a new patch.
On Wed, Dec 04, 2013 at 02:38:21AM +0100, Laurent Pinchart wrote:
Hi Thierry,
On Tuesday 03 December 2013 10:59:24 Thierry Reding wrote:
On Mon, Dec 02, 2013 at 01:52:20AM +0100, Laurent Pinchart wrote: [...]
diff --git a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
[...]
@@ -170,7 +178,9 @@ static void shmob_drm_crtc_start(struct shmob_drm_crtc *scrtc) return;
/* Enable clocks before accessing the hardware. */
- shmob_drm_clk_on(sdev);
- ret = shmob_drm_clk_on(sdev);
- if (ret < 0)
return;Perhaps this should be printing an error or using WARN_ON()? Otherwise it might be very difficult to diagnose what's going wrong.
That's a good point. In practice, as the driver always uses a Renesas SoC MSTP clock, the clock code will print a message to the kernel log though, so I'm not sure whether we should duplicate that here. If you think we should I'll submit a new patch.
Is that one of the clocks implemented in drivers/sh/clk? There don't seem to be any error messages in the clk_enable() implementation.
Thierry
Hi Thierry,
On Wednesday 04 December 2013 11:01:07 Thierry Reding wrote:
On Wed, Dec 04, 2013 at 02:38:21AM +0100, Laurent Pinchart wrote:
On Tuesday 03 December 2013 10:59:24 Thierry Reding wrote:
On Mon, Dec 02, 2013 at 01:52:20AM +0100, Laurent Pinchart wrote: [...]
diff --git a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
[...]
@@ -170,7 +178,9 @@ static void shmob_drm_crtc_start(struct shmob_drm_crtc *scrtc)
return;/* Enable clocks before accessing the hardware. */
- shmob_drm_clk_on(sdev);
- ret = shmob_drm_clk_on(sdev);
- if (ret < 0)
return;Perhaps this should be printing an error or using WARN_ON()? Otherwise it might be very difficult to diagnose what's going wrong.
That's a good point. In practice, as the driver always uses a Renesas SoC MSTP clock, the clock code will print a message to the kernel log though, so I'm not sure whether we should duplicate that here. If you think we should I'll submit a new patch.
Is that one of the clocks implemented in drivers/sh/clk?
Correct, and in drivers/clk/shmobile/clk-mstp.c when http://www.spinics.net/lists/devicetree/msg12696.html will be merged.
There don't seem to be any error messages in the clk_enable() implementation.
There will be one when http://www.spinics.net/lists/linux-sh/msg25650.html will get merged :-) Before that enabling the clock never failed, with the patch applied the failure is logged.
On Wed, Dec 04, 2013 at 03:37:48PM +0100, Laurent Pinchart wrote:
Hi Thierry,
On Wednesday 04 December 2013 11:01:07 Thierry Reding wrote:
On Wed, Dec 04, 2013 at 02:38:21AM +0100, Laurent Pinchart wrote:
On Tuesday 03 December 2013 10:59:24 Thierry Reding wrote:
On Mon, Dec 02, 2013 at 01:52:20AM +0100, Laurent Pinchart wrote: [...]
diff --git a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
[...]
@@ -170,7 +178,9 @@ static void shmob_drm_crtc_start(struct shmob_drm_crtc *scrtc)
return;/* Enable clocks before accessing the hardware. */
- shmob_drm_clk_on(sdev);
- ret = shmob_drm_clk_on(sdev);
- if (ret < 0)
return;Perhaps this should be printing an error or using WARN_ON()? Otherwise it might be very difficult to diagnose what's going wrong.
That's a good point. In practice, as the driver always uses a Renesas SoC MSTP clock, the clock code will print a message to the kernel log though, so I'm not sure whether we should duplicate that here. If you think we should I'll submit a new patch.
Is that one of the clocks implemented in drivers/sh/clk?
Correct, and in drivers/clk/shmobile/clk-mstp.c when http://www.spinics.net/lists/devicetree/msg12696.html will be merged.
There don't seem to be any error messages in the clk_enable() implementation.
There will be one when http://www.spinics.net/lists/linux-sh/msg25650.html will get merged :-) Before that enabling the clock never failed, with the patch applied the failure is logged.
Alright,
Reviewed-by: Thierry Reding treding@nvidia.com
Hello,
On Wednesday 04 December 2013 16:59:22 Thierry Reding wrote:
On Wed, Dec 04, 2013 at 03:37:48PM +0100, Laurent Pinchart wrote:
On Wednesday 04 December 2013 11:01:07 Thierry Reding wrote:
On Wed, Dec 04, 2013 at 02:38:21AM +0100, Laurent Pinchart wrote:
On Tuesday 03 December 2013 10:59:24 Thierry Reding wrote:
On Mon, Dec 02, 2013 at 01:52:20AM +0100, Laurent Pinchart wrote: [...]
diff --git a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c
[...]
@@ -170,7 +178,9 @@ static void shmob_drm_crtc_start(struct shmob_drm_crtc *scrtc) return;
/* Enable clocks before accessing the hardware. */
- shmob_drm_clk_on(sdev);
- ret = shmob_drm_clk_on(sdev);
- if (ret < 0)
return;Perhaps this should be printing an error or using WARN_ON()? Otherwise it might be very difficult to diagnose what's going wrong.
That's a good point. In practice, as the driver always uses a Renesas SoC MSTP clock, the clock code will print a message to the kernel log though, so I'm not sure whether we should duplicate that here. If you think we should I'll submit a new patch.
Is that one of the clocks implemented in drivers/sh/clk?
Correct, and in drivers/clk/shmobile/clk-mstp.c when http://www.spinics.net/lists/devicetree/msg12696.html will be merged.
There don't seem to be any error messages in the clk_enable() implementation.
There will be one when http://www.spinics.net/lists/linux-sh/msg25650.html will get merged :-) Before that enabling the clock never failed, with the patch applied the failure is logged.
Alright,
Reviewed-by: Thierry Reding treding@nvidia.com
Thank you.
Dave, could you please pick this patch up for v3.14 ?
dri-devel@lists.freedesktop.org
-
Laurent Pinchart -
Laurent Pinchart -
Thierry Reding