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

On 9/29/26 20:18, Andrew Lunn wrote:

quoted

+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

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

quoted

+		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

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

quoted

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

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

quoted

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