| Message ID | 56B4A877.4020800@laposte.net |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
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; }
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.
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.
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;
Signed-off-by: Sebastian Frias <sf84@laposte.net> --- drivers/net/ethernet/aurora/nb8800.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)