All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] net: macb: add fixed-link node support
@ 2017-06-23 14:54 Michael Grzeschik
  2017-06-25 19:22 ` David Miller
  2017-11-06  9:26 ` Nicolas Ferre
  0 siblings, 2 replies; 13+ messages in thread
From: Michael Grzeschik @ 2017-06-23 14:54 UTC (permalink / raw)
  To: netdev; +Cc: nicolas.ferre, kernel

In case the MACB is directly connected to a
non-mdio PHY/device, it should be possible to provide
a fixed link configuration in the DT.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
  v1 -> v2: - moved of_phy_connect case and phy_connect_direct into if else cases
            - moved of_pphy_register_fixed_link into if(np) case
	    - added of_node_put(bp->phy_node); into macb_remove
---
 drivers/net/ethernet/cadence/macb.c | 98 ++++++++++++++++++++++---------------
 drivers/net/ethernet/cadence/macb.h |  1 +
 2 files changed, 60 insertions(+), 39 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index 91f7492623d3f..5781f1ede8c6e 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -425,32 +425,40 @@ static int macb_mii_probe(struct net_device *dev)
 	int phy_irq;
 	int ret;
 
-	phydev = phy_find_first(bp->mii_bus);
-	if (!phydev) {
-		netdev_err(dev, "no PHY found\n");
-		return -ENXIO;
-	}
+	if (bp->phy_node) {
+		phydev = of_phy_connect(dev, bp->phy_node,
+					&macb_handle_link_change, 0,
+					bp->phy_interface);
+		if (!phydev)
+			return -ENODEV;
+	} else {
+		phydev = phy_find_first(bp->mii_bus);
+		if (!phydev) {
+			netdev_err(dev, "no PHY found\n");
+			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,
-						pdata->phy_irq_pin, "phy int");
-			if (!ret) {
-				phy_irq = gpio_to_irq(pdata->phy_irq_pin);
-				phydev->irq = (phy_irq < 0) ? PHY_POLL : phy_irq;
+		pdata = dev_get_platdata(&bp->pdev->dev);
+		if (pdata) {
+			if (gpio_is_valid(pdata->phy_irq_pin)) {
+				ret = devm_gpio_request(&bp->pdev->dev,
+							pdata->phy_irq_pin, "phy int");
+				if (!ret) {
+					phy_irq = gpio_to_irq(pdata->phy_irq_pin);
+					phydev->irq = (phy_irq < 0) ? PHY_POLL : phy_irq;
+				}
+			} else {
+				phydev->irq = PHY_POLL;
 			}
-		} else {
-			phydev->irq = PHY_POLL;
 		}
-	}
 
-	/* attach the mac to the phy */
-	ret = phy_connect_direct(dev, phydev, &macb_handle_link_change,
-				 bp->phy_interface);
-	if (ret) {
-		netdev_err(dev, "Could not attach to PHY\n");
-		return ret;
+		/* attach the mac to the phy */
+		ret = phy_connect_direct(dev, phydev, &macb_handle_link_change,
+					 bp->phy_interface);
+		if (ret) {
+			netdev_err(dev, "Could not attach to PHY\n");
+			return ret;
+		}
 	}
 
 	/* mask with MAC supported features */
@@ -499,26 +507,37 @@ static int macb_mii_init(struct macb *bp)
 
 	np = bp->pdev->dev.of_node;
 	if (np) {
-		/* try dt phy registration */
-		err = of_mdiobus_register(bp->mii_bus, 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);
 
-		/* 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;
+			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 (err)
+					goto err_out_unregister_bus;
+			}
 		}
 	} else {
 		for (i = 0; i < PHY_MAX_ADDR; i++)
@@ -3438,6 +3457,7 @@ static int macb_remove(struct platform_device *pdev)
 		clk_disable_unprepare(bp->hclk);
 		clk_disable_unprepare(bp->pclk);
 		clk_disable_unprepare(bp->rx_clk);
+		of_node_put(bp->phy_node);
 		free_netdev(dev);
 	}
 
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index ec037b0fa2a4d..2510661102bad 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -930,6 +930,7 @@ struct macb {
 	struct macb_or_gem_ops	macbgem_ops;
 
 	struct mii_bus		*mii_bus;
+	struct device_node	*phy_node;
 	int 			link;
 	int 			speed;
 	int 			duplex;
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 13+ messages in thread
* Re: [PATCH v2] net: macb: add fixed-link node support
  2017-06-23 14:54 [PATCH v2] net: macb: add fixed-link node support Michael Grzeschik
@ 2017-06-25 19:22 ` David Miller
  2017-11-06  9:26 ` Nicolas Ferre
  1 sibling, 0 replies; 13+ messages in thread
From: David Miller @ 2017-06-25 19:22 UTC (permalink / raw)
  To: m.grzeschik; +Cc: netdev, nicolas.ferre, kernel

From: Michael Grzeschik <m.grzeschik@pengutronix.de>
Date: Fri, 23 Jun 2017 16:54:10 +0200

> In case the MACB is directly connected to a
> non-mdio PHY/device, it should be possible to provide
> a fixed link configuration in the DT.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

Applied, thanks.

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH v2] net: macb: add fixed-link node support
  2017-06-23 14:54 [PATCH v2] net: macb: add fixed-link node support Michael Grzeschik
  2017-06-25 19:22 ` David Miller
@ 2017-11-06  9:26 ` Nicolas Ferre
  2017-11-06 11:10   ` [PATCH 1/2] net: macb: add of_phy_deregister_fixed_link to error paths Michael Grzeschik
  1 sibling, 1 reply; 13+ messages in thread
From: Nicolas Ferre @ 2017-11-06  9:26 UTC (permalink / raw)
  To: Michael Grzeschik, netdev; +Cc: kernel, Andrew Lunn

On 23/06/2017 at 16:54, Michael Grzeschik wrote:
> In case the MACB is directly connected to a
> non-mdio PHY/device, it should be possible to provide
> a fixed link configuration in the DT.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>   v1 -> v2: - moved of_phy_connect case and phy_connect_direct into if else cases
>             - moved of_pphy_register_fixed_link into if(np) case
> 	    - added of_node_put(bp->phy_node); into macb_remove

Hi Michael,

by re-scanning my email backlog, I found this remark that may apply to
your patch (already in Linus' tree actually)...

> ---
>  drivers/net/ethernet/cadence/macb.c | 98 ++++++++++++++++++++++---------------
>  drivers/net/ethernet/cadence/macb.h |  1 +
>  2 files changed, 60 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> index 91f7492623d3f..5781f1ede8c6e 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -425,32 +425,40 @@ static int macb_mii_probe(struct net_device *dev)
>  	int phy_irq;
>  	int ret;
>  
> -	phydev = phy_find_first(bp->mii_bus);
> -	if (!phydev) {
> -		netdev_err(dev, "no PHY found\n");
> -		return -ENXIO;
> -	}
> +	if (bp->phy_node) {
> +		phydev = of_phy_connect(dev, bp->phy_node,
> +					&macb_handle_link_change, 0,
> +					bp->phy_interface);
> +		if (!phydev)
> +			return -ENODEV;
> +	} else {
> +		phydev = phy_find_first(bp->mii_bus);
> +		if (!phydev) {
> +			netdev_err(dev, "no PHY found\n");
> +			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,
> -						pdata->phy_irq_pin, "phy int");
> -			if (!ret) {
> -				phy_irq = gpio_to_irq(pdata->phy_irq_pin);
> -				phydev->irq = (phy_irq < 0) ? PHY_POLL : phy_irq;
> +		pdata = dev_get_platdata(&bp->pdev->dev);
> +		if (pdata) {
> +			if (gpio_is_valid(pdata->phy_irq_pin)) {
> +				ret = devm_gpio_request(&bp->pdev->dev,
> +							pdata->phy_irq_pin, "phy int");
> +				if (!ret) {
> +					phy_irq = gpio_to_irq(pdata->phy_irq_pin);
> +					phydev->irq = (phy_irq < 0) ? PHY_POLL : phy_irq;
> +				}
> +			} else {
> +				phydev->irq = PHY_POLL;
>  			}
> -		} else {
> -			phydev->irq = PHY_POLL;
>  		}
> -	}
>  
> -	/* attach the mac to the phy */
> -	ret = phy_connect_direct(dev, phydev, &macb_handle_link_change,
> -				 bp->phy_interface);
> -	if (ret) {
> -		netdev_err(dev, "Could not attach to PHY\n");
> -		return ret;
> +		/* attach the mac to the phy */
> +		ret = phy_connect_direct(dev, phydev, &macb_handle_link_change,
> +					 bp->phy_interface);
> +		if (ret) {
> +			netdev_err(dev, "Could not attach to PHY\n");
> +			return ret;
> +		}
>  	}
>  
>  	/* mask with MAC supported features */
> @@ -499,26 +507,37 @@ static int macb_mii_init(struct macb *bp)
>  
>  	np = bp->pdev->dev.of_node;
>  	if (np) {
> -		/* try dt phy registration */
> -		err = of_mdiobus_register(bp->mii_bus, np);
> +		if (of_phy_is_fixed_link(np)) {
> +			if (of_phy_register_fixed_link(np) < 0) {

remark done here by Andrew:
https://www.spinics.net/lists/netdev/msg421186.html

about missing of_phy_deregister_fixed_link().

Can you tell me if you would have time to have a look at it?


Thanks, best regards,
  Nicolas

> +				dev_err(&bp->pdev->dev,
> +					"broken fixed-link specification\n");
> +				goto err_out_unregister_bus;
> +			}
> +			bp->phy_node = of_node_get(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;
> +			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 (err)
> +					goto err_out_unregister_bus;
> +			}
>  		}
>  	} else {
>  		for (i = 0; i < PHY_MAX_ADDR; i++)
> @@ -3438,6 +3457,7 @@ static int macb_remove(struct platform_device *pdev)
>  		clk_disable_unprepare(bp->hclk);
>  		clk_disable_unprepare(bp->pclk);
>  		clk_disable_unprepare(bp->rx_clk);
> +		of_node_put(bp->phy_node);
>  		free_netdev(dev);
>  	}
>  
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index ec037b0fa2a4d..2510661102bad 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -930,6 +930,7 @@ struct macb {
>  	struct macb_or_gem_ops	macbgem_ops;
>  
>  	struct mii_bus		*mii_bus;
> +	struct device_node	*phy_node;
>  	int 			link;
>  	int 			speed;
>  	int 			duplex;
> 


-- 
Nicolas Ferre

^ permalink raw reply	[flat|nested] 13+ messages in thread
* [PATCH 1/2] net: macb: add of_phy_deregister_fixed_link to error paths
  2017-11-06  9:26 ` Nicolas Ferre
@ 2017-11-06 11:10   ` Michael Grzeschik
  2017-11-06 11:10     ` [PATCH 2/2] net: macb: add of_node_put " Michael Grzeschik
                       ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Michael Grzeschik @ 2017-11-06 11:10 UTC (permalink / raw)
  To: nicolas.ferre; +Cc: netdev, kernel, andrew

We add the call of_phy_deregister_fixed_link to all associated
error paths for memory clean up.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/net/ethernet/cadence/macb_main.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 6df2cad61647a..2c2acd011329a 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -611,6 +611,8 @@ static int macb_mii_init(struct macb *bp)
 err_out_unregister_bus:
 	mdiobus_unregister(bp->mii_bus);
 err_out_free_mdiobus:
+	if ((np) && (of_phy_is_fixed_link(np)))
+		of_phy_deregister_fixed_link(np);
 	mdiobus_free(bp->mii_bus);
 err_out:
 	return err;
@@ -3552,6 +3554,8 @@ static int macb_probe(struct platform_device *pdev)
 err_out_unregister_mdio:
 	phy_disconnect(dev->phydev);
 	mdiobus_unregister(bp->mii_bus);
+	if ((np) && (of_phy_is_fixed_link(np)))
+		of_phy_deregister_fixed_link(np);
 	mdiobus_free(bp->mii_bus);
 
 	/* Shutdown the PHY if there is a GPIO reset */
@@ -3574,6 +3578,7 @@ static int macb_remove(struct platform_device *pdev)
 {
 	struct net_device *dev;
 	struct macb *bp;
+	struct device_node *np = pdev->dev.of_node;
 
 	dev = platform_get_drvdata(pdev);
 
@@ -3582,6 +3587,8 @@ static int macb_remove(struct platform_device *pdev)
 		if (dev->phydev)
 			phy_disconnect(dev->phydev);
 		mdiobus_unregister(bp->mii_bus);
+		if ((np) && (of_phy_is_fixed_link(np)))
+			of_phy_deregister_fixed_link(np);
 		dev->phydev = NULL;
 		mdiobus_free(bp->mii_bus);
 
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 13+ messages in thread
* [PATCH 2/2] net: macb: add of_node_put to error paths
  2017-11-06 11:10   ` [PATCH 1/2] net: macb: add of_phy_deregister_fixed_link to error paths Michael Grzeschik
@ 2017-11-06 11:10     ` Michael Grzeschik
  2017-11-07  9:27       ` Nicolas Ferre
  2017-11-06 14:37     ` [PATCH 1/2] net: macb: add of_phy_deregister_fixed_link " Nicolas Ferre
  2017-11-08  4:22     ` David Miller
  2 siblings, 1 reply; 13+ messages in thread
From: Michael Grzeschik @ 2017-11-06 11:10 UTC (permalink / raw)
  To: nicolas.ferre; +Cc: netdev, kernel, andrew

We add the call of_node_put(bp->phy_node) to all associated error
paths for memory clean up.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/net/ethernet/cadence/macb_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 2c2acd011329a..2698a1fde39c7 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -611,6 +611,7 @@ 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);
 	mdiobus_free(bp->mii_bus);
@@ -3554,6 +3555,7 @@ static int macb_probe(struct platform_device *pdev)
 err_out_unregister_mdio:
 	phy_disconnect(dev->phydev);
 	mdiobus_unregister(bp->mii_bus);
+	of_node_put(bp->phy_node);
 	if ((np) && (of_phy_is_fixed_link(np)))
 		of_phy_deregister_fixed_link(np);
 	mdiobus_free(bp->mii_bus);
@@ -3587,6 +3589,7 @@ static int macb_remove(struct platform_device *pdev)
 		if (dev->phydev)
 			phy_disconnect(dev->phydev);
 		mdiobus_unregister(bp->mii_bus);
+		of_node_put(bp->phy_node);
 		if ((np) && (of_phy_is_fixed_link(np)))
 			of_phy_deregister_fixed_link(np);
 		dev->phydev = NULL;
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] net: macb: add of_phy_deregister_fixed_link to error paths
  2017-11-06 11:10   ` [PATCH 1/2] net: macb: add of_phy_deregister_fixed_link to error paths Michael Grzeschik
  2017-11-06 11:10     ` [PATCH 2/2] net: macb: add of_node_put " Michael Grzeschik
@ 2017-11-06 14:37     ` Nicolas Ferre
  2017-11-08  4:22     ` David Miller
  2 siblings, 0 replies; 13+ messages in thread
From: Nicolas Ferre @ 2017-11-06 14:37 UTC (permalink / raw)
  To: Michael Grzeschik; +Cc: netdev, kernel, andrew

On 06/11/2017 at 12:10, Michael Grzeschik wrote:
> We add the call of_phy_deregister_fixed_link to all associated
> error paths for memory clean up.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>

Thanks a lot for your quick answer!

Best regards,
  Nicolas

> ---
>  drivers/net/ethernet/cadence/macb_main.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 6df2cad61647a..2c2acd011329a 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -611,6 +611,8 @@ static int macb_mii_init(struct macb *bp)
>  err_out_unregister_bus:
>  	mdiobus_unregister(bp->mii_bus);
>  err_out_free_mdiobus:
> +	if ((np) && (of_phy_is_fixed_link(np)))
> +		of_phy_deregister_fixed_link(np);
>  	mdiobus_free(bp->mii_bus);
>  err_out:
>  	return err;
> @@ -3552,6 +3554,8 @@ static int macb_probe(struct platform_device *pdev)
>  err_out_unregister_mdio:
>  	phy_disconnect(dev->phydev);
>  	mdiobus_unregister(bp->mii_bus);
> +	if ((np) && (of_phy_is_fixed_link(np)))
> +		of_phy_deregister_fixed_link(np);
>  	mdiobus_free(bp->mii_bus);
>  
>  	/* Shutdown the PHY if there is a GPIO reset */
> @@ -3574,6 +3578,7 @@ static int macb_remove(struct platform_device *pdev)
>  {
>  	struct net_device *dev;
>  	struct macb *bp;
> +	struct device_node *np = pdev->dev.of_node;
>  
>  	dev = platform_get_drvdata(pdev);
>  
> @@ -3582,6 +3587,8 @@ static int macb_remove(struct platform_device *pdev)
>  		if (dev->phydev)
>  			phy_disconnect(dev->phydev);
>  		mdiobus_unregister(bp->mii_bus);
> +		if ((np) && (of_phy_is_fixed_link(np)))
> +			of_phy_deregister_fixed_link(np);
>  		dev->phydev = NULL;
>  		mdiobus_free(bp->mii_bus);
>  
> 


-- 
Nicolas Ferre

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] net: macb: add of_node_put to error paths
  2017-11-06 11:10     ` [PATCH 2/2] net: macb: add of_node_put " Michael Grzeschik
@ 2017-11-07  9:27       ` Nicolas Ferre
  2017-11-07  9:59         ` [PATCH v2] " Michael Grzeschik
  2017-11-07 10:00         ` [PATCH 2/2] " Michael Grzeschik
  0 siblings, 2 replies; 13+ messages in thread
From: Nicolas Ferre @ 2017-11-07  9:27 UTC (permalink / raw)
  To: Michael Grzeschik; +Cc: netdev, kernel, andrew

On 06/11/2017 at 12:10, Michael Grzeschik wrote:
> We add the call of_node_put(bp->phy_node) to all associated error
> paths for memory clean up.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 2c2acd011329a..2698a1fde39c7 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -611,6 +611,7 @@ 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);

