Re: [PATCH net-next v22 3/3] net: phy: Add driver for Motorcomm Quad 2.5GbE phy

[Andrew Lunn <andrew@lunn.ch>]

> +static int yt8824_restore_working_status(struct phy_device *phydev, int ret)
> +{
> +	struct yt8824_shared_priv *priv = phy_package_get_priv(phydev);
> +	int r;
> +
> +	/* configure normal test mode */
> +	r = yt8824_utp_set_template_test_mode
> +		(phydev, MDIO_PMA_10GBT_TESTMODE_NORMAL);

The opening ( should be on the line before. The phydev as well.

> + * yt8824_soft_reset() - called to do PHY software reset
> + * @phydev: a pointer to a &struct phy_device
> + *
> + * Returns: 0 or negative errno code
> + */
> +static int yt8824_soft_reset(struct phy_device *phydev)
> +{
> +	struct yt8824_shared_priv *priv = phy_package_get_priv(phydev);
> +	int ret;
> +
> +	mutex_lock(&priv->shared_lock);
> +	if (priv->package_mode == PHY_INTERFACE_MODE_INTERNAL) {
> +		/* test mode 1 */
> +		ret = yt8824_utp_set_template_test_mode
> +			(phydev, MDIO_PMA_10GBT_TESTMODE_1);

Please look through the code and fix all these problems.

Also, what value does the comment have?


> +		if (ret < 0)
> +			goto retry;
> +		ret = yt8824_utp_softreset_paged(phydev);
> +		if (ret < 0)
> +			goto retry;
> +		/* normal mode */
> +		ret = yt8824_utp_set_template_test_mode
> +			(phydev, MDIO_PMA_10GBT_TESTMODE_NORMAL);

And this comment? You only need comments if the code is not
obvious. The name of the function is often sufficient to explain what
is happening. 

> +	/* pll calibration */
> +	ret = ytphy_write_ext_with_lock(phydev, 0x0001, 0x0003);
> +	if (ret < 0)
> +		goto err_restore;

This comment is useful, it is not possible to know what 0x0001,
0x0003 means.

> +	ret = ytphy_write_ext_with_lock(phydev, 0xa20e, 0x0cba);

and this is just magic. Which is why we recommend #define, not magic
numbers.

> +	/* power down */
> +	ret = phy_modify(phydev, MII_BMCR, BMCR_PDOWN, BMCR_PDOWN);
> +	if (ret < 0)
> +		goto err_restore;
> +	ret = ytphy_write_ext_with_lock(phydev, 0xa20e, 0xcba);

You comment about the obvious power down, but nothing about what this
magic does :-(

	Andrew