This is the mail archive of the
mailing list for the eCos project.
Re: Fwd: RealTek 8139 ethernet driver
- From: Gary Thomas <gary at mlbassoc dot com>
- To: Eric Doenges <Eric dot Doenges at DynaPel dot de>
- Cc: eCos patches <ecos-patches at sources dot redhat dot com>
- Date: 30 Jul 2003 14:26:50 -0600
- Subject: Re: Fwd: RealTek 8139 ethernet driver
- Organization: MLB Associates
- References: <3F1D03A5.7060306@DynaPel.de> <3F28243D.6090203@eCosCentric.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).
> > 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
* 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
* 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 <email@example.com>