Message ID | 20180309221233.71202-1-brad.mouring@ni.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
Series | [v2,net-next,1/3] net: macb: Reorganize macb_mii bringup | expand |
On Fri, Mar 09, 2018 at 04:12:31PM -0600, Brad Mouring wrote: > The macb mii setup (mii_probe() and mii_init()) previously was > somewhat interspersed, likely a result of organic growth and hacking. Hi Brad For netdev it is normal to include a cover note for patch series, which explains the big picture of the series. The cover note is then used as the merge commit message. > > This change moves mii bus registration into mii_init and probing the > bus for devices into mii_probe. > > Signed-off-by: Brad Mouring <brad.mouring@ni.com> > Suggested-by: Andrew Lunn <andrew@lunn.ch> > --- > drivers/net/ethernet/cadence/macb_main.c | 89 ++++++++++++++++---------------- > 1 file changed, 45 insertions(+), 44 deletions(-) > > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c > index e84afcf1ecb5..7b69a291ab7a 100644 > --- a/drivers/net/ethernet/cadence/macb_main.c > +++ b/drivers/net/ethernet/cadence/macb_main.c > @@ -470,10 +470,48 @@ static void macb_handle_link_change(struct net_device *dev) > static int macb_mii_probe(struct net_device *dev) > { > struct macb *bp = netdev_priv(dev); > - struct macb_platform_data *pdata; > + struct macb_platform_data *pdata = dev_get_platdata(&bp->pdev->dev); > struct phy_device *phydev; > + struct device_node *np = bp->pdev->dev.of_node; > int phy_irq; > - int ret; > + int ret, i; These should be in 'Reverse Christmas Tree' order. i.e. longest first. That makes pdata tricky, so do the assignment in the body of the function. > + > + if (np) { > + if (of_phy_is_fixed_link(np)) { > + if (of_phy_register_fixed_link(np) < 0) { > + dev_err(&bp->pdev->dev, > + "broken fixed-link specification\n"); > + return -ENODEV; > + } > + bp->phy_node = of_node_get(np); > + } else { > + /* fallback to standard phy registration if no phy were > + * found during dt phy registration > + */ > + if (!phy_find_first(bp->mii_bus)) { > + for (i = 0; i < PHY_MAX_ADDR; i++) { > + struct phy_device *phydev; > + > + phydev = mdiobus_scan(bp->mii_bus, i); > + if (IS_ERR(phydev) && > + PTR_ERR(phydev) != -ENODEV) { > + ret = PTR_ERR(phydev); > + break; > + } > + } > + > + if (ret) > + return -ENODEV; > + } > + } > + } else { > + for (i = 0; i < PHY_MAX_ADDR; i++) > + bp->mii_bus->irq[i] = PHY_POLL; > + > + if (pdata) > + bp->mii_bus->phy_mask = pdata->phy_mask; I would say these two are to do with the mdio bus setup. Setting irq to POLL is not required, the core will do that. So you could add one patch removing that. Andrew
Hi Andrew, On Sat, Mar 10, 2018 at 05:17:18PM +0100, Andrew Lunn wrote: > On Fri, Mar 09, 2018 at 04:12:31PM -0600, Brad Mouring wrote: > > The macb mii setup (mii_probe() and mii_init()) previously was > > somewhat interspersed, likely a result of organic growth and hacking. > > Hi Brad > > For netdev it is normal to include a cover note for patch series, > which explains the big picture of the series. The cover note is then > used as the merge commit message. > > > > > This change moves mii bus registration into mii_init and probing the > > bus for devices into mii_probe. > > > > Signed-off-by: Brad Mouring <brad.mouring@ni.com> > > Suggested-by: Andrew Lunn <andrew@lunn.ch> > > --- > > drivers/net/ethernet/cadence/macb_main.c | 89 ++++++++++++++++---------------- > > 1 file changed, 45 insertions(+), 44 deletions(-) > > > > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c > > index e84afcf1ecb5..7b69a291ab7a 100644 > > --- a/drivers/net/ethernet/cadence/macb_main.c > > +++ b/drivers/net/ethernet/cadence/macb_main.c > > @@ -470,10 +470,48 @@ static void macb_handle_link_change(struct net_device *dev) > > static int macb_mii_probe(struct net_device *dev) > > { > > struct macb *bp = netdev_priv(dev); > > - struct macb_platform_data *pdata; > > + struct macb_platform_data *pdata = dev_get_platdata(&bp->pdev->dev); > > struct phy_device *phydev; > > + struct device_node *np = bp->pdev->dev.of_node; > > int phy_irq; > > - int ret; > > + int ret, i; > > These should be in 'Reverse Christmas Tree' order. i.e. longest > first. That makes pdata tricky, so do the assignment in the body of > the function. > > > + > > + if (np) { > > + if (of_phy_is_fixed_link(np)) { > > + if (of_phy_register_fixed_link(np) < 0) { > > + dev_err(&bp->pdev->dev, > > + "broken fixed-link specification\n"); > > + return -ENODEV; > > + } > > + bp->phy_node = of_node_get(np); > > + } else { > > + /* fallback to standard phy registration if no phy were > > + * found during dt phy registration > > + */ > > + if (!phy_find_first(bp->mii_bus)) { > > + for (i = 0; i < PHY_MAX_ADDR; i++) { > > + struct phy_device *phydev; > > + > > + phydev = mdiobus_scan(bp->mii_bus, i); > > + if (IS_ERR(phydev) && > > + PTR_ERR(phydev) != -ENODEV) { > > + ret = PTR_ERR(phydev); > > + break; > > + } > > + } > > + > > + if (ret) > > + return -ENODEV; > > + } > > + } > > + } else { > > + for (i = 0; i < PHY_MAX_ADDR; i++) > > + bp->mii_bus->irq[i] = PHY_POLL; > > + > > + if (pdata) > > + bp->mii_bus->phy_mask = pdata->phy_mask; > > I would say these two are to do with the mdio bus setup. Setting irq > to POLL is not required, the core will do that. So you could add one > patch removing that. > > Andrew Thank you for the excellent feedback and patience. I've prepped a set of patches addressing your points, but I'd like to test the changes on hardware first (both with a DT that calls out the phys and one that depends on default discovery) prior to a v3 (with appropriate cover letter). Brad
Hi Brad, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on net-next/master] url: https://github.com/0day-ci/linux/commits/Brad-Mouring/net-macb-Reorganize-macb_mii-bringup/20180311-133616 config: i386-randconfig-x012-201810 (attached as .config) compiler: gcc-7 (Debian 7.3.0-1) 7.3.0 reproduce: # save the attached .config to linux build tree make ARCH=i386 Note: it may well be a FALSE warning. FWIW you are at least aware of it now. http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings All warnings (new ones prefixed by >>): drivers/net/ethernet/cadence/macb_main.c: In function 'macb_probe': >> drivers/net/ethernet/cadence/macb_main.c:503:8: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized] if (ret) ^ drivers/net/ethernet/cadence/macb_main.c:477:6: note: 'ret' was declared here int ret, i; ^~~ vim +/ret +503 drivers/net/ethernet/cadence/macb_main.c 468 469 /* based on au1000_eth. c*/ 470 static int macb_mii_probe(struct net_device *dev) 471 { 472 struct macb *bp = netdev_priv(dev); 473 struct macb_platform_data *pdata = dev_get_platdata(&bp->pdev->dev); 474 struct phy_device *phydev; 475 struct device_node *np = bp->pdev->dev.of_node; 476 int phy_irq; 477 int ret, i; 478 479 if (np) { 480 if (of_phy_is_fixed_link(np)) { 481 if (of_phy_register_fixed_link(np) < 0) { 482 dev_err(&bp->pdev->dev, 483 "broken fixed-link specification\n"); 484 return -ENODEV; 485 } 486 bp->phy_node = of_node_get(np); 487 } else { 488 /* fallback to standard phy registration if no phy were 489 * found during dt phy registration 490 */ 491 if (!phy_find_first(bp->mii_bus)) { 492 for (i = 0; i < PHY_MAX_ADDR; i++) { 493 struct phy_device *phydev; 494 495 phydev = mdiobus_scan(bp->mii_bus, i); 496 if (IS_ERR(phydev) && 497 PTR_ERR(phydev) != -ENODEV) { 498 ret = PTR_ERR(phydev); 499 break; 500 } 501 } 502 > 503 if (ret) 504 return -ENODEV; 505 } 506 } 507 } else { 508 for (i = 0; i < PHY_MAX_ADDR; i++) 509 bp->mii_bus->irq[i] = PHY_POLL; 510 511 if (pdata) 512 bp->mii_bus->phy_mask = pdata->phy_mask; 513 514 } 515 516 if (bp->phy_node) { 517 phydev = of_phy_connect(dev, bp->phy_node, 518 &macb_handle_link_change, 0, 519 bp->phy_interface); 520 if (!phydev) 521 return -ENODEV; 522 } else { 523 phydev = phy_find_first(bp->mii_bus); 524 if (!phydev) { 525 netdev_err(dev, "no PHY found\n"); 526 return -ENXIO; 527 } 528 529 if (pdata) { 530 if (gpio_is_valid(pdata->phy_irq_pin)) { 531 ret = devm_gpio_request(&bp->pdev->dev, 532 pdata->phy_irq_pin, "phy int"); 533 if (!ret) { 534 phy_irq = gpio_to_irq(pdata->phy_irq_pin); 535 phydev->irq = (phy_irq < 0) ? PHY_POLL : phy_irq; 536 } 537 } else { 538 phydev->irq = PHY_POLL; 539 } 540 } 541 542 /* attach the mac to the phy */ 543 ret = phy_connect_direct(dev, phydev, &macb_handle_link_change, 544 bp->phy_interface); 545 if (ret) { 546 netdev_err(dev, "Could not attach to PHY\n"); 547 return ret; 548 } 549 } 550 551 /* mask with MAC supported features */ 552 if (macb_is_gem(bp) && bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) 553 phydev->supported &= PHY_GBIT_FEATURES; 554 else 555 phydev->supported &= PHY_BASIC_FEATURES; 556 557 if (bp->caps & MACB_CAPS_NO_GIGABIT_HALF) 558 phydev->supported &= ~SUPPORTED_1000baseT_Half; 559 560 phydev->advertising = phydev->supported; 561 562 bp->link = 0; 563 bp->speed = 0; 564 bp->duplex = -1; 565 566 return 0; 567 } 568 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index e84afcf1ecb5..7b69a291ab7a 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -470,10 +470,48 @@ static void macb_handle_link_change(struct net_device *dev) static int macb_mii_probe(struct net_device *dev) { struct macb *bp = netdev_priv(dev); - struct macb_platform_data *pdata; + struct macb_platform_data *pdata = dev_get_platdata(&bp->pdev->dev); struct phy_device *phydev; + struct device_node *np = bp->pdev->dev.of_node; int phy_irq; - int ret; + int ret, i; + + if (np) { + if (of_phy_is_fixed_link(np)) { + if (of_phy_register_fixed_link(np) < 0) { + dev_err(&bp->pdev->dev, + "broken fixed-link specification\n"); + return -ENODEV; + } + bp->phy_node = of_node_get(np); + } else { + /* fallback to standard phy registration if no phy were + * found during dt phy registration + */ + if (!phy_find_first(bp->mii_bus)) { + for (i = 0; i < PHY_MAX_ADDR; i++) { + struct phy_device *phydev; + + phydev = mdiobus_scan(bp->mii_bus, i); + if (IS_ERR(phydev) && + PTR_ERR(phydev) != -ENODEV) { + ret = PTR_ERR(phydev); + break; + } + } + + if (ret) + return -ENODEV; + } + } + } else { + for (i = 0; i < PHY_MAX_ADDR; i++) + bp->mii_bus->irq[i] = PHY_POLL; + + if (pdata) + bp->mii_bus->phy_mask = pdata->phy_mask; + + } if (bp->phy_node) { phydev = of_phy_connect(dev, bp->phy_node, @@ -488,7 +526,6 @@ static int macb_mii_probe(struct net_device *dev) return -ENXIO; } - pdata = dev_get_platdata(&bp->pdev->dev); if (pdata) { if (gpio_is_valid(pdata->phy_irq_pin)) { ret = devm_gpio_request(&bp->pdev->dev, @@ -533,7 +570,7 @@ static int macb_mii_init(struct macb *bp) { struct macb_platform_data *pdata; struct device_node *np; - int err = -ENXIO, i; + int err; /* Enable management port */ macb_writel(bp, NCR, MACB_BIT(MPE)); @@ -556,46 +593,10 @@ static int macb_mii_init(struct macb *bp) dev_set_drvdata(&bp->dev->dev, bp->mii_bus); np = bp->pdev->dev.of_node; - if (np) { - if (of_phy_is_fixed_link(np)) { - if (of_phy_register_fixed_link(np) < 0) { - dev_err(&bp->pdev->dev, - "broken fixed-link specification\n"); - goto err_out_unregister_bus; - } - bp->phy_node = of_node_get(np); - - err = mdiobus_register(bp->mii_bus); - } else { - /* try dt phy registration */ - err = of_mdiobus_register(bp->mii_bus, np); - /* fallback to standard phy registration if no phy were - * found during dt phy registration - */ - if (!err && !phy_find_first(bp->mii_bus)) { - for (i = 0; i < PHY_MAX_ADDR; i++) { - struct phy_device *phydev; - - phydev = mdiobus_scan(bp->mii_bus, i); - if (IS_ERR(phydev) && - PTR_ERR(phydev) != -ENODEV) { - err = PTR_ERR(phydev); - break; - } - } - - if (err) - goto err_out_unregister_bus; - } - } + if (np) { + err = of_mdiobus_register(bp->mii_bus, np); } else { - for (i = 0; i < PHY_MAX_ADDR; i++) - bp->mii_bus->irq[i] = PHY_POLL; - - if (pdata) - bp->mii_bus->phy_mask = pdata->phy_mask; - err = mdiobus_register(bp->mii_bus); } @@ -610,10 +611,10 @@ static int macb_mii_init(struct macb *bp) err_out_unregister_bus: mdiobus_unregister(bp->mii_bus); -err_out_free_mdiobus: - of_node_put(bp->phy_node); if (np && of_phy_is_fixed_link(np)) of_phy_deregister_fixed_link(np); +err_out_free_mdiobus: + of_node_put(bp->phy_node); mdiobus_free(bp->mii_bus); err_out: return err;
The macb mii setup (mii_probe() and mii_init()) previously was somewhat interspersed, likely a result of organic growth and hacking. This change moves mii bus registration into mii_init and probing the bus for devices into mii_probe. Signed-off-by: Brad Mouring <brad.mouring@ni.com> Suggested-by: Andrew Lunn <andrew@lunn.ch> --- drivers/net/ethernet/cadence/macb_main.c | 89 ++++++++++++++++---------------- 1 file changed, 45 insertions(+), 44 deletions(-)