Re: [PATCH v3 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] NULL pointer dereference in `icna35xx_get_current_mode()` due to uninitialized `connector`.
- [High] State leak and data race in backlight operations modifying `dsi->mode_flags`.
- [High] Incomplete static DSC configuration leads to invalid PPS payload and display failure.
- [High] Backlight callbacks attempt to send DSI commands when the panel is suspended.
- [Medium] Unbalanced reset GPIO teardown in the `icna35xx_prepare()` error path.
- [Low] Missing `select DRM_DISPLAY_DSC_HELPER` in Kconfig.
--

commit f579b883c0c01eb10daf710ef569cf31687d50a6
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 AYN Odin 2
Portal, Ayaneo Pocket DS, AYN Odin 3, and AYN Thor top panels.

> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> --- 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

Is a select DRM_DISPLAY_DSC_HELPER needed here? This driver uses
drm_dsc_pps_payload_pack(), which is guarded by CONFIG_DRM_DISPLAY_DSC_HELPER.
Without it, could this cause a build error?

> 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_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)

Can connector be NULL here if the panel is prepared before modes are explicitly
probed? Should this condition also check if (!connector) to prevent a
potential NULL pointer dereference?

[ ... ]

> +static struct panel_desc odin2portal_desc = {
> +	.modes = odin2portal_modes,
> +	.num_modes = ARRAY_SIZE(odin2portal_modes),
> +	.width_mm = 160,
> +	.height_mm = 89,
> +	.bpc = 8,
> +	.lanes = 4,
> +	.format = MIPI_DSI_FMT_RGB888,
> +	.mode_flags = MIPI_DSI_MODE_NO_EOT_PACKET | MIPI_DSI_CLOCK_NON_CONTINUOUS |
> +			MIPI_DSI_MODE_LPM,
> +	.init_sequence = icna3512_init_sequence,
> +	.dsc = {
> +		.dsc_version_major = 0x1,
> +		.dsc_version_minor = 0x1,
> +		.slice_height = 20,
> +		.slice_width = 540,
> +		.slice_count = 2,
> +		.bits_per_component = 8,
> +		.bits_per_pixel = 8 << 4,
> +		.block_pred_enable = true,
> +	},
> +};

Are these static DSC configurations missing runtime parameters like pic_width,
pic_height, and Rate Control (RC) parameters?

When drm_dsc_pps_payload_pack() is called during the initialization sequence,
could it pack zeroed values into the Picture Parameter Set and cause the
display's DSC decoder to fail?

[ ... ]

> +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 returning? Leaving
the reset line driven out of reset while the power supplies are disabled might
cause back-powering or current leakage into the panel IC.

[ ... ]

> +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);
> +	if (ret < 0)
> +		return ret;

Are these backlight callbacks reachable from userspace via sysfs when the panel
is suspended? If the display pipeline is suspended and regulators are
disabled, sending DSI DCS commands to powered-off hardware could result in
DSI host transmission timeouts.

Additionally, if mipi_dsi_dcs_set_display_brightness_large() fails and
returns early, does this leave the MIPI_DSI_MODE_LPM bit permanently cleared?

Furthermore, since backlight sysfs callbacks execute without DRM subsystem
locks, could modifying dsi->mode_flags here race concurrently with DRM atomic
commits and corrupt the DSI hardware configuration?

> +
> +	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);
> +	if (ret < 0)
> +		return ret;

Similarly, does this error path permanently alter the DSI mode flags and
potentially race with DRM atomic commits?

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