Re: [patch 3/3] Add tsi108 On Chip Ethernet device driver support

[Arjan van de Ven <arjan@infradead.org>]

On Tue, 2006-09-12 at 16:55 +0800, Zang Roy-r61911 wrote:
> The driver for tsi108/9 on chip Ethernet port

Hi,

I have some review comments about your driver; please consider them for
fixing....

> +
> +#undef DEBUG
> +#ifdef DEBUG
> +#define DBG(fmt...) do { printk(fmt); } while(0)
> +#else
> +#define DBG(fmt...) do { } while(0)
> +#endif

please don't do this, there is pr_debug and dev_dbg() for a reason
already. No reason to add a copy.

> +
> +typedef struct net_device net_device;
> +typedef struct sk_buff sk_buff;

Please don't do this...



> +	TSI108_ETH_WRITE_PHYREG(TSI108_MAC_MII_ADDR,
> +				(data->phy << TSI108_MAC_MII_ADDR_PHY) |
> +				(reg << TSI108_MAC_MII_ADDR_REG));
> +	mb();
> +	TSI108_ETH_WRITE_PHYREG(TSI108_MAC_MII_CMD, 0);
> +	mb();
> +	TSI108_ETH_WRITE_PHYREG(TSI108_MAC_MII_CMD, TSI108_MAC_MII_CMD_READ);
> +	mb();

what is this mb() for? do you want them to be an io memory barrier?

> +	while (TSI108_ETH_READ_REG(TSI108_MAC_MII_IND) &
> +	       TSI108_MAC_MII_IND_BUSY) ;

do you want this to be an infinate loop? At minimum the ";" needs to be
a cpu_relax() to avoid the cpu from burning up but it'd be a lot nicer
if this loop wasn't infinite




> +
> +static irqreturn_t tsi108_irq(int irq, void *dev_id, struct pt_regs *regs)
> +{
> +	if (irq == NO_IRQ)
> +		return IRQ_NONE;	/* Not our interrupt */
> +
> +	return tsi108_irq_one(dev_id);
> +}

hi this IRQ_NONE just looks odd, was this really needed?



> +	mb();
> +	while (tsi108_read_mii(data, PHY_CTRL, NULL) & PHY_CTRL_AUTONEG_START)
> +		;

another infinite loop that wants cpu_relax() for sure

> +		spin_unlock_irq(&phy_lock);
> +		msleep(10);
> +		spin_lock_irq(&phy_lock);
> +	}

hmm some places take phy_lock with disabling interrupts, while others
don't. I sort of fear "the others" may be buggy.... are you sure those
are ok?



Greetings,
   Arjan van de Ven