Re: [PATCH V3 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] The `enable` GPIO is incorrectly requested with an active state during `probe()`, violating typical power sequencing.
- [Medium] Missing cleanup of regulator and GPIOs in `prepare()` when MIPI DCS commands fail.
- [Medium] Panel-specific delay configuration fields are ignored in favor of hardcoded values.
- [Low] The driver calls an undeclared function `devm_drm_panel_add()`, causing a build failure.
--

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

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

This commit adds support for the TD4310 MIPI DSI panel controller
used in the Anbernic RG Vita-Pro handheld gaming console.

> 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..d277682f63a3a
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-anbernic-td4310.c

[ ... ]

> +static int panel_anbernic_td4310_prepare(struct drm_panel *panel)
> +{

[ ... ]

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

[Severity: Medium]
Does this code leave the vdd regulator and enable_gpio enabled if a DCS
command fails?

If dsi_ctx.accum_err is non-zero, this returns immediately instead of
jumping to the err_reset and err_enable cleanup labels.

> +
> +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);

[Severity: Medium]
Is it intentional to use a hardcoded 120ms delay here instead of using
the panel-specific disable_delay or unprepare_delay fields defined in
the panel_info struct?

[ ... ]

> +static int panel_anbernic_td4310_probe(struct mipi_dsi_device *dsi)
> +{
> +	struct device *dev = &dsi->dev;

[ ... ]

> +	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);

[Severity: High]
Could requesting this GPIO with GPIOD_OUT_HIGH drive the signal active
before the vdd regulator is enabled in panel_anbernic_td4310_prepare()?

This might violate hardware power sequencing by driving control signals
active while the main panel IC is unpowered.

> +	if (IS_ERR(ctx->enable_gpio))
> +		return dev_err_probe(dev, PTR_ERR(ctx->enable_gpio),
> +				     "Cannot get enable gpio\n");
> +
> +	ctx->vdd = devm_regulator_get(dev, "vdd");

[ ... ]

> +	ret = drm_panel_of_backlight(&ctx->panel);
> +	if (ret)
> +		return ret;
> +
> +	devm_drm_panel_add(dev, &ctx->panel);

[Severity: Low]
Is devm_drm_panel_add available in the DRM subsystem?

It looks like this might cause a build failure due to an implicit
function declaration.

> +
> +	ret = devm_mipi_dsi_attach(dev, dsi);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to attach to DSI host\n");

[ ... ]

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