[PATCH v3 26/27] [ARM] [NAND] [bcmring] add bcmring umi nand driver support

[Scott Branden]

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

________________________________________
From: Artem Bityutskiy [dedekind at infradead.org]
Sent: August 8, 2009 10:16 PM
To: Leo (Hao) Chen
Cc: linux-mtd at lists.infradead.org; linux-arm-kernel at lists.arm.linux.org.uk
Subject: RE: [PATCH v3 26/27] [ARM] [NAND] [bcmring] add bcmring umi nand driver support

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.

--
Best Regards,
Artem Bityutskiy (áÒÔ£Í âÉÔÀÃËÉÊ)


-------------------------------------------------------------------
List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel
FAQ:        http://www.arm.linux.org.uk/mailinglists/faq.php
Etiquette:  http://www.arm.linux.org.uk/mailinglists/etiquette.php