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

[Linus Walleij <linus.walleij@linaro.org>]

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.

> +#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()

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.

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

Yours,
Linus Walleij