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

[Tejas Vipin]

On 8/1/24 2:59 AM, Doug Anderson wrote:
> 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.
>

Only reason I converted the function at all was really for uniformity's
sake. But I don't think that's valid anymore seeing how there's already
other mipi_dsi funtions that I'm not converting and this function
probably wouldn't show up in the context of the other multi
functions.

> 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.
>

One thing that struck me as odd was that the callers of
mipi_dsi_dcs_get_display_brightness never bothered to print errors at
all? If we want to print errors for non-multi functions, then I think it
would be best to just modify the existing function. And in the case that
someone doesn't want those errors showing up, I agree with what Maxime
said [2] and let users handle it.

> 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

[2] https://lore.kernel.org/all/20240726-cerise-civet-of-reverence-ebeb9d@houat/
-- 
Tejas Vipin