| Message ID | 20220211051140.3785-1-luizluca@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: dsa: OF-ware slave_mii_bus (RFC) | expand |
Luiz Angelo Daros de Luca <luizluca@gmail.com> writes: > If found, register the DSA internal allocated slave_mii_bus with an OF > "mdio" child object. It can save some drivers from creating their > internal MDIO bus. > > Some doubts: > 1) is there any special reason for the absence of a "device_node dn" in > dsa_switch? Is there any constraint on where to place it? > 2) Is looking for "mdio" the best solution? > > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > --- > include/net/dsa.h | 2 ++ > net/dsa/dsa2.c | 8 +++++++- > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/include/net/dsa.h b/include/net/dsa.h > index b688ced04b0e..c01c059c5335 100644 > --- a/include/net/dsa.h > +++ b/include/net/dsa.h > @@ -421,6 +421,8 @@ struct dsa_switch { > u32 phys_mii_mask; > struct mii_bus *slave_mii_bus; > > + struct device_node *dn; > + > /* Ageing Time limits in msecs */ > unsigned int ageing_time_min; > unsigned int ageing_time_max; > diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c > index 909b045c9b11..db1aeb6b8352 100644 > --- a/net/dsa/dsa2.c > +++ b/net/dsa/dsa2.c > @@ -13,6 +13,7 @@ > #include <linux/slab.h> > #include <linux/rtnetlink.h> > #include <linux/of.h> > +#include <linux/of_mdio.h> > #include <linux/of_net.h> > #include <net/devlink.h> > #include <net/sch_generic.h> > @@ -869,6 +870,7 @@ static int dsa_switch_setup_tag_protocol(struct dsa_switch *ds) > static int dsa_switch_setup(struct dsa_switch *ds) > { > struct dsa_devlink_priv *dl_priv; > + struct device_node *dn; > struct dsa_port *dp; > int err; > > @@ -924,7 +926,9 @@ static int dsa_switch_setup(struct dsa_switch *ds) > > dsa_slave_mii_bus_init(ds); > > - err = mdiobus_register(ds->slave_mii_bus); > + dn = of_get_child_by_name(ds->dn, "mdio"); of_node_put(dn) after registration? Or else who will put it? > + > + err = of_mdiobus_register(ds->slave_mii_bus, dn); > if (err < 0) > goto free_slave_mii_bus; > } > @@ -1610,6 +1614,8 @@ static int dsa_switch_parse_of(struct dsa_switch *ds, struct device_node *dn) > { > int err; > > + ds->dn = dn; > + > err = dsa_switch_parse_member_of(ds, dn); > if (err) > return err;
> > @@ -924,7 +926,9 @@ static int dsa_switch_setup(struct dsa_switch *ds) > > > > dsa_slave_mii_bus_init(ds); > > > > - err = mdiobus_register(ds->slave_mii_bus); > > + dn = of_get_child_by_name(ds->dn, "mdio"); > > of_node_put(dn) after registration? Or else who will put it? Thanks Alvin, I'll need some help here. Is it expected for every code that receives a device_node and keeps a reference to it to call of_node_get()? I checked of_get_child_by_name and it seems it just parses the device_node but does not save it. I'll put it after the registration. I got confused looking at the dsa_port->dn usage. It is initialized at dsa_switch_parse_ports_of() with: for_each_available_child_of_node() { ... dsa_port_parse_of(dp, port); } dsa_port_parse_of(dp, dn) { dp->dn = dn; ... } But nobody calls of_node_put(port) when there is no error. Should it have used a of_node_put(port) after dsa_port_parse_of() is called and 'dp->dn = of_node_get(dn)' with a corresponding of_node_put() when the port is destroyed? And I dropped "ds->dn" as I can use "ds->dev->of_node". The code indicates it can be null but it looks like all methods I use seem to play nice with null values. It's getting even smaller: #include <linux/slab.h> #include <linux/rtnetlink.h> #include <linux/of.h> +#include <linux/of_mdio.h> #include <linux/of_net.h> #include <net/devlink.h> #include <net/sch_generic.h> @@ -869,6 +870,7 @@ static int dsa_switch_setup_tag_protocol(struct dsa_switch *ds) static int dsa_switch_setup(struct dsa_switch *ds) { struct dsa_devlink_priv *dl_priv; + struct device_node *dn; struct dsa_port *dp; int err; @@ -924,7 +926,10 @@ static int dsa_switch_setup(struct dsa_switch *ds) dsa_slave_mii_bus_init(ds); - err = mdiobus_register(ds->slave_mii_bus); + dn = of_get_child_by_name(ds->dev->of_node, "mdio"); + + err = of_mdiobus_register(ds->slave_mii_bus, dn); + of_node_put(dn); if (err < 0) goto free_slave_mii_bus; } Regards, Luiz
diff --git a/include/net/dsa.h b/include/net/dsa.h index b688ced04b0e..c01c059c5335 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -421,6 +421,8 @@ struct dsa_switch { u32 phys_mii_mask; struct mii_bus *slave_mii_bus; + struct device_node *dn; + /* Ageing Time limits in msecs */ unsigned int ageing_time_min; unsigned int ageing_time_max; diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c index 909b045c9b11..db1aeb6b8352 100644 --- a/net/dsa/dsa2.c +++ b/net/dsa/dsa2.c @@ -13,6 +13,7 @@ #include <linux/slab.h> #include <linux/rtnetlink.h> #include <linux/of.h> +#include <linux/of_mdio.h> #include <linux/of_net.h> #include <net/devlink.h> #include <net/sch_generic.h> @@ -869,6 +870,7 @@ static int dsa_switch_setup_tag_protocol(struct dsa_switch *ds) static int dsa_switch_setup(struct dsa_switch *ds) { struct dsa_devlink_priv *dl_priv; + struct device_node *dn; struct dsa_port *dp; int err; @@ -924,7 +926,9 @@ static int dsa_switch_setup(struct dsa_switch *ds) dsa_slave_mii_bus_init(ds); - err = mdiobus_register(ds->slave_mii_bus); + dn = of_get_child_by_name(ds->dn, "mdio"); + + err = of_mdiobus_register(ds->slave_mii_bus, dn); if (err < 0) goto free_slave_mii_bus; } @@ -1610,6 +1614,8 @@ static int dsa_switch_parse_of(struct dsa_switch *ds, struct device_node *dn) { int err; + ds->dn = dn; + err = dsa_switch_parse_member_of(ds, dn); if (err) return err;
If found, register the DSA internal allocated slave_mii_bus with an OF "mdio" child object. It can save some drivers from creating their internal MDIO bus. Some doubts: 1) is there any special reason for the absence of a "device_node dn" in dsa_switch? Is there any constraint on where to place it? 2) Is looking for "mdio" the best solution? Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> --- include/net/dsa.h | 2 ++ net/dsa/dsa2.c | 8 +++++++- 2 files changed, 9 insertions(+), 1 deletion(-)