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

[Kyle Switch]

On 9/29/26 20:18, Andrew Lunn wrote:
> > +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?
okay, meaningless comments will be removed.
> > +		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 :-(
okay, magic numbers will be replaced with meaningful definitions.
> 	Andrew