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 <[email protected]> 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/[email protected]?part=5
