Re: [PATCH 1/3] drm/bridge: analogix_dp: deassert DP_PHY_PD on power-on for exynos

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/bridge: analogix_dp_set_analog_power_down() breaks ANALOG_TOTAL
--

commit 91a05dce5d444dd4add713f60a3388d24c0834b4
Author: Alexandre Marquet <tb@a-marquet.fr>

drm/bridge: analogix_dp: deassert DP_PHY_PD on power-on for exynos

This commit modifies analogix_dp_set_analog_power_down() on Exynos platforms
to deassert DP_PHY_PD and assert individual block power-down bits before
powering on requested analog blocks.

> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> index c1344a3f013ab..9f634d4e47933 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> @@ -256,6 +256,19 @@ void analogix_dp_set_analog_power_down(struct analogix_dp_device *dp,
>  	if (dp->plat_data && analogix_dp_is_rockchip(dp->plat_data->dev_type))
>  		phy_pd_addr = ANALOGIX_DP_PD;
>  
> +	reg = readl(dp->reg_base + phy_pd_addr);
> +
> +	if (dp->plat_data && (dp->plat_data->dev_type == EXYNOS_DP)) {
> +		/* On exynos, register AUX_PD and CH[0-3]_PD have no effect if
> +		 * DP_PHY_PD is asserted. So, when powering any of them up, we
> +		 * need to deassert DP_PHY_PD first.
> +		 */
> +		if (!enable && (reg & DP_PHY_PD)) {
> +			reg &= ~DP_PHY_PD;
> +			reg |= AUX_PD | CH0_PD | CH1_PD | CH2_PD | CH3_PD;
> +		}
> +	}

[Severity: Medium]
What happens when analogix_dp_set_analog_power_down() is called on EXYNOS_DP
with block set to ANALOG_TOTAL and enable set to false while DP_PHY_PD is
asserted in ANALOGIX_DP_PHY_PD?

Because this check runs before switch (block) for all block values, it clears
DP_PHY_PD and sets AUX_PD | CH0_PD | CH1_PD | CH2_PD | CH3_PD in reg. When
execution then reaches the ANALOG_TOTAL case:

drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c:analogix_dp_set_analog_power_down() {
	...
	case ANALOG_TOTAL:
		if (dp->plat_data && analogix_dp_is_rockchip(dp->plat_data->dev_type))
			mask = DP_INC_BG;
		else
			mask = DP_PHY_PD;

		if (enable)
			reg |= mask;
		else
			reg &= ~mask;

		writel(reg, dp->reg_base + phy_pd_addr);
	...
}

reg &= ~DP_PHY_PD leaves AUX_PD and CH0_PD through CH3_PD set, so writel()
powers down AUX and all four channels instead of powering on everything as
described in the commit message.

Should the ANALOG_TOTAL case also clear AUX_PD | CH0_PD | CH1_PD | CH2_PD |
CH3_PD when enable is false, or should the pre-switch check exclude
ANALOG_TOTAL and POWER_ALL?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027-drm-misc-next-manta-auxbus-v1-0-3a45c3cf531d@a-marquet.fr?part=1