Re: [PATCH v4 2/2] drm: panel: add support for the Renesas R63419 based dual-DSI video mode Display Panels

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] DCS initialization sequence sends SET_DISPLAY_ON before EXIT_SLEEP_MODE.
- [High] Incorrect hardware power sequencing during teardown risks hardware damage.
- [High] Resource leak of `vdd_supplies` in the `renesas_r63419_prepare()` error path.
- [High] Panel is registered to DRM before DSI devices are attached, causing a race condition.
- [High] Secondary DSI device is registered with the primary's `of_node`, causing an unintended re-probe.
- [Medium] Silent ignore of errors on the secondary DSI link in multi-context DCS sequences.
--

commit 536a4ce2d0cffbeb00543674b725c12c37ef3272
Author: KancyJoe <kancy2333@outlook.com>

drm: panel: add support for the Renesas R63419 based dual-DSI video mode Display Panels
    
Implement support for the Renesas 63419 based dual-DSI video mode
Display Panels found in the Ayaneo gaming handled devices.

> diff --git a/drivers/gpu/drm/panel/panel-renesas-r63419.c b/drivers/gpu/drm/panel/panel-renesas-r63419.c
> new file mode 100644
> index 0000000000000..1643a9c78c3fd
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-renesas-r63419.c
[ ... ]
> +static int renesas_r63419_on(struct renesas_r63419_panel *ctx)
> +{
> +	struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi[0] };
> +	struct mipi_dsi_multi_context dsi_ctx1 = { .dsi = ctx->dsi[1] };
> +
> +	mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
> +	mipi_dsi_dcs_set_display_on_multi(&dsi_ctx1);
> +	mipi_dsi_msleep(&dsi_ctx, 150);
> +
> +	mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
> +	mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx1);
> +	mipi_dsi_msleep(&dsi_ctx, 50);

Are the DCS commands sent in the correct order? Standard MIPI DCS sequences
require exiting sleep mode (0x11) and waiting for the hardware to wake up
before setting the display on (0x29). Sending a Display On command while
the panel is still in sleep mode can prevent the display from turning on.

Also, does this silently ignore errors on the secondary DSI link?
If a command fails on dsi_ctx1, the error doesn't seem to be captured or
checked here. Additionally, mipi_dsi_msleep() only checks the primary
context, so if a failure occurs on the primary, the sleep is bypassed, and
commands to the secondary are sent without the required delay.

> +
> +	return dsi_ctx.accum_err;

Should this return a combination of both dsi_ctx.accum_err and
dsi_ctx1.accum_err?

> +}
[ ... ]
> +static int renesas_r63419_prepare(struct drm_panel *panel)
> +{
> +	struct renesas_r63419_panel *ctx = to_renesas_r63419_panel(panel);
> +	int ret;
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(renesas_r63419_vdd_supplies),
> +				    ctx->vdd_supplies);
> +	if (ret < 0)
> +		return ret;
> +
> +	usleep_range(1000, 2000);
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(renesas_r63419_vcc_supplies),
> +				    ctx->vcc_supplies);
> +	if (ret < 0)
> +		return ret;

If regulator_bulk_enable() for vcc_supplies fails here, does this leak the
regulator reference for the already-enabled vdd_supplies?

> +
> +	usleep_range(1000, 2000);
> +
> +	gpiod_set_value_cansleep(ctx->reset_gpio, 0);
> +
> +	usleep_range(3000, 4000);
> +
> +	ret = renesas_r63419_on(ctx);
> +	if (ret < 0) {
> +		dev_err(panel->dev, "Failed to initialize panel: %d\n", ret);
> +		regulator_bulk_disable(ARRAY_SIZE(renesas_r63419_vcc_supplies),
> +				       ctx->vcc_supplies);
> +		regulator_bulk_disable(ARRAY_SIZE(renesas_r63419_vdd_supplies),
> +				       ctx->vdd_supplies);
> +
> +		gpiod_set_value_cansleep(ctx->reset_gpio, 1);

Could this hardware power sequencing risk hardware damage? The power
regulators are disabled before the reset GPIO is asserted. Leaving a logic
high voltage driven on an IO pin of an unpowered IC can back-power the
chip through its ESD protection diodes. The reset GPIO should likely be
asserted before disabling the regulators.

> +
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int renesas_r63419_unprepare(struct drm_panel *panel)
> +{
> +	struct renesas_r63419_panel *ctx = to_renesas_r63419_panel(panel);
> +
> +	regulator_bulk_disable(ARRAY_SIZE(renesas_r63419_vcc_supplies), ctx->vcc_supplies);
> +	regulator_bulk_disable(ARRAY_SIZE(renesas_r63419_vdd_supplies), ctx->vdd_supplies);
> +
> +	gpiod_set_value_cansleep(ctx->reset_gpio, 1);

The same power sequencing issue seems to exist here during teardown.

> +
> +	return 0;
> +}
[ ... ]
> +static int renesas_r63419_probe(struct mipi_dsi_device *dsi)
> +{
[ ... ]
> +	/* Copy current DSI info */
> +	strscpy(info.type, dsi->name, sizeof(info.type));
> +	info.channel = dsi->channel;
> +	info.node = dev_of_node(dev);
> +
> +	/* Register the second DSI device */
> +	ctx->dsi[1] = devm_mipi_dsi_device_register_full(dev, dsi1_host, &info);

Could setting info.node to dev_of_node(dev) for the secondary DSI device
cause an unintended re-probe loop? Because the secondary device shares the
same of_node as the primary device, the MIPI DSI bus driver matching logic
will match it against the same renesas_r63419_driver and invoke probe()
again for the secondary device.

> +	if (IS_ERR(ctx->dsi[1]))
> +		return dev_err_probe(dev, PTR_ERR(ctx->dsi[1]),
> +				     "Failed to register second DSI device\n");
> +
> +	ctx->dsi[0] = dsi;
> +	mipi_dsi_set_drvdata(dsi, ctx);
> +
> +	/* Get panel orientation */
> +	ret = of_drm_get_panel_orientation(dev->of_node, &ctx->orientation);
> +	if (ret < 0 && ret != -ENODEV)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to get panel orientation\n");
> +
> +	ctx->panel.prepare_prev_first = true;
> +
> +	ret = drm_panel_of_backlight(&ctx->panel);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get backlight\n");
> +
> +	ret = devm_drm_panel_add(dev, &ctx->panel);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to add panel\n");
> +
> +	/* Configure and attach both DSI devices */
> +	for (i = 0; i < ARRAY_SIZE(ctx->dsi); i++) {
> +		ctx->dsi[i]->lanes = ctx->desc->lanes;
> +		ctx->dsi[i]->format = ctx->desc->format;
> +		ctx->dsi[i]->mode_flags = ctx->desc->mode_flags;
> +
> +		ret = devm_mipi_dsi_attach(dev, ctx->dsi[i]);

Does registering the panel to DRM before attaching the DSI devices
introduce a race condition? Calling devm_drm_panel_add() makes the panel
immediately visible to consumers. If a consumer prepares the panel right
away, it will attempt to send DCS commands over unattached DSI links.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519-topic-sm8650-ayaneo-pocket-s2-r63419-v4-0-b8929af5e951@linaro.org?part=2