[PATCH 0/3] drm/panel: jh057n00900: Move dsi init sequence to prepare

[PATCH 0/3] drm/panel: jh057n00900: Move dsi init sequence to prepare

[Guido Günther]

If the panel is wrapped in a panel_bridge it gets prepar()ed before the
upstream DSI bridge which can cause hangs (e.g. with imx-nwl since clocks
are not enabled yet). To avoid this move the panel's first DSI access to
enable() so the upstream bridge can prepare the DSI host controller in
it's pre_enable().

The second patch makes the disable() call symmetric to the above and the third
one just eases debugging.

Guido Günther (3):
  drm/panel: jh057n00900: Move panel DSI init to enable()
  drm/panel: jh057n00900: Move mipi_dsi_dcs_set_display_off to disable()
  drm/panel: jh057n00900: Print error code on all DRM_DEV_ERROR()s

 .../drm/panel/panel-rocktech-jh057n00900.c    | 31 ++++++++++++-------
 1 file changed, 19 insertions(+), 12 deletions(-)

-- 
2.20.1

[PATCH 1/3] drm/panel: jh057n00900: Move panel DSI init to enable()

[Guido Günther]

If the panel is wrapped in a panel_bridge it gets prepar()ed before the
upstream DSI bridge which can cause hangs (e.g. with imx-nwl since clocks
are not enabled yet). To avoid this move the panel's first DSI access to
enable() so the upstream bridge can prepare the DSI host controller in
it's pre_enable().

This is also in line with other panel drivers.

Signed-off-by: Guido Günther <agx@sigxcpu.org>
---
 .../gpu/drm/panel/panel-rocktech-jh057n00900.c    | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c b/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
index 1274b54f2672..c6b4bfd79fde 100644
--- a/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
+++ b/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
@@ -143,6 +143,14 @@ static int jh057n_init_sequence(struct jh057n *ctx)
 static int jh057n_enable(struct drm_panel *panel)
 {
 	struct jh057n *ctx = panel_to_jh057n(panel);
+	int ret;
+
+	ret = jh057n_init_sequence(ctx);
+	if (ret < 0) {
+		DRM_DEV_ERROR(ctx->dev, "Panel init sequence failed: %d\n",
+			      ret);
+		return ret;
+	}
 
 	return backlight_enable(ctx->backlight);
 }
@@ -197,13 +205,6 @@ static int jh057n_prepare(struct drm_panel *panel)
 	gpiod_set_value_cansleep(ctx->reset_gpio, 0);
 	msleep(20);
 
-	ret = jh057n_init_sequence(ctx);
-	if (ret < 0) {
-		DRM_DEV_ERROR(ctx->dev, "Panel init sequence failed: %d\n",
-			      ret);
-		return ret;
-	}
-
 	ctx->prepared = true;
 
 	return 0;
-- 
2.20.1

[PATCH 2/3] drm/panel: jh057n00900: Move mipi_dsi_dcs_set_display_off to disable()

[Guido Günther]

This makes it symmetric with the panel init happening in enable().

Signed-off-by: Guido Günther <agx@sigxcpu.org>
---
 drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c b/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
