Re: [PATCH v3 06/40] mtd: rawnand: Use the new ECC engine type enumeration

[Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>]

On Thu, 19 Sep 2019 21:31:06 +0200
Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:

> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 00a261284aad..ad0b892c2523 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -4835,7 +4835,7 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type)
>  
>  static const char * const nand_ecc_modes[] = {
>  	[NAND_ECC_NONE]		= "none",
> -	[NAND_ECC_SOFT]		= "soft",
> +	[NAND_SOFT_ECC_ENGINE]		= "soft",

Not sure why this one is changed. This string array still describes ECC
modes, not ECC engine types.

>  	[NAND_ECC_HW]		= "hw",
>  	[NAND_ECC_HW_SYNDROME]	= "hw_syndrome",
>  	[NAND_ECC_HW_OOB_FIRST]	= "hw_oob_first",
> @@ -4863,21 +4863,44 @@ static int of_get_nand_ecc_mode(struct device_node *np)
>  	if (err < 0)
>  		return err;
>  
> -	for (i = NAND_ECC_NONE; i < ARRAY_SIZE(nand_ecc_modes); i++)
> -		if (!strcasecmp(pm, nand_ecc_modes[i]))
> +	for (i = NAND_NO_ECC_ENGINE;
> +	     i < ARRAY_SIZE(nand_ecc_engine_providers); i++)
> +		if (!strcasecmp(pm, nand_ecc_engine_providers[i]))
>  			return i;

Hm, you still need to support the old bindings (I wonder how that can
work). What should be done instead is have a conversion table that turns
an ecc_mode string into a engine_type+placement pair, so you don't have
to update the DT bindings (though we might want to expose new props for
the new model, like ecc-placement and ecc-engine).

>  
> +	for (i = NAND_ECC_SYNDROME_OOB_PLACEMENT;
> +	     i < ARRAY_SIZE(nand_ecc_engine_oob_placement); i++)
> +		if (!strcasecmp(pm, nand_ecc_engine_oob_placement[i]))
> +			return NAND_HW_ECC_ENGINE;
> +

I also don't understand how this one works, placement does not give any
clue on the type of ECC engine (at least it shouldn't).

>  	/*
>  	 * For backward compatibility we support few obsoleted values that don't
> -	 * have their mappings into the nand_ecc_mode enum anymore (they were
> -	 * merged with other enums).
> +	 * have their mappings into the nand_ecc_engine_providers enum anymore
> +	 * (they were merged with other enums).
>  	 */
>  	if (!strcasecmp(pm, "soft_bch"))
> -		return NAND_ECC_SOFT;
> +		return NAND_SOFT_ECC_ENGINE;
>  
>  	return -ENODEV;
>  }

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/