linux-kernel - Re: [PATCH] drm/panel: himax-hx8394: 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] [day] [month] [year] [list]

Message-ID: <CAD=FV=UATxXTwbXo5Dd-dn89hos2qF7agP-v4aHTc8w0J+yyPw@mail.gmail.com>
Date: Mon, 24 Mar 2025 10:03:12 -0700
From: Doug Anderson <dianders@...omium.org>
To: Tejas Vipin <tejasvipin76@...il.com>
Cc: neil.armstrong@...aro.org, maarten.lankhorst@...ux.intel.com, 
	mripard@...nel.org, tzimmermann@...e.de, airlied@...il.com, simona@...ll.ch, 
	megi@....cz, javierm@...hat.com, quic_jesszhan@...cinc.com, 
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org, 
	asrivats@...hat.com
Subject: Re: [PATCH] drm/panel: himax-hx8394: transition to mipi_dsi wrapped functions

Hi,

On Sat, Mar 22, 2025 at 10:30 PM Tejas Vipin <tejasvipin76@...il.com> wrote:
>
> @@ -493,35 +481,28 @@ static int hx8394_enable(struct drm_panel *panel)
>  {
>         struct hx8394 *ctx = panel_to_hx8394(panel);
>         struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> -       int ret;
> -
> -       ret = ctx->desc->init_sequence(ctx);
> -       if (ret) {
> -               dev_err(ctx->dev, "Panel init sequence failed: %d\n", ret);
> -               return ret;
> -       }
> +       struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
>
> -       ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> -       if (ret) {
> -               dev_err(ctx->dev, "Failed to exit sleep mode: %d\n", ret);
> -               return ret;
> -       }
> +       ctx->desc->init_sequence(&dsi_ctx);
>
> +       mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
>         /* Panel is operational 120 msec after reset */
> -       msleep(120);
> +       mipi_dsi_msleep(&dsi_ctx, 120);
>
> -       ret = mipi_dsi_dcs_set_display_on(dsi);
> -       if (ret) {
> -               dev_err(ctx->dev, "Failed to turn on the display: %d\n", ret);
> +       mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
> +       if (dsi_ctx.accum_err)
>                 goto sleep_in;
> -       }
>
>         return 0;
>
>  sleep_in:
> +       int ret = dsi_ctx.accum_err;

I can't quite remember if kernel conventions allow local variables
defined in the middle of code like you're doing. Maybe safer to define
"int ret" at the top of the function?


> +
> +       dsi_ctx.accum_err = 0;
> +
>         /* This will probably fail, but let's try orderly power off anyway. */
> -       if (!mipi_dsi_dcs_enter_sleep_mode(dsi))
> -               msleep(50);
> +       mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
> +       mipi_dsi_msleep(&dsi_ctx, 50);
>
>         return ret;
>  }

If I'm reading it right, the above is a slight change in behavior.
Previously if the init_sequence() or mipi_dsi_dcs_exit_sleep_mode()
returned an error then you wouldn't run the
mipi_dsi_dcs_enter_sleep_mode(). Now you will.

If we're certain that this change is correct it should be mentioned in
the commit message. If not then we should make it function the way it
used to.

If we do want to keep it the way you have it, my own personal
preference would also be to rearrange the code and get rid of the
"goto", though I can see some argument for keeping the "goto" to make
it obvious where error handling is... Up to you.