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 <[email protected]> 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/[email protected]?part=2
