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

[Andrew Lunn]

> +#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;

> +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 ()?

> +		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?

> +			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.

> +			/* 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.

> +	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.

> +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.

> +	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().

	Andrew