Message-ID: <20090701130018.115ce0ea@lxorguk.ukuu.org.uk>
Date:	Wed, 1 Jul 2009 13:00:18 +0100
From:	Alan Cox <alan@...rguk.ukuu.org.uk>
To:	"Scott Branden" <sbranden@...adcom.com>
Cc:	"Jean-Christophe PLAGNIOL-VILLARD" <plagnioj@...osoft.com>,
	"Leo (Hao) Chen" <leochen@...adcom.com>,
	"linux-arm-kernel@...ts.arm.linux.org.uk" 
	<linux-arm-kernel@...ts.arm.linux.org.uk>,
	"Linux Kernel" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1 6/6][ARM]  new ARM SoC support: BCMRing

> We would really like to know what needs to change in our patchset.  We have a large codebase of drivers for multiple chipsets already written that we have been maintaining internally and releasing for customer development.  For code that patches existing linux code we follow the style dictated in those files.  But for our new code we do not.
> 
> For our new code we have many developers who have written the code and thus a few "linux standard" coding styles have not been followed.  Much driver code is split into an os-less portion and a linux wrapper portion.  People have written following ISO/IEC 9899:1999 coding which allows such things as // comments.  Also using tabs in our code is not a requirement.  
> 
> My question is do we need to change our entire codebase over to match linux coding standards for comments and whitespace so it can pass a checkpatch script?  I'm sure there is a standard linux kernel community response to this but I need to forward that answer (whatever it is) to our many developers so we know what it is required to get our existing code integrated into the standard linux kernel.
> 

There isn't a general rule because there isn't a general case.

If you look at existing kernel code we have various drivers which have
components shared with other OS systems (usually header files). Some of
those are even generated from firmware builds and/or vhdl tools. (obvious
example drivers/scsi/dpt_i2o.c and drivers/scsi/dpt/*h - the driver is
Linux format completely, the Linux header bits are likewise but various
structures and layout shared across all of the board uses are not)

For stuff that never needs editing its not really too important unless
its horribly obfuscated. Some people run their shared stuff through some
quick formatting scripts so the Linux submission of the header looks a
bit more Linuxy - and if its a header and the reformatting is automated
it avoids creating work maintaining two copies.

Anything which is kernel code to be maintained/edited in the kernel
really needs to be in the kernel style. A lot of that is pretty easy to
automate: indent will fix the formatting, and a simple search and
replace script can turn comments of the form 

	// foo

into
	/* foo */


Taking the submission itself there are some obvious stylist things I'd
sat to fix first (beyond trivia like spaces and comments)

Use of gSysCtlfoo and similar for globals. That's asking for collisions
if everyone does it so the kernel preference is for names of the form

	myplatform_xxx

eg

	bcmring_arch_warm_reboot

(and generally preferring low case names for variables)

The code uses a lot of sempahores as mutexes which is something we've
been working hard to stamp out (to make pre-emptible real time merging
in the future easier)

There are various oddities in the code that are weird (eg defining MIN
when the kernel has min and min_t anyway)


Some people unfortunately like jumping up and down about spaces but not
code. The basic code is readable if oddly styled (from a kernel viewpoint)
and its possible to understand what it is doing on a first pass. I'd
rather read good poetry written in very bad hand writing than bad poetry
written in beautiful handwriting, and I think the same is true of code.

So my suggestions for a first pass would be

Bang the Linux specific bits through indent, some scripts to clean up the
non shared bits of the codebase. Fix up the little niggly things like MIN
v min and the global names. That should be mostly automatable and pretty
quick to do. Checkpatch is a handy guide but don't treat it as some kind
of dictat, at times it makes very bad suggestions for readability.

At that point we can concentrate on the underlying code, which I think is
sound, and some case by case discussion for the shared platform code
depending upon what it is and how it is used.

The kernel is an awful lot of code from an awful lot of people and to
some extent you do have to impose a style so anyone can work on it
sensibly. There are however cases where you've got shared code and want
different answers.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists