[PATCH 2/2] drm/panel: Add driver for Sony ACX424AKP panel

[Linus Walleij]

On Mon, Sep 2, 2019 at 12:32 PM Thierry Reding <thierry.reding at gmail.com> wrote:
> On Mon, Sep 02, 2019 at 11:06:33AM +0200, Linus Walleij wrote:

> > +     /*
> > +      * This depends on the clocking HS vs LP rate, this value
> > +      * is calculated as:
> > +      * vrefresh = (clock * 1000) / (htotal*vtotal)
> > +      */
> > +     .vrefresh = 816,
>
> That's a bit odd. My understanding is that command mode can be done in
> continuous mode or in non-continuous mode. In continuous mode you would
> typically achieve a similar refresh rate as in video mode. For non-
> continuous mode you basically have a variable refresh rate.
>
> For continuous mode you probably want 60 Hz here and for VRR there's
> probably no sensible value to set this to. In the latter case, I don't
> think it makes sense to set anything arbitrary like you have above.
> Perhaps better to just set it to 0 as a way of signalling that this is
> actually dependent on when the display hardware sends a new frame?

I guess I should set it to 60.

> Have you actually run this is command mode?

Yes that is what I am primarily using.

> If so, what's the actual
> refresh rate? Do you do on-demand updates or do you run in continuous
> mode?

I run continuous mode, so the refresh rate is essentially dependent on
the HS frequency that the host uses. For command mode we use as
fast as it can go which is 420MHz, backwards calculated to ~816Hz
updates.

I took some data from the system when running:
175608 "vblank" interrupts in 25 minutes, yielding
175608/(25*60) ~= 117 Hz so I guess that is the pace the
hardware actually recieves it and triggers new updates.

> > +     ret = mipi_dsi_dcs_read(dsi, 0xda, &id1, len);
> > +     if (ret < 0) {
> > +             DRM_DEV_ERROR(acx->dev, "could not read device ID byte 1\n");
> > +             return ret;
> > +     }
> > +     ret = mipi_dsi_dcs_read(dsi, 0xdb, &id2, len);
> > +     if (ret < 0) {
> > +             DRM_DEV_ERROR(acx->dev, "could not read device ID byte 2\n");
> > +             return ret;
> > +     }
> > +     ret = mipi_dsi_dcs_read(dsi, 0xdc, &id3, len);
> > +     if (ret < 0) {
> > +             DRM_DEV_ERROR(acx->dev, "could not read device ID byte 3\n");
> > +             return ret;
> > +     }
> > +
> > +     val = (id3 << 8) | id2;
>
> Don't you want to OR in id1 here as well? Seems a bit odd to read it but
> then not use it.

The way I have understood these "MTP IDs" is that the first byte
should just be checked for not being 0 so I will add a check like that.

The only other DSI panel doing this is panel-samsung-s6e8aa0.c function
s6e8aa0_read_mtp_id() which also reads three bytes and ignores the
first byte, also the second byte being version and the third ID matches
the data this display returns.

I'll try to make it a bit clearer and inspect the individual bytes since they
seem to have individual meanings.

> > +     struct device *dev = &dsi->dev;
> > +     struct acx424akp *acx;
> > +     int ret;
> > +     int i;
>
> unsigned int?

Just following the common idiom for integer enumerator i to be a
plain int but sure, if you prefer :)

> > +     /* Read device ID */
> > +     i = 0;
> > +     do {
> > +             ret = acx424akp_read_id(acx);
> > +             if (ret)
> > +                     continue;
> > +     } while (ret && i++ < 5);
>
> Seems rather redundant to have both an "if (ret) continue;" and the ret
> check in the while's condition. A more idiomatic way to write this would
> be:
>
>         for (i = 0; i < 5; i++) {
>                 ret = acx424akp_read_id(acx);
>                 if (!ret)
>                         break;
>         }

OK! I fix.

Yours,
Linus Walleij