CNS11XX FreeBSD port completed

Rui Paulo rpaulo at freebsd.org
Tue Dec 29 13:53:39 UTC 2009


Hi,
On 25 Dec 2009, at 17:48, Yohanes Nugroho wrote:

> Hi,
> 
> To make it easy for others to read the changes I have made, attached
> is the diff version against SVN head. There is one change that may be
> should not be commited. In vfs_mount.c, I added
> 
> pause("WAIT", hz * 10);
> 
> That line can be removed if the patch from this:
> 
> http://lists.freebsd.org/pipermail/freebsd-current/2009-October/012361.html
> 
> is applied

Some comments:
* please read style(9) and try to follow these guidelines on your code

* did you forget to add a kernel config file for this board?

* I would like for you to reconsider the code that's under C++ comments. Either remove it or please use ANSI-style /* */ C comments. This is a style issue.

* please add your email to all the files you are adding (like you do on if_ecereg.h, for example)

* econa_machdep.c is a 4-claus BSD license. Can you check if we already include "This product includes software developed by Brini." on our documentation/marketing material?

* there are some lines that go beyond 80 columns. Can you fix them?

* in if_ece.c::poweron(), can't you use bus_read_4 instead of using a volatile and pointing it to some specific memory address?

* please avoid doing in-code variable declarations like "for (int i = 0 ..."

* what about the #if 0's ?

* do you have the specs of the ethernet controller ? stuff like "mac_port_config |= ((0x1 << 18));" would be better if "18" was a define with an indication of what the bit actually does. Something like "mac_port_config |= ((0x1 << MAC_PORT_0_DISABLE));"

* there are many white lines that you can remove

* defines should be like "#define<tab>...". This is especially visible in econa_reg.h

The rest looks fine.

Thanks!
--
Rui Paulo



More information about the freebsd-arm mailing list