Hi,
On Tue, 2009-07-28 at 15:38 -0700, Leo (Hao) Chen wrote:
> include/linux/mtd/nand_bcm_umi.h | 241 ++++++++
It does not look like you need to have this header file in
include/linux/mtd/. Also, this file contains huge inline functions,
which is not nice. We are trying not to make functions longer than
1-2 lines inline and let the compiler do the decisions. Of course,
there are exceptions, but your 'nand_bcm_umi_bch_correct_page' does
not look like this. Could you please go through all your inlines and
un-inline non-few-liners?
Sorry, I do not have time to do any real review, but from the stylistic
POW you have too many
/*********************************************************************/
things, as well as ThisLovelyWindowsStyle naming, which we do not
appreciate very much in the Linux Kernel.

Hi Artem,
Thanks for the comments.
This particular code (nand_bcm_umi.h) is shared between multiple OS's, simulation environments, and non-OS's. As you are aware, this alone presents maintenance challenges. The simplest approach was to make an include file and wrap that for each particular environment. We will look at splitting this into a C and H file.
The file passes Linux's checkpatch scripts. I don't see a need for any further requirement? It would be sad if a particular linux maintainer was stuck on other stylistic concerns because other maintainers are not. Especially when this code is not specifically written for linux and is supported, tested, and maintained by us.
Regards,
Scott

On 08/09/2009 10:12 AM, Scott Branden wrote:
> Hi Artem,>> Thanks for the comments.>> This particular code (nand_bcm_umi.h) is shared between multiple> OS's, simulation environments, and non-OS's. As you are aware, this> alone presents maintenance challenges. The simplest approach was to> make an include file and wrap that for each particular environment.> We will look at splitting this into a C and H file.
Well, the idea is that include/linux/mtd the external MTD interface,
so you should put only the stuff which is about MTD<->other subsystems.
There are also headers that are about MTD core <-> MTD drivers interface.
So if you need to share something between different parts of your driver,
this should be rather in drivers/mtd/nand/, not in include/linux/mtd.
> The file passes Linux's checkpatch scripts. I don't see a need for> any further requirement? It would be sad if a particular linux> maintainer was stuck on other stylistic concerns because other> maintainers are not. Especially when this code is not specifically> written for linux and is supported, tested, and maintained by us.
I do not have a strong opinion here, and I'm personally fine with this.
I mean, I do not mind if you ignore my /**...**/ and 'WindowsStyleVars'
comment.

> -----Original Message-----> From: Artem Bityutskiy [mailto:dedekind1@gmail.com] > Sent: August 9, 2009 12:27 AM> To: Scott Branden> Cc: Artem Bityutskiy; Leo (Hao) Chen; > linux-mtd@lists.infradead.org; linux-arm-kernel@lists.arm.linux.org.uk> Subject: Re: [PATCH v3 26/27] [ARM] [NAND] [bcmring] add > bcmring umi nand driver support> > On 08/09/2009 10:12 AM, Scott Branden wrote:> > Hi Artem,> >> > Thanks for the comments.> >> > This particular code (nand_bcm_umi.h) is shared between > multiple OS's, > > simulation environments, and non-OS's. As you are aware, > this alone > > presents maintenance challenges. The simplest approach was > to make an > > include file and wrap that for each particular environment.> > We will look at splitting this into a C and H file.> > Well, the idea is that include/linux/mtd the external MTD > interface, so you should put only the stuff which is about > MTD<->other subsystems.> There are also headers that are about MTD core <-> MTD > drivers interface.> > So if you need to share something between different parts of > your driver, this should be rather in drivers/mtd/nand/, not > in include/linux/mtd.>
Understood - it looks like nand_bcm_umi.h is in the incorrect location. We will move it to drivers/mtd/nand and adjust Makefiles accordingly.
> > The file passes Linux's checkpatch scripts. I don't see a need for > > any further requirement? It would be sad if a particular linux > > maintainer was stuck on other stylistic concerns because other > > maintainers are not. Especially when this code is not specifically > > written for linux and is supported, tested, and maintained by us.> > I do not have a strong opinion here, and I'm personally fine > with this.> I mean, I do not mind if you ignore my /**...**/ and > 'WindowsStyleVars'> comment.> > --> Best Regards,> Artem Bityutskiy (Артём Битюцкий)> >