[U-Boot-Users] AVR32 patchset available

[Haavard Skinnemoen]

On Thu, 31 Aug 2006 16:06:56 +0200
Wolfgang Denk <wd at denx.de> wrote:

> Dear Haarvard,
> 
> in message <20060831150308.22fdbab7 at cad-250-152.norway.atmel.com> you
> wrote:
> > 
> > > Yes, please read the README, and follow the rules given there.
> > 
> > Uh...could you please be more specific? I know some of the patches
> > are too large, but it's hard to split them up any further. I
> > thought I'd keep them off the list to avoid bothering people who
> > don't really care about the AVR32 port, but I can of course post
> > them if you want me to.
> 
> I mean the Coding Style rules; a cursory scan showed at least
> incorrect indentation (not by 8 characters, by space instead of TAB)
> in several files (cpu/at32ap/dcache_clean.c,
> cpu/at32ap/dcache_invalidate.c, and include/asm-avr32/posix_types.h)
> and trailing empty lines in some (include/asm-avr32/sections.h and
> include/asm-avr32/setup.h).

Ah, I guess I have to check my setup. Both git and quilt are supposed to
catch things like this...

> Some files look pretty odd and should be cleaned up; for example:
> 
> "cpu/at32ap/pio2.h":

Yup, that's an autogenerated file. I'll give them an overhaul.

>      88 /* Bitfields in BSR */
>      89
> 
> Get rid of this.

Will do.

>      90 /* Bitfields in ABSR */
>      91 #define PIO2_P0_OFFSET                          0
>      92 #define PIO2_P0_SIZE                            1

>     ... etc.
> 
> This just makes to code difficult  to  read  and  understand.  Please
> cleanup.

I agree, they don't make much sense for the PIO controller, and they
aren't really used anyway, so I'll just get rid of them.

> The same applies to other files as well.

Ok, I'll clean up the register definition headers.

> Please get rid of include/asm-avr32/errno-base.h and use  justerrno.h
> like other architectures do.

Ok, will do.

> Your global data structure contains a field "jt". Please add  comment
> what it's needed for.

I haven't got the faintest clue what it's needed for, but all
architectures have got one. I'll add a comment saying "jump table".

> Please get rid of include/asm-avr32/initcalls.h

Where should I move the prototypes?

Actually, I see there are a couple of prototypes that aren't needed in
the current patchset. I'll remove them as well.

> > The only other things I can think about are CHANGELOG and CREDITS
> > entries, although the Subject: line in each patch should be suitable
> > for the changelog, no?
> 
> Special formatting is required for the CHANGELOG entry.

Documented where?

> > > The patch might have been built against an  ancient  version  of
> > > the software - this is of no use.
> > 
> > Indeed. But you really shouldn't apply the "combined" patch -- I
> > intended that one for people who want to test stuff without having
> > to figure out the exact git state I made the patchset against.
> 
> That's what I did. I just used the latest version.

There's a newer version than 1.1.4?

> > Also, as I mentioned in the previous mail, some of the patches have
> > been submitted before, and some should not be applied at all. Here
> > are the patches you may want consider:
> > 
> > http://avr32linux.org/patches/u-boot/1.1.4-hs1/broken-out/avr32-arch.patch
> > http://avr32linux.org/patches/u-boot/1.1.4-hs1/broken-out/at32ap-cpu.patch
> > http://avr32linux.org/patches/u-boot/1.1.4-hs1/broken-out/atmel-usart.patch
> > http://avr32linux.org/patches/u-boot/1.1.4-hs1/broken-out/atstk1000-board.patch
> 
> See comments above. Please clean up and resubmit on the mailing list.

Ok, I'll do that. Thanks for taking the time to review it.

> > If you want me to combine these patches into a single package,
> > please let me know.
> 
> No, please stick to the rules as speicfied in the README.

I take it you think the current division is fine, then?

Thanks,

Haavard