CNS11XX FreeBSD port completed

[Yohanes Nugroho]

On Sun, Jan 3, 2010 at 6:09 PM, Stanislav Sedov <stas at freebsd.org> wrote:

Hi Stanislav,

Thank you for looking so thoroughly at my code. Here is my latest patch attempt.

>> +void *
>> +initarm(void *arg, void *arg2)
>> +{
>> +     struct pv_addr  kernel_l1pt;
>> +     int loop, i;
>> +     u_int l1pagetable;
>> +     vm_offset_t freemempos;
>> +     vm_offset_t afterkern;
>> +     uint32_t memsize;
>> +     vm_offset_t lastaddr;
>> +     volatile uint32_t * ddr = (uint32_t *)0x4000000C;
>> +     int mem_info;
>
> Style(9) recommends sorting local variable declarations by size, then
> alphabetically.  This makes reading easier.

actually, i just added the last 2 variables. Almost all of the
*_machdep in sys/arm is copied from one source. But I have reordered
them in my latest patch. I ordered the size from large to small.
Reading some random kernel code, it doesn't seem to be consistent
(some orders from small to large, and I think most doesn't order them
alphabetically).

>> +
>> +     boot_arg1 = arg;
>> +     boot_arg2 = arg2;
>> +     boothowto = RB_VERBOSE;
>> +     boothowto |=  RB_SINGLE;
>
> Why are you setting boothowto here?  Is it possible for the board to boot
> to multiuser?

I looked at sa11x0/assabet_machdep.c, and it is how they initialize
the boot flags.

The device can boot to multi user, so I have removed the line (the
single user mode was good for testing).

>> +static inline uint32_t
>> +RD4(struct ece_softc *sc, bus_size_t off)
>> +{
>> +     return (bus_read_4(sc->mem_res, off));
>> +}
>> +
>> +static inline void
>> +WR4(struct ece_softc *sc, bus_size_t off, uint32_t val)
>> +{
>> +     bus_write_4(sc->mem_res, off, val);
>> +}
>
> Funcctions without local variable should have an extra blank line at the
> beginning, per style(9).  Also, all-capitals names are usually used for macros,
> not function names.

ok, about the naming. When creating the code, I was copying the style
from arm at91 source code where all of the files uses WR4 and RD4
*function*, the same style is also used in dev/mwl/mwlhal.c. I have
replaced the names, and added the whitespace.

>> +static int eceioctl(struct ifnet * ifp, u_long, caddr_t);
>            ^^^
> Use TAB after the return value type in prototypes.

I thought that was only for prototypes in header. When I looked at
most networking code, it just uses space for internal prototypes
(if_jme.c, if_age.c, if_dc.c, etc). I was again wrong in looking the
styles of other code. I have added the tabs as you suggested.

> Missing spaces.  The same for other parts of code.
> Can you, please, review your code considering these
> comments and send the patch again?  In gerneal, the
> code looks great, but there're some minor nits that
> should be fixed before it hits the base.

Ok, I have tried to read the latest patch several times, I hope I got
it right this time.

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