Re: [PATCH RFC 04/10] drm/panel: novatek-nt36672a: add Tianma FHD+ video mode panel

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Hardware initialization and teardown sequence reversal
- [Low] Unconditional delay on error path due to bypassing `mipi_dsi_msleep()`
--

commit 3b4bbeb999e6951e81e05057ded69945b990ecb4
Author: YİĞİTCAN KAVAKLI <kavakliyigitcan@gmail.com>

drm/panel: novatek-nt36672a: add Tianma FHD+ video mode panel

This commit adds support for the 1080x2340 Tianma video mode panel with
Novatek NT36672A driver IC. It adds display mode timings, DCS initialization
sequences, and standard DRM panel enable and disable callbacks to coordinate
display commands with the active DSI video stream.

> diff --git a/drivers/gpu/drm/panel/panel-novatek-nt36672a.c b/drivers/gpu/drm/panel/panel-novatek-nt36672a.c
> index 464d9ce47d87d..b69213790441f 100644
> --- a/drivers/gpu/drm/panel/panel-novatek-nt36672a.c
> +++ b/drivers/gpu/drm/panel/panel-novatek-nt36672a.c
> @@ -92,18 +92,12 @@ static int nt36672a_panel_unprepare(struct drm_panel *panel)
>  	struct mipi_dsi_multi_context dsi_ctx = { .dsi = pinfo->link };
>  
>  	/* send off cmds */
>  	if (pinfo->desc->send_deinit_cmds)
>  		pinfo->desc->send_deinit_cmds(&dsi_ctx);

[Severity: High]
Does this sequence execute deinitialization commands after the panel is already
turned off? The new disable callback will turn off the display before the
unprepare callback runs, meaning the panel might not be able to process these
deinitialization commands correctly.

[ ... ]
> @@ -146,19 +139,41 @@ static int nt36672a_panel_prepare(struct drm_panel *panel)
>  
>  	mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
>  
> -	/* 0x46 = 70 ms delay */
> -	mipi_dsi_msleep(&dsi_ctx, 70);
> -
> -	mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
> +	/* 80 ms delay as per DCS spec / downstream timing */
> +	msleep(80);

[Severity: Low]
Does this unconditional msleep() cause unnecessary delays on error paths?
Because this replaces mipi_dsi_msleep(), if the preceding DCS commands
fail, dsi_ctx.accum_err is set, but this code will still unconditionally
sleep for 80ms.

>  
>  	/* Send rest of the init cmds */
>  	if (pinfo->desc->send_init_cmds_2)
>  		pinfo->desc->send_init_cmds_2(&dsi_ctx);

[Severity: High]
Will the panel properly handle these secondary initialization commands while 
the display is still off? Since this patch moves the set display on command
into the enable callback, these post-on brightness settings are being sent
before the display is turned on, which might cause initialization failures or
visual artifacts.

[ ... ]
> +static int nt36672a_panel_enable(struct drm_panel *panel)
> +{
> +	struct nt36672a_panel *pinfo = to_nt36672a_panel(panel);
> +	struct mipi_dsi_multi_context dsi_ctx = { .dsi = pinfo->link };
> +
> +	mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
> +
> +	/* Frame settling delay */
> +	msleep(20);

[Severity: Low]
Does this msleep() also need to check for preceding errors? This creates an
unconditional sleep that ignores potential errors from the preceding 
mipi_dsi_dcs_set_display_on_multi() call.

> +
> +	return dsi_ctx.accum_err;
> +}
> +
> +static int nt36672a_panel_disable(struct drm_panel *panel)
> +{
> +	struct nt36672a_panel *pinfo = to_nt36672a_panel(panel);
> +	struct mipi_dsi_multi_context dsi_ctx = { .dsi = pinfo->link };
> +
> +	mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
> +
> +	/* 120ms delay as per DCS spec */
> +	msleep(120);

[Severity: Low]
Similar to the enable path, is this unconditional sleep intended? It ignores
potential errors from the preceding mipi_dsi_dcs_set_display_off_multi() call,
causing unnecessary delays during teardown failures.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911-ginkgo-submission-final-v1-0-0e68e63a18d4@gmail.com?part=4