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