Re: [PATCH v9 17/38] net: cirrus: add DT support for Cirrus EP93xx
From: Simon Horman <horms@kernel.org>
Date: 2024-03-28 20:34:33
Also in:
lkml
On Tue, Mar 26, 2024 at 12:18:44PM +0300, Nikita Shubin via B4 Relay wrote:
quoted hunk ↗ jump to hunk
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 Reviewed-by: Andrew Lunn <andrew@lunn.ch> Tested-by: Alexander Sverdlin <alexander.sverdlin@gmail.com> Reviewed-by: Linus Walleij <redacted> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> --- drivers/net/ethernet/cirrus/ep93xx_eth.c | 63 ++++++++++++++++---------------- 1 file changed, 32 insertions(+), 31 deletions(-)diff --git a/drivers/net/ethernet/cirrus/ep93xx_eth.c b/drivers/net/ethernet/cirrus/ep93xx_eth.c index 1f495cfd7959..2523d9c9d1b8 100644 --- a/drivers/net/ethernet/cirrus/ep93xx_eth.c +++ b/drivers/net/ethernet/cirrus/ep93xx_eth.c@@ -16,13 +16,12 @@ #include <linux/ethtool.h> #include <linux/interrupt.h> #include <linux/moduleparam.h> +#include <linux/of.h> #include <linux/platform_device.h> #include <linux/delay.h> #include <linux/io.h> #include <linux/slab.h> -#include <linux/platform_data/eth-ep93xx.h> - #define DRV_MODULE_NAME "ep93xx-eth" #define RX_QUEUE_ENTRIES 64@@ -738,25 +737,6 @@ static const struct net_device_ops ep93xx_netdev_ops = { .ndo_set_mac_address = eth_mac_addr, }; -static struct net_device *ep93xx_dev_alloc(struct ep93xx_eth_data *data) -{ - struct net_device *dev; - - dev = alloc_etherdev(sizeof(struct ep93xx_priv)); - if (dev == NULL) - return NULL; - - eth_hw_addr_set(dev, data->dev_addr); - - dev->ethtool_ops = &ep93xx_ethtool_ops; - dev->netdev_ops = &ep93xx_netdev_ops; - - dev->features |= NETIF_F_SG | NETIF_F_HW_CSUM; - - return dev; -} - - static void ep93xx_eth_remove(struct platform_device *pdev) { struct net_device *dev;@@ -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;
Please consider preserving reverse xmas tree order - longest line to shortest, for local variables in Networking code.
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"); + + 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");
This function, not entirely due to this patch, seems to leak resources in error paths. F.e., here base_addr is not unmapped and mem->start is not released. I expect that to resolve this problem it would be best to move to idiomatic error handling by: * using a ladder of goto labels in ep93xx_eth_probe(); and * as a clean up, remove the conditions from ep93xx_eth_remove.
+
+ 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);This doesn't look right. eth_hw_addr_set() expects it's second argument to be a char * which represents an Ethernet address. But the type of base_addr + 0x50 is __iomem *. I suspect that you need some construction based on readb/readw/readl to copy the Ethernet address from io memory into a buffer, taking into account endiness and possibly taking into account unaligned access. Also, it would be good a descriptively named #define in place of 0x50.
+ 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);
...