Received-SPF: pass (google.com: domain of linux-kernel+bounces-13189-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20231229135154.675946-1-dario.binacchi@amarulasolutions.com> <20231229135154.675946-8-dario.binacchi@amarulasolutions.com>
In-Reply-To: <20231229135154.675946-8-dario.binacchi@amarulasolutions.com>
From: Linus Walleij <linus.walleij@linaro.org>
Date: Fri, 29 Dec 2023 18:43:11 +0100
Message-ID: <CACRpkdbHodbAwoaTyxTX4LxYm6ZrBV6m6ht31Y2OaUPxS0Zhrw@mail.gmail.com>
Subject: Re: [PATCH 7/8] drm/panel: nt35510: refactor panel initialization
To: Dario Binacchi <dario.binacchi@amarulasolutions.com>
Cc: linux-kernel@vger.kernel.org, linux-amarula@amarulasolutions.com, 
	Alexandre Torgue <alexandre.torgue@foss.st.com>, Daniel Vetter <daniel@ffwll.ch>, 
	David Airlie <airlied@gmail.com>, Jessica Zhang <quic_jesszhan@quicinc.com>, 
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Maxime Ripard <mripard@kernel.org>, 
	Neil Armstrong <neil.armstrong@linaro.org>, Sam Ravnborg <sam@ravnborg.org>, 
	Thomas Zimmermann <tzimmermann@suse.de>, dri-devel@lists.freedesktop.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Dec 29, 2023 at 2:52=E2=80=AFPM Dario Binacchi
<dario.binacchi@amarulasolutions.com> wrote:

> The previous implementation did not make it easy to support new
> NT35510-based panels with different initialization sequences.
> This patch, preparatory for future developmentes, simplifies the
> addition of new NT35510-based displays and also avoids the risk of
> creating regressions on already managed panels.
>
> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>

The idea is to have the driver adapt to different panels, and encode a deep
understanding just like we do with all hardware drivers.

NAK.

This patch:

- Deletes a lot of useful documentation on how the panel works.

- Deletes defines and replaces them with magic numbers

All it achieves is a bit of "magic sequences because we are used to
magic sequences" and that doesn't look like an improvement at all,
instead it creates a dumber driver which has no explanations at all
to what is going on.

Please rewrite the patch in the same style as the original driver.
The fact that you (probably) are not used to writing display drivers
in this way is not an excuse to destroy this nice structure.

There are things that can be done, like create an abstraction for
sequence encoding with less open coded command issue
statements, by adding helpers to the DRM core, so if that is what
you want to do, then do that instead?

Yours,
Linus Walleij