[PATCH v2 1/4] drm: rcar-du: dsi: Properly stop video mode TX

[Biju Das]

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 at ideasonboard.com>
> >>
> >> 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 at ideasonboard.com>
> >> ---
> >>   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