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

[Tejas Vipin <tejasvipin76@gmail.com>]

On 7/16/24 10:31 PM, Dmitry Baryshkov wrote:
> 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@gmail.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?
> 

I think this would be good. For the wrapper to make a multi() function
non-multi, what do you think about a macro that would just pass a 
default dsi_ctx to the multi() func and return on error? In this case
it would also be good to let the code fill inline instead of generating
a whole new function imo. 

So in this scenario all the mipi dsi functions would be multi functions,
and a function could be called non-multi like MACRO_NAME(func) at the
call site.

I also think there is merit in keeping DSI_CTX_NO_OP.

What do you think?

-- 
Tejas Vipin