Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1327687 > unrolled thread

[PATCH] net: ethernet: support "fixed-link" DT node on nb8800 driver

Started bySebastian Frias <sf84@laposte.net>
First post2016-02-05 14:40 +0100
Last post2016-02-05 17:00 +0100
Articles 20 on this page of 24 — 4 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] net: ethernet: support "fixed-link" DT node on nb8800 driver Sebastian Frias <sf84@laposte.net> - 2016-02-05 14:40 +0100
    Re: [PATCH] net: ethernet: support "fixed-link" DT node on nb8800 driver Måns Rullgård <mans@mansr.com> - 2016-02-05 14:40 +0100
      [PATCH v2] net: ethernet: support "fixed-link" DT key/node on nb8800  driver Sebastian Frias <sf84@laposte.net> - 2016-02-05 14:50 +0100
        Re: [PATCH v2] net: ethernet: support "fixed-link" DT key/node on nb8800 driver Måns Rullgård <mans@mansr.com> - 2016-02-05 15:00 +0100
          Re: [PATCH v2] net: ethernet: support "fixed-link" DT key/node on  nb8800 driver Sebastian Frias <sf84@laposte.net> - 2016-02-05 15:10 +0100
            Re: [PATCH v2] net: ethernet: support "fixed-link" DT key/node on nb8800 driver Måns Rullgård <mans@mansr.com> - 2016-02-05 15:20 +0100
              [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800  driver Sebastian Frias <sf84@laposte.net> - 2016-02-05 15:30 +0100
                Re: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver Måns Rullgård <mans@mansr.com> - 2016-02-05 15:40 +0100
                  Re: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800  driver Sebastian Frias <sf84@laposte.net> - 2016-02-05 16:00 +0100
                    Re: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver Måns Rullgård <mans@mansr.com> - 2016-02-05 16:10 +0100
                      Re: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver Måns Rullgård <mans@mansr.com> - 2016-02-05 16:30 +0100
                        [PATCH v5] net: ethernet: nb8800: support fixed-link DT node Sebastian Frias <sf84@laposte.net> - 2016-02-08 11:30 +0100
                          Re: [PATCH v5] net: ethernet: nb8800: support fixed-link DT node Måns Rullgård <mans@mansr.com> - 2016-02-08 14:20 +0100
                        Re: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800  driver Sebastian Frias <sf84@laposte.net> - 2016-02-08 11:40 +0100
                          Re: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver Måns Rullgård <mans@mansr.com> - 2016-02-08 14:40 +0100
                            Re: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800  driver Mason <slash.tmp@free.fr> - 2016-02-08 15:20 +0100
                              Re: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800  driver Sebastian Frias <sf84@laposte.net> - 2016-02-08 15:40 +0100
                                Re: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver Måns Rullgård <mans@mansr.com> - 2016-02-08 15:50 +0100
                            Re: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800  driver Sebastian Frias <sf84@laposte.net> - 2016-02-08 15:40 +0100
                              Re: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver Måns Rullgård <mans@mansr.com> - 2016-02-08 16:00 +0100
                      Re: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800  driver Sebastian Frias <sf84@laposte.net> - 2016-02-05 16:30 +0100
                  [PATCH v4] net: ethernet: nb8800: support fixed-link DT node Sebastian Frias <sf84@laposte.net> - 2016-02-05 16:00 +0100
      Re: [PATCH] net: ethernet: support "fixed-link" DT node on nb8800 driver Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-02-05 17:00 +0100
        Re: [PATCH] net: ethernet: support "fixed-link" DT node on nb8800 driver Måns Rullgård <mans@mansr.com> - 2016-02-05 17:00 +0100

Page 1 of 2  [1] 2  Next page →

#1327687 — [PATCH] net: ethernet: support "fixed-link" DT node on nb8800 driver

FromSebastian Frias <sf84@laposte.net>
Date2016-02-05 14:40 +0100
Subject[PATCH] net: ethernet: support "fixed-link" DT node on nb8800 driver
Message-ID<qYOMa-P6-25@gated-at.bofh.it>
Signed-off-by: Sebastian Frias <sf84@laposte.net>
---
  drivers/net/ethernet/aurora/nb8800.c | 19 +++++++++++++++----
  1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/aurora/nb8800.c 
b/drivers/net/ethernet/aurora/nb8800.c
index ecc4a33..1353fee 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -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;
-- 
2.1.4

[toc] | [next] | [standalone]

#1327689

FromMåns Rullgård <mans@mansr.com>
Date2016-02-05 14:40 +0100
Message-ID<qYOMb-P6-35@gated-at.bofh.it>
In reply to#1327687
Sebastian Frias <sf84@laposte.net> writes:

> Signed-off-by: Sebastian Frias <sf84@laposte.net>
> ---
>  drivers/net/ethernet/aurora/nb8800.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/aurora/nb8800.c
> b/drivers/net/ethernet/aurora/nb8800.c
> index ecc4a33..1353fee 100644
> --- a/drivers/net/ethernet/aurora/nb8800.c
> +++ b/drivers/net/ethernet/aurora/nb8800.c
> @@ -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");

Line is longer than 80 chars.

> +				goto err_free_bus;
> +			}
> +			priv->phy_node = of_node_get(pdev->dev.of_node);
> +		}
> +		else {

Wrong brace placement.

> +			dev_err(&pdev->dev, "no PHY specified\n");
> +			ret = -ENODEV;
> +			goto err_free_bus;
> +		}
>  	}
>
>  	priv->mii_bus = bus;
> -- 
> 2.1.4
>

-- 
Måns Rullgård

[toc] | [prev] | [next] | [standalone]

#1327707 — [PATCH v2] net: ethernet: support "fixed-link" DT key/node on nb8800 driver

FromSebastian Frias <sf84@laposte.net>
Date2016-02-05 14:50 +0100
Subject[PATCH v2] net: ethernet: support "fixed-link" DT key/node on nb8800 driver
Message-ID<qYOVQ-SY-19@gated-at.bofh.it>
In reply to#1327689
Signed-off-by: Sebastian Frias <sf84@laposte.net>
---
  drivers/net/ethernet/aurora/nb8800.c | 15 ++++++++++++---
  1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/aurora/nb8800.c 
b/drivers/net/ethernet/aurora/nb8800.c
index ecc4a33..dd7bedc 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -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;
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1327713 — Re: [PATCH v2] net: ethernet: support "fixed-link" DT key/node on nb8800 driver

FromMåns Rullgård <mans@mansr.com>
Date2016-02-05 15:00 +0100
SubjectRe: [PATCH v2] net: ethernet: support "fixed-link" DT key/node on nb8800 driver
Message-ID<qYP5w-Xo-3@gated-at.bofh.it>
In reply to#1327707
Sebastian Frias <sf84@laposte.net> writes:

> Signed-off-by: Sebastian Frias <sf84@laposte.net>
> ---
>  drivers/net/ethernet/aurora/nb8800.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/aurora/nb8800.c
> b/drivers/net/ethernet/aurora/nb8800.c
> index ecc4a33..dd7bedc 100644
> --- a/drivers/net/ethernet/aurora/nb8800.c
> +++ b/drivers/net/ethernet/aurora/nb8800.c
> @@ -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;
> +		}
>  	}

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


-- 
Måns Rullgård

[toc] | [prev] | [next] | [standalone]

#1327728 — Re: [PATCH v2] net: ethernet: support "fixed-link" DT key/node on nb8800 driver

FromSebastian Frias <sf84@laposte.net>
Date2016-02-05 15:10 +0100
SubjectRe: [PATCH v2] net: ethernet: support "fixed-link" DT key/node on nb8800 driver
Message-ID<qYPfd-1gO-13@gated-at.bofh.it>
In reply to#1327713
On 02/05/2016 02:58 PM, Måns Rullgård wrote:
> Sebastian Frias <sf84@laposte.net> writes:
>
>> Signed-off-by: Sebastian Frias <sf84@laposte.net>
>> ---
>>   drivers/net/ethernet/aurora/nb8800.c | 15 ++++++++++++---
>>   1 file changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/aurora/nb8800.c
>> b/drivers/net/ethernet/aurora/nb8800.c
>> index ecc4a33..dd7bedc 100644
>> --- a/drivers/net/ethernet/aurora/nb8800.c
>> +++ b/drivers/net/ethernet/aurora/nb8800.c
>> @@ -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;
>> +		}
>>   	}
>
> 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;
> 	}
>
>

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.

[toc] | [prev] | [next] | [standalone]

#1327762 — Re: [PATCH v2] net: ethernet: support "fixed-link" DT key/node on nb8800 driver

FromMåns Rullgård <mans@mansr.com>
Date2016-02-05 15:20 +0100
SubjectRe: [PATCH v2] net: ethernet: support "fixed-link" DT key/node on nb8800 driver
Message-ID<qYPoT-1l4-37@gated-at.bofh.it>
In reply to#1327728
Sebastian Frias <sf84@laposte.net> writes:

> On 02/05/2016 02:58 PM, Måns Rullgård wrote:
>> Sebastian Frias <sf84@laposte.net> writes:
>>
>>> Signed-off-by: Sebastian Frias <sf84@laposte.net>
>>> ---
>>>   drivers/net/ethernet/aurora/nb8800.c | 15 ++++++++++++---
>>>   1 file changed, 12 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/aurora/nb8800.c
>>> b/drivers/net/ethernet/aurora/nb8800.c
>>> index ecc4a33..dd7bedc 100644
>>> --- a/drivers/net/ethernet/aurora/nb8800.c
>>> +++ b/drivers/net/ethernet/aurora/nb8800.c
>>> @@ -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;
>>> +		}
>>>   	}
>>
>> 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;
>> 	}
>>
>>
>
> 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.

-- 
Måns Rullgård

[toc] | [prev] | [next] | [standalone]

#1327796 — [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver

FromSebastian Frias <sf84@laposte.net>
Date2016-02-05 15:30 +0100
Subject[PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver
Message-ID<qYPyz-1p9-41@gated-at.bofh.it>
In reply to#1327762
Signed-off-by: Sebastian Frias <sf84@laposte.net>
---
  drivers/net/ethernet/aurora/nb8800.c | 14 +++++++++++++-
  1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/aurora/nb8800.c 
b/drivers/net/ethernet/aurora/nb8800.c
index ecc4a33..e1fb071 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -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

[toc] | [prev] | [next] | [standalone]

#1327802 — Re: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver

FromMåns Rullgård <mans@mansr.com>
Date2016-02-05 15:40 +0100
SubjectRe: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver
Message-ID<qYPIf-1t6-19@gated-at.bofh.it>
In reply to#1327796
Sebastian Frias <sf84@laposte.net> writes:

> Signed-off-by: Sebastian Frias <sf84@laposte.net>

Please change the subject to something like "net: ethernet: nb8800:
support fixed-link DT node" and add a comment body.

> ---
>  drivers/net/ethernet/aurora/nb8800.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/aurora/nb8800.c
> b/drivers/net/ethernet/aurora/nb8800.c
> index ecc4a33..e1fb071 100644
> --- a/drivers/net/ethernet/aurora/nb8800.c
> +++ b/drivers/net/ethernet/aurora/nb8800.c
> @@ -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

-- 
Måns Rullgård

[toc] | [prev] | [next] | [standalone]

#1327818 — Re: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver

FromSebastian Frias <sf84@laposte.net>
Date2016-02-05 16:00 +0100
SubjectRe: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver
Message-ID<qYQ1A-1Bz-3@gated-at.bofh.it>
In reply to#1327802
On 02/05/2016 03:34 PM, Måns Rullgård wrote:
> Sebastian Frias <sf84@laposte.net> writes:
>
>> Signed-off-by: Sebastian Frias <sf84@laposte.net>
>
> 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?

>
>> ---
>>   drivers/net/ethernet/aurora/nb8800.c | 14 +++++++++++++-
>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/aurora/nb8800.c
>> b/drivers/net/ethernet/aurora/nb8800.c
>> index ecc4a33..e1fb071 100644
>> --- a/drivers/net/ethernet/aurora/nb8800.c
>> +++ b/drivers/net/ethernet/aurora/nb8800.c
>> @@ -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
>

[toc] | [prev] | [next] | [standalone]

#1327828 — Re: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver

FromMåns Rullgård <mans@mansr.com>
Date2016-02-05 16:10 +0100
SubjectRe: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver
Message-ID<qYQbg-1UO-25@gated-at.bofh.it>
In reply to#1327818
Sebastian Frias <sf84@laposte.net> writes:

> On 02/05/2016 03:34 PM, Måns Rullgård wrote:
>> Sebastian Frias <sf84@laposte.net> writes:
>>
>>> Signed-off-by: Sebastian Frias <sf84@laposte.net>
>>
>> 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.

>>> ---
>>>   drivers/net/ethernet/aurora/nb8800.c | 14 +++++++++++++-
>>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/ethernet/aurora/nb8800.c
>>> b/drivers/net/ethernet/aurora/nb8800.c
>>> index ecc4a33..e1fb071 100644
>>> --- a/drivers/net/ethernet/aurora/nb8800.c
>>> +++ b/drivers/net/ethernet/aurora/nb8800.c
>>> @@ -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
>>

-- 
Måns Rullgård

[toc] | [prev] | [next] | [standalone]

#1327835 — Re: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver

FromMåns Rullgård <mans@mansr.com>
Date2016-02-05 16:30 +0100
SubjectRe: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver
Message-ID<qYQuB-22s-5@gated-at.bofh.it>
In reply to#1327828
Sebastian Frias <sf84@laposte.net> writes:

> On 02/05/2016 04:08 PM, Måns Rullgård wrote:
>> Sebastian Frias <sf84@laposte.net> writes:
>>
>>> On 02/05/2016 03:34 PM, Måns Rullgård wrote:
>>>> Sebastian Frias <sf84@laposte.net> writes:
>>>>
>>>>> Signed-off-by: Sebastian Frias <sf84@laposte.net>
>>>>
>>>> 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.
>
> 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.

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

>>>>> ---
>>>>>    drivers/net/ethernet/aurora/nb8800.c | 14 +++++++++++++-
>>>>>    1 file changed, 13 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/aurora/nb8800.c
>>>>> b/drivers/net/ethernet/aurora/nb8800.c
>>>>> index ecc4a33..e1fb071 100644
>>>>> --- a/drivers/net/ethernet/aurora/nb8800.c
>>>>> +++ b/drivers/net/ethernet/aurora/nb8800.c
>>>>> @@ -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
>>>>
>>

-- 
Måns Rullgård

[toc] | [prev] | [next] | [standalone]

#1328887 — [PATCH v5] net: ethernet: nb8800: support fixed-link DT node

FromSebastian Frias <sf84@laposte.net>
Date2016-02-08 11:30 +0100
Subject[PATCH v5] net: ethernet: nb8800: support fixed-link DT node
Message-ID<qZReW-3Ut-5@gated-at.bofh.it>
In reply to#1327835
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

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 <sf84@laposte.net>
---
  drivers/net/ethernet/aurora/nb8800.c | 14 +++++++++++++-
  1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/aurora/nb8800.c 
b/drivers/net/ethernet/aurora/nb8800.c
index ecc4a33..e1fb071 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -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

[toc] | [prev] | [next] | [standalone]

#1329024 — Re: [PATCH v5] net: ethernet: nb8800: support fixed-link DT node

FromMåns Rullgård <mans@mansr.com>
Date2016-02-08 14:20 +0100
SubjectRe: [PATCH v5] net: ethernet: nb8800: support fixed-link DT node
Message-ID<qZTTs-5KP-3@gated-at.bofh.it>
In reply to#1328887
Sebastian Frias <sf84@laposte.net> writes:

> 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
>
> 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 <sf84@laposte.net>

Acked-by: Mans Rullgard <mans@mansr.com>

> ---
>  drivers/net/ethernet/aurora/nb8800.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/aurora/nb8800.c
> b/drivers/net/ethernet/aurora/nb8800.c
> index ecc4a33..e1fb071 100644
> --- a/drivers/net/ethernet/aurora/nb8800.c
> +++ b/drivers/net/ethernet/aurora/nb8800.c
> @@ -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
>

-- 
Måns Rullgård

[toc] | [prev] | [next] | [standalone]

#1328903 — Re: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver

FromSebastian Frias <sf84@laposte.net>
Date2016-02-08 11:40 +0100
SubjectRe: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver
Message-ID<qZRoC-3Xu-21@gated-at.bofh.it>
In reply to#1327835
On 02/05/2016 04:26 PM, Måns Rullgård wrote:
> Sebastian Frias <sf84@laposte.net> writes:
>
>> On 02/05/2016 04:08 PM, Måns Rullgård wrote:
>>> Sebastian Frias <sf84@laposte.net> writes:
>>>
>>>> On 02/05/2016 03:34 PM, Måns Rullgård wrote:
>>>>> Sebastian Frias <sf84@laposte.net> writes:
>>>>>
>>>>>> Signed-off-by: Sebastian Frias <sf84@laposte.net>
>>>>>
>>>>> 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.
>>
>> 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.
>
>> 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...




>
>>>>>> ---
>>>>>>     drivers/net/ethernet/aurora/nb8800.c | 14 +++++++++++++-
>>>>>>     1 file changed, 13 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/net/ethernet/aurora/nb8800.c
>>>>>> b/drivers/net/ethernet/aurora/nb8800.c
>>>>>> index ecc4a33..e1fb071 100644
>>>>>> --- a/drivers/net/ethernet/aurora/nb8800.c
>>>>>> +++ b/drivers/net/ethernet/aurora/nb8800.c
>>>>>> @@ -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
>>>>>
>>>
>

[toc] | [prev] | [next] | [standalone]

#1329054 — Re: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver

FromMåns Rullgård <mans@mansr.com>
Date2016-02-08 14:40 +0100
SubjectRe: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver
Message-ID<qZUcP-5U1-47@gated-at.bofh.it>
In reply to#1328903
Sebastian Frias <sf84@laposte.net> writes:

> On 02/05/2016 04:26 PM, Måns Rullgård wrote:
>> Sebastian Frias <sf84@laposte.net> writes:
>>
>>> On 02/05/2016 04:08 PM, Måns Rullgård wrote:
>>>> Sebastian Frias <sf84@laposte.net> writes:
>>>>
>>>>> On 02/05/2016 03:34 PM, Måns Rullgård wrote:
>>>>>> Sebastian Frias <sf84@laposte.net> writes:
>>>>>>
>>>>>>> Signed-off-by: Sebastian Frias <sf84@laposte.net>
>>>>>>
>>>>>> 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.
>>>
>>> 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.
>>
>>> 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.

Thanks for your patience.

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

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.

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

-- 
Måns Rullgård

[toc] | [prev] | [next] | [standalone]

#1329098 — Re: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver

FromMason <slash.tmp@free.fr>
Date2016-02-08 15:20 +0100
SubjectRe: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver
Message-ID<qZUPw-6rJ-15@gated-at.bofh.it>
In reply to#1329054
On 08/02/2016 14:37, Måns Rullgård wrote:

> Sebastian Frias wrote:
> 
>> 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/

[toc] | [prev] | [next] | [standalone]

#1329124 — Re: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver

FromSebastian Frias <sf84@laposte.net>
Date2016-02-08 15:40 +0100
SubjectRe: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver
Message-ID<qZV8S-6zW-43@gated-at.bofh.it>
In reply to#1329098
On 02/08/2016 03:11 PM, Mason wrote:
> On 08/02/2016 14:37, Måns Rullgård wrote:
>
>> Sebastian Frias wrote:
>>
>>> 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?

[toc] | [prev] | [next] | [standalone]

#1329125 — Re: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver

FromMåns Rullgård <mans@mansr.com>
Date2016-02-08 15:50 +0100
SubjectRe: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver
Message-ID<qZViy-6Ep-1@gated-at.bofh.it>
In reply to#1329124
Sebastian Frias <sf84@laposte.net> writes:

> On 02/08/2016 03:11 PM, Mason wrote:
>> On 08/02/2016 14:37, Måns Rullgård wrote:
>>
>>> Sebastian Frias wrote:
>>>
>>>> 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?

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.

-- 
Måns Rullgård

[toc] | [prev] | [next] | [standalone]

#1329112 — Re: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver

FromSebastian Frias <sf84@laposte.net>
Date2016-02-08 15:40 +0100
SubjectRe: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver
Message-ID<qZV8R-6zW-11@gated-at.bofh.it>
In reply to#1329054
On 02/08/2016 02:37 PM, Måns Rullgård wrote:
> Sebastian Frias <sf84@laposte.net> writes:
>
>> On 02/05/2016 04:26 PM, Måns Rullgård wrote:
>>> Sebastian Frias <sf84@laposte.net> writes:
>>>
>>>> On 02/05/2016 04:08 PM, Måns Rullgård wrote:
>>>>> Sebastian Frias <sf84@laposte.net> writes:
>>>>>
>>>>>> On 02/05/2016 03:34 PM, Måns Rullgård wrote:
>>>>>>> Sebastian Frias <sf84@laposte.net> writes:
>>>>>>>
>>>>>>>> Signed-off-by: Sebastian Frias <sf84@laposte.net>
>>>>>>>
>>>>>>> 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.
>>>>
>>>> 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.
>>>
>>>> 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.
>
> Thanks for your patience.

:-)

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

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

[toc] | [prev] | [next] | [standalone]

#1329139 — Re: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver

FromMåns Rullgård <mans@mansr.com>
Date2016-02-08 16:00 +0100
SubjectRe: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver
Message-ID<qZVse-6HZ-7@gated-at.bofh.it>
In reply to#1329112
Sebastian Frias <sf84@laposte.net> writes:

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

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.

-- 
Måns Rullgård

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web