Messages in this thread | | | From | David Howells <> | Subject | Re: [PATCH 0/6] MN10300: Add the MN10300 architecture to Linux kernel [try #4] | Date | Thu, 08 Nov 2007 19:11:05 +0000 |
| |
Sam Ravnborg <sam@ravnborg.org> wrote:
> > arch/mn10300/Makefile: > > 1) Use KBUILD_CFLAGS & KBUILD_AFLAGS & KBUILD_CPPFLAGS - they > have replaced the former xFLAGS.
Done. What about ASFLAGS and LDFLAGS?
> 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
That makes sense if I'm not calculating UNIT or PROCESSOR anyway. However, see below.
> 4) Drop the symlinks - they are evil... > Use a structure like: > include/asm-mn10300/asb2303/proc/*.h
No. I think all the arch headers should really appear to be #included under asm/. That means that the structure would have to be:
include/asm-mn10300/asb2303/asm/proc/*.h
That then puts all these header files several levels further down, which isn't that good.
> Then you adjust -I include/asm-mn10300/asb2303 > and no symlinks needed. > And if you change processor kbuild will notice > and recompile everything.
That's the only plus, but it's a smaller plus than not interpolating several levels of almost empty directory into the paths for the proc- and unit-specific headers.
> No-one else does it this way but mn10300 could show how to do it.
I could. Or I could do what everyone else does.
Actually, what you perhaps ought to do for a start is to move the individual include/asm-$ARCH dirs to arch/$ARCH/asm and then you can avoid that symlink too.
> 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)/$@
Done.
> And then modify boot/Makefile accordingly
I don't see why it needs modification. $(KBUILD_IMAGE) == $(boot)/zImage for the zImage target.
> 6) archdep is long gone - delete the following
Done.
> 7) Please set KBUILD_DEFCONFIG := asb2303_defconfig > This enable build monkeys(1) to do "make ARCH=mn10300 defconfig"
Done.
> 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.
The copyright assignment on some of these files was made by MEI. I'm not sure I can change them.
> 2) The Image target is missing (as defined in arch/mn10300/Makefile > Can you just kill it?
Yes. Done.
> arch/mn10300/boot/compressed/Makefile: > 1) Is -tarditional really needed. We try to kill it all over (slowly)
Apparently not. Consider it gone.
> 2) Please name the linker script *.lds
Done.
> 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...
I don't remember why. It doesn't seem to be necessary now, however, so it's now gone.
> arch/mn10300/oprofile/Makefile: > +profdrvr-y := op_model_null.o > + > +oprofile-y := $(DRIVER_OBJS) $(profdrvr-y) > > The variable profdrvr seems useless.
Gone.
> 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.
Gone.
David - 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/
| |