okay

>  	if ((np) && (of_phy_is_fixed_link(np)))
>  		of_phy_deregister_fixed_link(np);
>  	mdiobus_free(bp->mii_bus);
> @@ -3554,6 +3555,7 @@ static int macb_probe(struct platform_device *pdev)
>  err_out_unregister_mdio:
>  	phy_disconnect(dev->phydev);
>  	mdiobus_unregister(bp->mii_bus);
> +	of_node_put(bp->phy_node);

yes

>  	if ((np) && (of_phy_is_fixed_link(np)))
>  		of_phy_deregister_fixed_link(np);
>  	mdiobus_free(bp->mii_bus);
> @@ -3587,6 +3589,7 @@ static int macb_remove(struct platform_device *pdev)
>  		if (dev->phydev)
>  			phy_disconnect(dev->phydev);
>  		mdiobus_unregister(bp->mii_bus);
> +		of_node_put(bp->phy_node);

Isn't this call already done some lines below, just before
"free_netdev(dev);"?


>  		if ((np) && (of_phy_is_fixed_link(np)))
>  			of_phy_deregister_fixed_link(np);
>  		dev->phydev = NULL;
> 

Thanks for your patch.

Regards,
-- 
Nicolas Ferre

^ permalink raw reply	[flat|nested] 13+ messages in thread
* [PATCH v2] net: macb: add of_node_put to error paths
  2017-11-07  9:27       ` Nicolas Ferre
@ 2017-11-07  9:59         ` Michael Grzeschik
  2017-11-07 11:12           ` Nicolas Ferre
  2017-11-08  6:29           ` David Miller
  2017-11-07 10:00         ` [PATCH 2/2] " Michael Grzeschik
  1 sibling, 2 replies; 13+ messages in thread
From: Michael Grzeschik @ 2017-11-07  9:59 UTC (permalink / raw)
  To: nicolas.ferre; +Cc: netdev, kernel, andrew

We add the call of_node_put(bp->phy_node) to all associated error
paths for memory clean up.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
v1 -> v2: removed extra of_node_put from macb_remove

 drivers/net/ethernet/cadence/macb_main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 2c2acd011329a..c66df096e81a5 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -611,6 +611,7 @@ 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);
 	mdiobus_free(bp->mii_bus);
@@ -3554,6 +3555,7 @@ static int macb_probe(struct platform_device *pdev)
 err_out_unregister_mdio:
 	phy_disconnect(dev->phydev);
 	mdiobus_unregister(bp->mii_bus);
+	of_node_put(bp->phy_node);
 	if ((np) && (of_phy_is_fixed_link(np)))
 		of_phy_deregister_fixed_link(np);
 	mdiobus_free(bp->mii_bus);
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] net: macb: add of_node_put to error paths
  2017-11-07  9:27       ` Nicolas Ferre
  2017-11-07  9:59         ` [PATCH v2] " Michael Grzeschik
@ 2017-11-07 10:00         ` Michael Grzeschik
  1 sibling, 0 replies; 13+ messages in thread
