[Bug 1000761] eCos support for MPC5xxx MCUs

[bugzilla-daemon at bugs dot ecos dot sourceware dot org]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1000761

--- Comment #32 from Jonathan Larmour <jifl@ecoscentric.com> 2012-05-31 14:54:24 BST ---
Actually, I can make a few more sweeping comments based on a scan of what's
there in the most recent version of the contribution posted:

The first one I should just reiterate is that the current copyright and licence
headers on some files not only don't fit the normal eCos template, but include
things like: Copyright (C) Freescale <year> All Rights Reserved. Most of these
do have a better licence, but not all do. For example, three instances of a
mpc5510.h do not.

All files have to be redistributed either under the normal eCos licence (GPL
plus an exception), or under a more liberal but compatible licence (such as BSD
without advertising clause, public domain, etc.).

"All Rights Reserved" means that there are no implicit rights to redistribute
or even use the code. We can never accept contributions with those terms,
although obviously I'm sure this is just an administrative oversight here.

Some of the licence headers do meet the requirements. Some of them look like
they do but actually are based on the old Red Hat copyright (look for mentions
of Red Hat, including in ChangeLog files). Some of them (e.g. var_cache.h) have
no header at all.

I'm not demanding fully accurate Purpose and Description fields filled in for
every file, but I would like to see it up to the same standard as other HALs,
with all files matching the full template (even if there are Freescale specific
bits immediately underneath the standard parts), and the names of files, and a
little one-line description just underneath, along with author and date. Author
can just be Freescale if you prefer, if relevant.

A lot of the spacing on code is a bit unusual - it appears a lot was created in
editors set to generate TABs, but with tab spacing set at 4 columns. TABs
always present a problem, and although some sneak in, we avoid them in general
(with the exception of ChangeLog files where they are a required part of the
syntax). I suggest either running a script over the files to fix these,
assuming consistent use, or you may wish to consider GNU indent.

For example I ran the following to make the code easier to follow for me:
cat > /tmp/blah.sh <<EOF
#!/bin/sh
for i in `find . -name ChangeLog -prune -o -type f -print` ; do  sed 's@    @  
 @g' $i > $i.jifl ; \mv $i.jifl $i ; done
EOF
(with a tab character inside the relevant part of the sed expression). Although
even that isn't perfect, for example I spotted the line continuation escapes
(the backslashes) in macros in var_cache.h are still all over the place.

That said, no contribution which complies with the GNU coding style will ever
be rejected, even though it does not match the traditional eCos coding style.
Unhelpfully the formal documentation of coding style was only ever written in a
document which was never made public by Cygnus/Red Hat, so we have to go by
existing practice! But it does mean that if you wanted to use GNU indent, that
might work. I can't say that will definitely be the fastest route to resolve
things though.

Not all packages have top level ChangeLog files, and some of them which do
mention the wrong platforms in them.

Finally, there are a large number of duplicated files. Some of them just end up
that way naturally like all the plf_stub.h files, or mlt files, so I don't mind
them. But there are other files like lots of the var_regs.h, mpc5510.h, most
(all?) hal_diag.c's and so on, which need only exist in one place. In some
cases, there may be situations where a platform may need to override things,
such as hal_diag.c, but 99% of the time, a default implementation which lives
in the variant HAL could be used; and a facility to allow it to be overridden
provided instead. This principle may apply to other things too.

Some of them like all the plf_cache.h's aren't even used (if they were intended
to be used they would be included from var_cache.h but they aren't).

Since I've already had a look at which files are identical, to make things
easier for you, I will attach the complete list (for the existing contrib
anyway).

Jifl

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.