Re: [PATCH v2 2/2] drm/panel: Add driver for Novatek NT36523

[Jianhua Lu <lujianhua000@gmail.com>]

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@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 = 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