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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 25 Jul 2023 16:15:58 +0200
From:   Arnaud Ferraris <arnaud.ferraris@...labora.com>
To:     Linus Walleij <linus.walleij@...aro.org>
Cc:     Neil Armstrong <neil.armstrong@...aro.org>,
        Sam Ravnborg <sam@...nborg.org>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        Molly Sophia <mollysophia379@...il.com>
Subject: Re: [PATCH v5 2/2 RESEND] drm: panel: Add novatek nt35596s panel
 driver

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@...labora.com>  wrote:
>
>> From: Molly Sophia<mollysophia379@...il.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@...il.com>
>> Signed-off-by: Arnaud Ferraris<arnaud.ferraris@...labora.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