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

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

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