Re: [PATCH net-next v4 5/6] net: phy: mediatek: add calibration logic for AN7581

[Christian Marangi]

On Wed, Nov 04, 2026 at 04:47:09AM +0100, Andrew Lunn wrote:
> > +static int airoha_cal_cycle(struct phy_device *phydev, int devad,
> > +			    u32 regnum, u16 mask, u16 cal_val)
> > +{
> > +	struct airoha_socphy_shared *shared = phy_package_get_priv(phydev);
> > +	struct phy_device *phydev_p0;
> > +	int ret;
> > +
> > +	phydev_p0 = shared->phydev_p0;
> > +
> > +	phy_modify_mmd(phydev, devad, regnum, mask, cal_val);
> > +
> > +	ret = mtk_cal_cycle_wait(phydev_p0);
> > +	phydev_dbg(phydev, "cal_val: 0x%x, ret: %d\n", cal_val, ret);
> 
> What is the locking scheme here?
> 
> When using PHY packages, the typical usage is:
> 
> 	phy_lock_mdio_bus(phydev);
> 
> and then use
> 
> 	val = __phy_package_read(phydev, X, Y);
> 
> All the read/write helpers are __ indicating they assume the bus is
> locked.
> 
> Here you are doing something similar, accessing another PHY in the
> same package, but without locking. Is that safe?
> 

Hi Andrew, thanks a lot for the review tag.

It's safe in the measure that the worst is that link goes down during the
calibration... I will rework the function to assign the base address to the
default address for port 0 (it's hardcoded by the SoC) and use the lock and
read/write helper instead.

The main problem of using __phy_package_read is that I have to duplicate
the mtk_cal_cycle_wait function in the airoha-ge-soc.c as it would use
different helper... is it ok?

-- 
	Ansuel