Date:   Tue, 20 Nov 2018 20:18:46 +0530
From:   Jagan Teki <jagan@...rulasolutions.com>
To:     Maxime Ripard <maxime.ripard@...tlin.com>
Cc:     Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Sean Paul <sean@...rly.run>, David Airlie <airlied@...ux.ie>,
        Rob Herring <robh+dt@...nel.org>, Chen-Yu Tsai <wens@...e.org>,
        Icenowy Zheng <icenowy@...c.io>,
        Jernej Skrabec <jernej.skrabec@...l.net>,
        Vasily Khoruzhick <anarsoul@...il.com>,
        Thierry Reding <thierry.reding@...il.com>,
        Mark Rutland <mark.rutland@....com>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        devicetree <devicetree@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        Michael Trimarchi <michael@...rulasolutions.com>,
        TL Lim <tllim@...e64.org>, linux-sunxi@...glegroups.com,
        linux-amarula@...rulasolutions.com
Subject: Re: [PATCH v2 04/12] drm/sun4i: sun6i_mipi_dsi: Simplify drq set to
 support all modes

On Tue, Nov 20, 2018 at 8:02 PM Maxime Ripard <maxime.ripard@...tlin.com> wrote:
>
> 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@...tlin.com> 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@...rulasolutions.com>
> > > > ---
> > > >  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.

It was tested in burst mode panel, the same added in the series. It
worked because burst mode need 0 drq rate.

>
> > > >  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.

Got it. Loo like I was confused to test these two separate series in
separate panels and missed something. Is it Ok to combine all dsi
changes together and send it in next version.

Powered by blists - more mailing lists