This is the mail archive of the ecos-patches@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: New port for ARM Industrial Module AIM 711 - Checked by AntiVir DEM


On Tue, Apr 06, 2004 at 04:10:13PM +0200, Roland Ca?ebohm wrote:
> Hi Andrew,
> 
> thanks for looking.
> 
> On Dienstag, 6. April 2004 15:02, Andrew Lunn wrote:
> > On Wed, Mar 31, 2004 at 03:03:13PM +0200, Roland Ca?ebohm wrote:
> > > Hello,
> > >
> > > does anybody had time to look at the code or tested the
> > > run-time code?
> > > Or could somebody tell me how could I help to get the
> > > port integrated in the public cvs tree?
> >
> > Hi Roland
> >
> > I've started to look at this now. I will start with the HAL
> >
> > hal_arm_aim711.cdl:
> >
> > A few places you are missing the e from module.
> 
> Oh yes, I always lost it. :-(
> 
> >
> > hal_platform_setup.h:
> >
> > Remove the commented out instruction in CYGHWR_LED_MACRO
> > There is some dead code inside #if 0 which should be removed
> 
> It is for having a delay between POST codes to see where it stops
> if there is a failure.
> I think I make a cdl option for it.

OK. I just don't like dead code. It tends to accumulate with time
unless you remove it.

> > I don't particularly like the nameing of _period. The _ suggests is a
> > global variable you want to hide when in fact its a static. I also
> > don't get what _period is being used for. I would rename it something
> > more understandable, and without the _.
> 
> All that code is a copy of the snds and e7t hal which very semilar to
> the aim711. My intention was to make one variant hal for the s3c4510
> controller, which are used from the different platforms.
> But till now I haven't had the time to do this, so I have just made
> a copy of the snds hal and changed the things which are needed for
> the aim711.

OK leave it as it is. Changing it will make it harder for somebody
else to do a variant. Its easier to see a function is common if its
identical.

> > You include <string.h> but do not use anything from it as far as i can
> > see. You should not have this, it means you HAL is dependant on libc.
> 
> I needed memcpy(). Should I make a small loop to copy instead?

I missed that. The infra package implements memcpy, so it should
always be available. What i cannot find is a prototype in the infra
header files. I'm not sure what you are supposed to do!

> I will change these things and what John suggested
> as soon as possible and send it again to the list.

For the moment please just send the HAL. We can work on the others
once thats committed. 

     Andrew


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