Re: [PATCH v10 17/38] net: cirrus: add DT support for Cirrus EP93xx

[Simon Horman <horms@kernel.org>]

On Mon, Jun 17, 2024 at 12:36:51PM +0300, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
> 
> - add OF ID match table
> - get phy_id from the device tree, as part of mdio
> - copy_addr is now always used, as there is no SoC/board that aren't
> - dropped platform header
> 
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> Tested-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Hi Nikita,

Some minor feedback from my side.

...

> @@ -786,27 +766,47 @@ static void ep93xx_eth_remove(struct platform_device *pdev)
>  
>  static int ep93xx_eth_probe(struct platform_device *pdev)
>  {
> -	struct ep93xx_eth_data *data;
>  	struct net_device *dev;
>  	struct ep93xx_priv *ep;
>  	struct resource *mem;
> +	void __iomem *base_addr;
> +	struct device_node *np;
> +	u32 phy_id;
>  	int irq;
>  	int err;

nit: Please consider maintaining reverse xmas tree order - longest line
     to shortest - for local variable declarations. As preferred in
     Networking code.

     Edward Cree's tool can be of assistance here.
     https://github.com/ecree-solarflare/xmastree

>  
>  	if (pdev == NULL)
>  		return -ENODEV;
> -	data = dev_get_platdata(&pdev->dev);
>  
>  	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	irq = platform_get_irq(pdev, 0);
>  	if (!mem || irq < 0)
>  		return -ENXIO;
>  
> -	dev = ep93xx_dev_alloc(data);
> +	base_addr = ioremap(mem->start, resource_size(mem));
> +	if (!base_addr)
> +		return dev_err_probe(&pdev->dev, -EIO, "Failed to ioremap ethernet registers\n");

nit: Please consider line-wrapping to limiting lines to 80 columns wide
     where it can be trivially achieved, which seems to be the case here.
     80 columns is still preferred for Networking code.

     Flagged by checkpatch.pl --max-line-length=80

> +
> +	np = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0);
> +	if (!np)
> +		return dev_err_probe(&pdev->dev, -ENODEV, "Please provide \"phy-handle\"\n");
> +
> +	err = of_property_read_u32(np, "reg", &phy_id);
> +	of_node_put(np);
> +	if (err)
> +		return dev_err_probe(&pdev->dev, -ENOENT, "Failed to locate \"phy_id\"\n");
> +
> +	dev = alloc_etherdev(sizeof(struct ep93xx_priv));
>  	if (dev == NULL) {
>  		err = -ENOMEM;
>  		goto err_out;
>  	}
> +
> +	eth_hw_addr_set(dev, base_addr + 0x50);

base_addr is an __iomem address. As such I don't think it is correct
to pass it (+ offset) to eth_hw_addr_set. Rather, I would expect base_addr
to be read using a suitable iomem accessor, f.e. readl. And one possible
solution would be to use readl to read the mac address into a buffer
which is passed to eth_hw_addr_set.

Flagged by Sparse.

> +	dev->ethtool_ops = &ep93xx_ethtool_ops;
> +	dev->netdev_ops = &ep93xx_netdev_ops;
> +	dev->features |= NETIF_F_SG | NETIF_F_HW_CSUM;
> +
>  	ep = netdev_priv(dev);
>  	ep->dev = dev;
>  	SET_NETDEV_DEV(dev, &pdev->dev);

...