From: Michael Grzeschik @ 2017-11-07 10:00 UTC (permalink / raw)
  To: Nicolas Ferre; +Cc: netdev, kernel, andrew

[-- Attachment #1: Type: text/plain, Size: 2187 bytes --]

On Tue, Nov 07, 2017 at 10:27:20AM +0100, Nicolas Ferre wrote:
> On 06/11/2017 at 12:10, Michael Grzeschik wrote:
> > We add the call of_node_put(bp->phy_node) to all associated error
> > paths for memory clean up.
> > 
> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > ---
> >  drivers/net/ethernet/cadence/macb_main.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > index 2c2acd011329a..2698a1fde39c7 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -611,6 +611,7 @@ 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);
> 
> okay
> 
> >  	if ((np) && (of_phy_is_fixed_link(np)))
> >  		of_phy_deregister_fixed_link(np);
> >  	mdiobus_free(bp->mii_bus);
> > @@ -3554,6 +3555,7 @@ static int macb_probe(struct platform_device *pdev)
> >  err_out_unregister_mdio:
> >  	phy_disconnect(dev->phydev);
> >  	mdiobus_unregister(bp->mii_bus);
> > +	of_node_put(bp->phy_node);
> 
> yes
> 
> >  	if ((np) && (of_phy_is_fixed_link(np)))
> >  		of_phy_deregister_fixed_link(np);
> >  	mdiobus_free(bp->mii_bus);
> > @@ -3587,6 +3589,7 @@ static int macb_remove(struct platform_device *pdev)
> >  		if (dev->phydev)
> >  			phy_disconnect(dev->phydev);
> >  		mdiobus_unregister(bp->mii_bus);
> > +		of_node_put(bp->phy_node);
> 
> Isn't this call already done some lines below, just before
> "free_netdev(dev);"?

