Re: [PATCH v4 2/2] drm/panel: Add panel driver for Chipone ICNA35XX based panels

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Hardware back-powering due to incorrect reset GPIO initialization and error handling.
- [High] Backlight sysfs callbacks send MIPI DCS commands to an unpowered panel.
- [High] Data race on `dsi->mode_flags` corrupts hardware state configuration.
- [Medium] Shared static data mutation in DSI DSC configuration.
- [Low] Missing Kconfig Dependency for DRM_DISPLAY_DSC_HELPER causes a build failure.
--

commit cb6f31a048e6cc4b007cabd737760b76820c7eb7
Author: Teguh Sobirin <teguh@sobir.in>

drm/panel: Add panel driver for Chipone ICNA35XX based panels

This commit adds support for the ICNA3512 and ICNA3520 display controllers
used in the AYN Odin 2 Portal, Ayaneo Pocket DS, AYN Odin 3, and Thor top
panels. It introduces unique compatibles for each panel while sharing the
common DDIC driver handling.

> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 7450b27622a23..1368b5a0b6c91 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -105,6 +105,17 @@ config DRM_PANEL_BOE_TV101WUM_LL2
>  	  Say Y here if you want to support for BOE TV101WUM-LL2
>  	  WUXGA PANEL DSI Video Mode panel
>  
> +config DRM_PANEL_CHIPONE_ICNA35XX
> +	tristate "Chipone ICNA35XX panel driver"
> +	depends on OF
> +	depends on DRM_MIPI_DSI
> +	depends on BACKLIGHT_CLASS_DEVICE
> +	select DRM_DISPLAY_HELPER

[Severity: Low]
Will this cause a build failure if DRM_DISPLAY_DSC_HELPER isn't enabled
elsewhere?

The panel driver unconditionally calls drm_dsc_pps_payload_pack() in its
initialization sequence, such as in icna3512_init_sequence().

Without explicitly selecting the DSC helper here, building the kernel with
this panel driver could result in an undefined reference linker error.
Should we add a select for DRM_DISPLAY_DSC_HELPER?

> diff --git a/drivers/gpu/drm/panel/panel-chipone-icna35xx.c b/drivers/gpu/drm/panel/panel-chipone-icna35xx.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-chipone-icna35xx.c
[ ... ]
> +static int icna35xx_prepare(struct drm_panel *panel)
> +{
> +	struct panel_info *pinfo = to_panel_info(panel);
> +	int ret;
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(panel_supplies), pinfo->supplies);
> +	if (ret < 0) {
> +		dev_err(panel->dev, "failed to enable regulators: %d\n", ret);
> +		return ret;
> +	}
> +
> +	icna35xx_reset(pinfo);
> +
> +	ret = pinfo->desc->init_sequence(pinfo);
> +	if (ret < 0) {
> +		regulator_bulk_disable(ARRAY_SIZE(panel_supplies), pinfo->supplies);

[Severity: High]
Does this error path need to assert the reset GPIO before disabling the
regulators?

If the initialization sequence fails, the regulators are turned off while the
reset GPIO remains deasserted. Leaving the unpowered panel driven by the GPIO
signal could cause hardware back-powering issues.

[ ... ]
> +static int icna35xx_disable(struct drm_panel *panel)
> +{
> +	struct panel_info *pinfo = to_panel_info(panel);
> +	struct mipi_dsi_multi_context dsi_ctx = { .dsi = pinfo->dsi };
> +
> +	pinfo->dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;

[Severity: High]
Can this non-atomic modification to dsi->mode_flags race with the backlight
sysfs callbacks?

Since icna35xx_bl_update_status() also dynamically toggles MIPI_DSI_MODE_LPM
using non-atomic bitwise operations, a data race could occur if backlight
sysfs callbacks execute concurrently with DRM atomic modeset operations.

Could this silently corrupt the DSI host configuration by leaving the mode
flags in the wrong state? Is locking needed to serialize access to these
flags?

[ ... ]
> +static int icna35xx_bl_update_status(struct backlight_device *bl)
> +{
> +	struct mipi_dsi_device *dsi = bl_get_data(bl);
> +	u16 brightness = backlight_get_brightness(bl);
> +	int ret;
> +
> +	dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
> +
> +	ret = mipi_dsi_dcs_set_display_brightness_large(dsi, brightness);

[Severity: High]
Could this dispatch MIPI DCS commands to an unpowered panel?

Because the backlight device is registered manually, it remains accessible to
userspace even when the panel is unprepared and suspended. If userspace
writes to the sysfs brightness attribute at that time, it might trigger bus
timeouts or host controller crashes.

Should this callback, along with icna35xx_bl_get_brightness(), verify the
panel's power state before sending DCS transactions?

[ ... ]
> +static int icna35xx_probe(struct mipi_dsi_device *dsi)
> +{
> +	struct device *dev = &dsi->dev;
> +	struct panel_info *pinfo;
> +	int ret;
[ ... ]
> +	pinfo->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);

[Severity: High]
Could requesting this GPIO with GPIOD_OUT_LOW cause back-powering during
probe?

For a typical active-low reset pin, this logically deasserts it, potentially
driving a high voltage into the panel IC immediately during probe while its
regulators are still turned off.

Should it be initialized to a safe asserted state like GPIOD_OUT_HIGH
instead?

[ ... ]
> +	pinfo->dsi->lanes = pinfo->desc->lanes;
> +	pinfo->dsi->format = pinfo->desc->format;
> +	pinfo->dsi->mode_flags = pinfo->desc->mode_flags;
> +	pinfo->dsi->dsc = &pinfo->desc->dsc;

[Severity: Medium]
Since DSI host drivers dynamically compute and write DSC parameters into
dsi->dsc, will assigning this to the statically allocated pinfo->desc->dsc
mutate global data?

This could lead to state leakage across driver unbind and rebind cycles, or
cause race conditions if multiple panels of the same type are instantiated.
Should the driver allocate a per-instance struct drm_dsc_config instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260607-icna35xx-v4-0-64de514add34@gmail.com?part=2