(trying newer address for Steve, sorry for the spam) > On Sat, Mar 12, 2016 at 9:18 AM, Scott Wood <o...@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 <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? >> >> 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. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot