Re: [PATCH v2 2/4] drm: rcar-du: dsi: Improve DSI shutdown

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Mon, Aug 22, 2022 at 04:49:02PM +0300, Tomi Valkeinen wrote:
> On 22/08/2022 16:20, Laurent Pinchart wrote:
> > Hi Tomi,
> > 
> > Thank you for the patch.
> > 
> > On Mon, Aug 22, 2022 at 04:05:10PM +0300, Tomi Valkeinen wrote:
> >> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> >>
> >> Improve the DSI shutdown procedure by clearing various bits that were
> >> set while enabling the DSI output. There has been no clear issues caused
> >> by these, but it's safer to ensure that the features are disabled at the
> >> start of the next DSI enable.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> >> ---
> >>   drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c | 12 ++++++++++++
> >>   1 file changed, 12 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c
> >> index 7f2be490fcf8..6a10a35f1122 100644
> >> --- a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c
> >> @@ -441,9 +441,21 @@ static int rcar_mipi_dsi_startup(struct rcar_mipi_dsi *dsi,
> >>   
> >>   static void rcar_mipi_dsi_shutdown(struct rcar_mipi_dsi *dsi)
> >>   {
> >> +	/* Disable VCLKEN */
> >> +	rcar_mipi_dsi_clr(dsi, VCLKEN, VCLKEN_CKEN);
> >> +
> >> +	/* Disable DOT clock */
> >> +	rcar_mipi_dsi_clr(dsi, VCLKSET, VCLKSET_CKEN);
> > 
> > I think you can write 0 to those two registers, this will also be safer.
> > With this,
> 
> VCLKEN has only the single VCLKEN_CKEN bit and the rest of the bits are 
> reserved with default value of 0, however VCLKSET has other fields and 
> the default value of those fields is not 0.

But the two fields whose default value isn't 0 are set in the startup()
function (albeit incorrectly as discussed below), so it should be fine.

> Why do you think it's safer to set the whole register to 0? Isn't it 
> better to just do what we want to do, which makes the purpose clear and, 
> I think, is safer as we don't touch bits we don't know about?

Because it will ensure that we don't get surprises when we later restart
the device, such as mentioned below :-)

> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > I think there's a bug in rcar_mipi_dsi_startup() related to this by the
> > way, the function only uses rcar_mipi_dsi_set() to set bits, so if the
> > DSI format is modified between two starts, bad things will happen.
> 
> Oh, that's bad. rcar_mipi_dsi_set() is not a very good function as it's 
> easy to misuse it like that. I'll make a fix for that.
-- 
Regards,

Laurent Pinchart