[PATCH v8 3/6] mtd:nand:omap2: clean-up BCHx_HW and BCHx_SW ECC configurations in device_probe

[Gupta, Pekon]

Hi Brian,

Thanks for such detailed review, please see some replies below..

> From: Brian Norris [mailto:computersforpeace at gmail.com]
> > On Fri, Oct 11, 2013 at 07:06:40PM +0530, Pekon Gupta wrote:
[...]
> Why do you even need the #ifdef's for the #include's? It is not harmful
> to include headers for stuff that is only conditionally used. In fact,
> this creates a problem later when you try to use nand_bch_free(), and
> you have to surround it with extra #ifdef's.
> 
> In short: these #ifdef's just breed more #ifdef's and cause the code to
> become harder to read and less maintainable.
> 
There are 2 problems here:
(1) 
I have removed #ifdef across headers in next version. But I cannot remove
#ifdef across some callbacks for  cc.hwctl(), ecc.calculate(), ecc.correct(),
because then compilation would throw warnings for un-used functions.
(2)
OMAP driver uses generic lib/bch.c which is compiled only when
CONFIG_MTD_NAND_ECC_BCH is enabled. So to avoid compilation
issues, I had to put #ifdefs on all wrapper functions which use lib/bch.c
or nand_bch.c

I myself have tried in previous versions to avoid #ifdefs, but I ended
up in one or the other problem like (1) |  (2) above.
And this is where randconfig test failed earlier when Arnd Bergmann
commented, so some #ifdef would hv to be carried till be deprecate
some legacy ecc-schemes.
However, with patch split many redundant #ifdefs are now removed.


> (I commented about the #ifdef's around nand_bch_free() in v6, and you
> did not address the comment.)
> 
Done now.. My bad again, I somehow mis-interpreted nand_bch.c earlier.


> > @@ -1846,20 +1726,18 @@ static int omap_nand_probe(struct
> > -	info->nand.options	|= NAND_SKIP_BBTSCAN;
> > -#ifdef CONFIG_MTD_NAND_OMAP_BCH
> > +	nand_chip->options	|= NAND_SKIP_BBTSCAN |
> NAND_BUSWIDTH_AUTO;
> 
> I recommended (in v6) you split the NAND_BUSWIDTH_AUTO change to a
> new
> patch and to describe it in the commit. You did neither.
> 
Sorry missed this purposely because I could not separate out the changes.
But now I have re-worked this in next revision as a separate patch.


> > +	/* scan NAND device conncted to controller */
> > +	if (nand_scan_ident(mtd, 1, NULL)) {
> > +		err = -ENXIO;
> > +		goto out_release_mem_region;
> > +	}
> > +	pr_info("%s: detected %s NAND flash\n", DRIVER_NAME,
> > +			(nand_chip->options & NAND_BUSWIDTH_16) ?
> "x16" : "x8");
> 
> I recommended you kill this in v6, and you did not address my comments.
> 
Sorry, this was my bad.


with regards, pekon