You are absolutely right. Will send v2.

> >  		if ((np) && (of_phy_is_fixed_link(np)))
> >  			of_phy_deregister_fixed_link(np);
> >  		dev->phydev = NULL;
> > 
> 
> Thanks for your patch.
Thanks,
Michael

-- 
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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH v2] net: macb: add of_node_put to error paths
  2017-11-07  9:59         ` [PATCH v2] " Michael Grzeschik
@ 2017-11-07 11:12           ` Nicolas Ferre
  2017-11-08  6:29           ` David Miller
  1 sibling, 0 replies; 13+ messages in thread
From: Nicolas Ferre @ 2017-11-07 11:12 UTC (permalink / raw)
  To: Michael Grzeschik; +Cc: netdev, kernel, andrew

On 07/11/2017 at 10:59, Michael Grzeschik wrote:
> We add the call of_node_put(bp->phy_node) to all associated error
> paths for memory clean up.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

Thanks for your quick update:

Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>

Best regards,
  Nicolas

> ---
> v1 -> v2: removed extra of_node_put from macb_remove
> 
>  drivers/net/ethernet/cadence/macb_main.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 2c2acd011329a..c66df096e81a5 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -611,6 +611,7 @@ 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);
>  	mdiobus_free(bp->mii_bus);
> @@ -3554,6 +3555,7 @@ static int macb_probe(struct platform_device *pdev)
>  err_out_unregister_mdio:
>  	phy_disconnect(dev->phydev);
>  	mdiobus_unregister(bp->mii_bus);
> +	of_node_put(bp->phy_node);
>  	if ((np) && (of_phy_is_fixed_link(np)))
>  		of_phy_deregister_fixed_link(np);
>  	mdiobus_free(bp->mii_bus);
> 


-- 
Nicolas Ferre

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] net: macb: add of_phy_deregister_fixed_link to error paths
  2017-11-06 11:10   ` [PATCH 1/2] net: macb: add of_phy_deregister_fixed_link to error paths Michael Grzeschik
  2017-11-06 11:10     ` [PATCH 2/2] net: macb: add of_node_put " Michael Grzeschik
  2017-11-06 14:37     ` [PATCH 1/2] net: macb: add of_phy_deregister_fixed_link " Nicolas Ferre
