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