Re: [PATCH v2 04/12] drm/sun4i: sun6i_mipi_dsi: Simplify drq set to support all modes

[Maxime Ripard]

On Mon, Nov 19, 2018 at 04:52:17PM +0530, Jagan Teki wrote:
> On Mon, Nov 19, 2018 at 2:02 PM Maxime Ripard <maxime.ripard@xxxxxxxxxxx> wrote:
> >
> > On Fri, Nov 16, 2018 at 10:09:08PM +0530, Jagan Teki wrote:
> > > Allwinner MIPI DSI DRQ set value can be varied with respective
> > > video modes.
> > > - burst mode the set value is always 0
> > > - video modes whose front porch greater than 20, the set value
> > >   can be computed based front porch and bpp.
> > > - video modes whose front porch is not greater than 20, the set value
> > >   is simply 0
> > >
> > > This patch simplifies existing drq set value code by grouping
> > > into sun6i_dsi_get_drq and support all video modes.
> > >
> > > Signed-off-by: Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx>
> > > ---
> > >  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 38 ++++++++++++++++----------
> > >  1 file changed, 23 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > index efd08bfd0cb8..d60955880c43 100644
> > > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > @@ -363,6 +363,26 @@ static void sun6i_dsi_inst_init(struct sun6i_dsi *dsi,
> > >                    SUN6I_DSI_INST_JUMP_CFG_NUM(1));
> > >  };
> > >
> > > +static int sun6i_dsi_get_drq(struct sun6i_dsi *dsi,
> > > +                           struct drm_display_mode *mode)
> > > +{
> > > +     struct mipi_dsi_device *device = dsi->device;
> > > +     int drq = 0;
> >
> > So, here, you declaring a variable called drq, set to 0.
> >
> > > +     if (device->mode_flags & MIPI_DSI_MODE_VIDEO_BURST)
> > > +             return drq;
> >
> > That you return here. You could just return 0, to be clearer.
> >
> > > +     if ((mode->hsync_start - mode->hdisplay) > 20) {
> > > +             /* Maaaaaagic */
> > > +             u16 drq = (mode->hsync_start - mode->hdisplay) - 20;
> >
> > You re-declare a variable with the same name here, but a different
> > type....
> >
> > > +             drq *= mipi_dsi_pixel_format_to_bpp(device->format);
> > > +             drq /= 32;
> > > +     }
> > > +
> > > +     return drq;
> >
> > And then return the first one? How is that even working?
> 
> Will fix this.

I don't want you to only fix this, I also want you to contribute code
that is A) useful, B) tested.

> > >  static u16 sun6i_dsi_get_timings_vblk(struct sun6i_dsi *dsi,
> > >                                     struct drm_display_mode *mode, u16 hblk)
> > >  {
> > > @@ -478,21 +498,9 @@ static u16 sun6i_dsi_get_video_start_delay(struct sun6i_dsi *dsi,
> > >  static void sun6i_dsi_setup_burst(struct sun6i_dsi *dsi,
> > >                                 struct drm_display_mode *mode)
> > >  {
> > > -     struct mipi_dsi_device *device = dsi->device;
> > > -     u32 val = 0;
> > > -
> > > -     if ((mode->hsync_start - mode->hdisplay) > 20) {
> > > -             /* Maaaaaagic */
> > > -             u16 drq = (mode->hsync_start - mode->hdisplay) - 20;
> > > -
> > > -             drq *= mipi_dsi_pixel_format_to_bpp(device->format);
> > > -             drq /= 32;
> > > -
> > > -             val = (SUN6I_DSI_TCON_DRQ_ENABLE_MODE |
> > > -                    SUN6I_DSI_TCON_DRQ_SET(drq));
> > > -     }
> > > -
> > > -     regmap_write(dsi->regs, SUN6I_DSI_TCON_DRQ_REG, val);
> > > +     regmap_write(dsi->regs, SUN6I_DSI_TCON_DRQ_REG,
> > > +                  SUN6I_DSI_TCON_DRQ_ENABLE_MODE |
> > > +                  SUN6I_DSI_TCON_DRQ_SET(sun6i_dsi_get_drq(dsi, mode)));
> >
> > On top of that, you now enable the DRQ stuff all the time, while it
> 
> Earlier the val value is ENABLE_MODE ORed with drq value. for the
> condition drq is computed in if block otherwise the val is 0.
> So as I explained in commit message the drq value is 0
> - for video modes whose front porch is not greater than 20 and
> - for burst mode the val
> 
> ie reason I mode it common.

The previous code was enabling it if the front porch was larger than
20 only. You enable it all the time now, and you never explain why.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Attachment: signature.asc
Description: PGP signature