Re: [PATCH 2/2 v3 RESEND] mtd: fsl_upm: Support NAND ECC DTS properties

[Aaron Sierra <asierra@xes-inc.com>]

----- Original Message -----
> From: "Brian Norris" <computersforpeace@gmail.com>
> Sent: Wednesday, January 28, 2015 7:20:42 PM
> 
> On Wed, Jan 28, 2015 at 06:37:36PM -0600, Aaron Sierra wrote:
> > ----- Original Message -----
> > > From: "Brian Norris" <computersforpeace@gmail.com>
> 
> > > I was thinking about this a bit more, and it seems like we could really
> > > just factor this all into the core nand_base code with something like
> > > the following patch. It could possibly use some smarter logic to rule
> > > out certain combinations (but some of those are already caught in
> > > nand_scan_tail() anyway). What do you think?
> > 
> > Brian,
> > If the NAND device tree property fetching were moved out of fsl_upm,
> > I think it should not be called within nand_scan(). I think that
> > it's imperative that each driver be able to access these properties
> > before handing off to nand_scan(), since there are hardware ECC
> > modes that only drivers will know how to error check.
> 
> That's why nand_scan() is broken into nand_scan_ident() and
> nand_scan_tail() functions which can be called individually. This allows
> drivers to do the up-front initialization in nand_scan_ident(), do their
> own error checking and handling of these parameters, and then call
> nand_scan_tail(). See, for example, drivers/mtd/nand/omap2.c.

Thanks for pointing that out; I'll take a look.
 
> > Also, catching errors in nand_scan_tail() tends to result in BUG()s.
> 
> Well, some of those can be changed. Feel free to propose changes. I'd
> prefer to make nand_scan_tail() play nicer than to compensate in
> individual drivers.
> 
> > That said, this could be useful as a publicly exported function that
> > individual drivers are responsible for calling (maybe in of_mtd.c).
> 
> I don't think of_mtd.c should really contain a lot of mtd_info /
> nand_chip knowledge, if we can avoid it.
>
> I really do think that the nand_scan() option is a better idea, if we
> can work out the other details (BUG(), error checking, and keeping it
> flexible enough). I think it provides the best place to flesh out any
> other common DT handling for all NAND drivers.
> 
> Related: I believe the question came up recently about how to support a
> generic DT binding for using a GPIO as a NAND write-protect line. This
> would be another candidate for handling transparently in
> nand_scan_ident() and would then immediately apply to all NAND drivers,
> not just those that were rewritten to call another specialized init
> function.
> 
> I really don't want to encourage the anti-pattern of each driver
> reimplementing code that might as well be shared, if at all possible.
> Adding more decentralized helpers to of_mtd.c does not really help that
> cause.

Understood.

[ snipped function prototype discussion ]

> > You hinted at implementing stronger error checking. If we went
> > this route, would it make sense to only error check the software
> > ECC modes?
> 
> Yes. I just elided some of the details for now, since it's not actually
> necessary to do some of it (many other drivers can use SW ECC without
> the extra error checks).

OK, I'll rework the fsl_upm patch to work with your proposed patch.

-Aaron