Message-ID: <n5m7ubrimzctfh6uumh5anyt5cc7jnuph5opbi4wup3du7nhqe@dljwxfws6vla>
Date: Fri, 14 Nov 2025 15:27:14 +0100
From: Thierry Reding <thierry.reding@...il.com>
To: Svyatoslav Ryhel <clamor95@...il.com>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, 
	Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, 
	David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Jonathan Hunter <jonathanh@...dia.com>, Sowjanya Komatineni <skomatineni@...dia.com>, 
	Luca Ceresoli <luca.ceresoli@...tlin.com>, Prashant Gaikwad <pgaikwad@...dia.com>, 
	Michael Turquette <mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>, 
	Mikko Perttunen <mperttunen@...dia.com>, Mauro Carvalho Chehab <mchehab@...nel.org>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Jonas Schwöbel <jonasschwoebel@...oo.de>, 
	Dmitry Osipenko <digetx@...il.com>, Charan Pedumuru <charan.pedumuru@...il.com>, 
	Diogo Ivo <diogo.ivo@...nico.ulisboa.pt>, Aaron Kling <webgeek1234@...il.com>, 
	Arnd Bergmann <arnd@...db.de>, dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org, 
	linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org, linux-media@...r.kernel.org, 
	linux-clk@...r.kernel.org, linux-staging@...ts.linux.dev
Subject: Re: [PATCH v5 09/23] gpu: host1x: convert MIPI to use operation
 function pointers

On Wed, Oct 22, 2025 at 05:20:37PM +0300, Svyatoslav Ryhel wrote:
> Convert existing MIPI code to use operation function pointers, a necessary
> step for supporting Tegra20/Tegra30 SoCs. All common MIPI configuration
> that is SoC-independent remains in mipi.c, while all SoC-specific code is
> moved to tegra114-mipi.c (The naming matches the first SoC generation with
> a dedicated calibration block). Shared structures and function calls are
> placed into tegra-mipi-cal.h.
> 
> Signed-off-by: Svyatoslav Ryhel <clamor95@...il.com>
> Acked-by: Mikko Perttunen <mperttunen@...dia.com>
> ---
>  drivers/gpu/drm/tegra/dsi.c             |   1 +
>  drivers/gpu/host1x/Makefile             |   1 +
>  drivers/gpu/host1x/mipi.c               | 525 +++---------------------
>  drivers/gpu/host1x/tegra114-mipi.c      | 483 ++++++++++++++++++++++
>  drivers/staging/media/tegra-video/csi.c |   1 +
>  include/linux/host1x.h                  |  10 -
>  include/linux/tegra-mipi-cal.h          |  57 +++
>  7 files changed, 599 insertions(+), 479 deletions(-)
>  create mode 100644 drivers/gpu/host1x/tegra114-mipi.c
>  create mode 100644 include/linux/tegra-mipi-cal.h

Not sure if I missed this earlier, but I don't understand why the code
was moved around like this. tegra114-mipi.c now contains the code for
all of Tegra114, Tegra124, Tegra132 and Tegra210, so the name doesn't
make any more sense than the old name.

Furthermore, moving the header file contents now also means that we have
a cross-dependency within the series that makes this more difficult to
merge. Obviously that's something we can make work, /if/ there's a need,
but from what I can tell there's really no benefit to this extra churn.

I also don't fully understand the benefit of converting the code into
operation function pointers if we always use the same function pointers
for all generations. Effectively this adds boilerplate and an extra
indirection for no benefit at all.

Splitting the "SoC specific" parts from the generic parts also now
needlessly exports a symbol for absolutely no reason. Both files are
linked into the same driver/module, there's no need for an exported
symbol.

The only slight bit of information that might justify this is the hint
in this commit message that Tegra20/Tegra30 requires this. But I don't
see patches for this anywhere, making it impossible to review this
change.

Looking at other parts of the series, and given it's spread across a
large number of trees with compile-time dependencies between them, I
think it would be better to split this up differently. I think it could
be three series in total: one for the clock changes needed for this,
another with all of the MIPI changes (in which case it would make sense
to include the Tegra20/Tegra30 bits as well to actually show why the
rework in this patch is needed) and a final one with the staging/media
bits that make use if this all. Well, maybe also a fourth series that
adds the DT changes needed to make it all work.

I think if you avoid splitting the MIPI prototypes into tegra-mipi-cal.h
you should be able to get rid of the cross-dependency. Clock changes
should already be independent. Also, I think it's probably fine if we
keep all of the MIPI driver code in the one file that it's currently in.
This removes the need for the odd exported symbol. It's not a great deal
of code, and I doubt that Tegra20/Tegra30 support would add
significantly to it.

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists