Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 24 Aug 2022 14:10:32 +0100
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     "xiaowu.ding" <xiaowu.ding@jaguarmicro.com>
Cc:     davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, nicolas.ferre@microchip.com,
        claudiu.beznea@microchip.com, palmer@dabbelt.com,
        paul.walmsley@sifive.com, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org
Subject: Re: [PATCH net-next] driver: cadence macb driver support acpi mode
Message-ID: <YwYjSPl7murFFpJG@shell.armlinux.org.uk>
References: <20220824121351.578-1-xiaowu.ding@jaguarmicro.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220824121351.578-1-xiaowu.ding@jaguarmicro.com>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

On Wed, Aug 24, 2022 at 08:13:51PM +0800, xiaowu.ding wrote:
> -static bool macb_phy_handle_exists(struct device_node *dn)
> +static bool macb_of_phy_handle_exists(struct device_node *dn)
>  {
>  	dn = of_parse_phandle(dn, "phy-handle", 0);
>  	of_node_put(dn);
>  	return dn != NULL;
>  }
>  
> -static int macb_phylink_connect(struct macb *bp)
> +static int macb_of_phylink_connect(struct macb *bp)
>  {
>  	struct device_node *dn = bp->pdev->dev.of_node;
>  	struct net_device *dev = bp->dev;
> @@ -765,7 +767,7 @@ static int macb_phylink_connect(struct macb *bp)
>  	if (dn)
>  		ret = phylink_of_phy_connect(bp->phylink, dn, 0);
>  
> -	if (!dn || (ret && !macb_phy_handle_exists(dn))) {
> +	if (!dn || (ret && !macb_of_phy_handle_exists(dn))) {
>  		phydev = phy_find_first(bp->mii_bus);
>  		if (!phydev) {
>  			netdev_err(dev, "no PHY found\n");
> @@ -786,6 +788,166 @@ static int macb_phylink_connect(struct macb *bp)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_ACPI
> +
> +static bool macb_acpi_phy_handle_exists(struct fwnode_handle *fwnd)
> +{
> +	struct fwnode_handle *phy_node;
> +	bool flag = false;
> +	/* Only phy-handle is used for ACPI */
> +	phy_node = fwnode_find_reference(fwnd, "phy-handle", 0);
> +	flag = !IS_ERR_OR_NULL(phy_node);
> +
> +	if (flag)
> +		fwnode_handle_put(phy_node);
> +
> +	return flag;
> +}
> +
> +static int macb_acpi_phylink_connect(struct macb *bp)
> +{
> +	struct fwnode_handle *fwnd = bp->pdev->dev.fwnode;
> +	struct net_device *dev = bp->dev;
> +	struct phy_device *phydev;
> +	int ret;
> +
> +	if (fwnd)
> +		ret = phylink_fwnode_phy_connect(bp->phylink, fwnd, 0);
> +
> +	if (!fwnd || (ret && !macb_acpi_phy_handle_exists(fwnd))) {
> +		phydev = phy_find_first(bp->mii_bus);
> +		if (!phydev) {
> +			netdev_err(dev, "no PHY found\n");
> +			return -ENXIO;
> +		}
> +
> +		/* attach the mac to the phy */
> +		ret = phylink_connect_phy(bp->phylink, phydev);
> +	}
> +
> +	if (ret) {
> +		netdev_err(dev, "Could not attach PHY (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	phylink_start(bp->phylink);
> +
> +	return 0;
> +}

You shouldn't need this duplication. phylink_fwnode_phy_connect() can be
used to connect DT-based PHYs just fine, so you should be able to use it
for both cases without needing to resort to two copies. This is one of
the reasons the fwnode API exists.

The functionality of your macb_acpi_phy_handle_exists() and
macb_of_phy_handle_exists() should also be the same. Not that
fwnode_handle_put() is safe to call with NULL or err-pointer fwnodes.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!