Re: [PATCH net-next v22 3/3] net: phy: Add driver for Motorcomm Quad 2.5GbE phy
flat view
From: Andrew Lunn <andrew@lunn.ch>
Date: 2026-09-29 12:18:17
Also in:
linux-devicetree, lkml
quoted hunk ↗ jump to hunk
+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.
quoted hunk ↗ jump to hunk
+ * 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?
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
+ /* 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.
quoted hunk ↗ jump to hunk
+ /* 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