This is the mail archive of the ecos-discuss@sources.redhat.com mailing list for the eCos project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]