Re: [PATCH v2 net-next 03/15] net: mdio: add generic driver for NXP SJA1110 100BASE-TX embedded PHYs
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: 2026-01-22 13:31:31
Also in:
linux-devicetree, lkml
On Thu, Jan 22, 2026 at 02:20:22PM +0200, Andy Shevchenko wrote:
On Thu, Jan 22, 2026 at 12:56:42PM +0200, Vladimir Oltean wrote:quoted
This is the standalone variant of drivers/net/dsa/sja1105/sja1105_mdio.c. Same kind of differences between this driver and the embedded DSA one apply: regmap is being used for register access, and addresses are multiplied by 4 with regmap. In fact this is so generic that there is nothing NXP SJA1110 specific about it at all, and just instantiates mdio-regmap. I decided to name it mdio-regmap-simple.c in the style of drivers/mfd/simple-mfd-i2c.c which has support for various vendor compatible strings....quoted
+#include <linux/module.h> +#include <linux/of_mdio.h> +#include <linux/phy.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/mdio/mdio-regmap.h>...quoted
+static const struct mdio_regmap_simple_data nxp_sja1110_base_tx = { + .valid_addr = 0, + .autoscan = false, +};Actually the { } is enough to initialise that. But if you want to be super explicit... :-) ...
Yes, I guess I do.
quoted
+static int mdio_regmap_simple_probe(struct platform_device *pdev) +{ + const struct mdio_regmap_simple_data *data; + struct mdio_regmap_config config = {}; + struct device *dev = &pdev->dev; + struct regmap *regmap; + struct mii_bus *bus; + + if (!dev->of_node || !dev->parent)dev->of_node check is not needed, see below.
Oh.... this is a bug. dev->of_node should have been propagated to devm_mdio_regmap_register() -> devm_mdiobus_register(), turning it into devm_of_mdiobus_register(). It shows that my SJA1110 testing platform (Bluebox 3) doesn't have the CBTX PHY routed to pinout, since I didn't catch this... I'll fix this for v3.
quoted
+ return -ENODEV; + + regmap = dev_get_regmap(dev->parent, NULL); + if (!regmap) + return -ENODEV; + + data = device_get_match_data(dev); + + config.regmap = regmap; + config.parent = dev; + config.name = dev_name(dev); + /* The resource is optional, provided for finding the registers + * within a device-wide non-MMIO regmap + */ + config.resource = platform_get_resource(pdev, IORESOURCE_REG, 0);quoted
+ if (data) {We may always require data to be present. As you use a default one anyway.quoted
+ config.valid_addr = data->valid_addr; + config.autoscan = data->autoscan; + }And if it is not provided we will have a crash which is fine. It will just point that the code was not ever been run on real HW.
Hmm. This patch is super old, so I'm revisiting it with foreign eyes, same as you. I think the case with .valid_addr = 0 and .autoscan = false will constitute the vast majority of instantiations of this driver. I would like to avoid the proliferation of the same basic config with 100 different names (nxp_sja1110_base_tx, etc). So for v3 I'm planning to: - rename nxp_sja1110_base_tx to mdio_regmap_simple_default_data - delete the "if (data)" conditional and directly assign from device_get_match_data() to the config structure Thanks for taking a look.
quoted
+ return PTR_ERR_OR_ZERO(devm_mdio_regmap_register(dev, &config)); +}...quoted
+static struct platform_driver mdio_regmap_simple_driver = { + .probe = mdio_regmap_simple_probe, + .driver = { + .name = "mdio-regmap-simple", + .of_match_table = mdio_regmap_simple_match, + }, +};quoted
+Unneeded blank line.
Ok.
quoted
+module_platform_driver(mdio_regmap_simple_driver);-- With Best Regards, Andy Shevchenko