Hi Brian, Thanks for such detailed review, please see some replies below.. > From: Brian Norris [mailto:computersforpeace@xxxxxxxxx] > > 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
