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

2016-02-05 Thread Måns Rullgård 
Sebastian Frias  writes:

> Signed-off-by: Sebastian Frias 
> ---
>  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(>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(>dev, "bad fixed-link spec\n");
> + goto err_free_bus;
> + }
> + priv->phy_node = of_node_get(pdev->dev.of_node);
> + } else {
> + dev_err(>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(>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(>dev, "no PHY specified\n");
ret = -ENODEV;
goto err_free_bus;
}


-- 
Måns Rullgård

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

2016-02-05 Thread Sebastian Frias 

On 02/05/2016 02:58 PM, Måns Rullgård wrote:

Sebastian Frias  writes:


Signed-off-by: Sebastian Frias 
---
  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(>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(>dev, "bad fixed-link spec\n");
+   goto err_free_bus;
+   }
+   priv->phy_node = of_node_get(pdev->dev.of_node);
+   } else {
+   dev_err(>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(>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(>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

2016-02-05 Thread Måns Rullgård 
Sebastian Frias  writes:

> On 02/05/2016 02:58 PM, Måns Rullgård wrote:
>> Sebastian Frias  writes:
>>
>>> Signed-off-by: Sebastian Frias 
>>> ---
>>>   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(>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(>dev, "bad fixed-link spec\n");
>>> +   goto err_free_bus;
>>> +   }
>>> +   priv->phy_node = of_node_get(pdev->dev.of_node);
>>> +   } else {
>>> +   dev_err(>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(>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(>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

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

2016-02-05 Thread Sebastian Frias 


Signed-off-by: Sebastian Frias 
---
 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(>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(>dev, "bad fixed-link spec\n");
+   goto err_free_bus;
+   }
+   priv->phy_node = of_node_get(pdev->dev.of_node);
+   } else {
+   dev_err(>dev, "no PHY specified\n");
+   ret = -ENODEV;
+   goto err_free_bus;
+   }
}

priv->mii_bus = bus;
--
2.1.4