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
