Re: [PATCH] drm/panel: novatek-nt36672a: Convert to mipi_dsi_*_multi() helpers

[Doug Anderson <dianders@chromium.org>]

Hi,

On Sun, Feb 22, 2026 at 8:35 PM Chintan Patel <chintanlike@gmail.com> wrote:
>
> @@ -79,23 +79,17 @@ static inline struct nt36672a_panel *to_nt36672a_panel(struct drm_panel *panel)
>         return container_of(panel, struct nt36672a_panel, base);
>  }
>
> -static int nt36672a_send_cmds(struct drm_panel *panel, const struct nt36672a_panel_cmd *cmds,
> -                             int num)
> +static void nt36672a_send_cmds(struct mipi_dsi_multi_context *dsi_ctx,
> +                               const struct nt36672a_panel_cmd *cmds, int num)

nit: checkpatch --strict yells:

CHECK: Alignment should match open parenthesis
#37: FILE: drivers/gpu/drm/panel/panel-novatek-nt36672a.c:83:
+static void nt36672a_send_cmds(struct mipi_dsi_multi_context *dsi_ctx,
+                               const struct nt36672a_panel_cmd *cmds, int num)


Also: FWIW having a function like nt36672a_send_cmds() is discouraged
these days. One of the reasons for creating the "_multi" functions and
making them efficient was that init functions were preferred. For
instance, see the commit d6ddb6624a7f ("drm/panel: boe-tv101wum-nl6:
Don't use a table for initting panels") or commit 6f6fd690de1a
("drm/panel: innolux-p079zca: Don't use a table for initting panels").

I won't insist that you make the conversion here since I think you're
still providing a valuable cleanup with the patch you've already
written, but in case you want to go above-and-beyond you could try it.
You could even try running bloat-o-meter to see how the size changes.


> @@ -115,34 +109,26 @@ static int nt36672a_panel_power_off(struct drm_panel *panel)
>  static int nt36672a_panel_unprepare(struct drm_panel *panel)
>  {
>         struct nt36672a_panel *pinfo = to_nt36672a_panel(panel);
> -       int ret;
> +       struct mipi_dsi_multi_context dsi_ctx = { .dsi = pinfo->link };
>
>         /* send off cmds */
> -       ret = nt36672a_send_cmds(panel, pinfo->desc->off_cmds,
> -                                pinfo->desc->num_off_cmds);
> +       nt36672a_send_cmds(&dsi_ctx, pinfo->desc->off_cmds,
> +                          pinfo->desc->num_off_cmds);
>
> -       if (ret < 0)
> -               dev_err(panel->dev, "failed to send DCS off cmds: %d\n", ret);
> -
> -       ret = mipi_dsi_dcs_set_display_off(pinfo->link);
> -       if (ret < 0)
> -               dev_err(panel->dev, "set_display_off cmd failed ret = %d\n", ret);
> +       mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
> +       /* Reset error to continue power-down even if display off failed */
> +       dsi_ctx.accum_err = 0;
> +       mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
>
>         /* 120ms delay required here as per DCS spec */
>         msleep(120);
>
> -       ret = mipi_dsi_dcs_enter_sleep_mode(pinfo->link);
> -       if (ret < 0)
> -               dev_err(panel->dev, "enter_sleep cmd failed ret = %d\n", ret);

You've changed the order. Enter sleep mode used to be after the 120ms
sleep. Now it's before.


> -
>         /* 0x3C = 60ms delay */
>         msleep(60);
>
> -       ret = nt36672a_panel_power_off(panel);
> -       if (ret < 0)
> -               dev_err(panel->dev, "power_off failed ret = %d\n", ret);
> +       nt36672a_panel_power_off(panel);

You got rid of the extra print here, which is fine/right from my
perspective (since nt36672a_panel_power_off() already printed). ...but
nt36672a_panel_power_off()'s return code is never checked now. Just
make nt36672a_panel_power_off() return "void".


> @@ -170,51 +156,34 @@ static int nt36672a_panel_power_on(struct nt36672a_panel *pinfo)
>  static int nt36672a_panel_prepare(struct drm_panel *panel)
>  {
>         struct nt36672a_panel *pinfo = to_nt36672a_panel(panel);
> +       struct mipi_dsi_multi_context dsi_ctx = { .dsi = pinfo->link };
>         int err;
>
>         err = nt36672a_panel_power_on(pinfo);
>         if (err < 0)
> -               goto poweroff;
> +               return err;

I won't insist since it's a matter of opinion, but IMO get rid of the
`err` local variable and consistently use `dsi_ctx.accum_err` to store
the error code in this function. Yeah, it's a bit awkward, but having
the separate `err` variable just makes it too easy to mess up and use
it. As one example of where that happened, see commit 61ce50fd8196
("drm/panel: jdi-lpm102a188a: Fix error code in jdi_panel_prepare()").

Also: instead of returning right away, I think you want to make sure
that the GPIO gets set to 0 like the old code did, don't you? I'd be a
little hesitant changing this unless we can truly prove that the GPIO
set is not needed or unless we're certain that the GPIO set was
incorrect (in which case we should fix it in a separate patch).

...nicely, if you use `dsi_ctx.accum_err` you don't even need a
"goto". You can just let the "_multi" functions all run and be
no-ops...

-Doug