| Message ID | 20190303173527.31055-6-jagan@amarulasolutions.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/sun4i: sun6i_mipi_dsi: Fixes/updates | expand |
On Sun, Mar 03, 2019 at 11:05:27PM +0530, Jagan Teki wrote: > DSI timings are varies between burst/non-burst devices and > current driver is handling this support via if, else statements > which would difficult to read. > > Simplify it by using goto to make code more readable. > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > Tested-by: Merlijn Wajer <merlijn@wizzup.org> > --- > Note: This change is created based on previous version burst > changes [1] by addressing comment from [2] by Maxime to make > code readable. > > [1] https://patchwork.kernel.org/cover/10813623/ > [2] https://patchwork.kernel.org/patch/10666607/ > > drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 79 ++++++++++++++------------ > 1 file changed, 42 insertions(+), 37 deletions(-) > > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > index 31ec4048804d..898f32319c2d 100644 > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > @@ -550,7 +550,8 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi, > { > struct mipi_dsi_device *device = dsi->device; > unsigned int Bpp = mipi_dsi_pixel_format_to_bpp(device->format) / 8; > - u16 hbp = 0, hfp = 0, hsa = 0, hblk = 0, vblk = 0; > + u16 hbp, hfp, hsa, hblk; > + u16 vblk = 0; > u32 basic_ctl = 0; > size_t bytes; > u8 *buffer; > @@ -558,6 +559,7 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi, > /* Do all timing calculations up front to allocate buffer space */ > > if (device->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) { > + hbp = hfp = hsa = 0; This raises a checkpatch warning and isn't necessary > hblk = mode->hdisplay * Bpp; > basic_ctl = SUN6I_DSI_BASIC_CTL_VIDEO_BURST | > SUN6I_DSI_BASIC_CTL_HSA_HSE_DIS | > @@ -566,48 +568,51 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi, > if (device->lanes == 4) > basic_ctl |= SUN6I_DSI_BASIC_CTL_TRAIL_FILL | > SUN6I_DSI_BASIC_CTL_TRAIL_INV(0xc); > - } else { > - /* > - * A sync period is composed of a blanking packet (4 > - * bytes + payload + 2 bytes) and a sync event packet > - * (4 bytes). Its minimal size is therefore 10 bytes > - */ > + > + goto alloc_buf; And I'd rather not have a goto in the middle of the code for no particular reason. Maxime
On Mon, Mar 4, 2019 at 9:20 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > On Sun, Mar 03, 2019 at 11:05:27PM +0530, Jagan Teki wrote: > > DSI timings are varies between burst/non-burst devices and > > current driver is handling this support via if, else statements > > which would difficult to read. > > > > Simplify it by using goto to make code more readable. > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > > Tested-by: Merlijn Wajer <merlijn@wizzup.org> > > --- > > Note: This change is created based on previous version burst > > changes [1] by addressing comment from [2] by Maxime to make > > code readable. > > > > [1] https://patchwork.kernel.org/cover/10813623/ > > [2] https://patchwork.kernel.org/patch/10666607/ > > > > drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 79 ++++++++++++++------------ > > 1 file changed, 42 insertions(+), 37 deletions(-) > > > > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > > index 31ec4048804d..898f32319c2d 100644 > > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > > @@ -550,7 +550,8 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi, > > { > > struct mipi_dsi_device *device = dsi->device; > > unsigned int Bpp = mipi_dsi_pixel_format_to_bpp(device->format) / 8; > > - u16 hbp = 0, hfp = 0, hsa = 0, hblk = 0, vblk = 0; > > + u16 hbp, hfp, hsa, hblk; > > + u16 vblk = 0; > > u32 basic_ctl = 0; > > size_t bytes; > > u8 *buffer; > > @@ -558,6 +559,7 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi, > > /* Do all timing calculations up front to allocate buffer space */ > > > > if (device->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) { > > + hbp = hfp = hsa = 0; > > This raises a checkpatch warning and isn't necessary > > > hblk = mode->hdisplay * Bpp; > > basic_ctl = SUN6I_DSI_BASIC_CTL_VIDEO_BURST | > > SUN6I_DSI_BASIC_CTL_HSA_HSE_DIS | > > @@ -566,48 +568,51 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi, > > if (device->lanes == 4) > > basic_ctl |= SUN6I_DSI_BASIC_CTL_TRAIL_FILL | > > SUN6I_DSI_BASIC_CTL_TRAIL_INV(0xc); > > - } else { > > - /* > > - * A sync period is composed of a blanking packet (4 > > - * bytes + payload + 2 bytes) and a sync event packet > > - * (4 bytes). Its minimal size is therefore 10 bytes > > - */ > > + > > + goto alloc_buf; > > And I'd rather not have a goto in the middle of the code for no > particular reason. OK, will try to think for better simplification otherwise will drop it in next version.
diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c index 31ec4048804d..898f32319c2d 100644 --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c @@ -550,7 +550,8 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi, { struct mipi_dsi_device *device = dsi->device; unsigned int Bpp = mipi_dsi_pixel_format_to_bpp(device->format) / 8; - u16 hbp = 0, hfp = 0, hsa = 0, hblk = 0, vblk = 0; + u16 hbp, hfp, hsa, hblk; + u16 vblk = 0; u32 basic_ctl = 0; size_t bytes; u8 *buffer; @@ -558,6 +559,7 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi, /* Do all timing calculations up front to allocate buffer space */ if (device->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) { + hbp = hfp = hsa = 0; hblk = mode->hdisplay * Bpp; basic_ctl = SUN6I_DSI_BASIC_CTL_VIDEO_BURST | SUN6I_DSI_BASIC_CTL_HSA_HSE_DIS | @@ -566,48 +568,51 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi, if (device->lanes == 4) basic_ctl |= SUN6I_DSI_BASIC_CTL_TRAIL_FILL | SUN6I_DSI_BASIC_CTL_TRAIL_INV(0xc); - } else { - /* - * A sync period is composed of a blanking packet (4 - * bytes + payload + 2 bytes) and a sync event packet - * (4 bytes). Its minimal size is therefore 10 bytes - */ + + goto alloc_buf; + } + + /* + * A sync period is composed of a blanking packet (4 + * bytes + payload + 2 bytes) and a sync event packet + * (4 bytes). Its minimal size is therefore 10 bytes + */ #define HSA_PACKET_OVERHEAD 10 - hsa = max((unsigned int)HSA_PACKET_OVERHEAD, - (mode->hsync_end - mode->hsync_start) * Bpp - HSA_PACKET_OVERHEAD); - - /* - * The backporch is set using a blanking packet (4 - * bytes + payload + 2 bytes). Its minimal size is - * therefore 6 bytes - */ + hsa = max((unsigned int)HSA_PACKET_OVERHEAD, + (mode->hsync_end - mode->hsync_start) * Bpp - HSA_PACKET_OVERHEAD); + + /* + * The backporch is set using a blanking packet (4 + * bytes + payload + 2 bytes). Its minimal size is + * therefore 6 bytes + */ #define HBP_PACKET_OVERHEAD 6 - hbp = max((unsigned int)HBP_PACKET_OVERHEAD, - (mode->htotal - mode->hsync_end) * Bpp - HBP_PACKET_OVERHEAD); - - /* - * The frontporch is set using a blanking packet (4 - * bytes + payload + 2 bytes). Its minimal size is - * therefore 6 bytes - */ + hbp = max((unsigned int)HBP_PACKET_OVERHEAD, + (mode->htotal - mode->hsync_end) * Bpp - HBP_PACKET_OVERHEAD); + + /* + * The frontporch is set using a blanking packet (4 + * bytes + payload + 2 bytes). Its minimal size is + * therefore 6 bytes + */ #define HFP_PACKET_OVERHEAD 6 - hfp = max((unsigned int)HFP_PACKET_OVERHEAD, - (mode->hsync_start - mode->hdisplay) * Bpp - HFP_PACKET_OVERHEAD); - - /* - * The blanking is set using a sync event (4 bytes) - * and a blanking packet (4 bytes + payload + 2 - * bytes). Its minimal size is therefore 10 bytes. - */ + hfp = max((unsigned int)HFP_PACKET_OVERHEAD, + (mode->hsync_start - mode->hdisplay) * Bpp - HFP_PACKET_OVERHEAD); + + /* + * The blanking is set using a sync event (4 bytes) + * and a blanking packet (4 bytes + payload + 2 + * bytes). Its minimal size is therefore 10 bytes. + */ #define HBLK_PACKET_OVERHEAD 10 - hblk = max((unsigned int)HBLK_PACKET_OVERHEAD, - (mode->htotal - (mode->hsync_end - mode->hsync_start)) * Bpp - - HBLK_PACKET_OVERHEAD); + hblk = max((unsigned int)HBLK_PACKET_OVERHEAD, + (mode->htotal - (mode->hsync_end - mode->hsync_start)) * Bpp - + HBLK_PACKET_OVERHEAD); - if (device->lanes == 4) - vblk = sun6i_dsi_get_timings_vblk(dsi, mode, hblk); - } + if (device->lanes == 4) + vblk = sun6i_dsi_get_timings_vblk(dsi, mode, hblk); +alloc_buf: /* How many bytes do we need to send all payloads? */ bytes = max_t(size_t, max(max(hfp, hblk), max(hsa, hbp)), vblk); buffer = kmalloc(bytes, GFP_KERNEL);