CNS11XX FreeBSD port completed

[Yohanes Nugroho]

On Tue, Dec 29, 2009 at 8:53 PM, Rui Paulo <rpaulo at freebsd.org> wrote:

Hi Rui,

Thank you for spending some time to read my code. Here is the new patch.

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

ok, I just found out about this (I should have read this earlier).

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

it is in sys/arm/conf/CNS11XXNAS

> * 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.


ok, i have fixed this

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

ok, done

> * 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?

every *_machep.c in sys/arm was derived from the same file with the
same license. So I guess it is already been taken care.

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

ok, done

> * 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?

Looking a the implementation of other ports, I have moved this to
econa.c because the memory address space is far apart.

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

ok. I am working as a C++ programmer, and forgot about  that one loop.

> * what about the #if 0's ?

It has been removed. It was used for testing which one is faster,
sending framented or non-fragmented packets. Sending defragmented
packet is slower for this SoC (takes too much CPU time).

> * 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

ok, I have used whiltespace-mode in emacs to help me with this.

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

fixed.

Thanks for your help.

-- 
Regards
Yohanes
http://yohan.es/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cns11xx_20090103.diff
Type: text/x-patch
Size: 144557 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-arm/attachments/20100103/0048228d/cns11xx_20090103-0001.bin