On Thu, Dec 28, 2023 at 07:58:13PM +0300, Arınç ÜNAL wrote:
The implementation below follows this logic:
No switch MDIO bus defined: Register the MDIO bus, set the interrupts for
PHYs if "interrupt-controller" is defined at the switch node.
Switch MDIO bus defined: Register the MDIO bus, set the interrupts for PHYs
if ["interrupt-controller" is defined at the switch node and "interrupts"
is defined at the PHY nodes under the switch MDIO bus node].
I think this approach fits your description so I'd like to agree that this
should be the way for all DSA subdrivers. Please let me know what you
think.
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 391c4dbdff42..bbd230a73ead 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2155,15 +2155,21 @@ mt7530_setup_mdio(struct mt7530_priv *priv)
{
struct dsa_switch *ds = priv->ds;
struct device *dev = priv->dev;
+ struct device_node *np, *mnp;
struct mii_bus *bus;
static int idx;
int ret;
+ np = priv->dev->of_node;
+ mnp = of_get_child_by_name(np, "mdio");
+
Empty line between variable declarations and code. Or you can initialize
them as part of their declaration, but you need to stick to the "longest
line first" rule.
Also, it would be good to also check of_device_is_available(mnp).
+ ds->user_mii_bus = bus;
+
bus->priv = priv;
bus->name = KBUILD_MODNAME "-mii";
snprintf(bus->id, MII_BUS_ID_SIZE, KBUILD_MODNAME "-%d", idx++);
@@ -2174,10 +2180,11 @@ mt7530_setup_mdio(struct mt7530_priv *priv)
bus->parent = dev;
bus->phy_mask = ~ds->phys_mii_mask;
- if (priv->irq)
+ if (priv->irq && mnp == NULL)
mt7530_setup_mdio_irq(priv);
- ret = devm_mdiobus_register(dev, bus);
+ ret = devm_of_mdiobus_register(dev, bus, mnp);
+ of_node_put(mnp);
This is going to be interesting. There isn't really a correct way to
manage the reference to "mnp", as far as I can tell. Normally, it should
have been possible to release the reference as you did. But you need
something along the lines of what Luiz/Russell have been discussing
here:
https://lore.kernel.org/netdev/20231220045228.27079-2-luizluca@xxxxxxxxx/
In any case, the devres variant of of_mdiobus_register() seems incompatible
with the mt7530 driver owning the "mnp" node for any longer than this,
because it has no hook to call of_node_put() once the MDIO bus is unregistered.
if (ret) {
dev_err(dev, "failed to register MDIO bus: %d\n", ret);
if (priv->irq)
With this device tree:
switch {
interrupt-controller;
}
[ 1.420534] mt7530-mdio mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=17)
[ 1.433224] mt7530-mdio mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7530 PHY] (irq=18)
[ 1.445338] mt7530-mdio mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7530 PHY] (irq=19)
[ 1.457472] mt7530-mdio mdio-bus:1f lan4 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7530 PHY] (irq=20)
[ 1.469587] mt7530-mdio mdio-bus:1f wan (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7530 PHY] (irq=21)
With this device tree:
switch {
interrupt-controller;
mdio {
phy {
reg = <0>;
}
}
}
[ 1.413101] mt7530-mdio mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=POLL)
[ 1.429954] mt7530-mdio mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7530 PHY] (irq=POLL)
[ 1.443704] mt7530-mdio mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7530 PHY] (irq=POLL)
[ 1.455876] mt7530-mdio mdio-bus:1f lan4 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7530 PHY] (irq=POLL)
[ 1.468079] mt7530-mdio mdio-bus:1f wan (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7530 PHY] (irq=POLL)
With this device tree:
switch {
interrupt-controller;
mdio {
phy {
reg = <0>;
interrupts = <0>;
}
}
}
[ 1.420534] mt7530-mdio mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=17)
[ 1.433224] mt7530-mdio mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7530 PHY] (irq=18)
[ 1.445338] mt7530-mdio mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7530 PHY] (irq=19)
[ 1.457472] mt7530-mdio mdio-bus:1f lan4 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7530 PHY] (irq=20)
[ 1.469587] mt7530-mdio mdio-bus:1f wan (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7530 PHY] (irq=21)
Looks sane.
FWIW, I found Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
where internal PHYs don't have an 'interrupts' property, yet they are
probably still expected to use interrupts - according to ksz_irq_phy_setup().
Anyway, what's done is done, but I still don't see the point of making
the binding much more flexible than it needs to be.