On 7/6/07, Jeff Garzik <[EMAIL PROTECTED]> wrote:
OK, just looked through the driver. I think its structured inside-out from what it should be.
* 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.
I think the nice thing about the driver's organization is that it gives a clear overview of how the driver works, without delving into generation-specific details. This fixes a problem that the old driver had, where it was very easy to get lost in the woods of family-specific workarounds.
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.
Basically make a library of e1000-routines. The alternative is a polymorphic approach (i.e. using function pointers so generic code can call specific code). Both approaches are used extensively in the kernel -- I happen to think the latter approach is better suited to the e1000's current issue. (For what is a driver but specific code called from generic code? This just reuses this helpful design pattern at a lower level.)
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].
Given that the new driver would only support PCIe devices (i.e. relatively new/good HW) I would think this would cut down on the "workaround" hooks, leaving the "HW is genuinely different" hooks.
* The multitude of files makes it difficult to review. Much easier in one file, or a small few.
Removing support for pre-PCIe in the new driver would help alleviate this. Regards -- Andy - 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
