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

[Brian Norris <computersforpeace@gmail.com>]

Hi Pekon,

On 10/12/2013 04:58 PM, Gupta, Pekon wrote:
>> From: Brian Norris [mailto:computersforpeace@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.

First off, I should clarify this: the above comment was speaking *only* 
about surrounding the #include's with #ifdef's. Such #ifdef's only make 
everything else harder.

Now, I recognize that there are still many cases where you may need 
#ifdef's in the body of the code. I was only trying to hit the easiest ones.

> 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.

The __maybe_unused attribute can be given to functions that will compile 
but are unused, and the compiler will just remove unused ones from the 
binary without warning. That is probably (weakly) preferable to 
#ifdef's, if you can manage it. But I understand some #ifdef's are 
necessary.

> (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

nand_bch.c (and its corresponding nand_bch.h) has stub implementations 
so that extra #ifdef's often aren't needed. But include/linux/bch.h does 
not, so you are correct.

> 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.

Great, thanks for the effort.

Regards,
Brian