# Re: \[PATCH v2 net-next 02/15\] net: mdio: add driver for NXP SJA1110 100BASE-T1 embedded PHYs - From: Vladimir Oltean \ - Date: 2026-01-22 22:10:13 - List: netdev - Page: https://ratatoskr.run/netdev/2026/01/3383166 - Thread: https://ratatoskr.run/netdev/2026/01/3381930/t.md ```text On Thu, Jan 22, 2026 at 04:44:47PM +0200, Andy Shevchenko wrote: > On Thu, Jan 22, 2026 at 02:47:08PM +0200, Vladimir Oltean wrote: > > On Thu, Jan 22, 2026 at 02:12:21PM +0200, Andy Shevchenko wrote: > > > On Thu, Jan 22, 2026 at 12:56:41PM +0200, Vladimir Oltean wrote: > > ... > > > > > +static int sja1110_base_t1_mdio_read_c22(struct mii_bus *bus, int phy, int reg) > > > > +{ > > > > + struct sja1110_base_t1_private *priv = bus->priv; > > > > + struct regmap *regmap = priv->regmap; > > > > + unsigned int addr, val; > > > > + int err; > > > > + > > > > + addr = sja1110_base_t1_encode_addr(phy, SJA1110_C22, reg & 0x1f); > > > > > > GENMASK() ? Or do you have already a defined mask for this? > > > > Hmm, I can't find a definition for this. In the MDIO world it is > > "well known" that clause 22 offers a 5-bit register address space. > > So the 0x1f number doesn't seem too magical to me. > > > > But I think my assumptions date since before the MDIO bus API was split > > between separate clause 22 and clause 45 reads/writes. I don't know > > whether masking reg & 0x1f is the best practice. I'm surprised that > > __mdiobus_read() doesn't enforce a limit on "regnum", and I don't see > > other MDIO bus drivers explicitly C22 registers >= 32. I really don't > > know what is the best practice. > > Me neither. At bare minimum to check / perform two things: > - make sure this approach is consistent across the kernel > - define the magic with meaningful name > > Maybe (assuming second one is done) fix the rest in the future > via some helper function? I wasn't prepared to go down this rabbit hole, but it turns out that the __mdiobus_read() and __mdiobus_write() functions do support regnum >= 32. I don't have time to investigate why that is, plus the fact that the majority of drivers don't reject such register addresses but truncate them to 5 bits. In the next version I will: - add a "#define MII_BUS_MAX_C22_REGNUM 0x1f" in include/linux/phy.h and use it to reject registers out of range in mdio-sja1110-cbt1.c. - allow registers >= 32 in mdio-regmap.c and just disallow what exceeds the passed resource->end. Although standard MDIO framing has 5-bit register addresses, mdio-regmap.c represents a non-standard linear mapping of those registers inside an address space, with no such inherent limitation. Besides, as mentioned in commit f3b766d98131 ("net: phy: add basic driver for NXP CBTX PHY"), the NXP CBTX PHY register map extends well beyond the standard 32 registers, and I was wondering how to expose the rest. Turns out there isn't any problem as long as the PHY and its MDIO controller driver are paired together. > ... > > > > > +static int sja1110_base_t1_mdio_probe(struct platform_device *pdev) > > > > +{ > > > > + struct sja1110_base_t1_private *priv; > > > > + struct device *dev = &pdev->dev; > > > > + struct regmap *regmap; > > > > + struct resource *res; > > > > + struct mii_bus *bus; > > > > + int err; > > > > > > > + if (!dev->of_node || !dev->parent) > > > > > > Can we avoid dereferencing? And perhaps dev_fwnode(dev)? > > > > Avoid dereferencing what? > > of_node Why? The driver is useless when bound to a device without an of_node. of_mdiobus_register() will fall back gracefully to __mdiobus_register(), and still technically get registered, but its child PHYs will be inaccessible through phandles. > > > > + return -ENODEV; > > > > + > > > > + regmap = dev_get_regmap(dev->parent, NULL); > > > > + if (!regmap) > > > > + return -ENODEV; > > > > + > > > > + bus = mdiobus_alloc_size(sizeof(*priv)); > > > > + if (!bus) > > > > + return -ENOMEM; > > > > + > > > > + bus->name = "SJA1110 100base-T1 MDIO bus"; > > > > + snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev)); > > > > + bus->read = sja1110_base_t1_mdio_read_c22; > > > > + bus->write = sja1110_base_t1_mdio_write_c22; > > > > + bus->read_c45 = sja1110_base_t1_mdio_read_c45; > > > > + bus->write_c45 = sja1110_base_t1_mdio_write_c45; > > > > + bus->parent = dev; > > > > + priv = bus->priv; > > > > + priv->regmap = regmap; > > > > + > > > > + res = platform_get_resource(pdev, IORESOURCE_REG, 0); > > > > + if (res) > > > > + priv->base = res->start; > > > > + > > > > + err = of_mdiobus_register(bus, dev->of_node); > > > > Why would I use dev_fwnode() if I need to pass it as OF to > > of_mdiobus_register() here? > > dev_of_node() then. Wondering if we can use fwnode_mdiobus_register_phy() here > (I remember that OF/fwnode code in MDIO/PHY is not trivial, but I don't know > all the details). fwnode_mdiobus_register_phy() shall be read as: "hey MDIO bus, please register a PHY for this fwnode!" of_mdiobus_register() shall be read as: "I have this mii_bus structure and I want it registered as an active MDIO bus, associated with this OF node". So the two do not serve the same purpose; one is not the more generic variant of the other. There is no fwnode variant of of_mdiobus_register(). Perhaps this snippet from drivers/net/ethernet/marvell/mvmdio.c can clarify: /* For the platforms not supporting DT/ACPI fall-back * to mdiobus_register via of_mdiobus_register. */ if (is_acpi_node(pdev->dev.fwnode)) ret = acpi_mdiobus_register(bus, pdev->dev.fwnode); else ret = of_mdiobus_register(bus, pdev->dev.of_node); Out of the two API functions, I used OF because that's what I need to support. > > > > + if (err) > > > > + goto err_free_bus; > > > > + > > > > + priv->bus = bus; > > > > + platform_set_drvdata(pdev, priv); > > > > + > > > > + return 0; > > > > + > > > > +err_free_bus: > > > > + mdiobus_free(bus); > > > > + > > > > + return err; > > > > +} > > -- > With Best Regards, > Andy Shevchenko > > ```