OK, just looked through the driver. I think its structured inside-out
from what it should be.
Comments:
* is a clear improvement from current e1000
* The multitude of tiny, fine-grained operations for MAC, NVM, PHY, etc.
is a signal that organization is backwards. You should be creating
hardware-specific high level operations (PHY layer hooks, net_device
hooks, interrupt handler) that call out to more-generic functions when
necessary. Doing so eliminates the need to create a new hook for every
little twirl in the code path.
In the long run, a driver is easier to maintain if you can easily follow
the code path for a particular hardware generation. Creating
e1001_8257x_do_this_thing(), which calls more generic code as needed, is
easier to review and doesn't require all sorts of indirection through APIs.
Doing so also means that many workarounds for older hardware "disappear"
from the most-travelled code paths over time. The 64k boundary check
found in e1000new is an easy example of something that really shouldn't
pollute newer code at all [yes, even though it reduces to 'return 1' for
most].
* The multitude of files makes it difficult to review. Much easier in
one file, or a small few.
* bitfields
* check for PCI DMA mapping failure
* atomic_t irq_sem is reinventing the wheel (and too heavy for you
needs, too?). You might as well use a lock or mutex or whatnot at that
point, since you are using a locked instruction. tg3 might also have
some hints in this regard.
* like I noted in the last email, the quickest path to upstream is to
start SMALL. Create the smallest working driver, review it heavily, get
it upstream. Add all hardware&features after that.
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
