Re: [PATCH v5 2/2 RESEND] drm: panel: Add novatek nt35596s panel driver

[Linus Walleij]

Hi Arnaud & Molly,

overall the driver looks very good!

On Wed, Jul 19, 2023 at 5:20 PM Arnaud Ferraris
<arnaud.ferraris@xxxxxxxxxxxxx> wrote:

> From: Molly Sophia <mollysophia379@xxxxxxxxx>
>
> Novatek NT35596s is a generic DSI IC that drives command and video mode
> panels. Add the driver for it. Currently add support for the LCD panel
> from JDI connected with this IC, as found on Xiaomi Mi Mix2s phones.
>
> Signed-off-by: Molly Sophia <mollysophia379@xxxxxxxxx>
> Signed-off-by: Arnaud Ferraris <arnaud.ferraris@xxxxxxxxxxxxx>
(...)

>  .../gpu/drm/panel/panel-novatek-nt36672a.c    | 251 ++++++++++++++++--

So are you sure the nt35596s panel driver is so similar to nt36672a that
they should share the same driver?

With all the magic number sequences I'm not so sure, do they really share
any of the magic numbers?

If not, consider creating a new driver, and then base it on the
nt35510 instead.

> +static const struct nt36672a_panel_cmd jdi_nt35596s_video_on_cmds[] = {
> +       { .data = { 0xff, 0x24 } },
> +       { .data = { 0x9d, 0x34 } },
(...)

These are never nice. Do you have a datasheet so you can provide
defines for the magic hex values?

The construction with these .data seqence array
should be replaces with some open coded sequences
I feel, bit this pattern is already in the driver so I guess it's OK.

Yours,
Linus Walleij