Re: [PATCH 05/12] phy: phy-mtk-dp: Implement power_on and power_off PHY callbacks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The power_on and power_off callbacks use the digital register layout array to look up an analog register offset, causing register operations to modify the wrong hardware offset.
- [Medium] The error path in `mtk_dp_phy_power_on()` fails to unwind previously applied hardware state if a subsequent step fails, leaking power state.
--

commit db1c6c4d46cae7dc24770e2f52553b8292fbfbc9
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

phy: phy-mtk-dp: Implement power_on and power_off PHY callbacks

Add .power_on() and .power_off() callbacks to mtk_dp_phy_dev_ops
to be able to call those with phy_power_on() and phy_power_off()
API in the DisplayPort driver to be able to stop using all of
those hardcoded register writes in that external driver.

> diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c
> index ce33f6812bae6..586e727956332 100644
> --- a/drivers/phy/mediatek/phy-mtk-dp.c
> +++ b/drivers/phy/mediatek/phy-mtk-dp.c

[ ... ]

> @@ -200,6 +236,53 @@ static int mtk_dp_phy_configure(struct phy *phy, union phy_configure_opts *opts)
>  	return 0;
>  }
>  
> +static int mtk_dp_phy_power_on(struct phy *phy)
> +{
> +	struct mtk_dp_phy *dp_phy = phy_get_drvdata(phy);
> +	const struct mtk_dp_phy_pdata *pdata = dp_phy->pdata;
> +	const u8 *regs = pdata->regs_dig_glb;
> +	int ret;
> +
> +	/* Enable AUX Channel with RX De-Glitch and input clock detection */
> +	ret = regmap_write(dp_phy->regmap,
> +			   pdata->off_dig_glb + regs[DP_PHYD_AUX_RX_CTL],
> +			   PHYD_DIG_DPAUX_RX_EN |
> +			   PHYD_DIG_XTP_GLB_CKDET_EN |
> +			   PHYD_DIG_DPAUX_RX_DEGLITCH_EN);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_clear_bits(dp_phy->regmap,
> +				pdata->off_ana_glb + regs[DP_PHYA_GLB_FORCE_CTRL_1],

[Severity: High]
Here regs points to the digital register array pdata->regs_dig_glb, but it is
being indexed with DP_PHYA_GLB_FORCE_CTRL_1, which is an analog register
enum. Does this result in reading the wrong offset (fetching the offset for
DP_PHYD_BIT_RATE instead of DP_PHYA_GLB_FORCE_CTRL_1) and corrupting the
bitrate configuration?

> +				CKM_CKTX0_EN_FORCE_MODE);
> +	if (ret)
> +		return ret;

[Severity: Medium]
If regmap_clear_bits() fails here, the function returns the error without
unwinding the previous successful write to DP_PHYD_AUX_RX_CTL. Since the PHY
core will not call phy_power_off() if phy_power_on() returns an error, does
this leave the RX channel and clock detection permanently enabled?

> +
> +	return 0;
> +}
> +
> +static int mtk_dp_phy_power_off(struct phy *phy)
> +{
> +	struct mtk_dp_phy *dp_phy = phy_get_drvdata(phy);
> +	const struct mtk_dp_phy_pdata *pdata = dp_phy->pdata;
> +	const u8 *regs = pdata->regs_dig_glb;
> +	int ret;
> +
> +	ret = regmap_set_bits(dp_phy->regmap,
> +				pdata->off_ana_glb + regs[DP_PHYA_GLB_FORCE_CTRL_1],

[Severity: High]
Similarly to mtk_dp_phy_power_on(), this uses the digital register array
regs indexed with the analog enum DP_PHYA_GLB_FORCE_CTRL_1. Should this use
pdata->regs_ana_glb instead to avoid writing to the wrong hardware offset?

> +				CKM_CKTX0_EN_FORCE_MODE);
> +	if (ret)
> +		return ret;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701122008.19509-1-angelogioacchino.delregno@collabora.com?part=5

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy