net: ethernet: support "fixed-link" DT node on nb8800 driver
Subsystems (2)
PATCH 1/1 net: ethernet: support "fixed-link" DT node on nb8800 driver
--- drivers/net/ethernet/aurora/nb8800.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)
drivers/net/ethernet/aurora/nb8800.c +15 -4
@@ -1461,10 +1461,21 @@ static int nb8800_probe(struct platform_device *pdev) } priv->phy_node =of_parse_phandle (pdev->dev.of_node,"phy-handle" ,0 ); -if (!priv->phy_node) { -dev_err (&pdev->dev,"no PHY specified\n" ); - ret = -ENODEV ; - goto err_free_bus; +if (!priv->phy_node) + { +if (of_phy_is_fixed_link (pdev->dev.of_node)) { + ret =of_phy_register_fixed_link (pdev->dev.of_node); +if (ret <0 ) { +dev_err (&pdev->dev,"broken fixed-link specification\n" ); + goto err_free_bus; + } + priv->phy_node =of_node_get (pdev->dev.of_node); + } +else { +dev_err (&pdev->dev,"no PHY specified\n" ); + ret = -ENODEV ; + goto err_free_bus; + } } priv->mii_bus = bus;
Signature · 1 line
2.1.4
Quoted 15 lines · Sebastian Frias
Signed-off-by Sebastian Frias <[email protected]>--- drivers/net/ethernet/aurora/nb8800.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)drivers/net/ethernet/aurora/nb8800.c +0
-0@@ -1461,10 +1461,21 @@ static int nb8800_probe(struct platform_device *pdev) } priv->phy_node =of_parse_phandle (pdev->dev.of_node,"phy-handle" ,0 );
drivers/net/ethernet/aurora/nb8800.c static int nb8800_probe(struct platform_device · line 1470
Quoted 10 lines
drivers/net/ethernet/aurora/nb8800.c +6
-4-if (!priv->phy_node) { -dev_err (&pdev->dev,"no PHY specified\n" ); - ret = -ENODEV ; - goto err_free_bus; +if (!priv->phy_node) + { +if (of_phy_is_fixed_link (pdev->dev.of_node)) { + ret =of_phy_register_fixed_link (pdev->dev.of_node); +if (ret <0 ) { +dev_err (&pdev->dev,"broken fixed-link specification\n" );
Line is longer than 80 chars.
drivers/net/ethernet/aurora/nb8800.c static int nb8800_probe(struct platform_device *pdev) · line 1474
Quoted 5 lines
drivers/net/ethernet/aurora/nb8800.c +5
-0+ goto err_free_bus; + } + priv->phy_node =of_node_get (pdev->dev.of_node); + } +else {
Wrong brace placement.
Quoted 10 lines
drivers/net/ethernet/aurora/nb8800.c +4
-0+dev_err (&pdev->dev,"no PHY specified\n" ); + ret = -ENODEV ; + goto err_free_bus; + } }
priv->mii_bus= bus;Signature · 1 line
2.1.4
Signature · 1 line
Måns Rullgård
PATCH v2 1/1 net: ethernet: support "fixed-link" DT key/node on nb8800 driver
--- drivers/net/ethernet/aurora/nb8800.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)
drivers/net/ethernet/aurora/nb8800.c +12 -3
@@ -1462,9 +1462,18 @@ static int nb8800_probe(struct platform_device *pdev) priv->phy_node =of_parse_phandle (pdev->dev.of_node,"phy-handle" ,0 );if (!priv->phy_node) { -dev_err (&pdev->dev,"no PHY specified\n" ); - ret = -ENODEV ; - goto err_free_bus; +if (of_phy_is_fixed_link (pdev->dev.of_node)) { + ret =of_phy_register_fixed_link (pdev->dev.of_node); +if (ret <0 ) { +dev_err (&pdev->dev,"bad fixed-link spec\n" ); + goto err_free_bus; + } + priv->phy_node =of_node_get (pdev->dev.of_node); + }else { +dev_err (&pdev->dev,"no PHY specified\n" ); + ret = -ENODEV ; + goto err_free_bus; + } } priv->mii_bus = bus;
Signature · 1 line
2.1.4
Re: [PATCH v2] net: ethernet: support "fixed-link" DT key/node on nb8800 driver
Quoted 20 lines · Sebastian Frias
Signed-off-by Sebastian Frias <[email protected]>--- drivers/net/ethernet/aurora/nb8800.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)drivers/net/ethernet/aurora/nb8800.c +3
-3@@ -1462,9 +1462,18 @@ static int nb8800_probe(struct platform_device *pdev) priv->phy_node =of_parse_phandle (pdev->dev.of_node,"phy-handle" ,0 );if (!priv->phy_node) { -dev_err (&pdev->dev,"no PHY specified\n" ); - ret = -ENODEV ; - goto err_free_bus; +if (of_phy_is_fixed_link (pdev->dev.of_node)) { + ret =of_phy_register_fixed_link (pdev->dev.of_node); +if (ret <0 ) {
drivers/net/ethernet/aurora/nb8800.c static int nb8800_probe(struct platform_device *pdev) · line 1477
Quoted 10 lines
drivers/net/ethernet/aurora/nb8800.c +9
-0+dev_err (&pdev->dev,"bad fixed-link spec\n" ); + goto err_free_bus; + } + priv->phy_node =of_node_get (pdev->dev.of_node); + }else { +dev_err (&pdev->dev,"no PHY specified\n" ); + ret = -ENODEV ; + goto err_free_bus; + } }
Maybe it would be clearer to reduce the if() nesting a bit, like this for instance:
if (of_phy_is_fixed_link(pdev->dev.of_node)) {
ret = of_phy_register_fixed_link(pdev->dev.of_node);
if (ret < 0) {
dev_err(&pdev->dev, "bad fixed-link spec\n");
goto err_free_bus;
}
priv->phy_node = of_node_get(pdev->dev.of_node);
}
if (!priv->phy_node)
priv->phy_node = of_parse_phandle(pdev->dev.of_node,
"phy-handle", 0);
if (!priv->phy_node) {
dev_err(&pdev->dev, "no PHY specified\n");
ret = -ENODEV;
goto err_free_bus;
}Signature · 1 line
Måns Rullgård
Re: [PATCH v2] net: ethernet: support "fixed-link" DT key/node on nb8800 driver
Quoted 46 lines · Måns Rullgård
Quoted 20 lines · Sebastian Frias
Signed-off-by Sebastian Frias <[email protected]>--- drivers/net/ethernet/aurora/nb8800.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)drivers/net/ethernet/aurora/nb8800.c +3
-3@@ -1462,9 +1462,18 @@ static int nb8800_probe(struct platform_device *pdev) priv->phy_node =of_parse_phandle (pdev->dev.of_node,"phy-handle" ,0 );if (!priv->phy_node) { -dev_err (&pdev->dev,"no PHY specified\n" ); - ret = -ENODEV ; - goto err_free_bus; +if (of_phy_is_fixed_link (pdev->dev.of_node)) { + ret =of_phy_register_fixed_link (pdev->dev.of_node); +if (ret <0 ) {drivers/net/ethernet/aurora/nb8800.c static int nb8800_probe(struct platform_device *pdev) · line 1477
Quoted 10 lines
drivers/net/ethernet/aurora/nb8800.c +9
-0+dev_err (&pdev->dev,"bad fixed-link spec\n" ); + goto err_free_bus; + } + priv->phy_node =of_node_get (pdev->dev.of_node); + }else { +dev_err (&pdev->dev,"no PHY specified\n" ); + ret = -ENODEV ; + goto err_free_bus; + } }Maybe it would be clearer to reduce the
if()nesting a bit, like this for instance:if (of_phy_is_fixed_link(pdev->dev.of_node)) { ret = of_phy_register_fixed_link(pdev->dev.of_node); if (ret < 0) { dev_err(&pdev->dev, "bad fixed-link spec\n"); goto err_free_bus; } priv->phy_node = of_node_get(pdev->dev.of_node); } if (!priv->phy_node)
Quoted 10 lines
priv->phy_node = of_parse_phandle(pdev->dev.of_node,
"phy-handle", 0);
if (!priv->phy_node) {
dev_err(&pdev->dev, "no PHY specified\n");
ret = -ENODEV;
goto err_free_bus;
}Thanks Måns for your comments.
With old code + my patch, we only hit 1 comparison in the general case, and a 2nd one in "fixed-link" case. With your suggestion above, it would mean that we hit 3 comparisons all the time.
If you are ok with the 3 comparisons, I can post a v3.
Re: [PATCH v2] net: ethernet: support "fixed-link" DT key/node on nb8800 driver
Quoted 58 lines · Sebastian Frias
Quoted 46 lines · Måns Rullgård
Quoted 20 lines · Sebastian Frias
Signed-off-by Sebastian Frias <[email protected]>--- drivers/net/ethernet/aurora/nb8800.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)drivers/net/ethernet/aurora/nb8800.c +3
-3@@ -1462,9 +1462,18 @@ static int nb8800_probe(struct platform_device *pdev) priv->phy_node =of_parse_phandle (pdev->dev.of_node,"phy-handle" ,0 );if (!priv->phy_node) { -dev_err (&pdev->dev,"no PHY specified\n" ); - ret = -ENODEV ; - goto err_free_bus; +if (of_phy_is_fixed_link (pdev->dev.of_node)) { + ret =of_phy_register_fixed_link (pdev->dev.of_node); +if (ret <0 ) {drivers/net/ethernet/aurora/nb8800.c static int nb8800_probe(struct platform_device *pdev) · line 1477
Quoted 10 lines
drivers/net/ethernet/aurora/nb8800.c +9
-0+dev_err (&pdev->dev,"bad fixed-link spec\n" ); + goto err_free_bus; + } + priv->phy_node =of_node_get (pdev->dev.of_node); + }else { +dev_err (&pdev->dev,"no PHY specified\n" ); + ret = -ENODEV ; + goto err_free_bus; + } }Maybe it would be clearer to reduce the
if()nesting a bit, like this for instance:if (of_phy_is_fixed_link(pdev->dev.of_node)) { ret = of_phy_register_fixed_link(pdev->dev.of_node); if (ret < 0) { dev_err(&pdev->dev, "bad fixed-link spec\n"); goto err_free_bus; } priv->phy_node = of_node_get(pdev->dev.of_node); } if (!priv->phy_node)Quoted 10 lines
priv->phy_node = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0); if (!priv->phy_node) { dev_err(&pdev->dev, "no PHY specified\n"); ret = -ENODEV; goto err_free_bus; }
Quoted 6 lines
Thanks Måns for your comments.
With old code + my patch, we only hit 1 comparison in the general case, and a 2nd one in "fixed-link" case.
With your suggestion above, it would mean that we hit 3 comparisons all the time.
If you are ok with the 3 comparisons, I can post a v3.
This is code that runs once so IMO clarity is more important than a minuscule speed difference.
Signature · 1 line
Måns Rullgård
PATCH v3 1/1 net: ethernet: support "fixed-link" DT node on nb8800 driver
--- drivers/net/ethernet/aurora/nb8800.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
drivers/net/ethernet/aurora/nb8800.c +13 -1
@@ -1460,7 +1460,19 @@ static int nb8800_probe(struct platform_device *pdev) goto err_disable_clk; } - priv->phy_node =of_parse_phandle (pdev->dev.of_node,"phy-handle" ,0 ); +if (of_phy_is_fixed_link (pdev->dev.of_node)) { + ret =of_phy_register_fixed_link (pdev->dev.of_node); +if (ret <0 ) { +dev_err (&pdev->dev,"bad fixed-link spec\n" ); + goto err_free_bus; + } + priv->phy_node =of_node_get (pdev->dev.of_node); + } + +if (!priv->phy_node) + priv->phy_node =of_parse_phandle (pdev->dev.of_node, +"phy-handle" ,0 ); +if (!priv->phy_node) {dev_err (&pdev->dev,"no PHY specified\n" ); ret = -ENODEV ;
Signature · 1 line
2.1.4
Quoted 1 line · Sebastian Frias
Signed-off-by Sebastian Frias <[email protected]>
Please change the subject to something like "net: ethernet: nb8800:
support fixed-link DT node" and add a comment body.
Quoted 32 lines
--- drivers/net/ethernet/aurora/nb8800.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)drivers/net/ethernet/aurora/nb8800.c +13
-1@@ -1460,7 +1460,19 @@ static int nb8800_probe(struct platform_device *pdev) goto err_disable_clk; } - priv->phy_node =of_parse_phandle (pdev->dev.of_node,"phy-handle" ,0 ); +if (of_phy_is_fixed_link (pdev->dev.of_node)) { + ret =of_phy_register_fixed_link (pdev->dev.of_node); +if (ret <0 ) { +dev_err (&pdev->dev,"bad fixed-link spec\n" ); + goto err_free_bus; + } + priv->phy_node =of_node_get (pdev->dev.of_node); + } + +if (!priv->phy_node) + priv->phy_node =of_parse_phandle (pdev->dev.of_node, +"phy-handle" ,0 ); +if (!priv->phy_node) {dev_err (&pdev->dev,"no PHY specified\n" ); ret = -ENODEV ;Signature · 1 line
2.1.4
Signature · 1 line
Måns Rullgård
Quoted 6 lines · Måns Rullgård
Quoted 1 line · Sebastian Frias
Signed-off-by Sebastian Frias <[email protected]>Please change the subject to something like "net: ethernet: nb8800:
support fixed-link DT node" and add a comment body.
The subject is pretty explicit for such a simple patch, what else could I add that wouldn't be unnecessary chat?
Quoted 34 lines
Quoted 33 lines
--- drivers/net/ethernet/aurora/nb8800.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)drivers/net/ethernet/aurora/nb8800.c +13
-2@@ -1460,7 +1460,19 @@ static int nb8800_probe(struct platform_device *pdev) goto err_disable_clk; } - priv->phy_node =of_parse_phandle (pdev->dev.of_node,"phy-handle" ,0 ); +if (of_phy_is_fixed_link (pdev->dev.of_node)) { + ret =of_phy_register_fixed_link (pdev->dev.of_node); +if (ret <0 ) { +dev_err (&pdev->dev,"bad fixed-link spec\n" ); + goto err_free_bus; + } + priv->phy_node =of_node_get (pdev->dev.of_node); + } + +if (!priv->phy_node) + priv->phy_node =of_parse_phandle (pdev->dev.of_node, +"phy-handle" ,0 ); +if (!priv->phy_node) {dev_err (&pdev->dev,"no PHY specified\n" ); ret = -ENODEV ; --2.1.4
PATCH v4 1/1 net: ethernet: nb8800: support fixed-link DT node
In reply to Måns RullgårdProperly handles the case where the PHY is not connected to the real MDIO bus
--- drivers/net/ethernet/aurora/nb8800.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
drivers/net/ethernet/aurora/nb8800.c +13 -1
@@ -1460,7 +1460,19 @@ static int nb8800_probe(struct platform_device *pdev) goto err_disable_clk; } - priv->phy_node =of_parse_phandle (pdev->dev.of_node,"phy-handle" ,0 ); +if (of_phy_is_fixed_link (pdev->dev.of_node)) { + ret =of_phy_register_fixed_link (pdev->dev.of_node); +if (ret <0 ) { +dev_err (&pdev->dev,"bad fixed-link spec\n" ); + goto err_free_bus; + } + priv->phy_node =of_node_get (pdev->dev.of_node); + } + +if (!priv->phy_node) + priv->phy_node =of_parse_phandle (pdev->dev.of_node, +"phy-handle" ,0 ); +if (!priv->phy_node) {dev_err (&pdev->dev,"no PHY specified\n" ); ret = -ENODEV ;
Signature · 1 line
2.1.4
Quoted 10 lines · Sebastian Frias
Quoted 6 lines · Måns Rullgård
Quoted 1 line · Sebastian Frias
Signed-off-by Sebastian Frias <[email protected]>Please change the subject to something like "net: ethernet: nb8800:
support fixed-link DT node" and add a comment body.The subject is pretty explicit for such a simple patch, what else could I add that wouldn't be unnecessary chat?
It's customary to include a description body even if it's little more than a restatement of the subject. Also, while the subject usually only says _what_ the patch does, the body should additionally state _why_ it is needed.
Quoted 33 lines
Quoted 33 lines
Quoted 32 lines
--- drivers/net/ethernet/aurora/nb8800.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)drivers/net/ethernet/aurora/nb8800.c +13
-2@@ -1460,7 +1460,19 @@ static int nb8800_probe(struct platform_device *pdev) goto err_disable_clk; } - priv->phy_node =of_parse_phandle (pdev->dev.of_node,"phy-handle" ,0 ); +if (of_phy_is_fixed_link (pdev->dev.of_node)) { + ret =of_phy_register_fixed_link (pdev->dev.of_node); +if (ret <0 ) { +dev_err (&pdev->dev,"bad fixed-link spec\n" ); + goto err_free_bus; + } + priv->phy_node =of_node_get (pdev->dev.of_node); + } + +if (!priv->phy_node) + priv->phy_node =of_parse_phandle (pdev->dev.of_node, +"phy-handle" ,0 ); +if (!priv->phy_node) {dev_err (&pdev->dev,"no PHY specified\n" ); ret = -ENODEV ; --2.1.4
Signature · 1 line
Måns Rullgård
Quoted 10 lines · Måns Rullgård
Quoted 8 lines · Sebastian Frias
Quoted 6 lines · Måns Rullgård
Quoted 1 line · Sebastian Frias
Signed-off-by Sebastian Frias <[email protected]>Please change the subject to something like "net: ethernet: nb8800:
support fixed-link DT node" and add a comment body.
Quoted 7 lines
Quoted 2 lines
The subject is pretty explicit for such a simple patch, what else could I add that wouldn't be unnecessary chat?
It's customary to include a description body even if it's little more than a restatement of the subject. Also, while the subject usually only says _what_ the patch does, the body should additionally state _why_ it is needed.
I understand, but _why_ it is needed is also obvious in this case; I mean, without the patch "fixed-link" cannot be used. Other patches may not be as obvious/simple and thus justify and require more details.
Anyway, I added "Properly handles the case where the PHY is not connected to the real MDIO bus" would that be ok?
Quoted 35 lines
Quoted 34 lines
Quoted 34 lines
Quoted 33 lines
--- drivers/net/ethernet/aurora/nb8800.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)drivers/net/ethernet/aurora/nb8800.c +13
-2@@ -1460,7 +1460,19 @@ static int nb8800_probe(struct platform_device *pdev) goto err_disable_clk; } - priv->phy_node =of_parse_phandle (pdev->dev.of_node,"phy-handle" ,0 ); +if (of_phy_is_fixed_link (pdev->dev.of_node)) { + ret =of_phy_register_fixed_link (pdev->dev.of_node); +if (ret <0 ) { +dev_err (&pdev->dev,"bad fixed-link spec\n" ); + goto err_free_bus; + } + priv->phy_node =of_node_get (pdev->dev.of_node); + } + +if (!priv->phy_node) + priv->phy_node =of_parse_phandle (pdev->dev.of_node, +"phy-handle" ,0 ); +if (!priv->phy_node) {dev_err (&pdev->dev,"no PHY specified\n" ); ret = -ENODEV ; --2.1.4
Quoted 14 lines · Sebastian Frias
Quoted 13 lines · Måns Rullgård
Quoted 10 lines · Sebastian Frias
Quoted 6 lines · Måns Rullgård
Quoted 1 line · Sebastian Frias
Signed-off-by Sebastian Frias <[email protected]>Please change the subject to something like "net: ethernet: nb8800:
support fixed-link DT node" and add a comment body.The subject is pretty explicit for such a simple patch, what else could I add that wouldn't be unnecessary chat?
Quoted 7 lines
Quoted 4 lines
It's customary to include a description body even if it's little more than a restatement of the subject. Also, while the subject usually only says _what_ the patch does, the body should additionally state _why_ it is needed.
I understand, but _why_ it is needed is also obvious in this case; I mean, without the patch "fixed-link" cannot be used.
Then say so.
Quoted 5 lines
Other patches may not be as obvious/simple and thus justify and require more details.
Anyway, I added "Properly handles the case where the PHY is not connected to the real MDIO bus" would that be ok?
Have you read Documentation/SubmittingPatches? Do so (again) and pay special attention to section 2 "Describe your changes."
Quoted 34 lines
Quoted 34 lines
Quoted 33 lines
Quoted 33 lines
Quoted 32 lines
--- drivers/net/ethernet/aurora/nb8800.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)drivers/net/ethernet/aurora/nb8800.c +13
-2@@ -1460,7 +1460,19 @@ static int nb8800_probe(struct platform_device *pdev) goto err_disable_clk; } - priv->phy_node =of_parse_phandle (pdev->dev.of_node,"phy-handle" ,0 ); +if (of_phy_is_fixed_link (pdev->dev.of_node)) { + ret =of_phy_register_fixed_link (pdev->dev.of_node); +if (ret <0 ) { +dev_err (&pdev->dev,"bad fixed-link spec\n" ); + goto err_free_bus; + } + priv->phy_node =of_node_get (pdev->dev.of_node); + } + +if (!priv->phy_node) + priv->phy_node =of_parse_phandle (pdev->dev.of_node, +"phy-handle" ,0 ); +if (!priv->phy_node) {dev_err (&pdev->dev,"no PHY specified\n" ); ret = -ENODEV ; --2.1.4
Signature · 1 line
Måns Rullgård
Quoted 4 lines · Måns Rullgård
drivers/net/ethernet/aurora/nb8800.c static int nb8800_probe(struct platform_device *pdev) · line 1469
Quoted 2 lines
drivers/net/ethernet/aurora/nb8800.c +2
-0+if (ret <0 ) { +dev_err (&pdev->dev,"broken fixed-link specification\n" );Line is longer than 80 chars.
This is actually okay, though I would recommend to move long string literal to the next line.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 8 lines · Andy Shevchenko
Quoted 4 lines · Måns Rullgård
drivers/net/ethernet/aurora/nb8800.c static int nb8800_probe(struct platform_device *pdev) · line 1469
Quoted 2 lines
drivers/net/ethernet/aurora/nb8800.c +2
-0+if (ret <0 ) { +dev_err (&pdev->dev,"broken fixed-link specification\n" );Line is longer than 80 chars.
This is actually okay, though I would recommend to move long string literal to the next line.
I only mentioned it because fixing it was trivial.
Signature · 1 line
Måns Rullgård
PATCH v5 1/1 net: ethernet: nb8800: support fixed-link DT node
In reply to Måns RullgårdUnder some circumstances, for example when connecting to a switch:
https://stackoverflow.com/questions/31046172/device-tree-for-phy-less-connection-to-a-dsa-switch
the ethernet port will not be connected to a PHY.
In that case a "fixed-link" DT node can be used to replace it.
This patch adds support for the "fixed-link" node to the nb8800 driver.
--- drivers/net/ethernet/aurora/nb8800.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
drivers/net/ethernet/aurora/nb8800.c +13 -1
@@ -1460,7 +1460,19 @@ static int nb8800_probe(struct platform_device *pdev) goto err_disable_clk; } - priv->phy_node =of_parse_phandle (pdev->dev.of_node,"phy-handle" ,0 ); +if (of_phy_is_fixed_link (pdev->dev.of_node)) { + ret =of_phy_register_fixed_link (pdev->dev.of_node); +if (ret <0 ) { +dev_err (&pdev->dev,"bad fixed-link spec\n" ); + goto err_free_bus; + } + priv->phy_node =of_node_get (pdev->dev.of_node); + } + +if (!priv->phy_node) + priv->phy_node =of_parse_phandle (pdev->dev.of_node, +"phy-handle" ,0 ); +if (!priv->phy_node) {dev_err (&pdev->dev,"no PHY specified\n" ); ret = -ENODEV ;
Signature · 1 line
2.1.4
Quoted 26 lines · Måns Rullgård
Quoted 14 lines · Sebastian Frias
Quoted 13 lines · Måns Rullgård
Quoted 10 lines · Sebastian Frias
Quoted 6 lines · Måns Rullgård
Quoted 1 line · Sebastian Frias
Signed-off-by Sebastian Frias <[email protected]>Please change the subject to something like "net: ethernet: nb8800:
support fixed-link DT node" and add a comment body.The subject is pretty explicit for such a simple patch, what else could I add that wouldn't be unnecessary chat?
Quoted 7 lines
Quoted 4 lines
It's customary to include a description body even if it's little more than a restatement of the subject. Also, while the subject usually only says _what_ the patch does, the body should additionally state _why_ it is needed.
I understand, but _why_ it is needed is also obvious in this case; I mean, without the patch "fixed-link" cannot be used.
Then say so.
Quoted 8 lines
Quoted 5 lines
Other patches may not be as obvious/simple and thus justify and require more details.
Anyway, I added "Properly handles the case where the PHY is not connected to the real MDIO bus" would that be ok?
Have you read
Documentation/SubmittingPatches? Do so (again) and pay special attention to section 2 "Describe your changes."
I just sent v5.
If for whatever reason, you or anybody else think that the comment is not good, would you mind proposing a comment that would make everybody happy so that the patch goes thru? And if you or anybody else does not want the patch, could you please say so as well?
I have to admit this process (sending patches and getting it reviewed) could benefit from more clarifications.
For example, the process could say that at least 2 reviewers must agree on it (on the comments made to the patch and on the patch itself). I could also say that reviewers are to express not only their opinion but to clearly and unequivocally accept or reject.
For instance, right now, it is not clear to me if your comments are "nice to have" or "blocking" the patch. I don't know if the patch is welcome or not, etc.
So I submitted v5, but maybe it was not even necessary, it's hard to know where in the submission process we are.
By the way, I know some people like the command line, email, etc. but there ought to be other tools better suited for patch review...
Quoted 36 lines
Quoted 35 lines
Quoted 35 lines
Quoted 34 lines
Quoted 34 lines
Quoted 33 lines
--- drivers/net/ethernet/aurora/nb8800.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)drivers/net/ethernet/aurora/nb8800.c +13
-2@@ -1460,7 +1460,19 @@ static int nb8800_probe(struct platform_device *pdev) goto err_disable_clk; } - priv->phy_node =of_parse_phandle (pdev->dev.of_node,"phy-handle" ,0 ); +if (of_phy_is_fixed_link (pdev->dev.of_node)) { + ret =of_phy_register_fixed_link (pdev->dev.of_node); +if (ret <0 ) { +dev_err (&pdev->dev,"bad fixed-link spec\n" ); + goto err_free_bus; + } + priv->phy_node =of_node_get (pdev->dev.of_node); + } + +if (!priv->phy_node) + priv->phy_node =of_parse_phandle (pdev->dev.of_node, +"phy-handle" ,0 ); +if (!priv->phy_node) {dev_err (&pdev->dev,"no PHY specified\n" ); ret = -ENODEV ; --2.1.4
Re: [PATCH v5] net: ethernet: nb8800: support fixed-link DT node
In reply to Sebastian FriasQuoted 5 lines · Sebastian Frias
Under some circumstances, for example when connecting to a switch:
https://stackoverflow.com/questions/31046172/device-tree-for-phy-less-connection-to-a-dsa-switch
Quoted 7 lines
the ethernet port will not be connected to a PHY.
In that case a "fixed-link" DT node can be used to replace it.This patch adds support for the "fixed-link" node to the nb8800 driver.
Signed-off-by Sebastian Frias <[email protected]>
Quoted 33 lines
--- drivers/net/ethernet/aurora/nb8800.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)drivers/net/ethernet/aurora/nb8800.c +13
-1@@ -1460,7 +1460,19 @@ static int nb8800_probe(struct platform_device *pdev) goto err_disable_clk; } - priv->phy_node =of_parse_phandle (pdev->dev.of_node,"phy-handle" ,0 ); +if (of_phy_is_fixed_link (pdev->dev.of_node)) { + ret =of_phy_register_fixed_link (pdev->dev.of_node); +if (ret <0 ) { +dev_err (&pdev->dev,"bad fixed-link spec\n" ); + goto err_free_bus; + } + priv->phy_node =of_node_get (pdev->dev.of_node); + } + +if (!priv->phy_node) + priv->phy_node =of_parse_phandle (pdev->dev.of_node, +"phy-handle" ,0 ); +if (!priv->phy_node) {dev_err (&pdev->dev,"no PHY specified\n" ); ret = -ENODEV ;Signature · 1 line
2.1.4
Signature · 1 line
Måns Rullgård
Quoted 30 lines · Sebastian Frias
Quoted 29 lines · Måns Rullgård
Quoted 14 lines · Sebastian Frias
Quoted 13 lines · Måns Rullgård
Quoted 10 lines · Sebastian Frias
Quoted 6 lines · Måns Rullgård
Quoted 1 line · Sebastian Frias
Signed-off-by Sebastian Frias <[email protected]>Please change the subject to something like "net: ethernet: nb8800:
support fixed-link DT node" and add a comment body.The subject is pretty explicit for such a simple patch, what else could I add that wouldn't be unnecessary chat?
Quoted 7 lines
Quoted 4 lines
It's customary to include a description body even if it's little more than a restatement of the subject. Also, while the subject usually only says _what_ the patch does, the body should additionally state _why_ it is needed.
I understand, but _why_ it is needed is also obvious in this case; I mean, without the patch "fixed-link" cannot be used.
Then say so.
Quoted 3 lines
Other patches may not be as obvious/simple and thus justify and require more details.
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
Anyway, I added "Properly handles the case where the PHY is not connected to the real MDIO bus" would that be ok?
Have you read
Documentation/SubmittingPatches? Do so (again) and pay special attention to section 2 "Describe your changes."I just sent v5.
Thanks for your patience.
Quoted 14 lines
If for whatever reason, you or anybody else think that the comment is not good, would you mind proposing a comment that would make everybody happy so that the patch goes thru?
And if you or anybody else does not want the patch, could you please say so as well?I have to admit this process (sending patches and getting it reviewed) could benefit from more clarifications.
For example, the process could say that at least 2 reviewers must agree on it (on the comments made to the patch and on the patch itself).
I could also say that reviewers are to express not only their opinion but to clearly and unequivocally accept or reject.
Quoted 5 lines
For instance, right now, it is not clear to me if your comments are "nice to have" or "blocking" the patch.
I don't know if the patch is welcome or not, etc.
So I submitted v5, but maybe it was not even necessary, it's hard to know where in the submission process we are.
In this case, it's ultimately up to Dave Miller. He'll take into account whatever comments others have made and decide whether he wants to accept it.
Quoted 2 lines
By the way, I know some people like the command line, email, etc. but there ought to be other tools better suited for patch review...
Some kernel subsystems use http://patchwork.ozlabs.org/ to track status of various patches.
Signature · 1 line
Måns Rullgård
Quoted 7 lines · Måns Rullgård
Quoted 2 lines · Sebastian Frias
By the way, I know some people like the command line, email, etc. but there ought to be other tools better suited for patch review...
Some kernel subsystems use http://patchwork.ozlabs.org/ to track status of various patches.
There's also a kernel bugzilla, but it may be for actual bugs.
https://bugzilla.kernel.org/
Quoted 35 lines · Måns Rullgård
Quoted 33 lines · Sebastian Frias
Quoted 32 lines · Måns Rullgård
Quoted 14 lines · Sebastian Frias
Quoted 13 lines · Måns Rullgård
Quoted 10 lines · Sebastian Frias
Quoted 6 lines · Måns Rullgård
Quoted 1 line · Sebastian Frias
Signed-off-by Sebastian Frias <[email protected]>Please change the subject to something like "net: ethernet: nb8800:
support fixed-link DT node" and add a comment body.The subject is pretty explicit for such a simple patch, what else could I add that wouldn't be unnecessary chat?
Quoted 7 lines
Quoted 4 lines
It's customary to include a description body even if it's little more than a restatement of the subject. Also, while the subject usually only says _what_ the patch does, the body should additionally state _why_ it is needed.
I understand, but _why_ it is needed is also obvious in this case; I mean, without the patch "fixed-link" cannot be used.
Then say so.
Quoted 5 lines
Other patches may not be as obvious/simple and thus justify and require more details.
Anyway, I added "Properly handles the case where the PHY is not connected to the real MDIO bus" would that be ok?
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Have you read
Documentation/SubmittingPatches? Do so (again) and pay special attention to section 2 "Describe your changes."I just sent v5.
Thanks for your patience.
:-)
Quoted 21 lines
Quoted 15 lines
If for whatever reason, you or anybody else think that the comment is not good, would you mind proposing a comment that would make everybody happy so that the patch goes thru?
And if you or anybody else does not want the patch, could you please say so as well?I have to admit this process (sending patches and getting it reviewed) could benefit from more clarifications.
For example, the process could say that at least 2 reviewers must agree on it (on the comments made to the patch and on the patch itself).
I could also say that reviewers are to express not only their opinion but to clearly and unequivocally accept or reject.Quoted 5 lines
For instance, right now, it is not clear to me if your comments are "nice to have" or "blocking" the patch.
I don't know if the patch is welcome or not, etc.
So I submitted v5, but maybe it was not even necessary, it's hard to know where in the submission process we are.
Quoted 3 lines
In this case, it's ultimately up to Dave Miller. He'll take into account whatever comments others have made and decide whether he wants to accept it.
Ok, thanks.
Quoted 7 lines
Quoted 3 lines
By the way, I know some people like the command line, email, etc. but there ought to be other tools better suited for patch review...
Some kernel subsystems use http://patchwork.ozlabs.org/ to track status of various patches.
Thanks, I see that netdev is part of it, and that the patches are there:
https://patchwork.ozlabs.org/patch/580217/
seems like a slight layer over plain email and mailinglists; I was thinking of something more in the line of https://www.gerritcodereview.com/
I believe Google uses Gerrit for Android.
I think Gerrit would probably be too big (and being written in Java, using Prolog and other DSLs, implementing its own Git server in Java, etc, may make some -or lots?- of kernel developers cry :-) ) However, in Gerrit it is easier to know where in the "review" process we are, because people have to explicitly give a score "+/- X" when commenting on a patch.
Also, the diff can operate between different versions of the patches themselves to see if the inlined comments were addressed.
Quoted 4 lines · Mason
Quoted 2 lines · Måns Rullgård
Sebastian Frias wrote:
Quoted 9 lines
Quoted 5 lines
Quoted 2 lines
By the way, I know some people like the command line, email, etc. but there ought to be other tools better suited for patch review...
Some kernel subsystems use http://patchwork.ozlabs.org/ to track status of various patches.
There's also a kernel bugzilla, but it may be for actual bugs.
https://bugzilla.kernel.org/
Thanks, and what would be the definition of a bug?
I mean, would the issue from this thread qualify? Should I have created a bug report before submitting the patch?
Quoted 11 lines · Sebastian Frias
Quoted 10 lines · Mason
Quoted 7 lines · Måns Rullgård
Quoted 2 lines · Sebastian Frias
By the way, I know some people like the command line, email, etc. but there ought to be other tools better suited for patch review...
Some kernel subsystems use http://patchwork.ozlabs.org/ to track status of various patches.
Quoted 7 lines
Quoted 3 lines
There's also a kernel bugzilla, but it may be for actual bugs.
https://bugzilla.kernel.org/Thanks, and what would be the definition of a bug?
I mean, would the issue from this thread qualify? Should I have created a bug report before submitting the patch?
No, that is not necessary. My understanding is that the bugzilla is more of a place to report a bug you've found but don't have a patch for. Patches always go through the mailing lists.
Signature · 1 line
Måns Rullgård
Quoted 11 lines · Sebastian Frias
Quoted 6 lines
Quoted 2 lines
By the way, I know some people like the command line, email, etc. but there ought to be other tools better suited for patch review...
Some kernel subsystems use http://patchwork.ozlabs.org/ to track status of various patches.
Thanks, I see that netdev is part of it, and that the patches are there:
Quoted 12 lines
seems like a slight layer over plain email and mailinglists; I was thinking of something more in the line of
https://www.gerritcodereview.com/
I believe Google uses Gerrit for Android.
I think Gerrit would probably be too big (and being written in Java, using Prolog and other DSLs, implementing its own Git server in Java, etc, may make some -or lots?- of kernel developers cry :-) ) However, in Gerrit it is easier to know where in the "review" process we are, because people have to explicitly give a score "+/- X" when commenting on a patch.
Also, the diff can operate between different versions of the patches themselves to see if the inlined comments were addressed.
Gerrit has some merits, but for seasoned developers it's largely a nuisance. It's probably good at keeping junior/undisciplined developers from doing too much damage by strictly enforcing a cumbersome process.
Signature · 1 line
Måns Rullgård
David Miller
maintainerRe: [PATCH v5] net: ethernet: nb8800: support fixed-link DT node
In reply to Sebastian FriasFrom: Sebastian Frias <[email protected]>
Date: Mon, 08 Feb 2016 11:23:04 +0100
Quoted 6 lines
Under some circumstances, for example when connecting to a switch:
https://stackoverflow.com/questions/31046172/device-tree-for-phy-less-connection-to-a-dsa-switch
Quoted 7 lines
the ethernet port will not be connected to a PHY.
In that case a "fixed-link" DT node can be used to replace it.This patch adds support for the "fixed-link" node to the nb8800 driver.
Signed-off-by Sebastian Frias <[email protected]>
This doesn't apply, please respin against my tree.
Re: [PATCH v5] net: ethernet: nb8800: support fixed-link DT node
Quoted 1 line · David Miller
This doesn't apply, please respin against my tree.
I fixed several formatting issues with Sebastian's patch, and submitted v6.
Regards.