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:

quoted

This driver is the standalone variant of drivers/net/dsa/sja1105/sja1105_mdio.c.
In terms of differences:

- this one uses regmaps provided by the parent as a method to abstract
  away the sja1105_xfer_u32() calls for register access
- the driver prefix has been changed from sja1105 to sja1110 (this MDIO
  controller is not present on the older SJA1105 family)
- in the sja1105 driver, each memory word has 32 bits, so addresses as
  seen by regmap need to be multiplied by 4. This affects what
  sja1110_base_t1_encode_addr() returns, and is different compared to
  sja1105_base_t1_encode_addr().

...

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.

quoted

+	err = regmap_read(regmap, priv->base + addr, &val);
+	if (err)
+		return err;
+
+	return val & 0xffff;

lower_16_bits() from wordpart.h?

Ok, let's say so.

quoted

+}

...

quoted

+static int sja1110_base_t1_mdio_read_c45(struct mii_bus *bus, int phy,
+					 int mmd, 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_C45_ADDR, mmd);
+	err = regmap_write(regmap, priv->base + addr, reg);
+	if (err)
+		return err;
+
+	addr = sja1110_base_t1_encode_addr(phy, SJA1110_C45_DATA, mmd);
+	err = regmap_read(regmap, priv->base + addr, &val);
+	if (err)
+		return err;
+
+	return val & 0xffff;

Ditto.

quoted

+}

...

quoted

+static int sja1110_base_t1_mdio_write_c22(struct mii_bus *bus, int phy, int reg,
+					  u16 val)
+{
+	struct sja1110_base_t1_private *priv = bus->priv;
+	struct regmap *regmap = priv->regmap;
+	unsigned int addr;
+
+	addr = sja1110_base_t1_encode_addr(phy, SJA1110_C22, reg & 0x1f);
+	return regmap_write(regmap, priv->base + addr, val & 0xffff);

val is already u16.

Ok.

quoted

+}

...

quoted

+static int sja1110_base_t1_mdio_write_c45(struct mii_bus *bus, int phy,
+					  int mmd, int reg, u16 val)
+{
+	struct sja1110_base_t1_private *priv = bus->priv;
+	struct regmap *regmap = priv->regmap;
+	unsigned int addr;
+	int err;
+
+	addr = sja1110_base_t1_encode_addr(phy, SJA1110_C45_ADDR, mmd);
+	err = regmap_write(regmap, priv->base + addr, reg);
+	if (err)
+		return err;
+
+	addr = sja1110_base_t1_encode_addr(phy, SJA1110_C45_DATA, mmd);
+	return regmap_write(regmap, priv->base + addr, val & 0xffff);

Ditto.

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?

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?

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

...

quoted

+static const struct of_device_id sja1110_base_t1_mdio_match[] = {
+	{ .compatible = "nxp,sja1110-base-t1-mdio", },

Inner comma is redundant.

Ok.

quoted

+	{},

Terminator is terminator, trailing comma is confusing here.

Ok.

quoted

+};

...

quoted

+static struct platform_driver sja1110_base_t1_mdio_driver = {
+	.probe = sja1110_base_t1_mdio_probe,
+	.remove = sja1110_base_t1_mdio_remove,
+	.driver = {
+		.name = "sja1110-base-t1-mdio",
+		.of_match_table = sja1110_base_t1_mdio_match,
+	},
+};

quoted

+

Redundant blank line.

Ok.

quoted

+module_platform_driver(sja1110_base_t1_mdio_driver);

-- 
With Best Regards,
Andy Shevchenko