Re: [PATCH 2/3] nand: omap2: Remove horrible ifdefs to fix module probe

[Roger Quadros <rogerq@ti.com>]

On 09/08/2014 11:45 AM, Roger Quadros wrote:
> On 09/06/2014 10:56 PM, Ezequiel Garcia wrote:
>> The current code abuses ifdefs to determine if the selected ECC scheme
>> is supported by the running kernel. As a result the code is hard to read,
>> and it also fails to load as a module.
>>
>> This commit removes all the ifdefs and instead introduces a function
>> omap2_nand_ecc_check() to check if the ECC is supported by using
>> IS_ENABLED(CONFIG_xxx).
>>
>> Since IS_ENABLED() is true when a config is =y or =m, this change fixes the
>> module so it can be loaded with no issues.
>>
>> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> 
> Didn't apply cleanly on 3.17-rc4. Needs a rebase?
> 
>> ---
>>  drivers/mtd/nand/omap2.c          | 134 +++++++++++++++++++++-----------------
>>  include/linux/platform_data/elm.h |  14 ++++
>>  2 files changed, 87 insertions(+), 61 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
>> index 1170389..987dd94 100644
>> --- a/drivers/mtd/nand/omap2.c
>> +++ b/drivers/mtd/nand/omap2.c
>> @@ -136,7 +136,6 @@
>>  
>>  #define BADBLOCK_MARKER_LENGTH		2
>>  
>> -#ifdef CONFIG_MTD_NAND_OMAP_BCH
>>  static u_char bch16_vector[] = {0xf5, 0x24, 0x1c, 0xd0, 0x61, 0xb3, 0xf1, 0x55,
>>  				0x2e, 0x2c, 0x86, 0xa3, 0xed, 0x36, 0x1b, 0x78,
>>  				0x48, 0x76, 0xa9, 0x3b, 0x97, 0xd1, 0x7a, 0x93,
>> @@ -144,7 +143,6 @@ static u_char bch16_vector[] = {0xf5, 0x24, 0x1c, 0xd0, 0x61, 0xb3, 0xf1, 0x55,
>>  static u_char bch8_vector[] = {0xf3, 0xdb, 0x14, 0x16, 0x8b, 0xd2, 0xbe, 0xcc,
>>  	0xac, 0x6b, 0xff, 0x99, 0x7b};
>>  static u_char bch4_vector[] = {0x00, 0x6b, 0x31, 0xdd, 0x41, 0xbc, 0x10};
>> -#endif
>>  
>>  /* oob info generated runtime depending on ecc algorithm and layout selected */
>>  static struct nand_ecclayout omap_oobinfo;
>> @@ -1292,7 +1290,6 @@ static int __maybe_unused omap_calculate_ecc_bch(struct mtd_info *mtd,
>>  	return 0;
>>  }
>>  
>> -#ifdef CONFIG_MTD_NAND_OMAP_BCH
>>  /**
>>   * erased_sector_bitflips - count bit flips
>>   * @data:	data sector buffer
>> @@ -1593,33 +1590,71 @@ static int omap_read_page_bch(struct mtd_info *mtd, struct nand_chip *chip,
>>  /**
>>   * is_elm_present - checks for presence of ELM module by scanning DT nodes
>>   * @omap_nand_info: NAND device structure containing platform data
>> - * @bch_type: 0x0=BCH4, 0x1=BCH8, 0x2=BCH16
>>   */
>> -static int is_elm_present(struct omap_nand_info *info,
>> -			struct device_node *elm_node, enum bch_ecc bch_type)
>> +static bool is_elm_present(struct omap_nand_info *info,
>> +			   struct device_node *elm_node)
>>  {
>>  	struct platform_device *pdev;
>> -	struct nand_ecc_ctrl *ecc = &info->nand.ecc;
>> -	int err;
>> +
>>  	/* check whether elm-id is passed via DT */
>>  	if (!elm_node) {
>> -		pr_err("nand: error: ELM DT node not found\n");
>> -		return -ENODEV;
>> +		dev_err(&info->pdev->dev, "ELM devicetree node not found\n");
>> +		return false;
>>  	}
>>  	pdev = of_find_device_by_node(elm_node);
>>  	/* check whether ELM device is registered */
>>  	if (!pdev) {
>> -		pr_err("nand: error: ELM device not found\n");
>> -		return -ENODEV;
>> +		dev_err(&info->pdev->dev, "ELM device not found\n");
>> +		return false;
>>  	}
>>  	/* ELM module available, now configure it */
>>  	info->elm_dev = &pdev->dev;
>> -	err = elm_config(info->elm_dev, bch_type,
>> -		(info->mtd.writesize / ecc->size), ecc->size, ecc->bytes);
>> +	return true;
>> +}
>>  
>> -	return err;
>> +static bool omap2_nand_ecc_check(struct omap_nand_info *info,
>> +				 struct omap_nand_platform_data	*pdata)
>> +{
> 
> I like the idea of this function and it being called in probe.
> 
> It would be also be nice (maybe later) to get rid of all the ECC decision making
> done in gpmc_probe_nand_child() based on presence/absence of the elm_of_node.
> I guess we need to add new DT entries for "bch4sw" and "bch8sw" so that DT/platform data
> can explicitly specify what it needs. I don't see how ELM works with the non-DT boot
> way as there is no elm_of_node there.
> 
> Also, the elm_of_node should not be set via platform data or in gpmc.c but parsed here
> directly in the NAND driver.
> 
> But changes should be done in a separate patch. Just brought them out
> here for discussion.
> 
>> +	bool ecc_needs_bch, ecc_needs_omap_bch, ecc_needs_elm;
>> +
>> +	switch (info->ecc_opt) {
>> +	case OMAP_ECC_BCH4_CODE_HW_DETECTION_SW:
>> +	case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW:
>> +		ecc_needs_omap_bch = false;
>> +		ecc_needs_bch = true;
>> +		ecc_needs_elm = false;
>> +		break;
>> +	case OMAP_ECC_BCH4_CODE_HW:
>> +	case OMAP_ECC_BCH8_CODE_HW:
>> +	case OMAP_ECC_BCH16_CODE_HW:
>> +		ecc_needs_omap_bch = true;
>> +		ecc_needs_bch = false;
>> +		ecc_needs_elm = true;
>> +		break;
>> +	default:
>> +		ecc_needs_omap_bch = false;
>> +		ecc_needs_bch = false;
>> +		ecc_needs_elm = false;
>> +		break;
>> +	}
>> +
>> +	if (ecc_needs_bch && !IS_ENABLED(CONFIG_MTD_NAND_ECC_BCH)) {
>> +		dev_err(&info->pdev->dev,
>> +			"CONFIG_MTD_NAND_ECC_BCH not enabled\n");
>> +		return false;
>> +	}
>> +	if (ecc_needs_omap_bch && !IS_ENABLED(CONFIG_MTD_NAND_OMAP_BCH)) {
>> +		dev_err(&info->pdev->dev,
>> +			"CONFIG_MTD_NAND_OMAP_BCH not enabled\n");
>> +		return false;
>> +	}
>> +	if (ecc_needs_elm && !is_elm_present(info, pdata->elm_of_node)) {
>> +		dev_err(&info->pdev->dev, "ELM not available\n");
>> +		return false;
>> +	}
>> +
>> +	return true;
>>  }
>> -#endif /* CONFIG_MTD_NAND_ECC_BCH */
>>  
>>  static int omap_nand_probe(struct platform_device *pdev)
>>  {
>> @@ -1797,6 +1832,11 @@ static int omap_nand_probe(struct platform_device *pdev)
>>  		goto return_error;
>>  	}
>>  
>> +	if (!omap2_nand_ecc_check(info, pdata)) {
>> +		err = -EINVAL;
>> +		goto return_error;
>> +	}
>> +
>>  	/* populate MTD interface based on ECC scheme */
>>  	nand_chip->ecc.layout	= &omap_oobinfo;
>>  	ecclayout		= &omap_oobinfo;
>> @@ -1826,7 +1866,6 @@ static int omap_nand_probe(struct platform_device *pdev)
>>  		break;
>>  
>>  	case OMAP_ECC_BCH4_CODE_HW_DETECTION_SW:
>> -#ifdef CONFIG_MTD_NAND_ECC_BCH
>>  		pr_info("nand: using OMAP_ECC_BCH4_CODE_HW_DETECTION_SW\n");
>>  		nand_chip->ecc.mode		= NAND_ECC_HW;
>>  		nand_chip->ecc.size		= 512;
>> @@ -1858,14 +1897,8 @@ static int omap_nand_probe(struct platform_device *pdev)
>>  			err = -EINVAL;
>>  		}
>>  		break;
>> -#else
>> -		pr_err("nand: error: CONFIG_MTD_NAND_ECC_BCH not enabled\n");
>> -		err = -EINVAL;
>> -		goto return_error;
>> -#endif
>>  
>>  	case OMAP_ECC_BCH4_CODE_HW:
>> -#ifdef CONFIG_MTD_NAND_OMAP_BCH
>>  		pr_info("nand: using OMAP_ECC_BCH4_CODE_HW ECC scheme\n");
>>  		nand_chip->ecc.mode		= NAND_ECC_HW;
>>  		nand_chip->ecc.size		= 512;
>> @@ -1887,21 +1920,15 @@ static int omap_nand_probe(struct platform_device *pdev)
>>  		/* reserved marker already included in ecclayout->eccbytes */
>>  		ecclayout->oobfree->offset	=
>>  				ecclayout->eccpos[ecclayout->eccbytes - 1] + 1;
>> -		/* This ECC scheme requires ELM H/W block */
>> -		if (is_elm_present(info, pdata->elm_of_node, BCH4_ECC) < 0) {
>> -			pr_err("nand: error: could not initialize ELM\n");
>> -			err = -ENODEV;
>> +
>> +		err = elm_config(info->elm_dev, BCH4_ECC,
>> +				 info->mtd.writesize / nand_chip->ecc.size,
>> +				 nand_chip->ecc.size, nand_chip->ecc.bytes);
>> +		if (err < 0)
>>  			goto return_error;
>> -		}
>>  		break;
>> -#else
>> -		pr_err("nand: error: CONFIG_MTD_NAND_OMAP_BCH not enabled\n");
>> -		err = -EINVAL;
>> -		goto return_error;
>> -#endif
>>  
>>  	case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW:
>> -#ifdef CONFIG_MTD_NAND_ECC_BCH
>>  		pr_info("nand: using OMAP_ECC_BCH8_CODE_HW_DETECTION_SW\n");
>>  		nand_chip->ecc.mode		= NAND_ECC_HW;
>>  		nand_chip->ecc.size		= 512;
>> @@ -1934,14 +1961,8 @@ static int omap_nand_probe(struct platform_device *pdev)
>>  			goto return_error;
>>  		}
>>  		break;
>> -#else
>> -		pr_err("nand: error: CONFIG_MTD_NAND_ECC_BCH not enabled\n");
>> -		err = -EINVAL;
>> -		goto return_error;
>> -#endif
>>  
>>  	case OMAP_ECC_BCH8_CODE_HW:
>> -#ifdef CONFIG_MTD_NAND_OMAP_BCH
>>  		pr_info("nand: using OMAP_ECC_BCH8_CODE_HW ECC scheme\n");
>>  		nand_chip->ecc.mode		= NAND_ECC_HW;
>>  		nand_chip->ecc.size		= 512;
>> @@ -1953,12 +1974,13 @@ static int omap_nand_probe(struct platform_device *pdev)
>>  		nand_chip->ecc.calculate	= omap_calculate_ecc_bch;
>>  		nand_chip->ecc.read_page	= omap_read_page_bch;
>>  		nand_chip->ecc.write_page	= omap_write_page_bch;
>> -		/* This ECC scheme requires ELM H/W block */
>> -		err = is_elm_present(info, pdata->elm_of_node, BCH8_ECC);
>> -		if (err < 0) {
>> -			pr_err("nand: error: could not initialize ELM\n");
>> +
>> +		err = elm_config(info->elm_dev, BCH8_ECC,
>> +				 info->mtd.writesize / nand_chip->ecc.size,
>> +				 nand_chip->ecc.size, nand_chip->ecc.bytes);
>> +		if (err < 0)
>>  			goto return_error;
>> -		}
>> +
>>  		/* define ECC layout */
>>  		ecclayout->eccbytes		= nand_chip->ecc.bytes *
>>  							(mtd->writesize /
>> @@ -1970,14 +1992,8 @@ static int omap_nand_probe(struct platform_device *pdev)
>>  		ecclayout->oobfree->offset	=
>>  				ecclayout->eccpos[ecclayout->eccbytes - 1] + 1;
>>  		break;
>> -#else
>> -		pr_err("nand: error: CONFIG_MTD_NAND_OMAP_BCH not enabled\n");
>> -		err = -EINVAL;
>> -		goto return_error;
>> -#endif
>>  
>>  	case OMAP_ECC_BCH16_CODE_HW:
>> -#ifdef CONFIG_MTD_NAND_OMAP_BCH
>>  		pr_info("using OMAP_ECC_BCH16_CODE_HW ECC scheme\n");
>>  		nand_chip->ecc.mode		= NAND_ECC_HW;
>>  		nand_chip->ecc.size		= 512;
>> @@ -1988,12 +2004,13 @@ static int omap_nand_probe(struct platform_device *pdev)
>>  		nand_chip->ecc.calculate	= omap_calculate_ecc_bch;
>>  		nand_chip->ecc.read_page	= omap_read_page_bch;
>>  		nand_chip->ecc.write_page	= omap_write_page_bch;
>> -		/* This ECC scheme requires ELM H/W block */
>> -		err = is_elm_present(info, pdata->elm_of_node, BCH16_ECC);
>> -		if (err < 0) {
>> -			pr_err("ELM is required for this ECC scheme\n");
>> +
>> +		err = elm_config(info->elm_dev, BCH16_ECC,
>> +				 info->mtd.writesize / nand_chip->ecc.size,
>> +				 nand_chip->ecc.size, nand_chip->ecc.bytes);
>> +		if (err < 0)
>>  			goto return_error;
>> -		}
>> +
>>  		/* define ECC layout */
>>  		ecclayout->eccbytes		= nand_chip->ecc.bytes *
>>  							(mtd->writesize /
>> @@ -2005,11 +2022,6 @@ static int omap_nand_probe(struct platform_device *pdev)
>>  		ecclayout->oobfree->offset	=
>>  				ecclayout->eccpos[ecclayout->eccbytes - 1] + 1;
>>  		break;
>> -#else
>> -		pr_err("nand: error: CONFIG_MTD_NAND_OMAP_BCH not enabled\n");
>> -		err = -EINVAL;
>> -		goto return_error;
>> -#endif
>>  	default:
>>  		pr_err("nand: error: invalid or unsupported ECC scheme\n");
>>  		err = -EINVAL;
>> diff --git a/include/linux/platform_data/elm.h b/include/linux/platform_data/elm.h
>> index 780d1e9..25d1bca 100644
>> --- a/include/linux/platform_data/elm.h
>> +++ b/include/linux/platform_data/elm.h
>> @@ -42,8 +42,22 @@ struct elm_errorvec {
>>  	int error_loc[16];
>>  };
>>  
>> +#if IS_ENABLED(CONFIG_MTD_NAND_OMAP_BCH)

I still get the following error if I set CONFIG_MTD_NAND_OMAP2 to y and
CONFIG_MTD_NAND_OMAP_BCH to m.

CONFIG_MTD_NAND_OMAP_BCH is used to select the ELM driver and it must be limited to
be built-in if CONFIG_MTD_NAND_OMAP2 is built-in.

Maybe it should be a sub option of CONFIG_MTD_NAND_OMAP2.
IMHO the elm.c file must be moved from mtd/devices to mtd/nand and renamed to omap_elm.c

drivers/built-in.o: In function `omap_nand_probe':
/work/linux-2.6/drivers/mtd/nand/omap2.c:2010: undefined reference to `elm_config'
/work/linux-2.6/drivers/mtd/nand/omap2.c:1980: undefined reference to `elm_config'
/work/linux-2.6/drivers/mtd/nand/omap2.c:1927: undefined reference to `elm_config'
drivers/built-in.o: In function `omap_elm_correct_data':
/work/linux-2.6/drivers/mtd/nand/omap2.c:1444: undefined reference to `elm_decode_bch_error_page'
make: *** [vmlinux] Error 1

>>  void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc,
>>  		struct elm_errorvec *err_vec);
>>  int elm_config(struct device *dev, enum bch_ecc bch_type,
>>  	int ecc_steps, int ecc_step_size, int ecc_syndrome_size);
>> +#else
>> +void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc,
>> +		struct elm_errorvec *err_vec)
>> +{
>> +}
>> +
>> +int elm_config(struct device *dev, enum bch_ecc bch_type,
>> +	int ecc_steps, int ecc_step_size, int ecc_syndrome_size)
>> +{
>> +	return -ENOSYS;
>> +}
>> +#endif /* CONFIG_MTD_NAND_ECC_BCH */
>> +
>>  #endif /* __ELM_H */
>>
> 

cheers,
-roger