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

[Arnaud Ferraris <arnaud.ferraris@collabora.com>]

Hi Linus,

Le 23/07/2023 à 18:45, Linus Walleij a écrit :
> Hi Arnaud & Molly,
>
> overall the driver looks very good!
>
> On Wed, Jul 19, 2023 at 5:20 PM Arnaud Ferraris
> <arnaud.ferraris@collabora.com>  wrote:
>
>> From: Molly Sophia<mollysophia379@gmail.com>
>>
>> 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@gmail.com>
>> Signed-off-by: Arnaud Ferraris<arnaud.ferraris@collabora.com>
> (...)
>
>>   .../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?

Their magic number sequences are quite different, however all the other 
bits (power/reset sequences and timings) fit in, which isn't the case of 
the other novatek panel drivers. Moreover, they have similar (although 
not identical) resolutions.

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

I would gladly make this a new driver, but I'd rather base it on the 
nt36672a for the reasons mentioned above. I do fear, however, that this 
would lead to unnecessary code duplication, as 90% of the driver (magic 
number sequences excluded) would be identical to the nt36672a (and to be 
fully honest, I don't think I have the needed knowledge to make it 
"better").

>> +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?

I found an old (2012) datasheet for the NT35596 (without the final "S") 
which is marked "Draft", so I'm really unsure this will be enough to 
make sense of those numbers.

> 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

Cheers,
Arnaud