[U-Boot] [PATCH 1/4] arm: iproc: add NAND driver

[Steve Rae]

On Fri, Mar 11, 2016 at 11:55 AM, Scott Wood <oss at buserror.net> wrote:
> On Fri, 2016-03-11 at 11:47 -0800, Steve Rae wrote:
>> On Fri, Mar 11, 2016 at 11:29 AM, Scott Wood <oss at buserror.net> wrote:
>> > On Thu, 2016-03-10 at 14:26 -0800, Steve Rae wrote:
>> > > From: Jiandong Zheng <jdzheng at broadcom.com>
>> > >
>> > > Add support for the iproc NAND, and enable on Cygnus and NSP boards.
>> > >
>> > > Signed-off-by: Jiandong Zheng <jdzheng at broadcom.com>
>> > > Signed-off-by: Steve Rae <srae at broadcom.com>
>> > > ---
>> > > There was a previous attempt to implement this "iproc NAND"
>> > > (see: http://patchwork.ozlabs.org/patch/505399), however, due to the
>> > > amount of changes required, it seemed better to implement the code
>> > > in a series of steps. This is the first step, where the "iproc_nand.c"
>> > > is essentially an empty file (with one function required to allow this
>> > > commit to build successfully).
>> >
>> > I don't see the value of applying a such a do-nothing patch.  It's fine to
>> > leave out unnecessary features, things that improve performance, etc. but
>> > to
>> > be applied a patchset should accomplish something useful and correct, not
>> > just
>> > be a staging area for future patches.
>>
>> I agree -- but with the previous attempt, there where so many things
>> that went wrong, that I cannot comprehend what is needed.
>> So, I wanted to break it down into pieces, so that I could get clear
>> responses to help me get it right.
>> right now, I understand that I need to move certain defines to Kconfig ....
>
> Go through the previous comments and address (or respond to) them one by one,
> then submit again.  If you want to break it into multiple patches, that's fine
> as long as the intermediate states are sane, but it should all be submitted at
> once as part of a patchset (again, except for unnecessary features).

OK - that was my plan (to address every previous comment)...
I was hoping to get "incremental" comments to indicate that I was
resolving them acceptably...
My plan was to increment v2, v3, vxxx until it was acceptable.
Would this be OK?

>
> Also, please keep discussion on the mailing list.
>

Sorry....

>>
>> >
>> > > diff --git a/arch/arm/include/asm/arch-bcmcygnus/configs.h
>> > > b/arch/arm/include/asm/arch-bcmcygnus/configs.h
>> > > index 3c07160..3bf7395 100644
>> > > --- a/arch/arm/include/asm/arch-bcmcygnus/configs.h
>> > > +++ b/arch/arm/include/asm/arch-bcmcygnus/configs.h
>> > > @@ -10,6 +10,7 @@
>> > >  #include <asm/iproc-common/configs.h>
>> > >
>> > >  /* uArchitecture specifics */
>> > > +#include <../../../drivers/mtd/nand/iproc_nand_cygnus.h>
>> >
>> > No.
>>
>> this "include" line is unacceptable?
>
> Using "../../.." to reach into a code directory's private headers is
> unacceptable, yes.
>
>> could you propose something that would work?
>
> If you want the info to be in the driver directory, use an ifdef there, based
> on a config symbol.  This seems like the better approach.

Maybe I misinterpreted the comments related to:

+#if defined(CONFIG_CYGNUS)
+#include "iproc_nand_cygnus.h"
+#elif defined(CONFIG_NS_PLUS)
+#include "iproc_nand_ns_plus.h"
+#else
+#error "Unsupported configuration"
+#endif

Scott - I thought the you did not like this logic -- now I am thinking
you didn't like the "CONFIG_*" names....
I'm guessing that the names should be:
CONFIG_SYS_BCM_CYGNUS
CONFIG_SYS_BCM_NSPLUS
and that they should be added to Kconfig?

>
> If you want the info to come via the config header, move it to someplace the
> config header can properly include.
>
> -Scott
>