Re: [PATCH v3 4/4] drm: panel: Add Jadard JD9365DA-H3 DSI panel

From: Jagan Teki <jagan@edgeble.ai>
To: Linus Walleij <linus.walleij@linaro.org>
Cc: devicetree@vger.kernel.org, dri-devel@lists.freedesktop.org,
	Rob Herring <robh+dt@kernel.org>,
	Thierry Reding <thierry.reding@gmail.com>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Sam Ravnborg <sam@ravnborg.org>
Subject: Re: [PATCH v3 4/4] drm: panel: Add Jadard JD9365DA-H3 DSI panel
Date: Tue, 8 Nov 2022 20:22:51 +0530	[thread overview]
Message-ID: <CA+VMnFz1h0MfwxiQeFCdvMJWQ9uKWvwstJvKnpDTKjaVHN3pYw@mail.gmail.com> (raw)
In-Reply-To: <CACRpkdaZnGgJ3egXEtoH0gTmR0m_-9Q+iGZr2eOx2JVHYgXCXA@mail.gmail.com>

On Tue, 8 Nov 2022 at 20:18, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Tue, Nov 8, 2022 at 3:12 PM Jagan Teki <jagan@edgeble.ai> wrote:
> > On Tue, 8 Nov 2022 at 19:31, Linus Walleij <linus.walleij@linaro.org> wrote:
> > >
> > > On Thu, Nov 3, 2022 at 3:12 PM Jagan Teki <jagan@edgeble.ai> wrote:
> > >
> > > > Jadard JD9365DA-H3 is WXGA MIPI DSI panel and it support TFT
> > > > dot matrix LCD with 800RGBx1280 dots at maximum.
> > > >
> > > > Add support for it.
> > > >
> > > > Cc: dri-devel@lists.freedesktop.org
> > > > Signed-off-by: Jagan Teki <jagan@edgeble.ai>
> > > > ---
> > > > Changes for v3:
> > > > - updatd to WXGA
> > > > - use JD9365DA_CMD_DCS and JD9365DA_CMD_DELAY
> > >
> > > My comments on v2 have not been addressed, for example I asked to
> > > remove the delay from sequences and just use an explicit delay and
> > > to then use the existing sequence sending macro.
> >
> > True, I responded on the same day [1], since I didn't get the reply I
> > have posted by assuming my comment is valid. Would you please check
> > and respond?
> >
> > [1] https://lore.kernel.org/all/CA+VMnFz0w-6O=wt3iuJo1BhQgPZ2XbpX6JdDz6vg_JW9nHTR2A@mail.gmail.com/
>
> OK I see, sorry for not reading close.
>
> The driver just supports one single variant.
>
> What you are doing is preparing the ground for more variants
> that may or may not exist. This creates the antipattern "big upfront design"
> i.e. abstractions added for things that do not yet exist.
>
> I think it is better to strip it down to just open coding the delay after
> the init sequence. When the next variant appears, if ever, they can
> add abstraction. Maybe they need the same delay in the same
> place? Who knows...

I understand your point, but delays are strictly mentioned by the
panel vendor init sequence, cz101b4001 do you think adding in the
generic or common code is still valid since we have code added for
cz101b4001 specifically via driver data?

Thanks,
Jagan.