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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] Missing 120ms delay after MIPI_DCS_ENTER_SLEEP_MODE before asserting reset and removing power.
- [Medium] The driver ignores transmission errors from DSC-related MIPI DSI multi-context commands in `novatek_prepare()`, masking failures and skipping proper hardware cleanup.
- [Medium] The backlight update_status callback races with the panel unprepare path, allowing DCS commands to be sent to an unpowered panel.
- [Low] Missing dependency for DRM DSC helpers in Kconfig, leading to undefined references at link time.
- [Low] Explicit invocation of `drm_panel_remove()` in the error path for a panel registered with `devm_drm_panel_add()`.
--

commit 51663d35dbaba4a4aee3bdfad1a94da90787d8f6
Author: Pengyu Luo <mitltlatltl@gmail.com>
Subject: drm/panel: Add Novatek NT36536 panel driver

This commit adds a new driver for Novatek NT36536 display panels.
It specifically implements support for the CSOT PP8807HB1-1 dual-link 10-bit
panel used in the LENOVO Legion Y700 Gen4.

> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -620,6 +620,17 @@ config DRM_PANEL_NOVATEK_NT36523
>  	  around the Novatek NT36523 display controller, such as some
>  	  Boe panels used in Xiaomi Mi Pad 5 and 5 Pro tablets.
>  
> +config DRM_PANEL_NOVATEK_NT36536
> +	tristate "Novatek NT36536 panel driver"
> +	depends on OF
> +	depends on DRM_MIPI_DSI
> +	depends on BACKLIGHT_CLASS_DEVICE
> +	select DRM_KMS_HELPER

[Severity: Low]
Does this Kconfig entry need to select DRM_DISPLAY_DSC_HELPER and 
DRM_DISPLAY_HELPER? Building this driver on a configuration that does not 
incidentally enable CONFIG_DRM_DISPLAY_DSC_HELPER could result in a build 
failure due to an undefined reference to drm_dsc_pps_payload_pack().

> +	help
> +	  Say Y here if you want to enable support for Novatek NT36536-based
> +	  display panels, such as the one found in the LENOVO Legion Y700
> +	  Gen4.
> +
[ ... ]
> diff --git a/drivers/gpu/drm/panel/panel-novatek-nt36536.c b/drivers/gpu/drm/panel/panel-novatek-nt36536.c
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-novatek-nt36536.c
[ ... ]
> +static int novatek_prepare(struct drm_panel *panel)
> +{
[ ... ]
> +	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);
> +	mipi_dsi_msleep(&dsi_ctx, 28);
> +
> +	return backlight_enable(ctx->backlight);

[Severity: Medium]
Should the accumulated error in dsi_ctx.accum_err be checked before returning?
If any of the DSC MIPI DSI commands fail, the error is recorded but never
evaluated by novatek_prepare(). Returning the result of backlight_enable() 
directly could mask failures, skip proper hardware cleanup, and leave the 
panel in a corrupted state.

> +}
> +
> +static int novatek_off(struct mipi_dsi_multi_context *dsi_ctx)
> +{
> +	mipi_dsi_dcs_set_display_off_multi(dsi_ctx);
> +	mipi_dsi_dcs_enter_sleep_mode_multi(dsi_ctx);
> +
> +	return dsi_ctx->accum_err;
> +}
> +
> +static int novatek_unprepare(struct drm_panel *panel)
> +{
[ ... ]
> +	ret = novatek_off(&dsi_ctx);
> +	if (ret < 0)
> +		dev_err(dev, "Failed to un-initialize panel: %d\n", ret);
> +
> +	gpiod_set_value_cansleep(ctx->reset_gpio, 1);
> +	regulator_bulk_disable(ARRAY_SIZE(novatek_supplies), ctx->supplies);

[Severity: Medium]
Should there be a delay before removing power? novatek_off() sends the
MIPI_DCS_ENTER_SLEEP_MODE command, but the regulators are disabled and the 
reset GPIO is asserted immediately without waiting. Removing power immediately 
after sending Sleep In can violate the MIPI DCS specification.

> +
> +	return 0;
> +}
[ ... ]
> +static int novatek_bl_update_status(struct backlight_device *bl)
> +{
> +	struct novatek *ctx = bl_get_data(bl);
> +	u16 brightness = backlight_get_brightness(bl);
> +
> +	return mipi_dsi_dcs_set_display_brightness_large(to_primary_dsi(ctx),
> +							 brightness);

[Severity: Medium]
Could this DCS command be sent to an unpowered panel? A user-space write to
the backlight sysfs interface can trigger novatek_bl_update_status()
asynchronously. If this happens after novatek_unprepare() has disabled the 
regulators and asserted reset, it could result in DSI host timeouts or 
display controller lockups. Should there be a check to ensure the panel is 
currently prepared?

> +}
[ ... ]
> +static int novatek_probe(struct mipi_dsi_device *dsi)
> +{
[ ... ]
> +	ret = devm_drm_panel_add(dev, &ctx->panel);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "failed to add panel\n");
> +
> +	for (i = 0; i < num_dsi; i++) {
> +		ctx->dsi[i]->lanes = desc->lanes;
> +		ctx->dsi[i]->format = desc->format;
> +		ctx->dsi[i]->mode_flags = desc->mode_flags;
> +		ctx->dsi[i]->dsc = &ctx->dsc;
> +		ret = devm_mipi_dsi_attach(dev, ctx->dsi[i]);
> +		if (ret < 0) {
> +			drm_panel_remove(&ctx->panel);
> +			return dev_err_probe(dev, ret,
> +					     "Failed to attach to DSI host\n");

[Severity: Low]
Is the manual drm_panel_remove() call necessary here? The panel was 
registered using the managed API devm_drm_panel_add(), which automatically 
handles cleanup. Calling drm_panel_remove() explicitly on the error path 
could cause the devres framework to execute it a second time.

> +		}
> +	}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260705153515.46147-1-mitltlatltl@gmail.com?part=2