Re: [PATCH net-next v7 2/9] net: stmmac: dwmac-loongson: Refactor code for loongson_dwmac_probe()

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Tue, Dec 19, 2023 at 10:17:05PM +0800, Yanteng Si wrote:
> Add a setup() function to initialize data, and simplify code for
> loongson_dwmac_probe().

Not all changes in this patch are described.

> +static int loongson_gmac_data(struct pci_dev *pdev,
> +			      struct plat_stmmacenet_data *plat)
> +{
> +	loongson_default_data(pdev, plat);
> +
> +	plat->multicast_filter_bins = 256;
> +
> +	plat->mdio_bus_data->phy_mask = 0;
>  
> -	/* Default to phy auto-detection */
>  	plat->phy_addr = -1;
> +	plat->phy_interface = PHY_INTERFACE_MODE_RGMII_ID;

This presumably sets the default phy_interface mode?


> -	plat->bus_id = of_alias_get_id(np, "ethernet");
> -	if (plat->bus_id < 0)
> -		plat->bus_id = pci_dev_id(pdev);
> +	pci_set_master(pdev);
> +
> +	info = (struct stmmac_pci_info *)id->driver_data;
> +	ret = info->setup(pdev, plat);
> +	if (ret)
> +		goto err_disable_device;

loongson_gmac_data() gets called from here...

> +
> +	bus_id = of_alias_get_id(np, "ethernet");
> +	if (bus_id >= 0)
> +		plat->bus_id = bus_id;
>  
>  	phy_mode = device_get_phy_mode(&pdev->dev);
>  	if (phy_mode < 0) {

This gets the PHY interface mode, and errors out if it can't be found in
firmware.

> @@ -110,11 +137,7 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
>  	}
>  
>  	plat->phy_interface = phy_mode;

So this ends up always overwriting the value written in
loongson_gmac_data(). So it seems to be that initialising
plat->phy_interface in loongson_gmac_data() is just patch noise and
serves no real purpose.

> -	plat->mac_interface = PHY_INTERFACE_MODE_GMII;

This has now gone - and is not described, and I'm left wondering what
the implication of that is on the driver. It also makes me wonder
whether loongson_gmac_data() should've been setting mac_interface
rather than phy_interface.

>  	res.wol_irq = of_irq_get_byname(np, "eth_wake_irq");
>  	if (res.wol_irq < 0) {
> -		dev_info(&pdev->dev, "IRQ eth_wake_irq not found, using macirq\n");
> +		dev_info(&pdev->dev,
> +			 "IRQ eth_wake_irq not found, using macirq\n");

Whitespace cleanups should be a separate patch.

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