On Tue, Mar 07, 2023 at 11:34:55PM +0100, Linus Walleij wrote:
> 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 PM Jianhua Lu <lujianhua000@xxxxxxxxx> wrote:
>
> > Add a driver for panels using the Novatek NT36523 display driver IC.
> >
> > Signed-off-by: Jianhua Lu <lujianhua000@xxxxxxxxx>
>
> (...)
>
> 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 = INIT_DCS_CMD, \
> > + .len = sizeof((char[]){__VA_ARGS__}), \
> > + .data = (char[]){__VA_ARGS__} }
> > +
> > +#define _INIT_DELAY_CMD(...) { \
> > + .type = DELAY_CMD,\
> > + .len = sizeof((char[]){__VA_ARGS__}), \
> > + .data = (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 example
> 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?
Maybe I should create a wrapper of mipi_dsi_dcs_write_seq() for sync dual dsi mode.
>
> 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
