Re: Fwd: RealTek 8139 ethernet driver

[Gary Thomas <gary at mlbassoc dot com>]

On Wed, 2003-07-30 at 14:02, Jonathan Larmour wrote:
> Eric Doenges wrote:
> > [ I originally sent this mail to ecos-patches; about 5 Minutes
> >   after hitting the 'send' button I realized I should probably
> >   have sent it to ecos-devel instead, because it's not really
> >   a patch, so I'm doing this now. I'm sorry for any
> >   inconvinience this may cause and hope to be more intelligent
> >   about using the correct mailing list in the future. ]
> 
> Actually ecos-patches is right.
> 
> > 4. The source files contain the standard eCos header which I lifted
> >    from the Intel 82559 driver, even though the copyright has not
> >    yet been officially assigned to RedHat. My boss has agreed to
> >    OK the copyright transfer, so this should only be a matter of
> >    getting the paperwork done (hopefully sometime this week).
> 
> Good!
> 
> > 5. Finally, the code most likely does not adher to the eCos coding
> >    standard alluded to in the FAQ (since I couldn't find it).
> 
> "By example" ;-). Unfortunately the coding standards are in a Red Hat 
> confidential document(!). I believe in future we will be looking at GNU 
> coding standards for new code instead of the current established practice. 
> Certainly no code written to GNU coding standards will be turned away.
> 

For the most part, this code is reasonably well structured.  The few
things I saw at first glance were:
 * Lots of uses of "info->XXX".  The use of a structure with pointers
   like this, but the name "info" for the pointer could be improved.
   Much can be determined by reading code if it's obvious what things
   point to (and the name of the pointer object can help here)
 * I'd prefer that you tone-down the rhetoric and criticism of the 
   RealTek documentation.  Granted this may be poor, but pointing it
   out in a [permanent] public place won't make it better and may 
   actually discourage the vendor from making such documents public
   at all!
 * I think there is some confusion about CYG_PHYSICAL_ADDRESS().  This
   is definitely needed, but there may also be a need to translate from
   CPU addresses to PCI addresses (on some systems, the address space
   differs depending on which "side" of the PCI bus the activity takes
   place and often by more than just a MMU-style mapping).
 * I don't think that the PCI lookup table belongs in the code specific
   to the use/instance.  The PCI code(s) [and thus boards] supported by
   the driver will be the same whether this driver is used by a PC or
   an ARM based target.
 * There is no need to make devs/eth/rltk/8139/current/include/if_8139.h
   public - put it in the /src directory where it can be used by the
   driver.
 * Look again at the calling sequence for the "control" function.  It
   only should return 0 or 1 - not EINVAL and ESUCCESS.  There's no
   need for these to be used by a hardware driver.
 * "poll()" is exactly "deliver()" except used by non-interrupt driven
   environments.  Look at some other drivers to see how this is handled.
 * When an interrupt occurs, it is normally preferable to mask the
   interrupt using HAL_INTERRUPT_MASK() (or cyg_drv_interrupt_mask())
   rather than fiddling with the interrupt registers on the chip.  
   Again, there are many examples of how this is done.

As I said, this driver is already quite good.  A little cleanup and
the copyright assignment and you'll be fine.

BTW - I'm interested in this driver myself (I have a lonely 8139 card
here).  If you'd like to do a little cleaning up of the code, I'd be
glad to try it on something other than a PC.

-- 
Gary Thomas <gary@mlbassoc.com>
MLB Associates