lkml.org 
[lkml]   [2002]   [Feb]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] New driver 3Com 3C359 Tokenring Velocity XL
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"
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-03-22 13:24    [W:0.036 / U:18.536 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site