Re: [Patch net-next v2 2/2] net: phy: Add driver for Motorcomm Quad 2.5GbE phy

[Kyle Switch <kyle.switch@motor-comm.com>]

On 7/17/26 01:16, Andrew Lunn wrote:
>> +#define YT8824_RSSR_SPACE_MASK			BIT(0)
>> +#define YT8824_RSSR_FIBER_SPACE			(0x1)
>> +#define YT8824_RSSR_UTP_SPACE			(0x0)
> 
> Since there are only two pages, one bit:
> 
>> +/**
>> + * yt8824_read_page() - read reg page
>> + * @phydev: a pointer to a &struct phy_device
>> + *
>> + * returns current reg space of yt8824 (YT8824_RSSR_FIBER_SPACE/
>> + * YT8824_RSSR_UTP_SPACE) or negative errno code
>> + */
>> +static int yt8824_read_page(struct phy_device *phydev)
>> +{
>> +	int old_page;
>> +
>> +	old_page = ytphy_read_top_ext_with_lock(phydev, YT8521_REG_SPACE_SELECT_REG);
>> +	if (old_page < 0)
>> +		return old_page;
>> +
>> +	if ((old_page & YT8824_RSSR_SPACE_MASK) == YT8824_RSSR_FIBER_SPACE)
>> +		return YT8824_RSSR_FIBER_SPACE;
>> +
>> +	return YT8824_RSSR_UTP_SPACE;
> 
> You can simplify this to just
> 
>     return old_page & YT8824_RSSR_SPACE_MASK;
> 

Ans: Fixed in v3 patch, Please help check in updated patch.

>> +static int yt8824_write_page(struct phy_device *phydev, int page)
>> +{
>> +	int old_page;
>> +	u16 data;
>> +
>> +	old_page = ytphy_read_top_ext_with_lock(phydev, YT8521_REG_SPACE_SELECT_REG);
>> +	data = old_page & (~(0x1));
> 
> Use YT8824_RSSR_SPACE_MASK here.
> 
>> +	data |= page;
>> +
>> +	return ytphy_write_top_ext_with_lock(phydev, YT8521_REG_SPACE_SELECT_REG, data);
>> +};
>> +
>> +/**
>> + * configuration YT8824 to one template test mode.
>> + */
>> +static int yt8824_soft_reset_step1_paged(struct phy_device *phydev,
>> +					 int reg_space)
>> +{
>> +	int old_page;
>> +	int ret = 0;
>> +
>> +	old_page = phy_select_page(phydev, reg_space);
>> +	if (old_page < 0)
>> +		goto err_restore_page;
>> +
>> +	if (old_page >= 0) {
> 
> What is the purpose of this if ()?

Ans: remove the unnecessary and redundant judgment in v3 patch.

> 
>> +		if (reg_space == YT8824_RSSR_UTP_SPACE) {
>> +			ret =  __phy_write_mmd(phydev, 0x1, 0x0084, 0x2000);
> 
> No magic numbers. 0x0084 is probably not a vendor register either, so
> please use its proper name. And is 0x2000 a value, or should it bit it
> use the BIT() macro?

Ans: Fixed in v3 patch.

> 
>> +			ret = __phy_read(phydev, MII_BMCR);
>> +			if (ret < 0)
>> +				goto err_restore_page;
>> +			ret |= BMCR_RESET;
>> +			ret = __phy_write(phydev, MII_BMCR, ret);
>> +			if (ret < 0)
>> +				goto err_restore_page;
>> +			do {
>> +				msleep(50);
>> +				ret = __phy_read(phydev, MII_BMCR);
>> +				if (ret < 0)
>> +					goto err_restore_page;
>> +			} while ((ret & BMCR_RESET) && --retry);
> 
> 
>> +			ret = __phy_read(phydev, MII_BMCR);
>> +			if (ret < 0)
>> +				goto err_restore_page;
>> +			/* disable isolation */
>> +			ret &= ~BIT(10);
> 
> That is probably a standard BMCR bit, so there should be a#define for
> it.

Ans: Fix all BMCR bit in V3.

> 
>> +			/* soft reset */
>> +			ret |= BMCR_RESET;
>> +			ret = __phy_write(phydev, MII_BMCR, ret);
>> +			if (ret < 0)
>> +				return ret;
>> +			do {
>> +				msleep(50);
>> +				ret = __phy_read(phydev, MII_BMCR);
>> +				if (ret < 0)
>> +					goto err_restore_page;
>> +			} while ((ret & BMCR_RESET) && --retry);
>> +			if (ret & BMCR_RESET)
>> +				goto err_restore_page;
> 
> Look at all the duplicated code here. Please add some helpers.
> Also take a look at phy_device.c, and follow what it does.

Ans: Fixed in v3 patch using phy_read_poll_timeout to wait softret done.

> 
>> +	if (phydev->interface == PHY_INTERFACE_MODE_INTERNAL) {
>> +		/* invalid test mode */
>> +		ret = yt8824_soft_reset_step1_paged(phydev,
>> +						    YT8824_RSSR_UTP_SPACE);
> 
> Why not call this function
> yt8824_soft_reset_invalid_test_mode_paged(). Having the comment is a
> big hint your function naming is bad.

Ans: Fixed in v3 patch, to rename the function based on 
its approximate functionality

> 
>> +static int yt8824_internal_config_init_paged(struct phy_device *phydev,
>> +					     int reg_space)
>> +{
>> +	struct yt8521_priv *priv = phydev->priv;
>> +	int old_page;
>> +	int port = 0;
>> +	int ret = 0;
>> +
>> +	old_page = phy_select_page(phydev, reg_space);
>> +	if (old_page < 0)
>> +		goto err_restore_page;
>> +
>> +	port = phydev->mdio.addr - priv->phy_base_addr;
>> +	ret = ytphy_write_ext(phydev, 0x1, 0x3);
>> +	if (ret < 0)
>> +		goto err_restore_page;
>> +	ret = __phy_write(phydev, MII_BMCR, 0x1900);
> 
> All the magic numbers need to be replaced. But this is especially bad,
> because BMCR is well documented and has all its bits covered with
> existing #define's.
> 

Ans: All standard BMCR registers operations have been replaced in the v3 patch.
However, for some extend registers, they are designed to optimize 
the performance of phy8824. If they are also replaced, the entire function may 
become bloated, so they have not been replaced for the time being.

>> +	netdev_info(phydev->attached_dev,
>> +		    "%s done, phy addr: %d, phy base addr = %d\n",
>> +		    __func__, phydev->mdio.addr, priv->phy_base_addr);
> 
> Very unusual. This is a PHY driver, so it should be using
> phy_info(). But i also think this should be _dgb().

Ans: yes, it will be fixed in V3 using phydev_dbg().

> 
> 	Andrew