On Wed, 17 Jul 2024 at 12:58, Tejas Vipin <tejasvipin76@xxxxxxxxx> wrote:
>
>
>
> 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@xxxxxxxxx>
> >> ---
> >> 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.
Sounds good to me. I'd suggest to wait for a day or two for further
feedback / comments from other developers.
>
> I also think there is merit in keeping DSI_CTX_NO_OP.
>
> What do you think?
>
> --
> Tejas Vipin
--
With best wishes
Dmitry
