Messages in this thread | | | Date | Thu, 8 Nov 2007 19:29:38 +0100 | From | Sam Ravnborg <> | Subject | Re: [PATCH 0/6] MN10300: Add the MN10300 architecture to Linux kernel [try #4] |
| |
Hi David.
A few comments to patch #5. I only looked at the kbuild related stuff - and I have seen stuff much worse than this. Yet I have a few comments:
arch/mn10300/Makefile:
1) Use KBUILD_CFLAGS & KBUILD_AFLAGS & KBUILD_CPPFLAGS - they have replaced the former xFLAGS.
2) Consider using unit-y as replacement for UNIT. This enable you to do: unit-$(CONFIG_MN10300_UNIT_ASB2303) += asb2303
3) Same for PROCESSOR => processor-y
4) Drop the symlinks - they are evil... Use a structure like: include/asm-mn10300/asb2303/proc/*.h
Then you adjust -I include/asm-mn10300/asb2303 and no symlinks needed. And if you change processor kbuild will notice and recompile everything.
No-one else does it this way but mn10300 could show how to do it.
5) +zImage: vmlinux + $(Q)$(MAKE) $(build)=$(boot) $(KBUILD_IMAGE) + +all: zImage + +Image: vmlinux + $(Q)$(MAKE) $(build)=arch/mn10300/boot $@
This could be done as: +Image zImage: vmlinux + $(Q)$(MAKE) $(build)=$(boot) $(boot)/$@
And then modify boot/Makefile accordingly
6) archdep is long gone - delete the following
+archdep: scripts/mkdep symlinks + $(Q)$(MAKE) -C arch/mn10300/boot dep
7) Please set KBUILD_DEFCONFIG := asb2303_defconfig This enable build monkeys(1) to do "make ARCH=mn10300 defconfig"
(1) such as me
arch/mn10300/boot/Makefile:
1) You can safely remove the copyright of Linus.. Same goes for other places where his copyright are kept but filecontent is new.
2) The Image target is missing (as defined in arch/mn10300/Makefile Can you just kill it?
arch/mn10300/boot/compressed/Makefile: 1) Is -tarditional really needed. We try to kill it all over (slowly)
2) Please name the linker script *.lds
arch/mn10300/kernel/Makefile: +# This next line shouldn't be necessary, but it is. +CPPFLAGS += $(CPPFLAGS_$(@F)) +CPPFLAGS_asm-offsets.s = -D__ASM_OFFSETS_H__
Please explain why...
arch/mn10300/oprofile/Makefile: +profdrvr-y := op_model_null.o + +oprofile-y := $(DRIVER_OBJS) $(profdrvr-y)
The variable profdrvr seems useless.
arch/mn10300/unit-asb2303/Makefile:
Please drop this wrong comment: +# Note! Dependencies are done automagically by 'make dep', which also +# removes any old dependencies. DON'T put your own dependencies here +# unless it's something special (ie not a .c file). +# +# Note 2! The CFLAGS definitions are now in the main makefile... +
Seen in other places where they are equally wrong/outdated.
Sam
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |