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

[Chintan Patel]

Hi Doug,

Thanks for the detailed review — this is very helpful.

Good catch on a few of these.

> On Mon, Mar 2, 2026 at 7:55 PM Chintan Patel <chintanlike@xxxxxxxxx> wrote:
> > -static int nt36672a_panel_power_off(struct drm_panel *panel)
> > +static void nt36672a_panel_power_off(struct drm_panel *panel)
> >   {
> >          struct nt36672a_panel *pinfo = to_nt36672a_panel(panel);
> > -       int ret = 0;
> >
> >          gpiod_set_value(pinfo->reset_gpio, 1);
> >
> > -       ret = regulator_bulk_disable(ARRAY_SIZE(pinfo->supplies), pinfo->supplies);
> > -       if (ret)
> > -               dev_err(panel->dev, "regulator_bulk_disable failed %d\n", ret);
> > -
> > -       return ret;
> > +       if (regulator_bulk_disable(ARRAY_SIZE(pinfo->supplies), pinfo->supplies) < 0)
> > +               dev_err(panel->dev, "regulator_bulk_disable failed\n");
>
> nit: IMO It would have been OK to keep the local "ret" variable here,
> but I won't insist. That would have allowed you to keep printing the
> error code, which is nice. It's OK to have "ret" as a local variable
> even if you aren't returning it...

I’ll restore the local ret variable in nt36672a_panel_power_off() so we 
continue printing the regulator error code even though the function now 
returns void.


> >   }
> >
> >   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);
>
> Probably also need a `dsi_ctx.accum_err = 0;` here? Old code still
> sent the "display off" command even if nt36672a_send_cmds() returned
> an error I think?

You’re right about needing to reset dsi_ctx.accum_err before calling 
mipi_dsi_dcs_set_display_off_multi(). The old code would still send the 
display-off command even if nt36672a_send_cmds() failed, so I’ll add the 
reset there to preserve the original behavior.

> > +       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;
> >
> >          /* 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);
> > +       mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
> >
> >          /* 0x3C = 60ms delay */
> > -       msleep(60);
> > +       mipi_dsi_msleep(&dsi_ctx, 60);
>
> I think this one should still be a regular msleep(60), right? Prior
> code still did msleep(60) even if mipi_dsi_dcs_enter_sleep_mode()
> returned an error, so you probably still should too.

Agreed on keeping the msleep(60) as a regular sleep. Since the old code 
always slept regardless of DSI errors, using mipi_dsi_msleep() there 
would subtly change the power-down timing semantics.
> > -       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);
> >
> > -       return ret;
> > +       return 0;
>
> I didn't notice before, but I guess this is a minor change. Previously
> nt36672a_panel_unprepare() would ignore all errors (other than
> printing) except it would return the final error return from the
> regulator_bulk_disable() call. Now it will also ignore the error from
> the regulator_bulk_disable().
>
> IMO this is fine, but since it's a slight change in functionality it
> could be noted in the commit message. Something like:
>
> This patch is intended to functionally be a no-op, though there is one
> slight change. Previously a failure in regulator_bulk_disable() would
> have caused nt36672a_panel_unprepare() to return an error. Now it
> won't. No other errors in nt36672a_panel_unprepare() were propagated,
> so this makes things consistent.


I’ll also note in the commit message that nt36672a_panel_unprepare() no 
longer propagates the regulator error, since previously that was the 
only error returned and all others were ignored.

I’ll send a v3 shortly incorporating these adjustments.

Thanks again for the careful review.

-Chintan