diff mbox series

[v4,net-next,3/4] net: macb: Add phy-handle DT support

Message ID 20180312213435.115174-4-brad.mouring@ni.com
State Superseded, archived
Delegated to: David Miller
Headers show
Series [v4,net-next,1/4] net: macb: Reorganize macb_mii bringup | expand

Commit Message

Brad Mouring March 12, 2018, 9:34 p.m. UTC 
This optional binding (as described in the ethernet DT bindings doc)
directs the netdev to the phydev to use. This is useful for a phy
chip that has >1 phy in it, and two netdevs are using the same phy
chip (i.e. the second mac's phy lives on the first mac's MDIO bus)

The devicetree snippet would look something like this:

ethernet@feedf00d {
	...
	phy-handle = <&phy0> // the first netdev is physically wired to phy0
	...
	phy0: phy@0 {
		...
		reg = <0x0> // MDIO address 0
		...
	}
	phy1: phy@1 {
		...
		reg = <0x1> // MDIO address 1
		...
	}
...
}

ethernet@deadbeef {
	...
	phy-handle = <&phy1> // tells the driver to use phy1 on the
						 // first mac's mdio bus (it's wired thusly)
	...
}

The work done to add the phy_node in the first place (dacdbb4dfc1a1:
"net: macb: add fixed-link node support") will consume the
device_node (if found).

Signed-off-by: Brad Mouring <brad.mouring@ni.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 34 ++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 15 deletions(-)

Comments

