diff mbox series

[v2,net-next,1/3] net: macb: Reorganize macb_mii bringup

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

Commit Message

Brad Mouring March 9, 2018, 10:12 p.m. UTC 
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(-)

Comments

Andrew Lunn March 10, 2018, 4:17 p.m. UTC | #1 
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
Brad Mouring March 10, 2018, 10:19 p.m. UTC | #2 
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
kernel test robot March 11, 2018, 7:02 a.m. UTC | #3 
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 mbox series

Patch

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;