Hi,
> Subject: Re: [PATCH v2 1/4] drm: rcar-du: dsi: Properly stop video mode
> TX
>
> Hi,
>
> On 22/08/2022 16:25, Biju Das wrote:
> > Hi Tomi,
> >
> > Thanks for the patch.
> >
> >> Subject: [PATCH v2 1/4] drm: rcar-du: dsi: Properly stop video mode
> >> TX
> >>
> >> From: Tomi Valkeinen <tomi.valkeinen+renesas@xxxxxxxxxxxxxxxx>
> >>
> >> The driver does not explicitly stop the video mode transmission when
> >> disabling the output. While this doesn't seem to be causing any
> >> issues, lets follow the steps described in the documentation and add
> >> a
> >> rcar_mipi_dsi_stop_video() which stop the video mode transmission.
> >> This function will also be used in later patches to stop the video
> >> transmission even if the DSI IP is not shut down.
> >>
> >> Signed-off-by: Tomi Valkeinen
> >> <tomi.valkeinen+renesas@xxxxxxxxxxxxxxxx>
> >> ---
> >> drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c | 29
> +++++++++++++++++++++++++
> >> 1 file changed, 29 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c
> >> b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c
> >> index 62f7eb84ab01..7f2be490fcf8 100644
> >> --- a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c
> >> @@ -542,6 +542,34 @@ static int rcar_mipi_dsi_start_video(struct
> >> rcar_mipi_dsi *dsi)
> >> return 0;
> >> }
> >>
> >> +static void rcar_mipi_dsi_stop_video(struct rcar_mipi_dsi *dsi) {
> >> + u32 status;
> >> + int ret;
> >> +
> >> + /* Disable transmission in video mode. */
> >> + rcar_mipi_dsi_clr(dsi, TXVMCR, TXVMCR_EN_VIDEO);
> >> +
> >> + ret = read_poll_timeout(rcar_mipi_dsi_read, status,
> >> + !(status & TXVMSR_ACT),
> >> + 2000, 100000, false, dsi, TXVMSR);
> >> + if (ret < 0) {
> >> + dev_err(dsi->dev, "Failed to disable video transmission\n");
> >> + return;
> >> + }
> >> +
> >> + /* Assert video FIFO clear. */
> >> + rcar_mipi_dsi_set(dsi, TXVMCR, TXVMCR_VFCLR);
> >> +
> >> + ret = read_poll_timeout(rcar_mipi_dsi_read, status,
> >> + !(status & TXVMSR_VFRDY),
> >> + 2000, 100000, false, dsi, TXVMSR);
> >> + if (ret < 0) {
> >> + dev_err(dsi->dev, "Failed to assert video FIFO clear\n");
> >> + return;
> >
> > This return is not required.
>
> That is true, but I'd personally rather keep it there. If the return is
> removed, I can imagine how easily one could add a new piece of code in
> the end of the function, not realizing that one also needs to add a
> return (the one above) so that the code works correctly.
>
> It just feels a bit safer this way.
OK, I just thought of reducing number of lines and remove unwanted code
As return type is void.
if (ret < 0)
dev_err(dsi->dev, "Failed to assert video FIFO clear\n");
Any way there is a review process which will capture the issue you mentioned.
I am ok with your statement.
Cheers,
Biju
