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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

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



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux