Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] drm/panel: nt36850: variable shadowing in nt36850_disable() masks DCS errors - [Medium] drm/panel: nt36850: unconditional dereference of dsi1 in nt36850_disable() breaks single-DSI --
commit 08efb97fa82649f7024ae6605a2b0d90177ccd63 Author: Dmitry Baryshkov <[email protected]> drm/panel: add Novatek NT36850 panel driver Add support for the Truly WQHD dual-DSI command mode panel built around the Novatek NT36850, found on the display card for the Qualcomm SDM845 and SM8150 HDK. > diff --git a/drivers/gpu/drm/panel/panel-novatek-nt36850.c > b/drivers/gpu/drm/panel/panel-novatek-nt36850.c > --- /dev/null > +++ b/drivers/gpu/drm/panel/panel-novatek-nt36850.c > @@ -0,0 +1,351 @@ > [ ... ] > +static int nt36850_disable(struct drm_panel *panel) > +{ > + struct panel_info *pinfo = to_panel_info(panel); > + struct mipi_dsi_device *dsi0 = pinfo->dsi[0]; > + struct mipi_dsi_device *dsi1 = pinfo->dsi[1]; > + struct mipi_dsi_multi_context dsi_ctx = { .dsi = NULL }; > + > + dsi0->mode_flags &= ~MIPI_DSI_MODE_LPM; > + dsi1->mode_flags &= ~MIPI_DSI_MODE_LPM; [Severity: Medium] Does this code introduce a regression by unconditionally dereferencing a NULL pointer if a single-DSI panel is supported in the future? The dynamic nt36850_num_dsi_hosts check later in this function implies it is intended to support single-DSI setups, but dsi1 is used directly here without verification. > + > + for (int i = 0; i < nt36850_num_dsi_hosts(pinfo); i++) { > + struct mipi_dsi_multi_context dsi_ctx = { .dsi = pinfo->dsi[i]}; > + > + mipi_dsi_dcs_set_display_off_multi(&dsi_ctx); > + } [Severity: Medium] Is it a regression that the inner dsi_ctx shadows the function-scoped dsi_ctx? By declaring a new dsi_ctx inside the loop, the error state from mipi_dsi_dcs_set_display_off_multi gets instantly discarded when the block ends, rather than accumulating. > + mipi_dsi_msleep(&dsi_ctx, 120); [Severity: Medium] Because the inner dsi_ctx shadowed the outer one, does this blindly execute the delay using the unmodified outer context, even if the display off sequence commands failed? > + > + for (int i = 0; i < nt36850_num_dsi_hosts(pinfo); i++) { > + struct mipi_dsi_multi_context dsi_ctx = { .dsi = pinfo->dsi[i]}; > + > + mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx); > + } > + mipi_dsi_msleep(&dsi_ctx, 120); > + > + dsi0->mode_flags |= MIPI_DSI_MODE_LPM; > + dsi1->mode_flags |= MIPI_DSI_MODE_LPM; [Severity: Medium] Could this result in another NULL pointer dereference regression for single-DSI panels here when restoring the mode flags? > + > + return dsi_ctx.accum_err; > +} [Severity: Medium] Since the function-level dsi_ctx remains unmodified throughout, will this always return 0 and mask any hardware failures that occurred during the disable sequence? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
