On Thu, 31 Aug 2006 16:06:56 +0200 Wolfgang Denk wd@denx.de wrote:
Dear Haarvard,
in message 20060831150308.22fdbab7@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 */ 89Get 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.pa...
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