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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Unrolled resource leak and ignored DSI errors in `novatek_prepare()`.
- [Medium] DCS commands are sent unconditionally in backlight update handler.
- [Low] Missing Kconfig dependency for DSC helpers causes build failures.
- [Low] Manual call to `drm_panel_remove()` circumvents devres cleanup contract.
--

--- Patch [2]: [PATCH v3 2/2] drm/panel: Add Novatek NT36536 panel driver ---
commit c0321dc5c18b4a1a33b6415a4883e16993cea388
Author: Pengyu Luo <mitltlatltl@gmail.com>
drm/panel: Add Novatek NT36536 panel driver

This commit adds support for Novatek NT36536-based display panels,
specifically enabling the CSOT PP8807HB1-1 dual-link 10-bit panel found
in the LENOVO Legion Y700 Gen4.

> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index c0cc2bb4a3eb7..37b0938615673 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -650,6 +650,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?

Because the driver unconditionally uses drm_dsc_pps_payload_pack(), compiling
the driver without DSC helpers enabled globally will result in undefined
reference linker errors. It seems this should also select DRM_DISPLAY_HELPER.

> +	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
> new file mode 100644
> index 0000000000000..2a82b54880c30
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-novatek-nt36536.c

[ ... ]

> +static int novatek_prepare(struct drm_panel *panel)
> +{
> +	struct novatek *ctx = to_novatek(panel);
> +	struct mipi_dsi_device *dsi = to_primary_dsi(ctx);
> +	struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
> +	struct drm_dsc_picture_parameter_set pps;
> +	struct device *dev = &dsi->dev;
> +	int ret;
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(novatek_supplies),
> +				    ctx->supplies);
> +	if (ret < 0)
> +		return ret;
> +
> +	novatek_reset(ctx);
> +
> +	ret = ctx->desc->init_sequence(&dsi_ctx);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to initialize panel: %d\n", ret);
> +		gpiod_set_value_cansleep(ctx->reset_gpio, 1);
> +		regulator_bulk_disable(ARRAY_SIZE(novatek_supplies),
> +				       ctx->supplies);
> +		return ret;
> +	}
> +
> +	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: High]
Does this error path leak resources and ignore DSI errors?

If backlight_enable() fails, it returns an error code directly. At this point,
the panel regulators are still enabled and the reset GPIO is asserted.
Returning an error causes the DRM core to consider the prepare phase failed,
meaning novatek_unprepare() will never be called. This results in a permanent
power leak.

Additionally, dsi_ctx.accum_err is completely ignored after sending DSC
configuration commands, silently masking DSI transmission failures.

> +}

[ ... ]

> +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]
Can this send DCS commands unconditionally while the panel is unpowered?

Because the backlight brightness can be modified via sysfs at any time, this
can execute when the panel is suspended, unpowered, and regulators are
disabled. Sending DSI commands in this state typically results in DSI host
controller timeouts and kernel error logs. Should the driver verify the panel
is prepared before sending DCS commands?

> +}

[ ... ]

> +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);

[Severity: Low]
Is this manual call to drm_panel_remove() redundant?

The panel is registered via devm_drm_panel_add(), which schedules
drm_panel_remove() to run automatically via devres on failure. Manually
calling drm_panel_remove() in the devm_mipi_dsi_attach() error path
circumvents the devres cleanup contract and leads to duplicate removal
attempts by the driver core.

> +			return dev_err_probe(dev, ret,
> +					     "Failed to attach to DSI host\n");
> +		}
> +	}

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