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 <[email protected]> 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-3a45c3cf5...@a-marquet.fr?part=1
