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

[Chris Packham]

(trying newer address for Steve, sorry for the spam)

> On Sat, Mar 12, 2016 at 9:18 AM, 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?"
>>
>>> > > > > 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

Hi Steve,

Where did this get to? I find myself in need of a NAND driver for a
BCM58525 and this seems to be relevant.