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@xxxxxxxxxxxxxxxx>
> >>>>
> >>>> 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@xxxxxxxxxxxxxxxx>
> >>>> ---
> >>>> 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
