Re: [PATCH] net: Add support for the OpenCores 10/100 Mbps Ethernet MAC.

[Florian Fainelli <florian@openwrt.org>]

Hi Thierry,

Le Tuesday 24 March 2009 11:18:43 Thierry Reding, vous avez écrit :
> This patch adds a platform device driver that supports the OpenCores 10/100
> Mbps Ethernet MAC.
>
> The driver expects three resources: one IORESOURCE_MEM resource defines the
> memory region for the core's memory-mapped registers while a second
> IORESOURCE_MEM resource defines the network packet buffer space. The third
> resource, of type IORESOURCE_IRQ, associates an interrupt with the driver.

I am glad someone updated and submitted this driver, excellent work !. Few 
comments below.

> + * Copyright (C) 2007-2008 Avionic Design Development GmbH
> + * Copyright (C) 2008-2009 Avionic Design GmbH

I think there are some other authors like Simon Srot and Tensilica, unless you 
did wrote this completely from scratch and did not look at the uClincu 
open_eth driver at all ?

[snip]

> +/* function prototypes */
> +static int ethoc_set_mac_address(struct net_device *dev, void *addr);
> +static int ethoc_get_mac_address(struct net_device *dev, void *addr);
> +static int ethoc_rx(struct net_device *dev, int budget);
> +static void ethoc_tx(struct net_device *dev);
> +static irqreturn_t ethoc_interrupt(int irq, void *dev_id);

Why do you need these declarations ? Are not your functions properly ordered 
already ?

> +/**
> + * ethoc_probe() - initialize OpenCores ethernet MAC
> + * pdev:	platform device
> + */
> +static int ethoc_probe(struct platform_device *pdev)
> +{
> +	struct net_device *netdev = NULL;
> +	struct resource *res = NULL;
> +	struct resource *mmio = NULL;
> +	struct resource *mem = NULL;
> +	struct ethoc *priv = NULL;
> +	unsigned int phy;
> +	int ret = 0;
> +
> +	/* allocate networking device */
> +	netdev = alloc_etherdev(sizeof(struct ethoc));
> +	if (!netdev) {
> +		dev_err(&pdev->dev, "cannot allocate network device\n");
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	SET_NETDEV_DEV(netdev, &pdev->dev);
> +	platform_set_drvdata(pdev, netdev);
> +
> +	/* obtain I/O memory space */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "cannot obtain I/O memory space\n");
> +		ret = -ENXIO;
> +		goto free;
> +	}
> +
> +	mmio = devm_request_mem_region(&pdev->dev, res->start,
> +			res->end - res->start + 1, res->name);
> +	if (!res) {
> +		dev_err(&pdev->dev, "cannot request I/O memory space\n");
> +		ret = -ENXIO;
> +		goto free;
> +	}
> +
> +	netdev->base_addr = mmio->start;
> +
> +	/* obtain buffer memory space */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	if (!res) {
> +		dev_err(&pdev->dev, "cannot obtain memory space\n");
> +		ret = -ENXIO;
> +		goto free;
> +	}

That's what uClinux driver calls SRAM right ?

[snip]

> +	/* setup the net_device structure */
> +	netdev->open = ethoc_open;
> +	netdev->stop = ethoc_stop;
> +	netdev->do_ioctl = ethoc_ioctl;
> +	netdev->set_config = ethoc_config;
> +	netdev->set_mac_address = ethoc_set_mac_address;
> +	netdev->set_multicast_list = ethoc_set_multicast_list;
> +	netdev->change_mtu = ethoc_change_mtu;
> +	netdev->tx_timeout = ethoc_tx_timeout;
> +	netdev->get_stats = ethoc_stats;
> +	netdev->hard_start_xmit = ethoc_start_xmit;
> +	netdev->watchdog_timeo = ETHOC_TIMEOUT;
> +	netdev->features |= NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_HIGHDMA;

Please use netdev_ops.

[snip]

> +#ifndef LINUX_NET_ETHOC_H
> +#define LINUX_NET_ETHOC_H 1
> +
> +struct ethoc_platform_data {
> +	u8 hwaddr[IFHWADDRLEN];
> +	s8 phy_id;

What about allowing platform configuration of the RX/TX buffers size and 
number of them ?

Otherwise

Acked-by: Florian Fainelli <florian@openwrt.org>
-- 
Best regards, Florian Fainelli
Email : florian@openwrt.org
http://openwrt.org
-------------------------------