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().

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: use FIELD_PREP()

 MAINTAINERS                          |   1 +
 drivers/net/mdio/Kconfig             |   7 ++
 drivers/net/mdio/Makefile            |   1 +
 drivers/net/mdio/mdio-sja1110-cbt1.c | 179 +++++++++++++++++++++++++++
 4 files changed, 188 insertions(+)
 create mode 100644 drivers/net/mdio/mdio-sja1110-cbt1.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 92768bceb929..d3fec699c577 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18948,6 +18948,7 @@ M:	Vladimir Oltean <olteanv@gmail.com>
 L:	linux-kernel@vger.kernel.org
 S:	Maintained
 F:	drivers/net/dsa/sja1105
+F:	drivers/net/mdio/mdio-sja1110-cbt1.c
 F:	drivers/net/pcs/pcs-xpcs-nxp.c
 
 NXP TDA998X DRM DRIVER
diff --git a/drivers/net/mdio/Kconfig b/drivers/net/mdio/Kconfig
index 44380378911b..9819d1dc18de 100644
--- a/drivers/net/mdio/Kconfig
+++ b/drivers/net/mdio/Kconfig
@@ -136,6 +136,13 @@ config MDIO_MOXART
 	  This driver supports the MDIO interface found in the network
 	  interface units of the MOXA ART SoC
 
+config MDIO_SJA1110_CBT1
+	tristate "NXP SJA1110 100BASE-T1 MDIO bus"
+	help
+	  This driver supports the MDIO controller embedded in the NXP SJA1110
+	  automotive Ethernet switches, which is used to access the internal
+	  100BASE-T1 PHYs over SPI.
+
 config MDIO_OCTEON
 	tristate "Octeon and some ThunderX SOCs MDIO buses"
 	depends on (64BIT && OF_MDIO) || COMPILE_TEST
diff --git a/drivers/net/mdio/Makefile b/drivers/net/mdio/Makefile
index fbec636700e7..9abf20d1b030 100644
--- a/drivers/net/mdio/Makefile
+++ b/drivers/net/mdio/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_MDIO_MVUSB)		+= mdio-mvusb.o
 obj-$(CONFIG_MDIO_OCTEON)		+= mdio-octeon.o
 obj-$(CONFIG_MDIO_REALTEK_RTL9300)	+= mdio-realtek-rtl9300.o
 obj-$(CONFIG_MDIO_REGMAP)		+= mdio-regmap.o
+obj-$(CONFIG_MDIO_SJA1110_CBT1)		+= mdio-sja1110-cbt1.o
 obj-$(CONFIG_MDIO_SUN4I)		+= mdio-sun4i.o
 obj-$(CONFIG_MDIO_THUNDER)		+= mdio-thunder.o
 obj-$(CONFIG_MDIO_XGENE)		+= mdio-xgene.o
