* [PATCH v1] net: ag71xx: fix mdio subnode support @ 2019-09-30 9:33 Oleksij Rempel 2019-09-30 13:42 ` Andrew Lunn 0 siblings, 1 reply; 5+ messages in thread From: Oleksij Rempel @ 2019-09-30 9:33 UTC (permalink / raw) To: Jay Cliburn, Chris Snook Cc: Oleksij Rempel, Pengutronix Kernel Team, David S. Miller, netdev, linux-kernel The driver was working with fixed phy without any noticeable issues. This bug was uncovered by introducing dsa ar9331-switch driver. Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> --- drivers/net/ethernet/atheros/ag71xx.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c index 6703960c7cf5..d1101eea15c2 100644 --- a/drivers/net/ethernet/atheros/ag71xx.c +++ b/drivers/net/ethernet/atheros/ag71xx.c @@ -526,7 +526,7 @@ static int ag71xx_mdio_probe(struct ag71xx *ag) struct device *dev = &ag->pdev->dev; struct net_device *ndev = ag->ndev; static struct mii_bus *mii_bus; - struct device_node *np; + struct device_node *np, *mnp; int err; np = dev->of_node; @@ -571,7 +571,9 @@ static int ag71xx_mdio_probe(struct ag71xx *ag) msleep(200); } - err = of_mdiobus_register(mii_bus, np); + mnp = of_get_child_by_name(np, "mdio"); + err = of_mdiobus_register(mii_bus, mnp); + of_node_put(mnp); if (err) goto mdio_err_put_clk; -- 2.23.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v1] net: ag71xx: fix mdio subnode support 2019-09-30 9:33 [PATCH v1] net: ag71xx: fix mdio subnode support Oleksij Rempel @ 2019-09-30 13:42 ` Andrew Lunn 2019-09-30 14:29 ` Oleksij Rempel 0 siblings, 1 reply; 5+ messages in thread From: Andrew Lunn @ 2019-09-30 13:42 UTC (permalink / raw) To: Oleksij Rempel Cc: Jay Cliburn, Chris Snook, Pengutronix Kernel Team, David S. Miller, netdev, linux-kernel On Mon, Sep 30, 2019 at 11:33:10AM +0200, Oleksij Rempel wrote: > The driver was working with fixed phy without any noticeable issues. This bug > was uncovered by introducing dsa ar9331-switch driver. > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- > drivers/net/ethernet/atheros/ag71xx.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c > index 6703960c7cf5..d1101eea15c2 100644 > --- a/drivers/net/ethernet/atheros/ag71xx.c > +++ b/drivers/net/ethernet/atheros/ag71xx.c > @@ -526,7 +526,7 @@ static int ag71xx_mdio_probe(struct ag71xx *ag) > struct device *dev = &ag->pdev->dev; > struct net_device *ndev = ag->ndev; > static struct mii_bus *mii_bus; > - struct device_node *np; > + struct device_node *np, *mnp; > int err; > > np = dev->of_node; > @@ -571,7 +571,9 @@ static int ag71xx_mdio_probe(struct ag71xx *ag) > msleep(200); > } > > - err = of_mdiobus_register(mii_bus, np); > + mnp = of_get_child_by_name(np, "mdio"); > + err = of_mdiobus_register(mii_bus, mnp); > + of_node_put(mnp); > if (err) > goto mdio_err_put_clk; Hi Oleksij You need to keep backwards compatibility here. If you find an mdio node, use it, but if not, you need to still register np. This is also extending the driver binding, so you need to update the binding documentation. Andrew ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1] net: ag71xx: fix mdio subnode support 2019-09-30 13:42 ` Andrew Lunn @ 2019-09-30 14:29 ` Oleksij Rempel 2019-09-30 16:25 ` Andrew Lunn 0 siblings, 1 reply; 5+ messages in thread From: Oleksij Rempel @ 2019-09-30 14:29 UTC (permalink / raw) To: Andrew Lunn Cc: Jay Cliburn, Chris Snook, linux-kernel, Pengutronix Kernel Team, netdev, David S. Miller On Mon, Sep 30, 2019 at 03:42:09PM +0200, Andrew Lunn wrote: > On Mon, Sep 30, 2019 at 11:33:10AM +0200, Oleksij Rempel wrote: > > The driver was working with fixed phy without any noticeable issues. This bug > > was uncovered by introducing dsa ar9331-switch driver. > > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > > --- > > drivers/net/ethernet/atheros/ag71xx.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c > > index 6703960c7cf5..d1101eea15c2 100644 > > --- a/drivers/net/ethernet/atheros/ag71xx.c > > +++ b/drivers/net/ethernet/atheros/ag71xx.c > > @@ -526,7 +526,7 @@ static int ag71xx_mdio_probe(struct ag71xx *ag) > > struct device *dev = &ag->pdev->dev; > > struct net_device *ndev = ag->ndev; > > static struct mii_bus *mii_bus; > > - struct device_node *np; > > + struct device_node *np, *mnp; > > int err; > > > > np = dev->of_node; > > @@ -571,7 +571,9 @@ static int ag71xx_mdio_probe(struct ag71xx *ag) > > msleep(200); > > } > > > > - err = of_mdiobus_register(mii_bus, np); > > + mnp = of_get_child_by_name(np, "mdio"); > > + err = of_mdiobus_register(mii_bus, mnp); > > + of_node_put(mnp); > > if (err) > > goto mdio_err_put_clk; > > Hi Oleksij > > You need to keep backwards compatibility here. If you find an mdio > node, use it, but if not, you need to still register np. > > This is also extending the driver binding, so you need to update the > binding documentation. Hi Andrew, Normally i would agree. But in this case: - this driver is freshly added to the kernel and is different to OpenWrt implementation any way. No users from this side. - Devicetree binding says: Documentation/devicetree/bindings/net/qca,ar71xx.txt |Optional properties: |- phy-handle : phandle to the PHY device connected to this device. |- fixed-link : Assume a fixed link. See fixed-link.txt in the same directory. | Use instead of phy-handle. | |Optional subnodes: |- mdio : specifies the mdio bus, used as a container for phy nodes | according to phy.txt in the same directory So, it is driver bug ...ooOO (my personal bug :D) I would say: if some one already started to use this driver, didn't used documentation and didn't reported this bug, can't really blame any one here :) Regards, Oleksij -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1] net: ag71xx: fix mdio subnode support 2019-09-30 14:29 ` Oleksij Rempel @ 2019-09-30 16:25 ` Andrew Lunn 2019-09-30 17:52 ` Oleksij Rempel 0 siblings, 1 reply; 5+ messages in thread From: Andrew Lunn @ 2019-09-30 16:25 UTC (permalink / raw) To: Oleksij Rempel Cc: Jay Cliburn, Chris Snook, linux-kernel, Pengutronix Kernel Team, netdev, David S. Miller On Mon, Sep 30, 2019 at 04:29:07PM +0200, Oleksij Rempel wrote: > On Mon, Sep 30, 2019 at 03:42:09PM +0200, Andrew Lunn wrote: > > On Mon, Sep 30, 2019 at 11:33:10AM +0200, Oleksij Rempel wrote: > > > The driver was working with fixed phy without any noticeable issues. This bug > > > was uncovered by introducing dsa ar9331-switch driver. > > > > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > > > --- > > > drivers/net/ethernet/atheros/ag71xx.c | 6 ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c > > > index 6703960c7cf5..d1101eea15c2 100644 > > > --- a/drivers/net/ethernet/atheros/ag71xx.c > > > +++ b/drivers/net/ethernet/atheros/ag71xx.c > > > @@ -526,7 +526,7 @@ static int ag71xx_mdio_probe(struct ag71xx *ag) > > > struct device *dev = &ag->pdev->dev; > > > struct net_device *ndev = ag->ndev; > > > static struct mii_bus *mii_bus; > > > - struct device_node *np; > > > + struct device_node *np, *mnp; > > > int err; > > > > > > np = dev->of_node; > > > @@ -571,7 +571,9 @@ static int ag71xx_mdio_probe(struct ag71xx *ag) > > > msleep(200); > > > } > > > > > > - err = of_mdiobus_register(mii_bus, np); > > > + mnp = of_get_child_by_name(np, "mdio"); > > > + err = of_mdiobus_register(mii_bus, mnp); > > > + of_node_put(mnp); > > > if (err) > > > goto mdio_err_put_clk; > > > > Hi Oleksij > > > > You need to keep backwards compatibility here. If you find an mdio > > node, use it, but if not, you need to still register np. > > > > This is also extending the driver binding, so you need to update the > > binding documentation. > > Hi Andrew, > > Normally i would agree. But in this case: > - this driver is freshly added to the kernel and is different to OpenWrt > implementation any way. No users from this side. > - Devicetree binding says: > Documentation/devicetree/bindings/net/qca,ar71xx.txt > |Optional properties: > |- phy-handle : phandle to the PHY device connected to this device. > |- fixed-link : Assume a fixed link. See fixed-link.txt in the same directory. > | Use instead of phy-handle. > | > |Optional subnodes: > |- mdio : specifies the mdio bus, used as a container for phy nodes > | according to phy.txt in the same directory > > So, it is driver bug ...ooOO (my personal bug :D) Hi Oleksij Ah, O.K. You should of explained that in the commit message. Is the mdio support just in -rc, or is it older? You need to add a Fixes: tag. The patch subject should be [PATCH net] to indicate this is a fix to the net tree The patch should be against net, not next-next. Andrew ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1] net: ag71xx: fix mdio subnode support 2019-09-30 16:25 ` Andrew Lunn @ 2019-09-30 17:52 ` Oleksij Rempel 0 siblings, 0 replies; 5+ messages in thread From: Oleksij Rempel @ 2019-09-30 17:52 UTC (permalink / raw) To: Andrew Lunn Cc: Jay Cliburn, Chris Snook, linux-kernel, Pengutronix Kernel Team, netdev, David S. Miller On Mon, Sep 30, 2019 at 06:25:57PM +0200, Andrew Lunn wrote: > On Mon, Sep 30, 2019 at 04:29:07PM +0200, Oleksij Rempel wrote: > > On Mon, Sep 30, 2019 at 03:42:09PM +0200, Andrew Lunn wrote: > > > On Mon, Sep 30, 2019 at 11:33:10AM +0200, Oleksij Rempel wrote: > > > > The driver was working with fixed phy without any noticeable issues. This bug > > > > was uncovered by introducing dsa ar9331-switch driver. > > > > > > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > > > > --- > > > > drivers/net/ethernet/atheros/ag71xx.c | 6 ++++-- > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c > > > > index 6703960c7cf5..d1101eea15c2 100644 > > > > --- a/drivers/net/ethernet/atheros/ag71xx.c > > > > +++ b/drivers/net/ethernet/atheros/ag71xx.c > > > > @@ -526,7 +526,7 @@ static int ag71xx_mdio_probe(struct ag71xx *ag) > > > > struct device *dev = &ag->pdev->dev; > > > > struct net_device *ndev = ag->ndev; > > > > static struct mii_bus *mii_bus; > > > > - struct device_node *np; > > > > + struct device_node *np, *mnp; > > > > int err; > > > > > > > > np = dev->of_node; > > > > @@ -571,7 +571,9 @@ static int ag71xx_mdio_probe(struct ag71xx *ag) > > > > msleep(200); > > > > } > > > > > > > > - err = of_mdiobus_register(mii_bus, np); > > > > + mnp = of_get_child_by_name(np, "mdio"); > > > > + err = of_mdiobus_register(mii_bus, mnp); > > > > + of_node_put(mnp); > > > > if (err) > > > > goto mdio_err_put_clk; > > > > > > Hi Oleksij > > > > > > You need to keep backwards compatibility here. If you find an mdio > > > node, use it, but if not, you need to still register np. > > > > > > This is also extending the driver binding, so you need to update the > > > binding documentation. > > > > Hi Andrew, > > > > Normally i would agree. But in this case: > > - this driver is freshly added to the kernel and is different to OpenWrt > > implementation any way. No users from this side. > > - Devicetree binding says: > > Documentation/devicetree/bindings/net/qca,ar71xx.txt > > |Optional properties: > > |- phy-handle : phandle to the PHY device connected to this device. > > |- fixed-link : Assume a fixed link. See fixed-link.txt in the same directory. > > | Use instead of phy-handle. > > | > > |Optional subnodes: > > |- mdio : specifies the mdio bus, used as a container for phy nodes > > | according to phy.txt in the same directory > > > > So, it is driver bug ...ooOO (my personal bug :D) > > Hi Oleksij > > Ah, O.K. You should of explained that in the commit message. > > Is the mdio support just in -rc, or is it older? > > You need to add a Fixes: tag. > > The patch subject should be [PATCH net] to indicate this is a fix to > the net tree > > The patch should be against net, not next-next. Ok. thx! i'll fix it. -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-09-30 20:57 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-30 9:33 [PATCH v1] net: ag71xx: fix mdio subnode support Oleksij Rempel 2019-09-30 13:42 ` Andrew Lunn 2019-09-30 14:29 ` Oleksij Rempel 2019-09-30 16:25 ` Andrew Lunn 2019-09-30 17:52 ` Oleksij Rempel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).