Re: [PATCH v2 2/2] drm/panel: Add driver for Novatek NT37802

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [Medium] drm/panel: nt37802: state desync on DSI errors in disable and
  unprepare
- [Medium] drm/panel: nt37802: asymmetric display-on/off across prepare
  and disable
- [Medium] drm/panel: bogus voltage ranges in
  novatek_nt37802_set_supply_voltages()
- [Low] drm/panel: nt37802: use mipi_dsi_dcs_write_seq_multi() for init
  commands
--

commit a18941fccd5aa85d21c8730be5323c1f810aa7e5
Author: Jayant Shekhar <jayant.shekhar@oss.qualcomm.com>

drm/panel: Add driver for Novatek NT37802

This commit adds a DRM panel driver for the Novatek NT37802 1440x3200@120Hz
AMOLED DSI command-mode panel with DSC 1.2 compression and DCS brightness
control.

> diff --git a/drivers/gpu/drm/panel/panel-novatek-nt37802.c b/drivers/gpu/drm/panel/panel-novatek-nt37802.c
> new file mode 100644
> index 0000000000000..632520f04ffad
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-novatek-nt37802.c
> @@ -0,0 +1,544 @@
[ ... ]
> +struct nt37802_cmd {
> +	u8 len;
> +	u8 data[32];
> +};
> +
> +#define NT37802_CMD(...) \
> +	{ .len = sizeof((u8[]){ __VA_ARGS__ }), .data = { __VA_ARGS__ } }

[Severity: Low]
This isn't a bug, but could novatek_nt37802_write_init() use
mipi_dsi_dcs_write_seq_multi() and the standard DCS helpers (such as
mipi_dsi_dcs_set_column_address_multi(),
mipi_dsi_dcs_set_page_address_multi(), and mipi_dsi_dcs_set_tear_on_multi())
instead of a custom struct nt37802_cmd table?  Because data[32] is fixed-size,
even 2-byte commands in nt37802_init_cmds[] take 33 bytes each in .rodata.

[ ... ]

> +static int novatek_nt37802_set_supply_voltages(struct novatek_nt37802 *ctx)
> +{
> +	int ret;
> +
> +	ret = regulator_set_voltage(ctx->supplies[NT37802_SUPPLY_VDDIO].consumer,
> +				    1800000, 2000000);
> +	if (ret)
> +		return ret;
> +
> +	ret = regulator_set_voltage(ctx->supplies[NT37802_SUPPLY_VDD].consumer,
> +				    1100000, 1950000);
> +	if (ret)
> +		return ret;
> +
> +	return regulator_set_voltage(ctx->supplies[NT37802_SUPPLY_VCI].consumer,
> +				     3000000, 3544000);
> +}

[Severity: Medium]
Should the supply voltages be left to the board Device Tree constraints
instead of calling regulator_set_voltage() with wide ranges here?

regulator_set_voltage() picks the lowest voltage >= min_uV allowed by the
regulator's constraints, so requesting (1100000, 1950000) on vdd will drive
the rail down to 1.1V on any board whose PMIC LDO constraints permit 1.1V even
if the panel expects 1.2V.  In addition, calling regulator_set_voltage() on a
fixed or dummy regulator without voltage readback returns -EINVAL and aborts
novatek_nt37802_probe().

[ ... ]