diff --git a/drivers/net/mdio/mdio-sja1110-cbt1.c b/drivers/net/mdio/mdio-sja1110-cbt1.c
new file mode 100644
index 000000000000..f170b63c7f69
--- /dev/null
+++ b/drivers/net/mdio/mdio-sja1110-cbt1.c
@@ -0,0 +1,179 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright 2022-2026 NXP
+ *
+ * NXP SJA1110 100BASE-T1 MDIO bus driver
+ */
+#include <linux/module.h>
+#include <linux/of_mdio.h>
+#include <linux/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+struct sja1110_base_t1_private {
+	struct regmap *regmap;
+	struct mii_bus *bus;
+	unsigned int base;
+};
+
+enum sja1110_mdio_opcode {
+	SJA1110_C45_ADDR = 0,
+	SJA1110_C22 = 1,
+	SJA1110_C45_DATA = 2,
+	SJA1110_C45_DATA_AUTOINC = 3,
+};
+
+#define SJA1110_PHYADDR		GENMASK(11, 9)
+#define SJA1110_OPCODE		GENMASK(8, 7)
+#define SJA1110_XAD		GENMASK(6, 2)
+
+static unsigned int sja1110_base_t1_encode_addr(unsigned int phy,
+						enum sja1110_mdio_opcode op,
+						unsigned int xad)
+{
+	return FIELD_PREP(SJA1110_PHYADDR, phy) |
+	       FIELD_PREP(SJA1110_OPCODE, op) |
+	       FIELD_PREP(SJA1110_XAD, xad);
+}
+
+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);
+
+	err = regmap_read(regmap, priv->base + addr, &val);
+	if (err)
+		return err;
+
+	return val & 0xffff;
+}
+
+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;
+}
+
+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);
+}
+
+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);
+}
+
+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)
+		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);
+	if (err)
+		goto err_free_bus;
+
+	priv->bus = bus;
+	platform_set_drvdata(pdev, priv);
+
+	return 0;
+
+err_free_bus:
+	mdiobus_free(bus);
+
+	return err;
+}
+
+static void sja1110_base_t1_mdio_remove(struct platform_device *pdev)
+{
+	struct sja1110_base_t1_private *priv = platform_get_drvdata(pdev);
+
+	mdiobus_unregister(priv->bus);
+	mdiobus_free(priv->bus);
+}
+
+static const struct of_device_id sja1110_base_t1_mdio_match[] = {
+	{ .compatible = "nxp,sja1110-base-t1-mdio", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, sja1110_base_t1_mdio_match);
+
+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,
+	},
+};
+
+module_platform_driver(sja1110_base_t1_mdio_driver);
+
+MODULE_DESCRIPTION("NXP SJA1110 100BASE-T1 MDIO bus driver");
+MODULE_AUTHOR("Vladimir Oltean <vladimir.oltean@nxp.com>");
+MODULE_LICENSE("GPL");
-- 
2.34.1

On Thu, Jan 22, 2026 at 12:56:41PM +0200, Vladimir Oltean wrote:
> 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().

...

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

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

lower_16_bits() from wordpart.h?

> +}

...

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

> +}

...

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

> +}

...

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

> +}

...

> +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)?

> +		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);
> +	if (err)
> +		goto err_free_bus;
> +
> +	priv->bus = bus;
> +	platform_set_drvdata(pdev, priv);
> +
> +	return 0;
> +
> +err_free_bus:
> +	mdiobus_free(bus);
> +
> +	return err;
> +}

...

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

Inner comma is redundant.

> +	{},

Terminator is terminator, trailing comma is confusing here.

> +};

...

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

> +

Redundant blank line.

> +module_platform_driver(sja1110_base_t1_mdio_driver);

-- 
With Best Regards,
Andy Shevchenko

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:
> > 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().
> 
> ...
> 
> > +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.

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

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

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

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

> > +	if (err)
> > +		goto err_free_bus;
> > +
> > +	priv->bus = bus;
> > +	platform_set_drvdata(pdev, priv);
> > +
> > +	return 0;
> > +
> > +err_free_bus:
> > +	mdiobus_free(bus);
> > +
> > +	return err;
> > +}
> 
> ...
> 
> > +static const struct of_device_id sja1110_base_t1_mdio_match[] = {
> > +	{ .compatible = "nxp,sja1110-base-t1-mdio", },
> 
> Inner comma is redundant.

Ok.

> > +	{},
> 
> Terminator is terminator, trailing comma is confusing here.

Ok.

> > +};
> 
> ...
> 
> > +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,
> > +	},
> > +};
> 
> > +
> 
> Redundant blank line.

Ok.

> > +module_platform_driver(sja1110_base_t1_mdio_driver);
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>

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?

...

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

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

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

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

On Fri, Jan 23, 2026 at 12:10:03AM +0200, Vladimir Oltean wrote:
> 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:

...

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

dereferencing != use

> > > > > +		return -ENODEV;

What I meant is to avoid accessing of_node directly, use APIs: dev_of_node().

...

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

I see, so perhaps in the future we will see this snipped to be converted to
fwnode_mdiobus_register() then. Thank you for clarification.

> > > > > +	if (err)
> > > > > +		goto err_free_bus;

-- 
With Best Regards,
Andy Shevchenko

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

It could be historical, from before their were C22 and C45 operations.

Previously, both transaction types were passed through one call. The
MSB indicated if C45 should be performed, and there were some macros
to split the number part into MMD and register.

With the current implementation, it should be O.K. to add a range
change.

	Andrew