* [PATCH] drm: shmob_drm: Check clk_prepare_enable() return value
@ 2013-12-02 0:52 Laurent Pinchart
2013-12-03 9:59 ` Thierry Reding
0 siblings, 1 reply; 7+ messages in thread
From: Laurent Pinchart @ 2013-12-02 0:52 UTC (permalink / raw)
To: dri-devel
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);
--
1.8.3.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] drm: shmob_drm: Check clk_prepare_enable() return value
2013-12-02 0:52 [PATCH] drm: shmob_drm: Check clk_prepare_enable() return value Laurent Pinchart
@ 2013-12-03 9:59 ` Thierry Reding
2013-12-04 1:38 ` Laurent Pinchart
0 siblings, 1 reply; 7+ messages in thread
From: Thierry Reding @ 2013-12-03 9:59 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 585 bytes --]
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
[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm: shmob_drm: Check clk_prepare_enable() return value
2013-12-03 9:59 ` Thierry Reding
@ 2013-12-04 1:38 ` Laurent Pinchart
2013-12-04 10:01 ` Thierry Reding
0 siblings, 1 reply; 7+ messages in thread
From: Laurent Pinchart @ 2013-12-04 1:38 UTC (permalink / raw)
To: Thierry Reding; +Cc: Laurent Pinchart, dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 963 bytes --]
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.
--
Regards,
Laurent Pinchart
[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm: shmob_drm: Check clk_prepare_enable() return value
2013-12-04 1:38 ` Laurent Pinchart
@ 2013-12-04 10:01 ` Thierry Reding
2013-12-04 14:37 ` Laurent Pinchart
0 siblings, 1 reply; 7+ messages in thread
From: Thierry Reding @ 2013-12-04 10:01 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: Laurent Pinchart, dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 1229 bytes --]
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
[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm: shmob_drm: Check clk_prepare_enable() return value
2013-12-04 10:01 ` Thierry Reding
@ 2013-12-04 14:37 ` Laurent Pinchart
2013-12-04 15:59 ` Thierry Reding
0 siblings, 1 reply; 7+ messages in thread
From: Laurent Pinchart @ 2013-12-04 14:37 UTC (permalink / raw)
To: Thierry Reding, dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 1681 bytes --]
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.
--
Regards,
Laurent Pinchart
[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm: shmob_drm: Check clk_prepare_enable() return value
2013-12-04 14:37 ` Laurent Pinchart
@ 2013-12-04 15:59 ` Thierry Reding
2013-12-04 16:45 ` Laurent Pinchart
0 siblings, 1 reply; 7+ messages in thread
From: Thierry Reding @ 2013-12-04 15:59 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 1915 bytes --]
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>
[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm: shmob_drm: Check clk_prepare_enable() return value
2013-12-04 15:59 ` Thierry Reding
@ 2013-12-04 16:45 ` Laurent Pinchart
0 siblings, 0 replies; 7+ messages in thread
From: Laurent Pinchart @ 2013-12-04 16:45 UTC (permalink / raw)
To: Thierry Reding; +Cc: dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 2071 bytes --]
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 ?
--
Regards,
Laurent Pinchart
[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-12-04 16:45 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-02 0:52 [PATCH] drm: shmob_drm: Check clk_prepare_enable() return value Laurent Pinchart
2013-12-03 9:59 ` Thierry Reding
2013-12-04 1:38 ` Laurent Pinchart
2013-12-04 10:01 ` Thierry Reding
2013-12-04 14:37 ` Laurent Pinchart
2013-12-04 15:59 ` Thierry Reding
2013-12-04 16:45 ` Laurent Pinchart
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.