> +	mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
> +	mipi_dsi_msleep(&dsi_ctx, 120);
> +	mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
> +	mipi_dsi_msleep(&dsi_ctx, 120);
> +
> +	drm_dsc_pps_payload_pack(&pps, &ctx->dsc);
> +	mipi_dsi_picture_parameter_set_multi(&dsi_ctx, &pps);
> +	mipi_dsi_compression_mode_multi(&dsi_ctx, true);
> +	if (dsi_ctx.accum_err) {
> +		novatek_nt37802_set_reset(ctx, 1);
> +		novatek_nt37802_disable_supplies(ctx);
> +		return dsi_ctx.accum_err;
> +	}
> +
> +	ctx->prepared = true;
> +	return 0;
> +}
> +
> +static int novatek_nt37802_enable(struct drm_panel *panel)
> +{
> +	struct novatek_nt37802 *ctx = to_novatek_nt37802(panel);
> +
> +	if (!ctx->prepared)
> +		return -EPERM;
> +
> +	if (ctx->enabled)
> +		return 0;
> +
> +	ctx->enabled = true;
> +	return 0;
> +}
> +
> +static int novatek_nt37802_disable(struct drm_panel *panel)
> +{
> +	struct novatek_nt37802 *ctx = to_novatek_nt37802(panel);
> +	struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
> +
> +	if (!ctx->enabled)
> +		return 0;
> +
> +	/* The off sequence sends display-off in HS mode, then waits 10 ms. */
> +	ctx->dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
> +	mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
> +	mipi_dsi_msleep(&dsi_ctx, 10);
> +
> +	ctx->enabled = false;
> +	return dsi_ctx.accum_err;
> +}

[Severity: Medium]
Does sending mipi_dsi_dcs_set_display_on_multi() in novatek_nt37802_prepare()
but mipi_dsi_dcs_set_display_off_multi() in novatek_nt37802_disable() leave
the display off across a disable/enable cycle?

novatek_nt37802_enable() sends no DSI commands, so if drm_panel_disable() is
followed by drm_panel_enable() without an intervening unprepare/prepare cycle,
nothing sends MIPI_DCS_SET_DISPLAY_ON and the panel stays blank while the
backlight turns on.  Conversely, if drm_panel_prepare() is followed directly
by drm_panel_unprepare() without enable/disable, the panel enters sleep mode
without sending MIPI_DCS_SET_DISPLAY_OFF.

> +
> +static int novatek_nt37802_unprepare(struct drm_panel *panel)
> +{
> +	struct novatek_nt37802 *ctx = to_novatek_nt37802(panel);
> +	struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
> +
> +	if (!ctx->prepared)
> +		return 0;
> +
> +	ctx->dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
> +	mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
> +	mipi_dsi_msleep(&dsi_ctx, 120);
> +
> +	/* Assert the active-low reset after the downstream sleep delay. */
> +	novatek_nt37802_set_reset(ctx, 1);
> +	ctx->prepared = false;
> +	novatek_nt37802_disable_supplies(ctx);
> +
> +	return dsi_ctx.accum_err;
> +}

[Severity: Medium]
Can a DSI error in novatek_nt37802_unprepare() or novatek_nt37802_disable()
desynchronize ctx->prepared and ctx->enabled from the drm_panel core?

If mipi_dsi_dcs_enter_sleep_mode_multi() fails, novatek_nt37802_unprepare()
still asserts reset, sets ctx->prepared = false, disables all four supplies,
and returns the negative error code.  In drm_panel_unprepare(), a negative
return skips clearing panel->prepared:

drivers/gpu/drm/drm_panel.c:drm_panel_unprepare() {
    ...
	if (panel->funcs && panel->funcs->unprepare) {
		ret = panel->funcs->unprepare(panel);
		if (ret < 0)
			goto exit;
	}
	panel->prepared = false;
    ...
}

On the next resume or modeset, drm_panel_prepare() sees panel->prepared ==
true and returns early without calling novatek_nt37802_prepare() to re-enable
the regulators, and novatek_nt37802_enable() then fails with -EPERM because
ctx->prepared is false.  Similarly, if mipi_dsi_dcs_set_display_off_multi()
fails in novatek_nt37802_disable() after drm_panel_disable() has already
disabled the backlight, returning dsi_ctx.accum_err leaves panel->enabled ==
true so subsequent drm_panel_enable() calls return early without re-enabling
the backlight.

Dropping the driver's duplicate ctx->prepared and ctx->enabled flags (since
drm_panel already guards transitions with panel->prepared and panel->enabled)
and returning 0 from novatek_nt37802_unprepare() and
novatek_nt37802_disable() once teardown has completed would avoid wedging the
panel state.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-jshekhar-nt37802-v2-0-a8e9efdcabd7@oss.qualcomm.com?part=2