RE: [PATCH v2 2/2] netdev: driver: ethernet: Add TI CPSW driver

Joe

-----Original Message-----
From: Joe Perches [mailto:joe@perches.com]
Sent: Tuesday, February 21, 2012 4:34 AM
To: N, Mugunthan V
Cc: netdev@vger.kernel.org; davem@davemloft.net
Subject: Re: [PATCH v2 2/2] netdev: driver: ethernet: Add TI CPSW driver

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

quoted

This patch adds support for TI's CPSW driver.

The three port switch gigabit ethernet subsystem provides ethernet

packet

quoted

communication and can be configured as an ethernet switch. Supports
10/100/1000 Mbps.

some more notes.

quoted

diff --git a/drivers/net/ethernet/ti/cpsw.c

b/drivers/net/ethernet/ti/cpsw.c
[]

quoted

+#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, ...)

Agreed, will change the define.

[]

quoted

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

Agreed, will move defines outside the struct definition.

[]

quoted

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

Will change this debug.

[]

quoted

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

disable tyop

quoted

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

Agreed, will change this to dev_err

[]

quoted

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

Agreed, will make use of pr_fmt and remove embedded prefix in pr_<level>
uses

quoted

+	if (is_valid_ether_addr(data->slave_data[0].mac_addr)) {
+		memcpy(priv->mac_addr, data->slave_data[0].mac_addr,

ETH_ALEN);

quoted

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

Will use %pM to MAC address

quoted

+	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

Agreed, will remove this dev_err

[]

quoted

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

Agreed, will make it as macros

With regards
Mugunthan V N