On Tue, 2012-02-21 at 00:07 +0530, Mugunthan V N wrote:

This patch adds support for TI's CPSW driver.

The three port switch gigabit ethernet subsystem provides ethernet packet
communication and can be configured as an ethernet switch. Supports
10/100/1000 Mbps.

some more notes.

[]

+#define msg(level, type, format, ...)				\
+do {								\
+	if (netif_msg_##type(priv) && net_ratelimit())		\
+		dev_##level(priv->dev, format, ## __VA_ARGS__);	\
+} while (0)

This hides an always used priv variable.

I suggest you expand this to multiple #defines like:

#define cpsw_info(priv, type, format, ...)			\
do {								\
	if (netif_msg_##type(priv) && net_ratelimit())		\
		dev_info(priv->dev, format, ##__VA_ARGS__);	\
} while (0)

and change the uses from
	msg(info,...)
to
	cpsw_info(priv, type, format, ...)

[]

+struct cpsw_priv {
+	spinlock_t			lock;
+	struct platform_device		*pdev;
+	struct net_device		*ndev;
+	struct resource			*cpsw_res;
+	struct resource			*cpsw_ss_res;
+	struct napi_struct		napi;
+#define napi_to_priv(napi)	container_of(napi, struct cpsw_priv, napi)

I still think embedded #defines in structs are hard to read.

[]

+static int cpsw_poll(struct napi_struct *napi, int budget)
+{
+	struct cpsw_priv	*priv = napi_to_priv(napi);
+	int			num_tx, num_rx;
+
+	num_tx = cpdma_chan_process(priv->txch, 128);
+	num_rx = cpdma_chan_process(priv->rxch, budget);
+
+	if (num_rx || num_tx)
+		msg(dbg, intr, "poll %d rx, %d tx pkts\n", num_rx, num_tx);

		cpsw_dbg(priv, intr, etc...)
[]

+		*link = true;
+	} else {
+		mac_control = 0;
+		/* diable forwarding */

disable tyop

+	if (IS_ERR(slave->phy)) {
+		msg(err, ifup, "phy %s not found on slave %d\n",
+		    slave->data->phy_id, slave->slave_num);
+		slave->phy = NULL;
+	} else {
+		printk(KERN_ERR "\nCPSW phy found : id is : 0x%x\n",
+			slave->phy->phy_id);

Using leading newlines doesn't help much.
I suggest dev_err.
[]

+	ndev = alloc_etherdev(sizeof(struct cpsw_priv));
+	if (!ndev) {
+		pr_err("cpsw: error allocating net_device\n");

Add
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
before all #includes and remove the embedded prefix
from all pr_<level> uses.

+	if (is_valid_ether_addr(data->slave_data[0].mac_addr)) {
+		memcpy(priv->mac_addr, data->slave_data[0].mac_addr, ETH_ALEN);
+		printk(KERN_INFO"Detected MACID=%x:%x:%x:%x:%x:%x\n",
+			priv->mac_addr[0], priv->mac_addr[1],
+			priv->mac_addr[2], priv->mac_addr[3],
+			priv->mac_addr[4], priv->mac_addr[5]);

Use printf extension %pM to print the MAC address

		pr_info("Detected MAC=%pM\n", priv->mac_addr);

+	priv->slaves = kzalloc(sizeof(struct cpsw_slave) * data->slaves,
+			       GFP_KERNEL);
+	if (!priv->slaves) {
+		dev_err(priv->dev, "failed to allocate slave ports\n");

No real need for this dev_err
[]

+	memset(&dma_params, 0, sizeof(dma_params));
+	dma_params.dev			= &pdev->dev;
+	dma_params.dmaregs		= (void __iomem *)(((u32)priv->regs) +
+						data->cpdma_reg_ofs);
+	dma_params.rxthresh		= (void __iomem *)(((u32)priv->regs) +
+					data->cpdma_reg_ofs + CPDMA_RXTHRESH);
+	dma_params.rxfree		= (void __iomem *)(((u32)priv->regs) +
+					data->cpdma_reg_ofs + CPDMA_RXFREE);
+
+	dma_params.txhdp	= (void __iomem *)(((u32)priv->regs) +
+				data->cpdma_sram_ofs + CPDMA_TXHDP);
+	dma_params.rxhdp	= (void __iomem *)(((u32)priv->regs) +
+				data->cpdma_sram_ofs + CPDMA_RXHDP);
+	dma_params.txcp		= (void __iomem *)(((u32)priv->regs) +
+				data->cpdma_sram_ofs + CPDMA_TXCP);
+	dma_params.rxcp		= (void __iomem *)(((u32)priv->regs) +
+				data->cpdma_sram_ofs + CPDMA_RXCP);

These might be more readable if you didn't try to align it
or maybe add macros or inline functions like:

#define cpsw_sram_addr(priv, data, offset)			\
	(void __iomem *)(((u32)(priv)->regs) + (data)->cpdma_sram_ofs + offset)