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

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

On Mon, Aug 22, 2022 at 05:19:56PM +0300, Tomi Valkeinen wrote:
> On 22/08/2022 17:05, Laurent Pinchart wrote:
> > 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.
> 
> That is true. But I'd rather write 0 to VCLKEN in the startup, before 
> writing the configuration.

You can do both :-)

> >> 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 :-)
> 
> Well, but that's a bug in the startup code. I don't think the shutdown 
> code should do things to make startup work better if the startup does 
> something wrong. Nevertheless, while I slightly disagree, I'm fine with 
> writing zero there in shutdown.

I agree it needs to be fixed at start() time, but I think it's also good
practice to put the device in a fully known state after shutdown, at
least when it's easy to do so. It would also save an unnecessary read
access to the register.

-- 
Regards,

Laurent Pinchart