Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [Medium] drm/panel: nt37802: state desync on DSI errors in disable and unprepare - [Medium] drm/panel: nt37802: asymmetric display-on/off across prepare and disable - [Medium] drm/panel: bogus voltage ranges in novatek_nt37802_set_supply_voltages() - [Low] drm/panel: nt37802: use mipi_dsi_dcs_write_seq_multi() for init commands --
commit a4454941919dc768e5c78cce0316ea1f0954d638 Author: Jayant Shekhar <[email protected]> drm/panel: Add driver for Novatek NT37802 This commit adds a DRM panel driver for the Novatek NT37802 AMOLED DSI command-mode panel running at 1440x3200@120Hz with DSC 1.2 compression and enables it as a module in the arm64 defconfig. > diff --git a/drivers/gpu/drm/panel/panel-novatek-nt37802.c > b/drivers/gpu/drm/panel/panel-novatek-nt37802.c > new file mode 100644 > index 0000000000000..632520f04ffad > --- /dev/null > +++ b/drivers/gpu/drm/panel/panel-novatek-nt37802.c > @@ -0,0 +1,544 @@ [ ... ] > +struct nt37802_cmd { > + u8 len; > + u8 data[32]; > +}; > + > +#define NT37802_CMD(...) \ > + { .len = sizeof((u8[]){ __VA_ARGS__ }), .data = { __VA_ARGS__ } } [ ... ] > +static int novatek_nt37802_set_supply_voltages(struct novatek_nt37802 *ctx) > +{ > + int ret; > + > + ret = > regulator_set_voltage(ctx->supplies[NT37802_SUPPLY_VDDIO].consumer, > + 1800000, 2000000); > + if (ret) > + return ret; > + > + ret = regulator_set_voltage(ctx->supplies[NT37802_SUPPLY_VDD].consumer, > + 1100000, 1950000); > + if (ret) > + return ret; > + > + return regulator_set_voltage(ctx->supplies[NT37802_SUPPLY_VCI].consumer, > + 3000000, 3544000); > +} [Severity: Medium] Should the voltage constraints be defined in the board Device Tree via regulator-min-microvolt and regulator-max-microvolt rather than calling regulator_set_voltage() with wide ranges in novatek_nt37802_set_supply_voltages()? The upper bounds (2000000 uV, 1950000 uV, and 3544000 uV) appear to be Qualcomm PMIC5 LDO hardware maximums from downstream qcom,panel-supply-entries rather than exact panel requirements. In drivers/regulator/core.c, regulator_set_voltage() selects the lowest voltage permitted by the intersection of the consumer request and the regulator's constraints: regulator_set_voltage_unlocked() regulator_balance_voltage() regulator_get_optimal_voltage() _regulator_do_set_voltage() If a board's PMIC LDO constraints allow a voltage down to 1.1V while the panel expects 1.2V on vdd, requesting (1100000, 1950000) will lower the rail to 1.1V, and novatek_nt37802_probe() will also fail with -EINVAL on boards using fixed regulators without voltage adjustment constraints. [ ... ] > +static int novatek_nt37802_write_init(struct novatek_nt37802 *ctx) > +{ > + struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi }; > + unsigned int i; > + > + for (i = 0; i < ARRAY_SIZE(nt37802_init_cmds); i++) > + mipi_dsi_dcs_write_buffer_multi(&dsi_ctx, > + nt37802_init_cmds[i].data, > + nt37802_init_cmds[i].len); > + > + return dsi_ctx.accum_err; > +} [Severity: Low] This isn't a bug, but could novatek_nt37802_write_init() use mipi_dsi_dcs_write_seq_multi() and standard DCS helpers (such as mipi_dsi_dcs_set_column_address_multi(), mipi_dsi_dcs_set_page_address_multi(), and mipi_dsi_dcs_set_tear_on_multi()) with the caller's struct mipi_dsi_multi_context instead of defining the custom struct nt37802_cmd with a fixed 32-byte array for every entry in nt37802_init_cmds? > + > +static int novatek_nt37802_prepare(struct drm_panel *panel) > +{ > + struct novatek_nt37802 *ctx = to_novatek_nt37802(panel); > + struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi }; > + struct drm_dsc_picture_parameter_set pps; > + int ret; > + > + if (ctx->prepared) > + return 0; > + > + ret = novatek_nt37802_enable_supplies(ctx); > + if (ret) > + return ret; > + > + novatek_nt37802_reset(ctx); > + ctx->dsi->mode_flags |= MIPI_DSI_MODE_LPM; > + > + ret = novatek_nt37802_write_init(ctx); > + if (ret) { > + novatek_nt37802_set_reset(ctx, 1); > + novatek_nt37802_disable_supplies(ctx); > + return ret; > + } > + > + mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx); > + mipi_dsi_msleep(&dsi_ctx, 120); > + mipi_dsi_dcs_set_display_on_multi(&dsi_ctx); > + mipi_dsi_msleep(&dsi_ctx, 120); > + > + 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); > + if (dsi_ctx.accum_err) { > + novatek_nt37802_set_reset(ctx, 1); > + novatek_nt37802_disable_supplies(ctx); > + return dsi_ctx.accum_err; > + } > + > + ctx->prepared = true; > + return 0; > +} > + > +static int novatek_nt37802_enable(struct drm_panel *panel) > +{ > + struct novatek_nt37802 *ctx = to_novatek_nt37802(panel); > + > + if (!ctx->prepared) > + return -EPERM; > + > + if (ctx->enabled) > + return 0; > + > + ctx->enabled = true; > + return 0; > +} > + > +static int novatek_nt37802_disable(struct drm_panel *panel) > +{ > + struct novatek_nt37802 *ctx = to_novatek_nt37802(panel); > + struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi }; > + > + if (!ctx->enabled) > + return 0; > + > + /* The off sequence sends display-off in HS mode, then waits 10 ms. */ > + ctx->dsi->mode_flags &= ~MIPI_DSI_MODE_LPM; > + mipi_dsi_dcs_set_display_off_multi(&dsi_ctx); > + mipi_dsi_msleep(&dsi_ctx, 10); > + > + ctx->enabled = false; > + return dsi_ctx.accum_err; > +} [Severity: Medium] What happens if drm_panel_disable() is followed by drm_panel_enable() without an intervening unprepare and prepare cycle? novatek_nt37802_prepare() sends both mipi_dsi_dcs_exit_sleep_mode_multi() and mipi_dsi_dcs_set_display_on_multi(), while novatek_nt37802_enable() does not send any DSI commands. On the teardown side, novatek_nt37802_disable() sends mipi_dsi_dcs_set_display_off_multi() and novatek_nt37802_unprepare() only sends mipi_dsi_dcs_enter_sleep_mode_multi(). If a disable/enable cycle occurs without unprepare/prepare, the panel will be left with the display turned off while the backlight is enabled; conversely, if prepare is followed by unprepare without enable/disable, the panel enters sleep mode without sending display-off. Should mipi_dsi_dcs_set_display_off_multi() and mipi_dsi_dcs_enter_sleep_mode_multi() both be handled in novatek_nt37802_unprepare() (dropping the enable and disable callbacks, similar to panel-novatek-nt37801.c), or should display-on be paired in novatek_nt37802_enable() with display-off in novatek_nt37802_disable()? > + > +static int novatek_nt37802_unprepare(struct drm_panel *panel) > +{ > + struct novatek_nt37802 *ctx = to_novatek_nt37802(panel); > + struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi }; > + > + if (!ctx->prepared) > + return 0; > + > + ctx->dsi->mode_flags &= ~MIPI_DSI_MODE_LPM; > + mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx); > + mipi_dsi_msleep(&dsi_ctx, 120); > + > + /* Assert the active-low reset after the downstream sleep delay. */ > + novatek_nt37802_set_reset(ctx, 1); > + ctx->prepared = false; > + novatek_nt37802_disable_supplies(ctx); > + > + return dsi_ctx.accum_err; > +} [Severity: Medium] Can a DSI error in novatek_nt37802_unprepare() or novatek_nt37802_disable() permanently desynchronize ctx->prepared and ctx->enabled from the core panel->prepared and panel->enabled state? In drivers/gpu/drm/drm_panel.c, drm_panel_unprepare() and drm_panel_disable() already track panel->prepared and panel->enabled, and skip clearing them when the driver callback returns a negative error: drivers/gpu/drm/drm_panel.c:drm_panel_unprepare() { ... if (panel->funcs && panel->funcs->unprepare) { ret = panel->funcs->unprepare(panel); if (ret < 0) goto exit; } panel->prepared = false; ... } If mipi_dsi_dcs_enter_sleep_mode_multi() fails in novatek_nt37802_unprepare(), the driver still asserts reset, sets ctx->prepared = false, disables all four regulators, and returns the negative dsi_ctx.accum_err. Because drm_panel_unprepare() then leaves panel->prepared set to true, the next drm_panel_prepare() call returns early without calling novatek_nt37802_prepare() to re-enable the regulators, and novatek_nt37802_enable() then fails with -EPERM because ctx->prepared is false. Similarly, in drm_panel_disable(), backlight_disable() runs before panel->funcs->disable(). If mipi_dsi_dcs_set_display_off_multi() fails in novatek_nt37802_disable(), ctx->enabled is cleared and the negative error is returned, leaving panel->enabled set to true so subsequent drm_panel_enable() calls return early without re-enabling the backlight. Since struct drm_panel already maintains prepared and enabled state in the core, can ctx->prepared and ctx->enabled be dropped, and should novatek_nt37802_unprepare() return 0 after powering down the hardware (with mipi_dsi_dcs_enter_sleep_mode_multi() already logging the DSI error)? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
