The SH-Mobile LCD Controller is part of a PM Domain on all relevant SoCs
(clock domain on all, power domain on some).  Hence it may not be
sufficient to manage the LCDC module clock explicitly (e.g. if the
selected clock source differs from SHMOB_DRM_CLK_BUS).

Fix this by using Runtime PM instead.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c | 11 ++++++++++-
 drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c  |  5 +++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c
index fbfd906844da490c..84dbf35025d7be63 100644
--- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c
+++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c
@@ -9,6 +9,7 @@
 
 #include <linux/backlight.h>
 #include <linux/clk.h>
+#include <linux/pm_runtime.h>
 
 #include <drm/drm_crtc.h>
 #include <drm/drm_crtc_helper.h>
@@ -170,10 +171,16 @@ static void shmob_drm_crtc_start(struct shmob_drm_crtc *scrtc)
 	if (WARN_ON(format == NULL))
 		return;
 
+	ret = pm_runtime_resume_and_get(sdev->dev);
+	if (ret)
+		return;
+
 	/* Enable clocks before accessing the hardware. */
 	ret = shmob_drm_clk_on(sdev);
-	if (ret < 0)
+	if (ret < 0) {
+		pm_runtime_put(sdev->dev);
 		return;
+	}
 
 	/* Reset and enable the LCDC. */
 	lcdc_write(sdev, LDCNT2R, lcdc_read(sdev, LDCNT2R) | LDCNT2R_BR);
@@ -271,6 +278,8 @@ static void shmob_drm_crtc_stop(struct shmob_drm_crtc *scrtc)
 	/* Stop clocks. */
 	shmob_drm_clk_off(sdev);
 
+	pm_runtime_put(sdev->dev);
+
 	scrtc->started = false;
 }
 
diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c
index 30493ce874192e3e..4f01caa119637032 100644
--- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c
+++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c
@@ -13,6 +13,7 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/pm.h>
+#include <linux/pm_runtime.h>
 #include <linux/slab.h>
 
 #include <drm/drm_drv.h>
@@ -216,6 +217,10 @@ static int shmob_drm_probe(struct platform_device *pdev)
 	if (IS_ERR(sdev->mmio))
 		return PTR_ERR(sdev->mmio);
 
+	ret = devm_pm_runtime_enable(&pdev->dev);
+	if (ret)
+		return ret;
+
 	ret = shmob_drm_setup_clocks(sdev, pdata->clk_source);
 	if (ret < 0)
 		return ret;
-- 
2.34.1

Hi Geert,

Thank you for the patch.

On Thu, Jun 22, 2023 at 11:21:18AM +0200, Geert Uytterhoeven wrote:
> The SH-Mobile LCD Controller is part of a PM Domain on all relevant SoCs
> (clock domain on all, power domain on some).  Hence it may not be
> sufficient to manage the LCDC module clock explicitly (e.g. if the
> selected clock source differs from SHMOB_DRM_CLK_BUS).
> 
> Fix this by using Runtime PM instead.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c | 11 ++++++++++-
>  drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c  |  5 +++++
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c
> index fbfd906844da490c..84dbf35025d7be63 100644
> --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c
> +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c
> @@ -9,6 +9,7 @@
>  
>  #include <linux/backlight.h>
>  #include <linux/clk.h>
> +#include <linux/pm_runtime.h>
>  
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_crtc_helper.h>
> @@ -170,10 +171,16 @@ static void shmob_drm_crtc_start(struct shmob_drm_crtc *scrtc)
>  	if (WARN_ON(format == NULL))
>  		return;
>  
> +	ret = pm_runtime_resume_and_get(sdev->dev);
> +	if (ret)
> +		return;
> +
>  	/* Enable clocks before accessing the hardware. */
>  	ret = shmob_drm_clk_on(sdev);

This would be best located in the runtime PM resume handler. Same for
disabling clocks in the runtime PM suspend handler.

