Re: [PATCH v2 1/2] drm/mipi-dsi: add more multi functions for better error handling

[Doug Anderson]

Hi,

On Mon, Jul 29, 2024 at 11:07 PM Tejas Vipin <tejasvipin76@xxxxxxxxx> wrote:
> +/**
> + * mipi_dsi_dcs_get_display_brightness_multi() - gets the current brightness value
> + *    of the display
> + * @ctx: Context for multiple DSI transactions
> + * @brightness: brightness value
> + *
> + * Like mipi_dsi_dcs_get_display_brightness() but deals with errors in a way that
> + * makes it convenient to make several calls in a row.
> + */
> +void mipi_dsi_dcs_get_display_brightness_multi(struct mipi_dsi_multi_context *ctx,
> +                                              u16 *brightness)
> +{
> +       struct mipi_dsi_device *dsi = ctx->dsi;
> +       struct device *dev = &dsi->dev;
> +       int ret;
> +
> +       if (ctx->accum_err)
> +               return;
> +
> +       ret = mipi_dsi_dcs_get_display_brightness(dsi, brightness);
> +       if (ret < 0) {
> +               ctx->accum_err = ret;
> +               dev_err(dev, "Failed to get display brightness: %d\n",
> +                       ctx->accum_err);
> +       }
> +}
> +EXPORT_SYMBOL(mipi_dsi_dcs_get_display_brightness_multi);

I'd be interested in others' opinions, but this function strikes me as
one that *shouldn't* be converted to _multi.

Specifically the whole point of the _multi abstraction is that you can
fire off a whole pile of initialization commands without needing to
check for errors constantly. You can check for errors once at the end
of a sequence of commands and you can be sure that an error message
was printed for the command that failed and that all of the future
commands didn't do anything.

I have a hard time believing that _get_ brightness would be part of
this pile of initialization commands. ...and looking at how you use it
in the next patch I can see that, indeed, it's a bit awkward using the
_multi variant in the case you're using it.

The one advantage of the _multi functions is that they are also
"chatty" and we don't need to print the error everywhere. However, it
seems like we could just make the existing function print an error
message but still return the error directly. If this automatic
printing an error message is a problem for someone then I guess maybe
we've already reached the "tomorrow" [1] and need to figure out if we
need to keep two variants of the function around instead of marking
one as deprecated.

NOTE: If we don't convert this then the "set" function will still be
_multi but the "get" one won't be. I think that's fine since the "set"
function could plausibly be in a big sequence of commands but the
"get" function not so much...

[1] https://lore.kernel.org/r/CAD=FV=WbXdnM4or3Ae+nYoQW1Sce0jP6FWtCHShsALuEFNhiww@xxxxxxxxxxxxxx