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

[Dmitry Baryshkov]

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