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

Message

Guido Günther July 26, 2019, 9:21 a.m. UTC

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(-)

Comments

Sam Ravnborg July 26, 2019, 10:25 a.m. UTC | #1

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 <[e-mail]>

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

Guido Günther July 26, 2019, 1:16 p.m. UTC | #2

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 <[e-mail]>
> 
> 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
>