Re: [PATCH v6 08/10] drm/bridge: analogix_dp: Rename and simplify is_rockchip()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Modifying global connector state in atomic_check callback.
- [High] Calling sleeping functions from atomic commit callbacks.
--

commit c77fcbd07b0455b2897dddc1d458e692b59b3b2f
Author: Damon Ding <damon.ding@rock-chips.com>

drm/bridge: analogix_dp: Rename and simplify is_rockchip()

This commit renames the inline helper is_rockchip() to
analogix_dp_is_rockchip() and replaces its chained equality
comparisons with a switch-case layout for better readability.

> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 8cf6b73bceac0..116de3bd83a30 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -870,7 +870,7 @@ static int analogix_dp_bridge_atomic_check(struct drm_bridge *bridge,
>  	struct drm_display_info *di = &conn_state->connector->display_info;
>  	u32 mask = BIT(DRM_OUTPUT_COLOR_FORMAT_YCBCR444) | BIT(DRM_OUTPUT_COLOR_FORMAT_YCBCR422);
>  
> -	if (is_rockchip(dp->plat_data->dev_type)) {
> +	if (analogix_dp_is_rockchip(dp->plat_data->dev_type)) {
>  		if ((di->color_formats & mask)) {
>  			DRM_DEBUG_KMS("Swapping display color format from YUV to RGB\n");
>  			di->color_formats &= ~mask;
>  			di->color_formats |= BIT(DRM_OUTPUT_COLOR_FORMAT_RGB444);
>  			di->bpc = 8;
>  		}

[Severity: High]
This isn't a bug introduced by this patch, but does this correctly adhere to
DRM atomic KMS principles?

Mutating the global connector state (di->color_formats and di->bpc) inside
the atomic_check callback can cause unintended side effects. 

Since atomic_check may be invoked for test-only commits or in parallel,
modifying persistent properties here means these changes will remain even if
the commit fails or is rejected, potentially leading to incorrect functional
behavior in subsequent commits.

[ ... ]

> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> index 38fd8d5014d29..6207ded7ffd5e 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c

[ ... ]

> @@ -329,7 +329,7 @@ void analogix_dp_set_analog_power_down(struct analogix_dp_device *dp,
>  			reg &= ~mask;
>  
>  		writel(reg, dp->reg_base + phy_pd_addr);
> -		if (dp->plat_data && is_rockchip(dp->plat_data->dev_type))
> +		if (dp->plat_data && analogix_dp_is_rockchip(dp->plat_data->dev_type))
>  			usleep_range(10, 15);
>  		break;

[Severity: High]
This is a pre-existing issue, but is it safe to call usleep_range() here?

According to subsystem guidelines, atomic callbacks (like bridge enable and
disable routines) run in atomic context during non-blocking commits, where
sleeping functions are forbidden.

If analogix_dp_bridge_atomic_disable() invokes this function (for example,
via analogix_dp_disable_psr()), the sleep could cause kernel warnings,
system instability, or deadlocks. Should this be replaced with a non-sleeping
delay like udelay()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521080835.1362416-1-damon.ding@rock-chips.com?part=8