Kok, Auke wrote:
James Chapman wrote:
I briefly looked over your new driver. I think it might benefit by
moving common parts into one or more libraries (or modules) and have
separate chip-specific drivers for 82540, 82541, 82542, 82543, 82571
etc. Put all the chip-specific bits in a chip-specific driver and have
each driver call into the common (shared) code. The device
probe/init/remove would be done by the chip-specific code, not the
common code like it is now. When built as a module, the chip-specific
parts and the common parts could be built as separate modules;
modprobe would load the common parts automatically. Most of the code
would be in the common part since these chips are very similar.
splitting beyond the obvious does not make any sense and will just add
confusion.
Users wouldn't need to know which driver to load - the right driver
would be loaded automatically. Distros would ship all of them (and the
common bits). The few users who know which device(s) they have could
build only the required drivers if they wanted to do so.
I'm not against a pcie/pre-pcie split or even going a step
further and looking into using PHYlib as you suggest below, but we
should really avoid trying to create an unmaintainable mess where we
have to patch 7 drivers for each generic issue we find.
The generic issues would be in the common part so would be patched once.
Looking at your new driver code, there are separate source files for the
5 chips I mention, hence my leaning towards a driver per device. I
counted 24 funcptrs that are set up in order for the common code to call
out to driver-private chip specific functions. Wouldn't most of these go
away if the chip-specific code called common code, not the other way round?
Let me use an example to illustrate what I mean. e1000_setup_link() is
common code. It simply calls a function pointed to by func.setup_link,
which was initialized to a chip-specific function, e.g.
e1000_setup_link_82543() in e1000_init_mac_params_82543(). It turns out
that e1000_setup_link() is called by chip-specific code from
e1000_init_hw_82543() so the abstraction through e1000_setup_link()
wouldn't be needed for init. However, e1000_setup_link() is also called
from the common driver/ethtool entry points e1000_open() and
e1000_set_pause_params(). These entry points could be moved in to the
chip specific code, calling out to common code when necessary: the
funcptrs/abstractions wouldn't then be needed. This would obviously lead
to some common boilerplate code in each chip specific driver for the
driver entry points open/close etc and occasionally some common changes
might be needed in that code. However, I'm sure most bugs will be in the
guts of the driver, not the boilerplate code so the duplication of the
driver boilerplate code wouldn't matter so much. I think maintenance
effort would actually reduce with this kind of structure.
I don't want to sound negative though; it's great that you and Intel are
putting a lot of work into this driver. You know much more about the
actual chip feature differences/workarounds than I do so if you don't
think the approach I suggest will work, I'm happy to just drop this thread.
--
James Chapman
Katalix Systems Ltd
http://www.katalix.com
Catalysts for your Embedded Linux software development
-
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
