Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] Missing Kconfig dependency for DRM_DISPLAY_DSC_HELPER leads to linker errors. - [High] Improper use of DCS brightness helper inside a vendor sequence corrupts the initialization payload. - [Medium] Global static DSC configuration structure is assigned to device context and mutated by host drivers. - [Medium] Missing DRM backlight device registration and OF backlight mapping. - [Low] Panel physical dimensions are defined but not reported to DRM userspace. --
--- Patch [2]: [PATCH v2 2/2] drm/panel: Add Novatek NT36532 panel driver --- commit 5fca5767ad8caef139f4cb6c683a24fe7d77f970 Author: Junjie Cao <[email protected]> Subject: drm/panel: Add Novatek NT36532 panel driver This commit adds a driver for panels using the Novatek NT36532 Display Driver IC, specifically the CSOT PPC100HB1-1 found in the OnePlus Pad 2 tablets. > diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig > index cbdf7b8f7f7af..e88211e6859d7 100644 > --- a/drivers/gpu/drm/panel/Kconfig > +++ b/drivers/gpu/drm/panel/Kconfig > @@ -664,6 +664,16 @@ 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_NT36532 > + tristate "Novatek NT36532-based MIPI-DSI panels" > + depends on OF > + depends on DRM_MIPI_DSI > + help > + Say Y here if you want to enable support for the panels built > + around the Novatek NT36532 display controller, such as some > + CSOT panels used in OnePlus Pad 2 tablets. These panels are > + typically dual-DSI and may use DSC (Display Stream Compression). > + [Severity: High] Since the driver's csot_init_sequence() calls drm_dsc_pps_payload_pack(), does this Kconfig entry need to select DRM_DISPLAY_DSC_HELPER? Building this as a module without other DSC drivers enabled might result in linker errors for the missing symbol. > diff --git a/drivers/gpu/drm/panel/panel-novatek-nt36532.c > b/drivers/gpu/drm/panel/panel-novatek-nt36532.c > new file mode 100644 > index 0000000000000..03f9a1b6aa5a4 > --- /dev/null > +++ b/drivers/gpu/drm/panel/panel-novatek-nt36532.c [ ... ] > +static int nt36532_get_modes(struct drm_panel *panel, > + struct drm_connector *connector) > +{ > + struct nt36532 *ctx = to_nt36532(panel); > + const struct panel_info *panel_info = ctx->panel_info; > + > + return drm_connector_helper_get_modes_fixed(connector, > + &panel_info->display_mode); > +} [Severity: Low] The function drm_connector_helper_get_modes_fixed() relies on the width_mm and height_mm properties of the drm_display_mode passed to it. Since csot_display_mode does not set these fields, the dimensions will remain 0x0. Should nt36532_get_modes() manually set connector->display_info.width_mm and height_mm using the physical dimensions defined in csot_panel_info? [ ... ] > +static int csot_init_sequence(struct nt36532 *ctx) > +{ > + struct mipi_dsi_device *dsi = to_primary_dsi(ctx); > + struct drm_dsc_picture_parameter_set pps; > + struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi }; [ ... ] > + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x4e, 0xde); > + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x4f, 0xd9); > + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x50, 0xd6); > + mipi_dsi_dcs_set_display_brightness_multi(&dsi_ctx, 0x00d4); > + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x52, 0xc2); [Severity: High] The surrounding sequence appears to use 1-byte payload writes to sequential registers. The mipi_dsi_dcs_set_display_brightness_multi() helper transmits a 2-byte payload. Could this extra byte shift subsequent commands out of alignment and cause parsing errors in the display IC firmware? Should this just be a standard 1-byte mipi_dsi_dcs_write_seq_multi() call for register 0x51? [ ... ] > +static struct drm_dsc_config csot_dsc_cfg = { > + .dsc_version_major = 1, > + .dsc_version_minor = 2, > + .slice_height = 20, > + .slice_width = 750, > + .slice_count = 2, > + .bits_per_component = 8, > + .bits_per_pixel = 8 << 4, > + .block_pred_enable = true, > +}; [ ... ] > +static int nt36532_probe(struct mipi_dsi_device *dsi) > +{ [ ... ] > + for (i = 0; i < num_dsi; i++) { > + ctx->dsi[i]->lanes = panel_info->lanes; > + ctx->dsi[i]->format = panel_info->format; > + ctx->dsi[i]->mode_flags = panel_info->mode_flags; > + ctx->dsi[i]->dsc = panel_info->dsc_cfg; [Severity: Medium] This assigns a pointer to the global static structure csot_dsc_cfg directly to the device context. Because DSI host drivers (like the MSM DPU driver) actively mutate the DSC parameters at runtime by calling drm_dsc_compute_rc_parameters(), can this cause concurrent mutation of global state, especially on dual-DSI setups? [ ... ] > + ctx->panel.prepare_prev_first = true; > + > + ret = devm_drm_panel_add(dev, &ctx->panel); > + if (ret < 0) > + return dev_err_probe(dev, ret, "failed to add panel\n"); [Severity: Medium] Is a call to drm_panel_of_backlight() or devm_backlight_device_register() missing here? Without it, userspace might be unable to change the screen brightness, or an external backlight might fail to turn on when the panel is enabled. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
