[lkml]   [2010]   [May]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH] arch/tile: new multi-core architecture for Linux
> > There is also several TILE specific options missing the TILE_ prefix.
> > Like:
> > config XGBE_MAIN
> > tristate "Tilera GBE/XGBE character device support"
> >
> > Drop this:
> > config XGBE_MAIN
> > tristate "Tilera GBE/XGBE character device support"
> >
> > It is better to test for the gcc version and disable the option
> > only in the cases where it is known to fail.
> >
> Is the "Drop this" comment a cut and paste bug?
Yep - sorry.

> I'm guessing you were
> referring to CONFIG_WERROR, which enables -Werror support. The problem
> is that whether or not you can use -Werror really depends on not just
> the kernel version and the gcc version, but very likely also what
> drivers you have enabled. We always use it internally. I could also
> just pull this out completely (and just force it into "make" externally
> within our external build process), or move it to a "generic" configure
> option. In any case we can't just automate it, unfortunately.
As Arnd pointed out the drivers does not belong in the
arch/tile/* hirachy.
And we have some architectures that always uses -Werror unconditionally.
So for the arch part this way to deal with it should be safe.
And the more we can cover under -Werror the better.

I dunno how you best deal with the drivers.

> > Do not mess with CC like this:
> > CC = $(CROSS_COMPILE)gcc
> >
> > I guess you had to do this to support:
> > LIBGCC_PATH := `$(CC) -print-libgcc-file-name`
> >
> > If you follow other archs you could do like this:
> > LIBGCC_PATH := `$(CC) -print-libgcc-file-name`
> >
> I'm guessing you meant like what h8300 does, "$(shell
> $(CROSS-COMPILE)$(CC) $(KBUILD_CFLAGS) -print-libgcc-file-name)". That
> seems reasonable.
Correct - you are good at guessing :-)

> > arch/tile/include/asm/spinlock.h
> > Please make this a one-liner when you uses the asm-generic version only.
> > Same goes for byteorder (which includes linux/byteorder/little_endian.h)
> >
> I'm not sure what you mean when you say to use the asm-generic version
> of spinlock.h, since it's not SMP-ready. Also, I don't see an
> asm-generic/byteorder.h, so I'm puzzled there too.
What I wanted to say was that a header file that simply include
another header file then drop all the boilerplate stuff and
let the header file be a single line.
Both spinlock.h and byteorder.h matches this.

The other 15+ header files that simply include another
heder file you follow this style. So this is a small matter
of consistency.

> > In your mail you did not say anything about the checkpatch status.
> > It is better that you make your code reasonable checkpatch clean _before_
> > merging. Then you will not be hit by a lot of janitorial patches doing so.
> >
> I ran checkpatch over everything I submitted. There are many
> complaints, to be sure, but I did a first pass cleaning up everything
> that was plausible, so for example all the style issues were fixed, but
> things like some uses of volatile, some uses of init_MUTEX, etc., were
> not modified. However, I think it's in decent shape from a checkpatch
> point of view.
Good. Please include this information in you next submission.


 \ /
  Last update: 2010-05-25 07:05    [W:0.066 / U:1.596 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site