Date: Tue, 29 Dec 2009 13:53:31 +0000 From: Rui Paulo <rpaulo@freebsd.org> To: Yohanes Nugroho <yohanes@gmail.com> Cc: freebsd-arm@freebsd.org Subject: Re: CNS11XX FreeBSD port completed Message-ID: <BEE46B4D-5280-48DF-B2FD-36D1FEFA332A@freebsd.org> In-Reply-To: <260bb65e0912250948w6f714367w672a1ebf037fb7f7@mail.gmail.com> References: <260bb65e0912110627o6b67b399vabaae57477b91023@mail.gmail.com> <260bb65e0912250948w6f714367w672a1ebf037fb7f7@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi,
On 25 Dec 2009, at 17:48, Yohanes Nugroho wrote:
> Hi,
>=20
> 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
>=20
> pause("WAIT", hz * 10);
>=20
> That line can be removed if the patch from this:
>=20
> =
http://lists.freebsd.org/pipermail/freebsd-current/2009-October/012361.htm=
l
>=20
> 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 =3D =
0 ..."
* what about the #if 0's ?
* do you have the specs of the ethernet controller ? stuff like =
"mac_port_config |=3D ((0x1 << 18));" would be better if "18" was a =
define with an indication of what the bit actually does. Something like =
"mac_port_config |=3D ((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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?BEE46B4D-5280-48DF-B2FD-36D1FEFA332A>