Hi Simon!

On Tue, 2024-06-18 at 13:46 +0100, Simon Horman wrote:

On Mon, Jun 17, 2024 at 12:36:51PM +0300, Nikita Shubin via B4 Relay
wrote:

quoted

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 <redacted>

Hi Nikita,

Some minor feedback from my side.

Thanks for catches!

I hope can address them next spin or after series will apply (desirably
the last one).

...

quoted

@@ -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

quoted

 
        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

quoted

+
+       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.

quoted

+       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);

...