Date:   Wed, 8 Mar 2023 08:37:27 +0800
From:   Jianhua Lu <lujianhua000@gmail.com>
To:     Linus Walleij <linus.walleij@linaro.org>
Cc:     Konrad Dybcio <konrad.dybcio@linaro.org>,
        Thierry Reding <thierry.reding@gmail.com>,
        Sam Ravnborg <sam@ravnborg.org>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        dri-devel@lists.freedesktop.org,
        ~postmarketos/upstreaming@lists.sr.ht, phone-devel@vger.kernel.org
Subject: Re: [PATCH v2 2/2] drm/panel: Add driver for Novatek NT36523
Message-ID: <ZAfYx9Im0j5LMuMn@Gentoo>
References: <20230220121258.10727-1-lujianhua000@gmail.com>
 <20230220121258.10727-2-lujianhua000@gmail.com>
 <CACRpkdbZCZiMM_qeqMd9=txVvPVHEzM4szOnPR-gCYdiXW_9eA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CACRpkdbZCZiMM_qeqMd9=txVvPVHEzM4szOnPR-gCYdiXW_9eA@mail.gmail.com>
Precedence: bulk

On Tue, Mar 07, 2023 at 11:34:55PM +0100, Linus Walleij wrote:
> Hi Jianhua,
> 
> thanks for your patch!
> 
> It appears Konrad is working on a very similar driver, so I suggest merging
> them into one Novatek NT36523 driver.
> 
> Possibly we can fix this up first and then add Konrads Lenovo-panel with
> a patch on top.
> 
> On Mon, Feb 20, 2023 at 1:13 PM Jianhua Lu <lujianhua000@gmail.com> wrote:
> 
> > Add a driver for panels using the Novatek NT36523 display driver IC.
> >
> > Signed-off-by: Jianhua Lu <lujianhua000@gmail.com>
> 
> (...)
> 
> I like how you abstract the panel with init commands in the panel info.
> 
> > +enum dsi_cmd_type {
> > +       INIT_DCS_CMD,
> > +       DELAY_CMD,
> > +};
> > +
> > +struct panel_init_cmd {
> > +       enum dsi_cmd_type type;
> > +       size_t len;
> > +       const char *data;
> > +};
> > +
> > +#define _INIT_DCS_CMD(...) { \
> > +       .type = INIT_DCS_CMD, \
> > +       .len = sizeof((char[]){__VA_ARGS__}), \
> > +       .data = (char[]){__VA_ARGS__} }
> > +
> > +#define _INIT_DELAY_CMD(...) { \
> > +       .type = DELAY_CMD,\
> > +       .len = sizeof((char[]){__VA_ARGS__}), \
> > +       .data = (char[]){__VA_ARGS__} }
> 
> I have seen this type of reinvented wheels a few times now. Don't do this.
> 
> Look into other recently merged drivers and look how they do it, for example
> drivers/gpu/drm/panel/panel-himax-hx8394.c
> 
> For example:
> 
> - Use mipi_dsi_dcs_write_seq()
> 
> - If the delay is just used at one point in the sequence, do not invent
>   a command language like above for it, open code the delay instead
> 
> - Try to decode as much magic as possible, if you look in Konrads
>   driver you clearly see some standard MIPI commands, I bet you have
>   some too.
> 
> - Maybe use callbacks to send sequences instead of tables, like in
>   the himax driver?
I use init cmd tables in order to send init sequences to dsi0 and dsi1 at the
same time (sync dual dsi mode). I don't found a convenient way to use
mipi_dsi_dcs_write_seq() for sync dual dsi mode.
> 
> Other than that it seems like something that could also handle the Lenovo
> display, or the other way around, I don't know which driver is the best
> starting point, but this one has the right Novatek name at least.
> 
> Yours,
> Linus Walleij