Re: [PATCH V2 4/6] drm/panel: anbernic-td4310: Add RG Vita Pro panel

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Resource leak and hardware state mismatch on prepare error path.
- [High] Enable GPIO is incorrectly asserted during probe before the VDD regulator is enabled.
- [Medium] Panel-specific delay configuration fields are ignored in favor of hardcoded values.
- [Low] Extraneous semicolon after if-statement block.
--

commit b88e27c6a433bac0f5f02ad289ba084e02f359ed
Author: Chris Morgan <macromorgan@hotmail.com>

drm/panel: anbernic-td4310: Add RG Vita Pro panel

This commit adds support for the TD4310 DSI panel used in the Anbernic RG
Vita Pro handheld console. It introduces a new panel driver with power
sequencing, DCS initialization, and display mode definitions.

> diff --git a/drivers/gpu/drm/panel/panel-anbernic-td4310.c b/drivers/gpu/drm/panel/panel-anbernic-td4310.c
> new file mode 100644
> index 0000000000000..733c68ff9771b
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-anbernic-td4310.c
[ ... ]
> +static int panel_anbernic_td4310_prepare(struct drm_panel *panel)
> +{
> +	struct anbernic_panel_td4310 *ctx = panel_to_anbernic_panel_td4310(panel);
> +	struct mipi_dsi_device *dsi = ctx->dsi;
> +	struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
> +	int ret;
> +
> +	ret = regulator_enable(ctx->vdd);
> +	if (ret)
> +		return ret;
> +
> +	if (ctx->enable_gpio) {
> +		ret = gpiod_set_value_cansleep(ctx->enable_gpio, 1);
> +		if (ret)
> +			goto err_enable;
> +	};

This isn't a bug, but is there an extra semicolon here after the if block?

> +
> +	gpiod_set_value_cansleep(ctx->reset_gpio, 0);
> +	mipi_dsi_msleep(&dsi_ctx, 10);
> +
> +	ret = gpiod_set_value_cansleep(ctx->reset_gpio, 1);
> +	if (ret)
> +		goto err_reset;
> +	if (ctx->panel_info->reset_delay)
> +		mipi_dsi_msleep(&dsi_ctx, ctx->panel_info->reset_delay);
> +	ret = gpiod_set_value_cansleep(ctx->reset_gpio, 0);
> +	if (ret)
> +		goto err_reset;
> +
> +	if (ctx->panel_info->enable_delay)
> +		mipi_dsi_msleep(&dsi_ctx, ctx->panel_info->enable_delay);
> +
> +	mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
> +	mipi_dsi_msleep(&dsi_ctx, 100);

Does this code ignore the prepare_delay and init_delay fields defined in
anbernic_panel_td4310_info by using a hardcoded 100ms sleep?

> +	mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
> +	mipi_dsi_msleep(&dsi_ctx, 100);
> +
> +	return dsi_ctx.accum_err;

If mipi_dsi_dcs_exit_sleep_mode_multi or mipi_dsi_dcs_set_display_on_multi
encounters an error and populates dsi_ctx.accum_err, does this return bypass
the err_reset and err_enable cleanup paths?

If the DRM framework does not call unprepare when prepare fails, does
this leave the vdd regulator enabled indefinitely and the GPIOs in their
active states?

> +
> +err_reset:
> +	gpiod_set_value_cansleep(ctx->enable_gpio, 0);
> +err_enable:
> +	regulator_disable(ctx->vdd);
> +	return ret;
> +}
> +
> +static int panel_anbernic_td4310_unprepare(struct drm_panel *panel)
> +{
> +	struct anbernic_panel_td4310 *ctx = panel_to_anbernic_panel_td4310(panel);
> +	struct mipi_dsi_device *dsi = ctx->dsi;
> +	struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
> +
> +	mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
> +	mipi_dsi_msleep(&dsi_ctx, 120);
> +	mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
> +	mipi_dsi_msleep(&dsi_ctx, 120);

Are the disable_delay and unprepare_delay fields defined in
anbernic_panel_td4310_info intended to be used here instead of the hardcoded
120ms sleeps?

[ ... ]
> +static int panel_anbernic_td4310_probe(struct mipi_dsi_device *dsi)
> +{
> +	struct device *dev = &dsi->dev;
> +	struct anbernic_panel_td4310 *ctx;
> +	int ret;
[ ... ]
> +	ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(ctx->reset_gpio))
> +		return dev_err_probe(dev, PTR_ERR(ctx->reset_gpio),
> +				     "Cannot get reset gpio\n");
> +
> +	ctx->enable_gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_HIGH);

Does initializing enable_gpio with GPIOD_OUT_HIGH assert the GPIO
immediately during probe, before the vdd regulator is enabled in
panel_anbernic_td4310_prepare?

Could driving control pins active while the main IC power supply is off
cause leakage current or latch-up through ESD diodes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515221947.299229-1-macroalpha82@gmail.com?part=4