index c6b4bfd79fde..cc89831e30a6 100644
--- a/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
+++ b/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
@@ -158,19 +158,23 @@ static int jh057n_enable(struct drm_panel *panel)
 static int jh057n_disable(struct drm_panel *panel)
 {
 	struct jh057n *ctx = panel_to_jh057n(panel);
+	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
+	int ret;
+
+	ret = backlight_disable(ctx->backlight);
+	if (ret < 0)
+		return ret;
 
-	return backlight_disable(ctx->backlight);
+	return mipi_dsi_dcs_set_display_off(dsi);
 }
 
 static int jh057n_unprepare(struct drm_panel *panel)
 {
 	struct jh057n *ctx = panel_to_jh057n(panel);
-	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
 
 	if (!ctx->prepared)
 		return 0;
 
-	mipi_dsi_dcs_set_display_off(dsi);
 	regulator_disable(ctx->iovcc);
 	regulator_disable(ctx->vcc);
 	ctx->prepared = false;
-- 
2.20.1

[PATCH 3/3] drm/panel: jh057n00900: Print error code on all DRM_DEV_ERROR()s

[Guido Günther]

Most of them had these already but two mere missing. This eases
debugging.

Signed-off-by: Guido Günther <agx@sigxcpu.org>
---
 drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c b/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
index cc89831e30a6..a182ca7de701 100644
--- a/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
+++ b/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
@@ -127,7 +127,7 @@ static int jh057n_init_sequence(struct jh057n *ctx)
 
 	ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
 	if (ret < 0) {
-		DRM_DEV_ERROR(dev, "Failed to exit sleep mode\n");
+		DRM_DEV_ERROR(dev, "Failed to exit sleep mode: %d\n", ret);
 		return ret;
 	}
 	/* Panel is operational 120 msec after reset */
@@ -355,7 +355,9 @@ static int jh057n_probe(struct mipi_dsi_device *dsi)
 
 	ret = mipi_dsi_attach(dsi);
 	if (ret < 0) {
-		DRM_DEV_ERROR(dev, "mipi_dsi_attach failed. Is host ready?\n");
+		DRM_DEV_ERROR(dev,
+			      "mipi_dsi_attach failed (%d). Is host ready?\n",
+			      ret);
 		drm_panel_remove(&ctx->panel);
 		return ret;
 	}
-- 
2.20.1

Re: [PATCH 2/3] drm/panel: jh057n00900: Move mipi_dsi_dcs_set_display_off to disable()

[Sam Ravnborg]

Hi Guido.

On Fri, Jul 26, 2019 at 11:21:42AM +0200, Guido Günther wrote:
> This makes it symmetric with the panel init happening in enable().
> 
> Signed-off-by: Guido Günther <agx@sigxcpu.org>
> ---
>  drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c b/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
> index c6b4bfd79fde..cc89831e30a6 100644
> --- a/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
> +++ b/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
> @@ -158,19 +158,23 @@ static int jh057n_enable(struct drm_panel *panel)
>  static int jh057n_disable(struct drm_panel *panel)
>  {
>  	struct jh057n *ctx = panel_to_jh057n(panel);
> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> +	int ret;
> +
> +	ret = backlight_disable(ctx->backlight);
> +	if (ret < 0)
> +		return ret;
We most likely do not want to skip mipi_dsi_dcs_set_display_off()
just because we fail to disable backlight.
Most other panels disable backlight without a check for the return
value.

	Sam

Re: [PATCH 0/3] drm/panel: jh057n00900: Move dsi init sequence to prepare

[Sam Ravnborg]

Hi Guido.

On Fri, Jul 26, 2019 at 11:21:40AM +0200, Guido Günther wrote:
> If the panel is wrapped in a panel_bridge it gets prepar()ed before the
> upstream DSI bridge which can cause hangs (e.g. with imx-nwl since clocks
> are not enabled yet). To avoid this move the panel's first DSI access to
> enable() so the upstream bridge can prepare the DSI host controller in
> it's pre_enable().
> 
> The second patch makes the disable() call symmetric to the above and the third
> one just eases debugging.
> 
> Guido Günther (3):
>   drm/panel: jh057n00900: Move panel DSI init to enable()
>   drm/panel: jh057n00900: Move mipi_dsi_dcs_set_display_off to disable()
>   drm/panel: jh057n00900: Print error code on all DRM_DEV_ERROR()s

Patch 1 + 3 are both:
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

See comment on patch 2.

While you are touching this driver can you make an extra patch?

Today the driver calls the internal prepare,enable,disable,unprepare
functions.
The right way to do it is to use the
drm_panel_(prepare,enable,disable,unprepare) variants.

The benefit is that we can move a little logic to these functions
and the drivers will then benefit from this.

Two things I have in my local queue:
- Move bool for prepared/enabled
  (to protect that we do not prepare/enable twice)
- backlight support

This driver will benefit form both and this little modification will
make it simpler to introduce.
I can also prepare the patch if you prefer.

	Sam

Re: [PATCH 0/3] drm/panel: jh057n00900: Move dsi init sequence to prepare

[Guido Günther]

Hi Sam,
On Fri, Jul 26, 2019 at 12:25:29PM +0200, Sam Ravnborg wrote:
> Hi Guido.
> 
> On Fri, Jul 26, 2019 at 11:21:40AM +0200, Guido Günther wrote:
> > If the panel is wrapped in a panel_bridge it gets prepar()ed before the
> > upstream DSI bridge which can cause hangs (e.g. with imx-nwl since clocks
> > are not enabled yet). To avoid this move the panel's first DSI access to
> > enable() so the upstream bridge can prepare the DSI host controller in
> > it's pre_enable().
> > 
> > The second patch makes the disable() call symmetric to the above and the third
> > one just eases debugging.
> > 
> > Guido Günther (3):
> >   drm/panel: jh057n00900: Move panel DSI init to enable()
> >   drm/panel: jh057n00900: Move mipi_dsi_dcs_set_display_off to disable()
> >   drm/panel: jh057n00900: Print error code on all DRM_DEV_ERROR()s
> 
> Patch 1 + 3 are both:
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> 
> See comment on patch 2.

Fixed in v2.

> 
> While you are touching this driver can you make an extra patch?
> 
> Today the driver calls the internal prepare,enable,disable,unprepare
> functions.
> The right way to do it is to use the
> drm_panel_(prepare,enable,disable,unprepare) variants.

I hope I got this right in v2 but...

> 
> The benefit is that we can move a little logic to these functions
> and the drivers will then benefit from this.
> 
> Two things I have in my local queue:
> - Move bool for prepared/enabled
>   (to protect that we do not prepare/enable twice)
> - backlight support

...i hope so since what you have planned here would eliminate lots of
code duplication in the panel drivers.
Cheers and thanks for having a look!
 -- Guido

> 
> This driver will benefit form both and this little modification will
> make it simpler to introduce.
> I can also prepare the patch if you prefer.
> 
> 	Sam
>