Re: [PATCH] New driver 3Com 3C359 Tokenring Velocity XL

[Jeff Garzik <jgarzik@mandrakesoft.com>]

Thanks, applied to 2.4 and 2.5.  I removed the definition of
PCI_DEVICE_xxx from the top of 3c359.h...

Comments:
1) buggy use of PCI DMA API -- you should use memory returned from
pci_alloc_consistent, do not directly map memory created by
alloc_trdev() nor depend on the alignment returned by alloc_trdev()

2) support for ETHTOOL_GDRVINFO ioctl
3) support for ETHTOOL_[GS]MSGLVL ioctls... this implies that
'message_level' would only serve as a default for a value that can be
changed per-interface

4) ideally "\n" should not be in MODULE_DESCRIPTION

5) style: no need to cast to/from a void pointer, such as
	> struct xl_private *xl_priv = (struct xl_private *)dev->priv ;

6) hardcoded magic numbers for PKT_BUF_SZ limits (100 and 18000)

7) xl_probe duplicates error handling code... use the standard kernel
style of multiple goto targets, one for each needed stage of
cleanup-after-error:

	err_out3:
		pci_release_regions(pdev)
	err_out2:
		kfree(dev)
	err_out:
		return rc;

8) in  xl_hw_reset, you probably want to call yield [in 2.5] or
schedule_timeout

9) jiffies comparison bug: never directly compare jiffies, use
time_before() or time_after()

10) in xl_open, return the error value returned by request_irq, on error

11) same comment for xl_open as #7

12) xl_wait_misr_flags needs to call yield() or -something-, don't just
empty loop.  cpu_relax(), for example, if you cannot schedule...

Overall... good job, it's a readable, clean driver.

	Jeff




-- 
Jeff Garzik      | "Why is it that attractive girls like you
Building 1024    |  always seem to have a boyfriend?"
MandrakeSoft     | "Because I'm a nympho that owns a brewery?"
                 |             - BBC TV show "Coupling"