Hi,
On Mon, Mar 2, 2026 at 7:55 PM Chintan Patel <[email protected]> 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...
> }
>
> 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?
> + 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.
> - 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.
