Re: [PATCH v2 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 6 potential issue(s) to consider:
- [High] Missing DRM_DISPLAY_DSC_HELPER select causes link errors.
- [High] Missing NULL pointer check for `connector` in `icna35xx_get_current_mode()`.
- [High] Backlight operations send DCS commands without verifying panel power state.
- [High] Data race on `dsi->mode_flags` between sysfs backlight operations and DRM panel operations.
- [Medium] Failure to restore `MIPI_DSI_MODE_LPM` flag on error paths in backlight operations.
- [Medium] The error path in `icna35xx_prepare()` disables regulators without first asserting the reset GPIO.
--

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

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

This adds support for the ICNA3512 and ICNA3520 DDICs used in the AYN Odin 2
Portal and for both the AYN Odin 3 and the AYN Thor top panel respectively.

> 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

The driver code explicitly calls drm_dsc_pps_payload_pack(), which is
exported by drivers/gpu/drm/display/drm_dsc_helper.c. Should this config
also select DRM_DISPLAY_DSC_HELPER to avoid undefined reference link errors?

[ ... ]

> diff --git a/drivers/gpu/drm/panel/panel-chipone-icna35xx.c b/drivers/gpu/drm/panel/panel-chipone-icna35xx.c
> new file mode 100644
> index 0000000000000..958f205a7f4f9
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-chipone-icna35xx.c
> @@ -0,0 +1,619 @@

[ ... ]

> +static int icna35xx_get_current_mode(struct panel_info *pinfo)
> +{
> +	struct drm_connector *connector = pinfo->connector;
> +	struct drm_crtc_state *crtc_state;
> +	int i;
> +
> +	/* Return the default (first) mode if no info available yet */
> +	if (!connector->state || !connector->state->crtc)
> +		return 0;

Can connector be NULL here? It appears pinfo->connector is only initialized
during get_modes(). If the panel's prepare() callback is invoked before
get_modes() (for instance, if userspace forces a modeset without querying
modes), dereferencing connector->state when connector is NULL could cause a
kernel panic.

[ ... ]

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

Does this error path need to assert the reset GPIO before disabling the
regulators? The icna35xx_reset() function leaves the reset_gpio deasserted,
and properly powering down the panel usually requires asserting the reset
GPIO (driving it to 1, as done in icna35xx_unprepare()) to avoid backpowering
and leakage current.

> +		dev_err(panel->dev, "failed to initialize panel: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +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;

Could this non-atomic modification of dsi->mode_flags race with the sysfs
backlight callbacks?

Sysfs operations are protected by the backlight subsystem's ops_lock, while
DRM panel callbacks use modeset locks. Concurrent execution could lead to
interleaved read-modify-write operations, potentially corrupting flags like
MIPI_DSI_CLOCK_NON_CONTINUOUS.

> +
> +	mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
> +	mipi_dsi_msleep(&dsi_ctx, 50);

[ ... ]

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

Since userspace can trigger sysfs callbacks at any time, can this send DCS
commands when the panel is suspended or disabled?

Sending commands to an unpowered panel could cause DSI bus transfer timeouts
or hardware hangs. Does the driver need to check if the panel is prepared
before accessing the hardware?

> +	if (ret < 0)
> +		return ret;

If mipi_dsi_dcs_set_display_brightness_large() fails, the function returns
early without restoring the MIPI_DSI_MODE_LPM flag. Will this leave the
hardware in High Speed mode for all subsequent communications?

> +
> +	dsi->mode_flags |= MIPI_DSI_MODE_LPM;
> +
> +	return 0;
> +}
> +
> +static int icna35xx_bl_get_brightness(struct backlight_device *bl)
> +{
> +	struct mipi_dsi_device *dsi = bl_get_data(bl);
> +	u16 brightness;
> +	int ret;
> +
> +	dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
> +
> +	ret = mipi_dsi_dcs_get_display_brightness_large(dsi, &brightness);

The same concerns about unpowered panels apply here.

> +	if (ret < 0)
> +		return ret;

The same flag restoration issue on the error path also appears here.

> +
> +	dsi->mode_flags |= MIPI_DSI_MODE_LPM;
> +
> +	return brightness;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514-icna35xx-v2-0-45acd1dfa566@gmail.com?part=2