Andrew Lunn March 12, 2018, 9:57 p.m. UTC | #1 
> +			/* attempt to find a phy-handle */
> +			if (!(bp->phy_node = of_parse_phandle(np, "phy-handle", 0))) {
> +
> +				/* 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;
> +						}

Hi Brad

./scipts/checkpatch.pl ~/brad.mouring 
WARNING: line over 80 characters
#122: FILE: drivers/net/ethernet/cadence/macb_main.c:492:
+		if (!(bp->phy_node = of_parse_phandle(np, "phy-handle", 0))) {

ERROR: do not use assignment in if condition
#122: FILE: drivers/net/ethernet/cadence/macb_main.c:492:
+		if (!(bp->phy_node = of_parse_phandle(np, "phy-handle", 0))) {

CHECK: Blank lines aren't necessary after an open brace '{'
#123: FILE: drivers/net/ethernet/cadence/macb_main.c:493:
+		if (!(bp->phy_node = of_parse_phandle(np, "phy-handle", 0))) {
+

WARNING: line over 80 characters
#124: FILE: drivers/net/ethernet/cadence/macb_main.c:494:
+			/* fallback to standard phy registration if no phy were

ERROR: trailing whitespace
#130: FILE: drivers/net/ethernet/cadence/macb_main.c:500:
+^I$

WARNING: line over 80 characters
#131: FILE: drivers/net/ethernet/cadence/macb_main.c:501:
+					phydev = mdiobus_scan(bp->mii_bus, i);

WARNING: Too many leading tabs - consider code refactoring
#132: FILE: drivers/net/ethernet/cadence/macb_main.c:502:
+					if (IS_ERR(phydev) &&

etc

	Andrew
Florian Fainelli March 12, 2018, 10:30 p.m. UTC | #2 
On 03/12/2018 02:57 PM, Andrew Lunn wrote:
>> +			/* attempt to find a phy-handle */
>> +			if (!(bp->phy_node = of_parse_phandle(np, "phy-handle", 0))) {
>> +
>> +				/* 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;
>> +						}
> 
> Hi Brad

I would be a bit relaxed on these warnings because the existing function
indentation really makes it easy to be over 80 columns. Unless you have
a preliminary patch which, as I was suggesting earlier, re-arranges the
branches such that if (!np) is the first thing you test, I don't see how
this can look any better...

> 
> ./scipts/checkpatch.pl ~/brad.mouring 
> WARNING: line over 80 characters
> #122: FILE: drivers/net/ethernet/cadence/macb_main.c:492:
> +		if (!(bp->phy_node = of_parse_phandle(np, "phy-handle", 0))) {
> 
> ERROR: do not use assignment in if condition
> #122: FILE: drivers/net/ethernet/cadence/macb_main.c:492:
> +		if (!(bp->phy_node = of_parse_phandle(np, "phy-handle", 0))) {
> 
> CHECK: Blank lines aren't necessary after an open brace '{'
> #123: FILE: drivers/net/ethernet/cadence/macb_main.c:493:
> +		if (!(bp->phy_node = of_parse_phandle(np, "phy-handle", 0))) {
> +
> 
> WARNING: line over 80 characters
> #124: FILE: drivers/net/ethernet/cadence/macb_main.c:494:
> +			/* fallback to standard phy registration if no phy were
> 
> ERROR: trailing whitespace
> #130: FILE: drivers/net/ethernet/cadence/macb_main.c:500:
> +^I$
> 
> WARNING: line over 80 characters
> #131: FILE: drivers/net/ethernet/cadence/macb_main.c:501:
> +					phydev = mdiobus_scan(bp->mii_bus, i);
> 
> WARNING: Too many leading tabs - consider code refactoring
> #132: FILE: drivers/net/ethernet/cadence/macb_main.c:502:
> +					if (IS_ERR(phydev) &&
> 
> etc
> 
> 	Andrew
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Brad Mouring March 13, 2018, 1:49 p.m. UTC | #3 
On Mon, Mar 12, 2018 at 03:30:53PM -0700, Florian Fainelli wrote:
> On 03/12/2018 02:57 PM, Andrew Lunn wrote:
> >> +			/* attempt to find a phy-handle */
> >> +			if (!(bp->phy_node = of_parse_phandle(np, "phy-handle", 0))) {
> >> +
> >> +				/* 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;
> >> +						}
> > 
> > Hi Brad
> 
> I would be a bit relaxed on these warnings because the existing function
> indentation really makes it easy to be over 80 columns. Unless you have
> a preliminary patch which, as I was suggesting earlier, re-arranges the
> branches such that if (!np) is the first thing you test, I don't see how
> this can look any better...

I'll try to work this a bit more, thanks for the feedback and the
patience.
Brad Mouring March 13, 2018, 9:32 p.m. UTC | #4 
Consider the situation where a macb netdev is connected through
a phydev that sits on a mii bus other than the one provided to
this particular netdev. This situation is what this patchset aims
to accomplish through the existing phy-handle optional binding.

This optional binding (as described in the ethernet DT bindings doc)
directs the netdev to the phydev to use. This is precisely the
situation this patchset aims to solve, so it makes sense to introduce
the functionality to this driver (where the physical layout discussed
was encountered).

The devicetree snippet would look something like this:

...
   ethernet@feedf00d {
           ...
           phy-handle = <&phy0> // the first netdev is physically wired to phy0
           ...
           phy0: phy@0 {
                   ...
                   reg = <0x0> // MDIO address 0
                   ...
           }
           phy1: phy@1 {
                   ...
                   reg = <0x1> // MDIO address 1
                   ...
           }
           ...
   }

   ethernet@deadbeef {
           ...
           phy-handle = <&phy1> // tells the driver to use phy1 on the
                                // first mac's mdio bus (it's wired thusly)
           ...
   }
...

The work done to add the phy_node in the first place (dacdbb4dfc1a1:
"net: macb: add fixed-link node support") will consume the
device_node (if found).

v2: Reorganization of mii probe/init functions, suggested by Andrew Lunn
v3: Moved some of the bus init code back into init (erroneously moved to probe)
    some style issues, and an unintialized variable warning addressed.
v4: Add Reviewed-by: tags
    Skip fallback code if phy-handle phandle is found
v5: Cleanup formatting issues
    Fix compile failure introduced in 1/4 "net: macb: Reorganize macb_mii
        bringup"
    Fix typo in "Documentation: macb: Document phy-handle binding"
David Miller March 16, 2018, 3:15 p.m. UTC | #5 
From: Brad Mouring <brad.mouring@ni.com>
Date: Tue, 13 Mar 2018 16:32:12 -0500

> Consider the situation where a macb netdev is connected through
> a phydev that sits on a mii bus other than the one provided to
> this particular netdev. This situation is what this patchset aims
> to accomplish through the existing phy-handle optional binding.
> 
> This optional binding (as described in the ethernet DT bindings doc)
> directs the netdev to the phydev to use. This is precisely the
> situation this patchset aims to solve, so it makes sense to introduce
> the functionality to this driver (where the physical layout discussed
> was encountered).
> 
> The devicetree snippet would look something like this:
 ...
> The work done to add the phy_node in the first place (dacdbb4dfc1a1:
> "net: macb: add fixed-link node support") will consume the
> device_node (if found).
> 
> v2: Reorganization of mii probe/init functions, suggested by Andrew Lunn
> v3: Moved some of the bus init code back into init (erroneously moved to probe)
>     some style issues, and an unintialized variable warning addressed.
> v4: Add Reviewed-by: tags
>     Skip fallback code if phy-handle phandle is found
> v5: Cleanup formatting issues
>     Fix compile failure introduced in 1/4 "net: macb: Reorganize macb_mii
>         bringup"
>     Fix typo in "Documentation: macb: Document phy-handle binding"

Series applied, thank you.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index db1dc301bed3..4a2ad35d41aa 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -488,23 +488,27 @@  static int macb_mii_probe(struct net_device *dev)
 			}
 			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;
+			/* attempt to find a phy-handle */
+			if (!(bp->phy_node = of_parse_phandle(np, "phy-handle", 0))) {
+
+				/* 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;
 				}
-
-				if (ret)
-					return -ENODEV;
 			}
 		}
 	}