Re: [PATCH v2] net: ftgmac100: Get link speed and duplex for NC-SI

[Jakub Kicinski <kuba@kernel.org>]

On Thu, 22 Aug 2024 11:19:45 +0800 Jacky Chou wrote:
> The ethtool of this driver uses the phy API of ethtool
> to get the link information from PHY driver.
> Because the NC-SI is forced on 100Mbps and full duplex,
> the driver connects a fixed-link phy driver for NC-SI.

replace: the driver connects -> connect

> The ethtool will get the link information from the
> fixed-link phy driver.

Hm. I defer to the PHY experts on the merits.

> Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
> ---
> v2:
>   - use static for struct fixed_phy_status ncsi_phy_status
>   - Stop phy device at net_device stop when using NC-SI.
>   - Start phy device at net_device start when using NC-SI.
> ---
>  drivers/net/ethernet/faraday/ftgmac100.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
> index fddfd1dd5070..93862b027be0 100644
> --- a/drivers/net/ethernet/faraday/ftgmac100.c
> +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> @@ -26,6 +26,7 @@
>  #include <linux/of_net.h>
>  #include <net/ip.h>
>  #include <net/ncsi.h>
> +#include <linux/phy_fixed.h>

Keep the headers sorted, put the new one after of_net.h

>  #include "ftgmac100.h"
>  

> @@ -1794,6 +1805,7 @@ static int ftgmac100_probe(struct platform_device *pdev)
>  	struct net_device *netdev;
>  	struct ftgmac100 *priv;
>  	struct device_node *np;
> +	struct phy_device *phydev;

keep the variable declarations sorted longest to shortest if possible

>  	int err = 0;
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> @@ -1879,6 +1891,14 @@ static int ftgmac100_probe(struct platform_device *pdev)
>  			err = -EINVAL;
>  			goto err_phy_connect;
>  		}
> +
> +		phydev = fixed_phy_register(PHY_POLL, &ncsi_phy_status, NULL);
> +		err = phy_connect_direct(netdev, phydev, ftgmac100_adjust_link,
> +					 PHY_INTERFACE_MODE_MII);
> +		if (err) {
> +			dev_err(&pdev->dev, "Connecting PHY failed\n");
> +			goto err_phy_connect;
> +		}

Very suspicious that you register it but you never unregister it.
Are you sure the error path and .remove don't need to be changed?
-- 
pw-bot: cr