On Fri, Mar 11, 2016 at 11:55 AM, Scott Wood <o...@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 <o...@buserror.net> wrote: >> > On Thu, 2016-03-10 at 14:26 -0800, Steve Rae wrote: >> > > From: Jiandong Zheng <jdzh...@broadcom.com> >> > > >> > > Add support for the iproc NAND, and enable on Cygnus and NSP boards. >> > > >> > > Signed-off-by: Jiandong Zheng <jdzh...@broadcom.com> >> > > Signed-off-by: Steve Rae <s...@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 > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
