Looks pretty decent. Main comments (style mostly, driver operation path
seems sound):
* kill the bitfields and unions [in descriptor structs]. they are not
endian-safe as presented, generate poor code, and are otherwise undesirable.
* the basic operations are too verbose: E1000_READ_REG(hw, REGISTER) is
far more readable as ER32(REGISTER), following the style of other
drivers. Furthermore, the "E1000_" prefix, in addition to being overly
redundant (used in each register read/write), it is also incorrect,
because this is not E1000...
* in general, rename everything with "e1000_" prefix. this will
eliminate plenty of human confusion in the long run.
* API: unless you have chips in the lab that will require an API hook,
don't create one. For example, a direct call to
e1000_acquire_nvm_82575() should replace all ->acquire_nvm() hooks....
if there are no chips in pipeline GUARANTEED to have a different
->acquire_nvm() feature.
In general, I try to communicate that I am not opposed to these hooks,
you merely need to make sure they are needed in _each_ case. Otherwise
engineers WILL fall into the habit of writing bloated code simply
because that's what their chosen driver framework has always done.
Jeff
--
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
