Re: [PATCH v2] net: phy: add Maxlinear GPY115/21x/24x driver

Hi,

On Thu, Jun 03, 2021 at 03:34:38PM +0800, Xu Liang wrote:

This should use tab(s) to align indentation rather than spaces.

+static int gpy_config_init(struct phy_device *phydev)
+{
+	int ret, fw_ver;
+
+	/* Show GPY PHY FW version in dmesg */
+	fw_ver = phy_read(phydev, PHY_FWV);
+	if (fw_ver < 0)
+		return fw_ver;
+
+	phydev_info(phydev, "Firmware Version: 0x%04X (%s)\n", fw_ver,
+		    (fw_ver & PHY_FWV_REL_MASK) ? "release" : "test");

Does this need to print the firmware version each time config_init()
is called? Is it likely to change beyond? Would it be more sensible
to print it in the probe() method?

+static int gpy_config_aneg(struct phy_device *phydev)
+{
+	bool changed = false;
+	u32 adv;
+	int ret;
+
+	if (phydev->autoneg == AUTONEG_DISABLE) {
+		return phydev->duplex != DUPLEX_FULL
+			? genphy_setup_forced(phydev)
+			: genphy_c45_pma_setup_forced(phydev);

I think this needs a comment to describe what is going on here to
explain why the duplex setting influences whether we program the PHY
via C22 or C45.

+static void gpy_update_interface(struct phy_device *phydev)
+{
+	int ret;
+
+	/* Interface mode is fixed for USXGMII and integrated PHY */
+	if (phydev->interface == PHY_INTERFACE_MODE_USXGMII ||
+	    phydev->interface == PHY_INTERFACE_MODE_INTERNAL)
+		return;
+
+	/* Automatically switch SERDES interface between SGMII and 2500-BaseX
+	 * according to speed. Disable ANEG in 2500-BaseX mode.
+	 */
+	switch (phydev->speed) {
+	case SPEED_2500:
+		phydev->interface = PHY_INTERFACE_MODE_2500BASEX;
+		ret = phy_modify_mmd_changed(phydev, MDIO_MMD_VEND1,
+					     VSPEC1_SGMII_CTRL,
+					     VSPEC1_SGMII_CTRL_ANEN, 0);

Do you need to know if the bit was changed? It doesn't appear so, so
please consider using phy_modify_mmd() here and in the other part of
this switch block.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!