@ 2017-11-08  4:22     ` David Miller
  2017-11-08  8:41       ` Michael Grzeschik
  2 siblings, 1 reply; 13+ messages in thread
From: David Miller @ 2017-11-08  4:22 UTC (permalink / raw)
  To: m.grzeschik; +Cc: nicolas.ferre, netdev, kernel, andrew

From: Michael Grzeschik <m.grzeschik@pengutronix.de>
Date: Mon,  6 Nov 2017 12:10:04 +0100

> We add the call of_phy_deregister_fixed_link to all associated
> error paths for memory clean up.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 6df2cad61647a..2c2acd011329a 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -611,6 +611,8 @@ static int macb_mii_init(struct macb *bp)
>  err_out_unregister_bus:
>  	mdiobus_unregister(bp->mii_bus);
>  err_out_free_mdiobus:
> +	if ((np) && (of_phy_is_fixed_link(np)))

Please don't use so many parenthesis in your conditionals:

	if (np && of_phy_is_fixed_link(np))

is more than sufficient.

Please fix this in your entire set of patches.

Thank you.

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH v2] net: macb: add of_node_put to error paths
  2017-11-07  9:59         ` [PATCH v2] " Michael Grzeschik
  2017-11-07 11:12           ` Nicolas Ferre
@ 2017-11-08  6:29           ` David Miller
  1 sibling, 0 replies; 13+ messages in thread
From: David Miller @ 2017-11-08  6:29 UTC (permalink / raw)
  To: m.grzeschik; +Cc: nicolas.ferre, netdev, kernel, andrew

From: Michael Grzeschik <m.grzeschik@pengutronix.de>
Date: Tue,  7 Nov 2017 10:59:49 +0100

> @@ -611,6 +611,7 @@ 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);
>  	mdiobus_free(bp->mii_bus);

This is relative to another patch which has not been applied.

Please do not do this.

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] net: macb: add of_phy_deregister_fixed_link to error paths
  2017-11-08  4:22     ` David Miller
