MIME-Version: 1.0
References: <20230220121258.10727-1-lujianhua000@gmail.com> <20230220121258.10727-2-lujianhua000@gmail.com>
In-Reply-To: <20230220121258.10727-2-lujianhua000@gmail.com>
From:   Linus Walleij <linus.walleij@linaro.org>
Date:   Tue, 7 Mar 2023 23:34:55 +0100
Message-ID: <CACRpkdbZCZiMM_qeqMd9=txVvPVHEzM4szOnPR-gCYdiXW_9eA@mail.gmail.com>
Subject: Re: [PATCH v2 2/2] drm/panel: Add driver for Novatek NT36523
To:     Jianhua Lu <lujianhua000@gmail.com>,
        Konrad Dybcio <konrad.dybcio@linaro.org>
Cc:     Thierry Reding <thierry.reding@gmail.com>,
        Sam Ravnborg <sam@ravnborg.org>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        dri-devel@lists.freedesktop.org,
        ~postmarketos/upstreaming@lists.sr.ht, phone-devel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Jianhua,

thanks for your patch!

It appears Konrad is working on a very similar driver, so I suggest merging
them into one Novatek NT36523 driver.

Possibly we can fix this up first and then add Konrads Lenovo-panel with
a patch on top.

On Mon, Feb 20, 2023 at 1:13=E2=80=AFPM Jianhua Lu <lujianhua000@gmail.com>=
 wrote:

> Add a driver for panels using the Novatek NT36523 display driver IC.
>
> Signed-off-by: Jianhua Lu <lujianhua000@gmail.com>

(...)

I like how you abstract the panel with init commands in the panel info.

> +enum dsi_cmd_type {
> +       INIT_DCS_CMD,
> +       DELAY_CMD,
> +};
> +
> +struct panel_init_cmd {
> +       enum dsi_cmd_type type;
> +       size_t len;
> +       const char *data;
> +};
> +
> +#define _INIT_DCS_CMD(...) { \
> +       .type =3D INIT_DCS_CMD, \
> +       .len =3D sizeof((char[]){__VA_ARGS__}), \
> +       .data =3D (char[]){__VA_ARGS__} }
> +
> +#define _INIT_DELAY_CMD(...) { \
> +       .type =3D DELAY_CMD,\
> +       .len =3D sizeof((char[]){__VA_ARGS__}), \
> +       .data =3D (char[]){__VA_ARGS__} }

I have seen this type of reinvented wheels a few times now. Don't do this.

Look into other recently merged drivers and look how they do it, for exampl=
e
drivers/gpu/drm/panel/panel-himax-hx8394.c

For example:

- Use mipi_dsi_dcs_write_seq()

- If the delay is just used at one point in the sequence, do not invent
  a command language like above for it, open code the delay instead

- Try to decode as much magic as possible, if you look in Konrads
  driver you clearly see some standard MIPI commands, I bet you have
  some too.

- Maybe use callbacks to send sequences instead of tables, like in
  the himax driver?

Other than that it seems like something that could also handle the Lenovo
display, or the other way around, I don't know which driver is the best
starting point, but this one has the right Novatek name at least.

Yours,
Linus Walleij