Thread (48 messages) 48 messages, 6 authors, 2026-01-29

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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help