Re: [PATCH]: Preliminary release of Sun Neptune driver

[David Miller <davem@davemloft.net>]

From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Wed, 19 Sep 2007 14:59:00 -0700

> > +#ifndef PCI_DEVICE_ID_SUN_NEPTUNE
> > +#define PCI_DEVICE_ID_SUN_NEPTUNE	0xabcd
> > +#endif
> 
> Why bother defining the ID, and what good does driver_data do you?

Good point, I'll put this 0xabcd thing directly into the
table.

> > +#define nr64(reg)		readq(np->regs + (reg))
> > +#define nw64(val, reg)		writeq((val), np->regs + (reg))
> 
> Macro's that make assumptions about context (ie variable name np)
> are evil and bad style.

Too bad, it saves developers who write 20,000 line drivers from carpel
tunnel symdrome.

It's so repetitive to state things over and over again, and this macro
scheme is 1) established widely (tg3 amonst many other drivers do it)
and 2) forces the driver to use a consistent naming convention for
primary driver private struct variables.

> Wrapping locking is poor style and makes code review harder.

I disagree, because in this case the macro says exactly what the thing
is doing, locking the parent of the port.  The implementation is
arbitrary and can be reviewed independently of the uses.

> > +static int niu_wait_bits_clear_mac(struct niu *np, unsigned long reg, u64 bits,
> > +				   int limit, int delay)
> > +{
> > +	BUILD_BUG_ON(limit <= 0 || delay < 0);
> 
> There is no way compiler can evaluate limit or delay.

Check the callers, they all pass contant values.

> Why are you using __u32?  That is reserved for values going out
> to user space.

Right, I'll fix those up, thanks.