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

[Jeff Garzik <jeff@garzik.org>]

Stephen Hemminger wrote:
> O
>> +#define DRV_MODULE_NAME		"niu"
>> +#define PFX DRV_MODULE_NAME	": "
>> +#define DRV_MODULE_VERSION	"0.06"
>> +#define DRV_MODULE_RELDATE	"September 18, 2007"
>> +
>> +static char version[] __devinitdata =
>> +	DRV_MODULE_NAME ".c:v" DRV_MODULE_VERSION " (" DRV_MODULE_RELDATE ")\n";
>> +
>> +MODULE_AUTHOR("David S. Miller (davem@davemloft.net)");
>> +MODULE_DESCRIPTION("NIU ethernet driver");
>> +MODULE_LICENSE("GPL");
>> +MODULE_VERSION(DRV_MODULE_VERSION);
>> +
>> +#ifndef DMA_44BIT_MASK
>> +#define DMA_44BIT_MASK	0x00000fffffffffffULL
>> +#endif
>> +
>> +#ifndef PCI_DEVICE_ID_SUN_NEPTUNE
>> +#define PCI_DEVICE_ID_SUN_NEPTUNE	0xabcd
>> +#endif
> 
> Why bother defining the ID

Yes, its pointless to use anything but the hex number for PCI IDs that 
are only used in a single place.


>> +static struct pci_device_id niu_pci_tbl[] = {
>> +	{PCI_DEVICE(PCI_VENDOR_ID_SUN, PCI_DEVICE_ID_SUN_NEPTUNE),
>> +	 .driver_data = 0xff},
>> +	{}
>> +};
>> +
>> +MODULE_DEVICE_TABLE(pci, niu_pci_tbl);
>> +
>> +#define NIU_TX_TIMEOUT			(5 * HZ)
>> +
>> +#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.

No, that's a common and encouraged convenience that makes the code a lot 
easier to read.

That said, the 2-arg macro arg order is wrong.  It should be (reg,val) 
like tg3 and other drivers.

	Jeff