From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0a-00010702.pphosted.com ([148.163.156.75]:58334 "EHLO mx0b-00010702.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751057AbeCJWTh (ORCPT ); Sat, 10 Mar 2018 17:19:37 -0500 Date: Sat, 10 Mar 2018 16:19:20 -0600 From: Brad Mouring To: Andrew Lunn CC: Nicolas Ferre , Rob Herring , "David S . Miller" , Michael Grzeschik , Mark Rutland , , Julia Cartwright , Subject: Re: [PATCH v2 net-next 1/3] net: macb: Reorganize macb_mii bringup Message-ID: <20180310221920.GA76538@artie.amer.corp.natinst.com> References: <20180308173247.GA22340@lunn.ch> <20180309221233.71202-1-brad.mouring@ni.com> <20180310161718.GC29174@lunn.ch> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <20180310161718.GC29174@lunn.ch> Sender: netdev-owner@vger.kernel.org List-ID: 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 > > Suggested-by: Andrew Lunn > > --- > > 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Sat, 10 Mar 2018 16:19:20 -0600 From: Brad Mouring Subject: Re: [PATCH v2 net-next 1/3] net: macb: Reorganize macb_mii bringup Message-ID: <20180310221920.GA76538@artie.amer.corp.natinst.com> References: <20180308173247.GA22340@lunn.ch> <20180309221233.71202-1-brad.mouring@ni.com> <20180310161718.GC29174@lunn.ch> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <20180310161718.GC29174@lunn.ch> To: Andrew Lunn Cc: Nicolas Ferre , Rob Herring , "David S . Miller" , Michael Grzeschik , Mark Rutland , netdev@vger.kernel.org, Julia Cartwright , devicetree@vger.kernel.org List-ID: 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 > > Suggested-by: Andrew Lunn > > --- > > 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