From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932070AbcBEOOK (ORCPT ); Fri, 5 Feb 2016 09:14:10 -0500 Received: from unicorn.mansr.com ([81.2.72.234]:53405 "EHLO unicorn.mansr.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754273AbcBEOOD convert rfc822-to-8bit (ORCPT ); Fri, 5 Feb 2016 09:14:03 -0500 From: =?iso-8859-1?Q?M=E5ns_Rullg=E5rd?= To: Sebastian Frias Cc: "David S. Miller" , netdev@vger.kernel.org, LKML , mason Subject: Re: [PATCH v2] net: ethernet: support "fixed-link" DT key/node on nb8800 driver References: <56B4A445.7080402@laposte.net> <56B4A877.4020800@laposte.net> <56B4ACC4.1000607@laposte.net> Date: Fri, 05 Feb 2016 14:13:59 +0000 In-Reply-To: <56B4ACC4.1000607@laposte.net> (Sebastian Frias's message of "Fri, 05 Feb 2016 15:08:04 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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(&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