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