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

[Scott Wood <oss@buserror.net>]

On Fri, 2016-03-11 at 12:13 -0800, Steve Rae wrote:
> On Fri, Mar 11, 2016 at 11:55 AM, Scott Wood <oss@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@buserror.net> wrote:
> > > > On Thu, 2016-03-10 at 14:26 -0800, Steve Rae wrote:
> > > > > From: Jiandong Zheng <jdzheng@broadcom.com>
> > > > > 
> > > > > Add support for the iproc NAND, and enable on Cygnus and NSP boards.
> > > > > 
> > > > > Signed-off-by: Jiandong Zheng <jdzheng@broadcom.com>
> > > > > Signed-off-by: Steve Rae <srae@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