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

On 5/6/2021 12:24 am, Florian Fainelli wrote:

quoted

+/* PHY ID */
+#define PHY_ID_GPY 0x67C9DC00
+#define PHY_ID_MASK GENMASK(31, 4)

Consider initializing your phy_driver with PHY_ID_MATCH_MODEL() which
would take care of populating the mask accordingly.

Thanks, I will update.

quoted

+
+static int gpy_read_abilities(struct phy_device *phydev)
+{
+ int ret;
+
+ ret = genphy_read_abilities(phydev);
+ if (ret < 0)
+ return ret;
+
+ /* Detect 2.5G/5G support. */
+ ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_STAT2);
+ if (ret < 0)
+ return ret;
+ if (!(ret & MDIO_PMA_STAT2_EXTABLE))
+ return 0;
+
+ ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_EXTABLE);
+ if (ret < 0)
+ return ret;
+ if (!(ret & MDIO_PMA_EXTABLE_NBT))
+ return 0;
+
+ ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_NG_EXTABLE);
+ if (ret < 0)
+ return ret;
+
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
+ phydev->supported,
+ ret & MDIO_PMA_NG_EXTABLE_2_5GBT);
+
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
+ phydev->supported,
+ ret & MDIO_PMA_NG_EXTABLE_5GBT);

This does not access vendor specific registers, should not this be part
of the standard genphy_read_abilities() or moved to a helper?

genphy_read_abilities does not cover 2.5G.

genphy_c45_pma_read_abilities checks C45 ids and this check fail if 
is_c45 is not set.

Mix of C22 and C45 is handled here, as our device support C22 with 
extension of C45.

quoted

+
+ return 0;
+}
+
+static int gpy_config_init(struct phy_device *phydev)
+{
+ int ret;
+
+ /* Mask all interrupts */
+ ret = phy_write(phydev, PHY_IMASK, 0);
+ if (ret)
+ return ret;
+
+ /* Clear all pending interrupts */
+ ret = phy_read(phydev, PHY_ISTAT);
+ return ret < 0 ? ret : 0;

You can certainly simplify this to:

return phy_read(phydev, PHY_ISTAT);

I'm thinking to clearly differentiate negative, 0, and positive.

I had experience that a positive value is treated as error sometimes.

[snip]

quoted

+
+MODULE_DESCRIPTION("Maxlinear Ethernet GPY Driver");
+MODULE_AUTHOR("Maxlinear Corporation");

The author is not usually a company but an individual working on behalf
of that company.
-- 
Florian

Ok, I will update.