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:

quoted

On Thu, Jan 22, 2026 at 02:12:21PM +0200, Andy Shevchenko wrote:

quoted

On Thu, Jan 22, 2026 at 12:56:41PM +0200, Vladimir Oltean wrote:

...

quoted

quoted

quoted

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

...

quoted

quoted

quoted

+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;

quoted

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

quoted

quoted

quoted

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

quoted

quoted

quoted

+	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