> -	if (ret < 0)
> +	if (ret < 0) {
> +		pm_runtime_put(sdev->dev);
>  		return;
> +	}
>  
>  	/* Reset and enable the LCDC. */
>  	lcdc_write(sdev, LDCNT2R, lcdc_read(sdev, LDCNT2R) | LDCNT2R_BR);
> @@ -271,6 +278,8 @@ static void shmob_drm_crtc_stop(struct shmob_drm_crtc *scrtc)
>  	/* Stop clocks. */
>  	shmob_drm_clk_off(sdev);
>  
> +	pm_runtime_put(sdev->dev);
> +
>  	scrtc->started = false;
>  }
>  
> diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c
> index 30493ce874192e3e..4f01caa119637032 100644
> --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c
> +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c
> @@ -13,6 +13,7 @@
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/slab.h>
>  
>  #include <drm/drm_drv.h>
> @@ -216,6 +217,10 @@ static int shmob_drm_probe(struct platform_device *pdev)
>  	if (IS_ERR(sdev->mmio))
>  		return PTR_ERR(sdev->mmio);
>  
> +	ret = devm_pm_runtime_enable(&pdev->dev);
> +	if (ret)
> +		return ret;
> +

I would move this after shmob_drm_setup_clocks(), to ensure that the
runtime PM suspend and resume handlers will have access to clocks.

>  	ret = shmob_drm_setup_clocks(sdev, pdata->clk_source);
>  	if (ret < 0)
>  		return ret;

-- 
Regards,

Laurent Pinchart

On Fri, Jun 23, 2023 at 06:07:44PM +0300, Laurent Pinchart wrote:
> Hi Geert,
> 
> Thank you for the patch.
> 
> On Thu, Jun 22, 2023 at 11:21:18AM +0200, Geert Uytterhoeven wrote:
> > The SH-Mobile LCD Controller is part of a PM Domain on all relevant SoCs
> > (clock domain on all, power domain on some).  Hence it may not be
> > sufficient to manage the LCDC module clock explicitly (e.g. if the
> > selected clock source differs from SHMOB_DRM_CLK_BUS).
> > 
> > Fix this by using Runtime PM instead.
> > 
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> >  drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c | 11 ++++++++++-
> >  drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c  |  5 +++++
> >  2 files changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c
> > index fbfd906844da490c..84dbf35025d7be63 100644
> > --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c
> > +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c
> > @@ -9,6 +9,7 @@
> >  
> >  #include <linux/backlight.h>
> >  #include <linux/clk.h>
> > +#include <linux/pm_runtime.h>
> >  
> >  #include <drm/drm_crtc.h>
> >  #include <drm/drm_crtc_helper.h>
> > @@ -170,10 +171,16 @@ static void shmob_drm_crtc_start(struct shmob_drm_crtc *scrtc)
> >  	if (WARN_ON(format == NULL))
> >  		return;
> >  
> > +	ret = pm_runtime_resume_and_get(sdev->dev);
> > +	if (ret)
> > +		return;
> > +
> >  	/* Enable clocks before accessing the hardware. */
> >  	ret = shmob_drm_clk_on(sdev);
> 
> This would be best located in the runtime PM resume handler. Same for
> disabling clocks in the runtime PM suspend handler.

The driver should then depend on CONFIG_PM. There's no indirect
dependency through CONFIG_DRM as far as I can tell, but there's one
through ARCH_SHMOBILE. This then got me puzzled, as ARCH_SHMOBILE is
defined in arch/sh/Kconfig, and this driver depends on ARM. Am I missing
something ?

> > -	if (ret < 0)
> > +	if (ret < 0) {
> > +		pm_runtime_put(sdev->dev);
> >  		return;
> > +	}
> >  
> >  	/* Reset and enable the LCDC. */
> >  	lcdc_write(sdev, LDCNT2R, lcdc_read(sdev, LDCNT2R) | LDCNT2R_BR);
> > @@ -271,6 +278,8 @@ static void shmob_drm_crtc_stop(struct shmob_drm_crtc *scrtc)
> >  	/* Stop clocks. */
> >  	shmob_drm_clk_off(sdev);
> >  
> > +	pm_runtime_put(sdev->dev);
> > +
> >  	scrtc->started = false;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c
> > index 30493ce874192e3e..4f01caa119637032 100644
> > --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c
> > +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/module.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/pm.h>
> > +#include <linux/pm_runtime.h>
> >  #include <linux/slab.h>
> >  
> >  #include <drm/drm_drv.h>
> > @@ -216,6 +217,10 @@ static int shmob_drm_probe(struct platform_device *pdev)
> >  	if (IS_ERR(sdev->mmio))
> >  		return PTR_ERR(sdev->mmio);
> >  
> > +	ret = devm_pm_runtime_enable(&pdev->dev);
> > +	if (ret)
> > +		return ret;
> > +
> 
> I would move this after shmob_drm_setup_clocks(), to ensure that the
> runtime PM suspend and resume handlers will have access to clocks.
> 
> >  	ret = shmob_drm_setup_clocks(sdev, pdata->clk_source);
> >  	if (ret < 0)
> >  		return ret;

