linux-kernel - Re: [PATCH] drm/mipi-dsi: Introduce macros to create mipi_dsi_*

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <p7fahem6egnooi5org4lv3gtz2elafylvlnyily7buvzcpv2qh@vssvpfci3lwn>
Date: Tue, 16 Jul 2024 20:01:16 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Tejas Vipin <tejasvipin76@...il.com>
Cc: maarten.lankhorst@...ux.intel.com, mripard@...nel.org, 
	tzimmermann@...e.de, dianders@...omium.org, airlied@...il.com, daniel@...ll.ch, 
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/mipi-dsi: Introduce macros to create
 mipi_dsi_*_multi functions

On Tue, Jul 16, 2024 at 07:01:17PM GMT, Tejas Vipin wrote:
> Introduce 2 new macros, DSI_CTX_NO_OP and MIPI_DSI_ADD_MULTI_VARIANT.
> 
> DSI_CTX_NO_OP calls a function only if the context passed to it hasn't
> encountered any errors. It is a generic form of what mipi_dsi_msleep
> does.
> 
> MIPI_DSI_ADD_MULTI_VARIANT defines a multi style function of any
> mipi_dsi function that follows a certain style. This allows us to
> greatly reduce the amount of redundant code written for each multi
> function. It reduces the overhead for a developer introducing new
> mipi_dsi_*_multi functions.
> 
> Signed-off-by: Tejas Vipin <tejasvipin76@...il.com>
> ---
>  drivers/gpu/drm/drm_mipi_dsi.c | 286 ++++++++++-----------------------
>  1 file changed, 83 insertions(+), 203 deletions(-)
> 

[...]

> -void mipi_dsi_dcs_set_tear_on_multi(struct mipi_dsi_multi_context *ctx,
> -				    enum mipi_dsi_dcs_tear_mode mode)
> -{
> -	struct mipi_dsi_device *dsi = ctx->dsi;
> -	struct device *dev = &dsi->dev;
> -	ssize_t ret;
> -
> -	if (ctx->accum_err)
> -		return;
> -
> -	ret = mipi_dsi_dcs_set_tear_on(dsi, mode);
> -	if (ret < 0) {
> -		ctx->accum_err = ret;
> -		dev_err(dev, "sending DCS SET_TEAR_ON failed: %d\n",
> -			ctx->accum_err);
> -	}
> -}
> -EXPORT_SYMBOL(mipi_dsi_dcs_set_tear_on_multi);
> +#define MIPI_DSI_ADD_MULTI_VARIANT(proto, err, inner_func, ...)	\
> +proto {								\
> +	struct mipi_dsi_device *dsi = ctx->dsi;			\
> +	struct device *dev = &dsi->dev;				\
> +	int ret;						\
> +	\
> +	if (ctx->accum_err)					\
> +		return;						\
> +	\
> +	ret = inner_func(dsi, ##__VA_ARGS__);			\
> +	if (ret < 0) {						\
> +		ctx->accum_err = ret;				\
> +		dev_err(dev, err, ctx->accum_err);		\
> +	}							\
> +}								\
> +EXPORT_SYMBOL(inner_func##_multi);
> +
> +MIPI_DSI_ADD_MULTI_VARIANT(
> +	void mipi_dsi_picture_parameter_set_multi(
> +	struct mipi_dsi_multi_context *ctx,
> +	const struct drm_dsc_picture_parameter_set *pps),
> +	"sending PPS failed: %d\n",
> +	mipi_dsi_picture_parameter_set, pps);

I'd say that having everything wrapped in the macro looks completely
unreadable.

If you really insist, it can become something like:

MIPI_DSI_ADD_MULTI_VARIANT(mipi_dsi_picture_parameter_set
	MULTI_PROTO(const struct drm_dsc_picture_parameter_set *pps),
	MULTI_ARGS(pps),
	"sending PPS failed");

(note, I dropped the obvious parts: that the funciton is foo_multi, its
return type is void, first parameter is context, etc).

However it might be better to go other way arround.
Do we want for all the drivers to migrate to _multi()-kind of API? If
so, what about renaming the multi and non-multi functions accordingly
and making the old API a wrapper around the multi() function?

-- 
With best wishes
Dmitry