Re: [PATCH] drm/panel: raydium-rm692e5: transition to mipi_dsi wrapped functions

[Tejas Vipin <tejasvipin76@gmail.com>]

On 6/18/24 1:36 AM, Doug Anderson wrote:
> Hi,
> 
> On Sat, Jun 15, 2024 at 2:40 AM Tejas Vipin <tejasvipin76@gmail.com> wrote:
>>
>> @@ -168,48 +147,38 @@ static int rm692e5_prepare(struct drm_panel *panel)
>>         struct rm692e5_panel *ctx = to_rm692e5_panel(panel);
>>         struct drm_dsc_picture_parameter_set pps;
>>         struct device *dev = &ctx->dsi->dev;
>> -       int ret;
>> +       struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
>>
>> -       ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
>> -       if (ret < 0) {
>> -               dev_err(dev, "Failed to enable regulators: %d\n", ret);
>> -               return ret;
>> +       dsi_ctx.accum_err = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
>> +       if (dsi_ctx.accum_err) {
>> +               dev_err(dev, "Failed to enable regulators: %d\n", dsi_ctx.accum_err);
>> +               return dsi_ctx.accum_err;
>>         }
> 
> It would be my preference to get rid of the error print here since
> regulator_bulk_enable() already prints an error message.
> 
> 
>>         rm692e5_reset(ctx);
>>
>> -       ret = rm692e5_on(ctx);
>> -       if (ret < 0) {
>> -               dev_err(dev, "Failed to initialize panel: %d\n", ret);
>> +       dsi_ctx.accum_err = rm692e5_on(ctx);
>> +       if (dsi_ctx.accum_err) {
>> +               dev_err(dev, "Failed to initialize panel: %d\n", dsi_ctx.accum_err);
> 
> I'd probably change rm692e5_on() to take the "dsi_ctx" as a parameter
> and then you don't need to declare a new one there.
> 
> ...also, you don't need to add an error message since rm692e5_on()
> will have already printed one (since the "multi" style functions
> always print error messages for you).

I'm guessing that the change about regulator_bulk_enable and 
rm692e5 should also be applied to all the other panels where
similar behavior occurs?

> 
> 
> 
>>                 gpiod_set_value_cansleep(ctx->reset_gpio, 1);
>>                 regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
>> -               return ret;
>> +               return dsi_ctx.accum_err;
> 
> Not new for your patch, but it seems odd that we don't do this error
> handling (re-assert reset and disable the regulator) for errors later
> in the function. Shouldn't it do that? It feels like the error
> handling should be in an "err" label and we should end up doing that
> any time we return an error code... What do you think?

Personally I don't think this is necessary because imo labels
only get useful when there's a couple of them and/or they're 
jumped to multiple times. I don't think either would happen in
this particular function. But I guess if you have some convention
in mind, then it could be done?

> 
> 
> -Doug