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

[Steve Rae]

On Fri, Mar 11, 2016 at 12:18 PM, Scott Wood <oss at buserror.net> wrote:
> On Fri, 2016-03-11 at 12:13 -0800, Steve Rae wrote:
>> 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?
>
> It's OK if you mark them as [RFC PATCH] so it's clear that you don't mean them
> to be applied, only commented on -- and include a TODO list so we know what
> you plan to address before dropping the "RFC".
>
> Or just include a code fragment when replying to feedback, with a comment
> like, "Is this what you're looking for?"
>

I understand - I think I can proceed now -- THANKS!

>> > > > > 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?
>
> Correct.
>
> -Scott
>