linux-kernel - Re: [PATCH] drm/panel: raydium-rm692e5: transition to mipi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 17 Jun 2024 13:06:48 -0700
From: Doug Anderson <dianders@...omium.org>
To: Tejas Vipin <tejasvipin76@...il.com>
Cc: neil.armstrong@...aro.org, quic_jesszhan@...cinc.com, 
	maarten.lankhorst@...ux.intel.com, mripard@...nel.org, tzimmermann@...e.de, 
	airlied@...il.com, daniel@...ll.ch, dri-devel@...ts.freedesktop.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/panel: raydium-rm692e5: transition to mipi_dsi
 wrapped functions

Hi,

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



>                 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