Re: [PATCH RFC 2/2] drivers: gpu: drm: panel: Add BOE NT51021 driver

[Nickolay Goppen <setotau@mainlining.org>]

29.08.2026 23:19, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] The panel initialization sequence lacks the DCS 'Set Display On' (0x29) command.
> - [Medium] Missing reset assertion and stabilization delay during panel preparation.
> --
>
> commit 864f3dccc86bc8a97845ebafd73a7df32e9d6b9e
> Author: Nickolay Goppen <setotau@mainlining.org>
>
> drivers: gpu: drm: panel: Add BOE NT51021 driver
>
> This commit adds support for the BOE NT51021-based 8-inch and 10-inch DSI
> video mode panels used in Xiaomi Mi Pad 4 tablets. It includes panel
> initialization sequences and content-adaptive brightness control.
>
>> diff --git a/drivers/gpu/drm/panel/panel-novatek-nt51021.c b/drivers/gpu/drm/panel/panel-novatek-nt51021.c
>> new file mode 100644
>> --- /dev/null
>> +++ b/drivers/gpu/drm/panel/panel-novatek-nt51021.c
> [ ... ]
>> +static void nt51021_boe_reset(struct boe_nt51021_desc *ctx)
>> +{
>> +	gpiod_set_value_cansleep(ctx->reset_gpio, 0);
>> +	usleep_range(10000, 11000);
>> +}
> [Severity: Medium]
> Does the panel require an explicit reset assertion pulse before de-asserting
> the reset line? The current code only de-asserts the reset line here. Without
> an explicit assertion pulse and delay prior to de-assertion, the hardware
> might not be guaranteed a valid minimum reset pulse width once power is stable.
>
> [ ... ]

Downstream reset sequence is just a deassertion of a reset line [1].

[1] 
https://github.com/xiaomi-sdm660/android_kernel_xiaomi_sdm660/blob/11-EAS/arch/arm/boot/dts/qcom/xiaomi/mi/clover/dsi-panel-nt51021-boe-10wu-video.dtsi#L331

>> +static int nt51021_boe_8_init(struct boe_nt51021_desc *ctx)
>> +{
>> +	struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
> [ ... ]
>> +	mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
>> +	mipi_dsi_usleep_range(&dsi_ctx, 5000, 6000);
>> +	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x8f, 0x00);
>> +	mipi_dsi_usleep_range(&dsi_ctx, 1000, 2000);
>> +
>> +	nt51021_cabc_on(&dsi_ctx);
>> +
>> +	return dsi_ctx.accum_err;
>> +}
> [Severity: Medium]
> Does this initialization sequence miss a DCS "Set Display On" command?
> The panel is awakened from sleep mode via mipi_dsi_dcs_exit_sleep_mode_multi(),
> but it is never explicitly instructed to turn on the display output here in
> nt51021_boe_8_init(). This could cause the panel to remain blank on some DSI
> hosts.
>
> [ ... ]
>> +static int nt51021_boe_10wu_init(struct boe_nt51021_desc *ctx)
>> +{
>> +	struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
> [ ... ]
>> +	mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
>> +	mipi_dsi_usleep_range(&dsi_ctx, 5000, 6000);
>> +	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x8f, 0x00);
>> +	mipi_dsi_usleep_range(&dsi_ctx, 1000, 2000);
>> +
>> +	nt51021_cabc_on(&dsi_ctx);
>> +	mipi_dsi_usleep_range(&dsi_ctx, 1000, 2000);
>> +
>> +	return dsi_ctx.accum_err;
>> +}
> [Severity: Medium]
> Similarly to nt51021_boe_8_init(), the 10-inch variant initialization path
> also appears to omit the mipi_dsi_dcs_set_display_on_multi() command.
>
>> +static int nt51021_boe_off(struct boe_nt51021_desc *ctx)
>> +{
>> +	struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
>> +
>> +	nt51021_cabc_off(&dsi_ctx);
>> +
>> +	mipi_dsi_generic_write_seq_multi(&dsi_ctx, 0x8f, 0xa5);
>> +	mipi_dsi_msleep(&dsi_ctx, 20);
>> +	mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
> [Severity: Medium]
> The power-off sequence explicitly turns the display off using
> mipi_dsi_dcs_set_display_off_multi(). Should there be a matching turn-on
> command in the initialization sequences to ensure the display activates
> correctly across power cycles?
>
> [ ... ]
>> +static int nt51021_boe_prepare(struct drm_panel *panel)
>> +{
>> +	struct boe_nt51021_desc *ctx = to_boe_panel(panel);
>> +	struct device *dev = &ctx->dsi->dev;
>> +	int ret;
>> +
>> +	ret = regulator_bulk_enable(ARRAY_SIZE(nt51021_boe_supplies), ctx->supplies);
>> +	if (ret < 0) {
>> +		dev_err(dev, "Failed to enable regulators: %d\n", ret);
>> +		return ret;
>> +	}
>> +	nt51021_boe_reset(ctx);
> [Severity: Medium]
> Does the panel require a stabilization delay after enabling the regulators
> and before de-asserting the reset line? The current code immediately calls
> nt51021_boe_reset(ctx). Without a delay, the reset logic might execute
> before the regulators have fully ramped up, potentially leading to unstable
> hardware initialization.
>
> [ ... ]
>
-- 
Best regards,
Nickolay