Message-ID: <CAD=FV=Xzno3ReSyp9w+DC+nLoy1AXmcwd+j1=_XRxFi_k+bmng@mail.gmail.com>
Date: Mon, 21 Jul 2025 09:30:31 -0700
From: Doug Anderson <dianders@...omium.org>
To: Brigham Campbell <me@...ghamcampbell.com>
Cc: tejasvipin76@...il.com, diogo.ivo@...nico.ulisboa.pt, 
	skhan@...uxfoundation.org, linux-kernel-mentees@...ts.linux.dev, 
	dri-devel@...ts.freedesktop.org, linux-doc@...r.kernel.org, 
	linux-kernel@...r.kernel.org, 
	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>
Subject: Re: [PATCH v5 1/4] drm: Create mipi_dsi_dual* macros

Hi,

On Sat, Jul 19, 2025 at 1:27 AM Brigham Campbell <me@...ghamcampbell.com> wrote:
>
> @@ -827,6 +827,30 @@ void mipi_dsi_generic_write_multi(struct mipi_dsi_multi_context *ctx,
>  }
>  EXPORT_SYMBOL(mipi_dsi_generic_write_multi);
>
> +/**
> + * mipi_dsi_dual_generic_write_multi() - mipi_dsi_generic_write_multi() for
> + * two dsi channels, one after the other
> + * @dsi1: First dsi channel to write buffer to
> + * @dsi2: Second dsi channel to write buffer to
> + * @ctx: Context for multiple DSI transactions
> + * @payload: Buffer containing the payload
> + * @size: Size of payload buffer
> + *
> + * A wrapper around mipi_dsi_generic_write_multi() that allows the user to
> + * conveniently write to two dsi channels, one after the other.
> + */
> +void mpi_dsi_dual_generic_write_multi(struct mipi_dsi_device *dsi1,

BUG: above should be "mipi", not "mpi"


> +                                     struct mipi_dsi_device *dsi2,
> +                                     struct mipi_dsi_multi_context *ctx,
> +                                     const void *payload, size_t size)
> +{
> +       ctx->dsi = dsi1;
> +       mipi_dsi_generic_write_multi(ctx, data, len);

BUG: "data" and "len" are not valid local variables...


> @@ -431,6 +439,87 @@ void mipi_dsi_dcs_set_tear_off_multi(struct mipi_dsi_multi_context *ctx);
>                 mipi_dsi_dcs_write_buffer_multi(ctx, d, ARRAY_SIZE(d)); \
>         } while (0)
>
> +/**
> + * mipi_dsi_dual - send the same MIPI DSI command to two interfaces
> + *
> + * This macro will send the specified MIPI DSI command twice, once per each of
> + * the two interfaces supplied. This is useful for reducing duplication of code
> + * in panel drivers which use two parallel serial interfaces.
> + *
> + * WARNING: This macro reuses the _func argument and the optional trailing
> + * arguments twice each, which may cause unintended side effects. For example,
> + * adding the postfix increment ++ operator to one of the arguments to be
> + * passed to _func will cause the variable to be incremented twice instead of
> + * once and the variable will be its original value + 1 when sent to _dsi2.

It could be worth also pointing people to
mipi_dsi_dual_generic_write_seq_multi() and
mipi_dsi_dual_dcs_write_seq_multi() below?


> + *
> + * @_func: MIPI DSI function or macro to pass context and arguments into

nit: remove "or macro".


> + * @_dsi1: First DSI interface to act as recipient of the MIPI DSI command
> + * @_dsi2: Second DSI interface to act as recipient of the MIPI DSI command
> + * @_ctx: Context for multiple DSI transactions
> + * @...: Arguments to pass to MIPI DSI function or macro
> + */
> +#define mipi_dsi_dual(_func, _dsi1, _dsi2, _ctx, ...)           \
> +       do {                                                     \
> +               struct mipi_dsi_multi_context *_ctxcpy = (_ctx); \
> +               (_ctxcpy)->dsi = (_dsi1);                        \

nit: now that "_ctxcpy" is a local variable you no longer need the
extra parenthesis around it.


> +               (_func)((_ctxcpy), ##__VA_ARGS__);               \
> +               (_ctxcpy)->dsi = (_dsi2);                        \
> +               (_func)((_ctxcpy), ##__VA_ARGS__);               \
> +       } while (0)
> +
> +/**
> + * mipi_dsi_dual_generic_write_seq_multi - transmit data using a generic write
> + * packet to two dsi interfaces, one after the other
> + *
> + * This macro will send the specified generic packet twice, once per each of
> + * the two interfaces supplied. This is useful for reducing duplication of code
> + * in panel drivers which use two parallel serial interfaces.
> + *
> + * Note that if an error occurs while transmitting the packet to the first DSI
> + * interface, the packet will not be sent to the second DSI interface.
> + *
> + * This macro will print errors for you and error handling is optimized for
> + * callers that call this multiple times in a row.
> + *
> + * @_dsi1: First DSI interface to act as recipient of packet
> + * @_dsi2: Second DSI interface to act as recipient of packet
> + * @_ctx: Context for multiple DSI transactions
> + * @_seq: buffer containing the payload
> + */
> +#define mipi_dsi_dual_generic_write_seq_multi(_dsi1, _dsi2, _ctx, _seq...)     \
> +       do {                                                                    \
> +               static const u8 d[] = { _seq };                                 \
> +               mipi_dsi_dual_generic_write_multi(_dsi1, _dsi2, _ctx, d,        \
> +                                                        ARRAY_SIZE(d));        \

nit: the indentation of ARRAY_SIZE() is slightly off.


> +       } while (0)
> +
> +/**
> + * mipi_dsi_dual_dcs_write_seq_multi - transmit a DCS command with payload to
> + * two dsi interfaces, one after the other
> + *
> + * This macro will send the specified DCS command with payload twice, once per
> + * each of the two interfaces supplied. This is useful for reducing duplication
> + * of code in panel drivers which use two parallel serial interfaces.
> + *
> + * Note that if an error occurs while transmitting the payload to the first DSI
> + * interface, the payload will not be sent to the second DSI interface.
> + *
> + * This macro will print errors for you and error handling is optimized for
> + * callers that call this multiple times in a row.
> + *
> + * @_dsi1: First DSI interface to act as recipient of packet
> + * @_dsi2: Second DSI interface to act as recipient of packet
> + * @_ctx: Context for multiple DSI transactions
> + * @_cmd: Command
> + * @_seq: buffer containing the payload
> + */
> +#define mipi_dsi_dual_dcs_write_seq_multi(_dsi1, _dsi2, _ctx, _cmd, _seq)   \

BUG: doesn't "_seq" need to be "_seq..." ?

BUG: You need to remove the definition of this macro from
`panel-novatek-nt36523.c` or else it won't compile anymore since the
name of your macro is the exact same as theirs and they include this
header file. It would be OK w/ me if you squashed that into the same
patch since otherwise rejiggering things would just be churn...

I guess we also chose different argument orders than they did (that's
probably my fault, sorry!). They had the "ctx" still first and this
patch consistently has "dsi1" and "dsi2" first. I don't think it
really matters, but we should be consistent which means either
adjusting your patch or theirs. It's probably worth confirming that
the novatek driver at least compiles before you submit v6.


-Doug

Powered by blists - more mailing lists