Hi, Le Tue, Oct 27, 2026 at 12:10:51PM +0000, [email protected] a écrit : > 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.
Good point. Not sure if disabling ANALOG_TOTAL should power on everything, or power on the blocks that were on before asserting DP_PHY_PD. The current behaviour is the latter (noting that ANALOG_TOTAL is currently unused). > > 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? I would favour retaining the current behaviour, by excluding ANALOG_TOTAL from the pre-switch check. There is no such need for POWER_ALL, that ends up writing 0xFF or 0x00 to the whole register, hence asserting/deasserting DP_PHY_PD together with the rest. > > -- > Sashiko AI review · > https://sashiko.dev/#/patchset/20261027-drm-misc-next-manta-auxbus-v1-0-3a45c3cf5...@a-marquet.fr?part=1 Best regards, Alex.