-- 
Regards,

Laurent Pinchart

Hi Laurent,

On Fri, Jun 23, 2023 at 5:11 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Fri, Jun 23, 2023 at 06:07:44PM +0300, Laurent Pinchart wrote:
> > On Thu, Jun 22, 2023 at 11:21:18AM +0200, Geert Uytterhoeven wrote:
> > > The SH-Mobile LCD Controller is part of a PM Domain on all relevant SoCs
> > > (clock domain on all, power domain on some).  Hence it may not be
> > > sufficient to manage the LCDC module clock explicitly (e.g. if the
> > > selected clock source differs from SHMOB_DRM_CLK_BUS).
> > >
> > > Fix this by using Runtime PM instead.
> > >
> > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > ---
> > >  drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c | 11 ++++++++++-
> > >  drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c  |  5 +++++
> > >  2 files changed, 15 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c
> > > index fbfd906844da490c..84dbf35025d7be63 100644
> > > --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c
> > > +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c
> > > @@ -9,6 +9,7 @@
> > >
> > >  #include <linux/backlight.h>
> > >  #include <linux/clk.h>
> > > +#include <linux/pm_runtime.h>
> > >
> > >  #include <drm/drm_crtc.h>
> > >  #include <drm/drm_crtc_helper.h>
> > > @@ -170,10 +171,16 @@ static void shmob_drm_crtc_start(struct shmob_drm_crtc *scrtc)
> > >     if (WARN_ON(format == NULL))
> > >             return;
> > >
> > > +   ret = pm_runtime_resume_and_get(sdev->dev);
> > > +   if (ret)
> > > +           return;
> > > +
> > >     /* Enable clocks before accessing the hardware. */
> > >     ret = shmob_drm_clk_on(sdev);
> >
> > This would be best located in the runtime PM resume handler. Same for
> > disabling clocks in the runtime PM suspend handler.
>
> The driver should then depend on CONFIG_PM. There's no indirect
> dependency through CONFIG_DRM as far as I can tell, but there's one
> through ARCH_SHMOBILE. This then got me puzzled, as ARCH_SHMOBILE is
> defined in arch/sh/Kconfig, and this driver depends on ARM. Am I missing
> something ?

