Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230719152007.197710-1-arnaud.ferraris@collabora.com> <20230719152007.197710-3-arnaud.ferraris@collabora.com>
In-Reply-To: <20230719152007.197710-3-arnaud.ferraris@collabora.com>
From:   Linus Walleij <linus.walleij@linaro.org>
Date:   Sun, 23 Jul 2023 18:45:21 +0200
Message-ID: <CACRpkdZ2Fu8jyyyDd4e1yq8g4bDoj2gCd4KnntST8uYvDNKUZw@mail.gmail.com>
Subject: Re: [PATCH v5 2/2 RESEND] drm: panel: Add novatek nt35596s panel driver
To:     Arnaud Ferraris <arnaud.ferraris@collabora.com>
Cc:     Neil Armstrong <neil.armstrong@linaro.org>,
        Sam Ravnborg <sam@ravnborg.org>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        Sumit Semwal <sumit.semwal@linaro.org>,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
        Molly Sophia <mollysophia379@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Arnaud & Molly,

overall the driver looks very good!

On Wed, Jul 19, 2023 at 5:20=E2=80=AFPM 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?

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[] =3D =
{
> +       { .data =3D { 0xff, 0x24 } },
> +       { .data =3D { 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