Re: [PATCH v2 4/4] drm: panel: Add Jadard JD9365DA-H3 DSI panel

[Jagan Teki <jagan@edgeble.ai>]

Hi Linus,

On Tue, 13 Sept 2022 at 19:12, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Thu, Sep 8, 2022 at 4:00 PM Jagan Teki <jagan@edgeble.ai> wrote:
>
> > Jadard JD9365DA-H3 is WUXGA MIPI DSI panel and it support TFT
> > dot matrix LCD with 800RGBx1280 dots at maximum.
> >
> > Add support for it.
> >
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: Jagan Teki <jagan@edgeble.ai>
>
> I wrote to Jadard and asked for a datasheet. They didn't even answer,
> how rude.

That is the problem I always had for many years,  so I realized the
panel vendors here can help quite a bit. Try to contact via panel
vendors.

>
> > +#define _INIT_CMD_DCS(...)                                     \
> > +       {                                                       \
> > +               .type   = CMD_TYPE_DCS,                         \
> > +               .data   = (char[]){__VA_ARGS__},                \
> > +               .len    = sizeof((char[]){__VA_ARGS__})         \
> > +       }                                                       \
> > +
> > +#define _INIT_CMD_DELAY(...)                                   \
> > +       {                                                       \
> > +               .type   = CMD_TYPE_DELAY,                       \
> > +               .data   = (char[]){__VA_ARGS__},                \
> > +               .len    = sizeof((char[]){__VA_ARGS__})         \
> > +       }                                                       \
>
> I think the _MACRO namespace (macros starting with underscore)
> is reserved for the compiler.
>
> Just call them something unique if you have to do this, such as
> JD9365_INIT_CMD_DCS()

Make sense, I will correct this.

>
> But I think you should do something more elaborate here, see
> below.
>
> > +static const struct jadard_init_cmd cz101b4001_init_cmds[] = {
> > +       _INIT_CMD_DELAY(10),
> > +
> > +       _INIT_CMD_DCS(0xE0, 0x00),
> > +       _INIT_CMD_DCS(0xE1, 0x93),
> > +       _INIT_CMD_DCS(0xE2, 0x65),
> > +       _INIT_CMD_DCS(0xE3, 0xF8),
> > +       _INIT_CMD_DCS(0x80, 0x03),
> > +       _INIT_CMD_DCS(0xE0, 0x01),
>
> That's what we call a jam table.
> (...)
> > +       _INIT_CMD_DCS(0xE7, 0x0C),
> > +
> > +       _INIT_CMD_DELAY(120),
>
> You introduce _INIT_CMD_DELAY() for a delay just before and
> after the init sequence. Just open code these delays instead.
>
> Then you can drop the .type field from the DCS sequences because
> there is just one type.
>
>
> > +       for (i = 0; i < desc->num_init_cmds; i++) {
> > +               const struct jadard_init_cmd *cmd = &desc->init_cmds[i];
> > +
> > +               switch (cmd->type) {
> > +               case CMD_TYPE_DELAY:
> > +                       msleep(cmd->data[0]);
> > +                       err = 0;
> > +                       break;
> > +               case CMD_TYPE_DCS:
> > +                       err = mipi_dsi_dcs_write(dsi, cmd->data[0],
> > +                                                cmd->len <= 1 ? NULL : &cmd->data[1],
> > +                                                cmd->len - 1);
> > +                       break;
> > +               default:
> > +                       err = -EINVAL;
> > +               }
> > +
> > +               if (err < 0) {
> > +                       DRM_DEV_ERROR(dev, "failed to write CMD#0x%x\n", cmd->data[0]);
> > +                       return err;
> > +               }
> > +       }
>
> So add explicit delays before and after this for-loop.

I understand your point, I did think the same but since this delay is
specific to cz101b4001 init sequence adding it as part of the
cz101b4001 specific sequence makes sense to me instead of adding it on
common code. If there is another panel with a different delay sequence
might affect these delays in common code.

>
> But then you probably see that after that you can simply replace this entire
> for-loop with mipi_dsi_dcs_write_seq() so do that.
>
> Grep in the kernel tree for examples of mipi_dsi_dcs_write_seq().

Look like we don't need to take care of len here, I will try to use this.

Thanks,
Jagan.