Vommit 4bd65789ba847f39 ("drm: shmobile: Make DRM_SHMOBILE visible on
Renesas SoC platforms") in drm-next:

-       depends on DRM && ARM
-       depends on ARCH_SHMOBILE || COMPILE_TEST
+       depends on DRM
+       depends on ARCH_RENESAS || ARCH_SHMOBILE || COMPILE_TEST

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

On Fri, Jun 23, 2023 at 05:22:45PM +0200, Geert Uytterhoeven wrote:
> On Fri, Jun 23, 2023 at 5:11 PM Laurent Pinchart wrote:
> > On Fri, Jun 23, 2023 at 06:07:44PM +0300, Laurent Pinchart wrote:
> > > On Thu, Jun 22, 2023 at 11:21:18AM +0200, Geert Uytterhoeven wrote:
> > > > The SH-Mobile LCD Controller is part of a PM Domain on all relevant SoCs
> > > > (clock domain on all, power domain on some).  Hence it may not be
> > > > sufficient to manage the LCDC module clock explicitly (e.g. if the
> > > > selected clock source differs from SHMOB_DRM_CLK_BUS).
> > > >
> > > > Fix this by using Runtime PM instead.
> > > >
> > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > ---
> > > >  drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c | 11 ++++++++++-
> > > >  drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c  |  5 +++++
> > > >  2 files changed, 15 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c
> > > > index fbfd906844da490c..84dbf35025d7be63 100644
> > > > --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c
> > > > +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c
> > > > @@ -9,6 +9,7 @@
> > > >
> > > >  #include <linux/backlight.h>
> > > >  #include <linux/clk.h>
> > > > +#include <linux/pm_runtime.h>
> > > >
> > > >  #include <drm/drm_crtc.h>
> > > >  #include <drm/drm_crtc_helper.h>
> > > > @@ -170,10 +171,16 @@ static void shmob_drm_crtc_start(struct shmob_drm_crtc *scrtc)
> > > >     if (WARN_ON(format == NULL))
> > > >             return;
> > > >
> > > > +   ret = pm_runtime_resume_and_get(sdev->dev);
> > > > +   if (ret)
> > > > +           return;
> > > > +
> > > >     /* Enable clocks before accessing the hardware. */
> > > >     ret = shmob_drm_clk_on(sdev);
> > >
> > > This would be best located in the runtime PM resume handler. Same for
> > > disabling clocks in the runtime PM suspend handler.
> >
> > The driver should then depend on CONFIG_PM. There's no indirect
> > dependency through CONFIG_DRM as far as I can tell, but there's one
> > through ARCH_SHMOBILE. This then got me puzzled, as ARCH_SHMOBILE is
> > defined in arch/sh/Kconfig, and this driver depends on ARM. Am I missing
> > something ?
> 
> Vommit 4bd65789ba847f39 ("drm: shmobile: Make DRM_SHMOBILE visible on
> Renesas SoC platforms") in drm-next:
> 
> -       depends on DRM && ARM
> -       depends on ARCH_SHMOBILE || COMPILE_TEST
> +       depends on DRM
> +       depends on ARCH_RENESAS || ARCH_SHMOBILE || COMPILE_TEST

That's better indeed :-)

A dependency on CONFIG_PM is still needed as ARCH_RENESAS doesn't depend
on it.

-- 
Regards,

Laurent Pinchart

Hi Laurent,

On Fri, Jun 23, 2023 at 5:34 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Fri, Jun 23, 2023 at 05:22:45PM +0200, Geert Uytterhoeven wrote:
> > On Fri, Jun 23, 2023 at 5:11 PM Laurent Pinchart wrote:
> > > On Fri, Jun 23, 2023 at 06:07:44PM +0300, Laurent Pinchart wrote:
> > > > On Thu, Jun 22, 2023 at 11:21:18AM +0200, Geert Uytterhoeven wrote:
> > > > > The SH-Mobile LCD Controller is part of a PM Domain on all relevant SoCs
> > > > > (clock domain on all, power domain on some).  Hence it may not be
> > > > > sufficient to manage the LCDC module clock explicitly (e.g. if the
> > > > > selected clock source differs from SHMOB_DRM_CLK_BUS).
> > > > >
> > > > > Fix this by using Runtime PM instead.
> > > > >
> > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > > ---
> > > > >  drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c | 11 ++++++++++-
> > > > >  drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c  |  5 +++++
> > > > >  2 files changed, 15 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c
> > > > > index fbfd906844da490c..84dbf35025d7be63 100644
> > > > > --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c
> > > > > +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c
> > > > > @@ -9,6 +9,7 @@
> > > > >
> > > > >  #include <linux/backlight.h>
> > > > >  #include <linux/clk.h>
> > > > > +#include <linux/pm_runtime.h>
> > > > >
> > > > >  #include <drm/drm_crtc.h>
> > > > >  #include <drm/drm_crtc_helper.h>
> > > > > @@ -170,10 +171,16 @@ static void shmob_drm_crtc_start(struct shmob_drm_crtc *scrtc)
> > > > >     if (WARN_ON(format == NULL))
> > > > >             return;
> > > > >
> > > > > +   ret = pm_runtime_resume_and_get(sdev->dev);
> > > > > +   if (ret)
> > > > > +           return;
> > > > > +
> > > > >     /* Enable clocks before accessing the hardware. */
> > > > >     ret = shmob_drm_clk_on(sdev);
> > > >
> > > > This would be best located in the runtime PM resume handler. Same for
> > > > disabling clocks in the runtime PM suspend handler.
> > >
> > > The driver should then depend on CONFIG_PM. There's no indirect
> > > dependency through CONFIG_DRM as far as I can tell, but there's one
> > > through ARCH_SHMOBILE. This then got me puzzled, as ARCH_SHMOBILE is
> > > defined in arch/sh/Kconfig, and this driver depends on ARM. Am I missing
> > > something ?
> >
> > Vommit 4bd65789ba847f39 ("drm: shmobile: Make DRM_SHMOBILE visible on
> > Renesas SoC platforms") in drm-next:
> >
> > -       depends on DRM && ARM
> > -       depends on ARCH_SHMOBILE || COMPILE_TEST
> > +       depends on DRM
> > +       depends on ARCH_RENESAS || ARCH_SHMOBILE || COMPILE_TEST
>
> That's better indeed :-)
>
> A dependency on CONFIG_PM is still needed as ARCH_RENESAS doesn't depend
> on it.

ARCH_RMOBILE selects PM, so PM will be enabled on affected platforms.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Hi Geert,

On Fri, Jun 23, 2023 at 07:41:36PM +0200, Geert Uytterhoeven wrote:
> On Fri, Jun 23, 2023 at 5:34 PM Laurent Pinchart wrote:
> > On Fri, Jun 23, 2023 at 05:22:45PM +0200, Geert Uytterhoeven wrote:
> > > On Fri, Jun 23, 2023 at 5:11 PM Laurent Pinchart wrote:
> > > > On Fri, Jun 23, 2023 at 06:07:44PM +0300, Laurent Pinchart wrote:
> > > > > On Thu, Jun 22, 2023 at 11:21:18AM +0200, Geert Uytterhoeven wrote:
> > > > > > The SH-Mobile LCD Controller is part of a PM Domain on all relevant SoCs
> > > > > > (clock domain on all, power domain on some).  Hence it may not be
> > > > > > sufficient to manage the LCDC module clock explicitly (e.g. if the
> > > > > > selected clock source differs from SHMOB_DRM_CLK_BUS).
> > > > > >
> > > > > > Fix this by using Runtime PM instead.
> > > > > >
> > > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > > > ---
> > > > > >  drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c | 11 ++++++++++-
> > > > > >  drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c  |  5 +++++
> > > > > >  2 files changed, 15 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c
> > > > > > index fbfd906844da490c..84dbf35025d7be63 100644
> > > > > > --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c
> > > > > > +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c
> > > > > > @@ -9,6 +9,7 @@
> > > > > >
> > > > > >  #include <linux/backlight.h>
> > > > > >  #include <linux/clk.h>
> > > > > > +#include <linux/pm_runtime.h>
> > > > > >
> > > > > >  #include <drm/drm_crtc.h>
> > > > > >  #include <drm/drm_crtc_helper.h>
> > > > > > @@ -170,10 +171,16 @@ static void shmob_drm_crtc_start(struct shmob_drm_crtc *scrtc)
> > > > > >     if (WARN_ON(format == NULL))
> > > > > >             return;
> > > > > >
> > > > > > +   ret = pm_runtime_resume_and_get(sdev->dev);
> > > > > > +   if (ret)
> > > > > > +           return;
> > > > > > +
> > > > > >     /* Enable clocks before accessing the hardware. */
> > > > > >     ret = shmob_drm_clk_on(sdev);
> > > > >
> > > > > This would be best located in the runtime PM resume handler. Same for
> > > > > disabling clocks in the runtime PM suspend handler.
> > > >
> > > > The driver should then depend on CONFIG_PM. There's no indirect
> > > > dependency through CONFIG_DRM as far as I can tell, but there's one
> > > > through ARCH_SHMOBILE. This then got me puzzled, as ARCH_SHMOBILE is
> > > > defined in arch/sh/Kconfig, and this driver depends on ARM. Am I missing
> > > > something ?
> > >
> > > Vommit 4bd65789ba847f39 ("drm: shmobile: Make DRM_SHMOBILE visible on
> > > Renesas SoC platforms") in drm-next:
> > >
> > > -       depends on DRM && ARM
> > > -       depends on ARCH_SHMOBILE || COMPILE_TEST
> > > +       depends on DRM
> > > +       depends on ARCH_RENESAS || ARCH_SHMOBILE || COMPILE_TEST
> >
> > That's better indeed :-)
> >
> > A dependency on CONFIG_PM is still needed as ARCH_RENESAS doesn't depend
> > on it.
> 
> ARCH_RMOBILE selects PM, so PM will be enabled on affected platforms.

Which also means that you will never test compilation without CONFIG_PM,
while bots will. There's a risk of introducing compilation warnings.

-- 
Regards,

Laurent Pinchart