Hi,
On Sat, Jun 15, 2024 at 2:40 AM Tejas Vipin <tejasvipin76@xxxxxxxxx> 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).
> 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?
-Doug
