RE: [PATCH v2 1/4] drm: rcar-du: dsi: Properly stop video mode TX
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 > >> > >> 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 > >> > >> --- > >> 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, 10, 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, 10, 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
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 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 --- 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, 10, 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, 10, 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. Tomi
RE: [PATCH v2 1/4] drm: rcar-du: dsi: Properly stop video mode TX
Hi Tomi, Thanks for the patch. > Subject: [PATCH v2 1/4] drm: rcar-du: dsi: Properly stop video mode TX > > From: Tomi Valkeinen > > 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 > --- > 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, 10, 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, 10, false, dsi, TXVMSR); > + if (ret < 0) { > + dev_err(dsi->dev, "Failed to assert video FIFO clear\n"); > + return; This return is not required. Cheers, Biju > + } > +} > + > /* > - > * Bridge > */ > @@ -601,6 +629,7 @@ static void rcar_mipi_dsi_atomic_disable(struct > drm_bridge *bridge, { > struct rcar_mipi_dsi *dsi = bridge_to_rcar_mipi_dsi(bridge); > > + rcar_mipi_dsi_stop_video(dsi); > rcar_mipi_dsi_shutdown(dsi); > rcar_mipi_dsi_clk_disable(dsi); > } > -- > 2.34.1
Re: [PATCH v2 1/4] drm: rcar-du: dsi: Properly stop video mode TX
Hi Tomi, Thank you for the patch. On Mon, Aug 22, 2022 at 04:05:09PM +0300, Tomi Valkeinen wrote: > From: Tomi Valkeinen > > 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 Reviewed-by: Laurent Pinchart > --- > 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, 10, 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, 10, false, dsi, TXVMSR); > + if (ret < 0) { > + dev_err(dsi->dev, "Failed to assert video FIFO clear\n"); > + return; > + } > +} > + > /* > - > * Bridge > */ > @@ -601,6 +629,7 @@ static void rcar_mipi_dsi_atomic_disable(struct > drm_bridge *bridge, > { > struct rcar_mipi_dsi *dsi = bridge_to_rcar_mipi_dsi(bridge); > > + rcar_mipi_dsi_stop_video(dsi); > rcar_mipi_dsi_shutdown(dsi); > rcar_mipi_dsi_clk_disable(dsi); > } -- Regards, Laurent Pinchart