@ 2017-11-08  8:41       ` Michael Grzeschik
  0 siblings, 0 replies; 13+ messages in thread
From: Michael Grzeschik @ 2017-11-08  8:41 UTC (permalink / raw)
  To: David Miller; +Cc: nicolas.ferre, netdev, kernel, andrew

[-- Attachment #1: Type: text/plain, Size: 1611 bytes --]

On Wed, Nov 08, 2017 at 01:22:57PM +0900, David Miller wrote:
> From: Michael Grzeschik <m.grzeschik@pengutronix.de>
> Date: Mon,  6 Nov 2017 12:10:04 +0100
> 
> > We add the call of_phy_deregister_fixed_link to all associated
> > error paths for memory clean up.
> > 
> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > ---
> >  drivers/net/ethernet/cadence/macb_main.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > index 6df2cad61647a..2c2acd011329a 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -611,6 +611,8 @@ static int macb_mii_init(struct macb *bp)
> >  err_out_unregister_bus:
> >  	mdiobus_unregister(bp->mii_bus);
> >  err_out_free_mdiobus:
> > +	if ((np) && (of_phy_is_fixed_link(np)))
> 
> Please don't use so many parenthesis in your conditionals:
> 
> 	if (np && of_phy_is_fixed_link(np))
> 
> is more than sufficient.
> 
> Please fix this in your entire set of patches.

There are only two patches and not even in one series.
I will resend them both with this one fixed and create
a series. As the second one depends on this one.

Thanks,
Michael

-- 
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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-11-08  8:41 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-23 14:54 [PATCH v2] net: macb: add fixed-link node support Michael Grzeschik
2017-06-25 19:22 ` David Miller
2017-11-06  9:26 ` Nicolas Ferre
2017-11-06 11:10   ` [PATCH 1/2] net: macb: add of_phy_deregister_fixed_link to error paths Michael Grzeschik
2017-11-06 11:10     ` [PATCH 2/2] net: macb: add of_node_put " Michael Grzeschik
2017-11-07  9:27       ` Nicolas Ferre
2017-11-07  9:59         ` [PATCH v2] " Michael Grzeschik
2017-11-07 11:12           ` Nicolas Ferre
2017-11-08  6:29           ` David Miller
2017-11-07 10:00         ` [PATCH 2/2] " Michael Grzeschik
2017-11-06 14:37     ` [PATCH 1/2] net: macb: add of_phy_deregister_fixed_link " Nicolas Ferre
2017-11-08  4:22     ` David Miller
2017-11-08  8:41       ` Michael Grzeschik

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.