This is the mail archive of the
ecos-discuss@sources.redhat.com
mailing list for the eCos project.
Re: Fwd: RealTek 8139 ethernet driver
- From: Eric Doenges <Eric dot Doenges at DynaPel dot de>
- To: ecos-discuss at sources dot redhat dot com
- Date: Thu, 31 Jul 2003 09:29:27 +0200
- Subject: [ECOS] Re: Fwd: RealTek 8139 ethernet driver
Gary Thomas wrote:
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)
How about 'deviceInfo' then ?
* 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!
If they don't make their documentation public, they are going to sell
less chips, so I don't see this happening. This is not the WiFi market
where you can charge $25k for the privilige of being allowed to design
with your chipset. However, I do agree that I should tone down the
language (not out of respect for RealTek's feelings, but for my own
reputation)
* 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).
There is definitely confusion on my part. Unfortunately, I couldn't find
anything on this in either the offical eCos documentation or
Anthony Massa's book "Embedded Software Development with eCos", so I had
to go by trial and error and use what works on my platform. I'm open to
suggestions on how to do it right on all platforms.
* 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.
I agree with you in principle, but since vendor and device ID can be set
by the serial eeprom connected to the 8139, I'm not 100% shure about
this. There seem to be some vendors out there who don't understand
the whole point behind PCI device and vendor IDs and use whatever they
fancy.
* 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.
Ok.
* 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.
I basically copied the code from the Intel 82559 driver, which returns
0, -1 and -2. (Rereading the eCos documentation gives me the impression
you are right and the Intel driver is wrong =8^)
* "poll()" is exactly "deliver()" except used by non-interrupt driven
environments. Look at some other drivers to see how this is handled.
I've looked at the Intel 82559 driver, and it seems to be doing more or
less what I'm doing.
* 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.
I disagree. We are talking about PCI interrupts, which may be shared
(and are, on a number of platforms). If I disable the complete
interrupt, I disable all devices sharing that interrupt pin. I don't
think it's acceptable to do this when the interrupt can only be
reenabled from code run in a thread.
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.
--
--------------------------------------------------------------------
| Eric Doenges | DynaPel Laboratories GmbH |
| Tel: +49 89 962428 23 | Fraunhoferstrasse 9/2 |
| Fax: +49 89 962428 90 | D - 85737 Ismaning, Germany |
--------------------------------------------------------------------
--
Before posting, please read the FAQ: http://sources.redhat.com/fom/ecos
and search the list archive: http://sources.redhat.com/ml/ecos-discuss