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

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Boris,

Boris Brezillon <boris.brezillon@collabora.com> wrote on Sat, 12 Oct
2019 11:28:24 +0200:

> On Thu, 19 Sep 2019 21:31:06 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> 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).

As you know this series is already quite big :p. My point here is to
clarify a bit but now rework the DT properties which are things that
you can never get entirely rid of. This is exactly why I kept the
string properties as before: not changing anything in the DT
representation. Anyone with the time and desire to do so is welcome,
but I'm not willing to do it in this series :)

I'm addressing most of your comments (mainly enum/name changes) but the
string definitions will remain the same, even if they are not entirely
accurate. With these bits kept intact, the below logic works, I know
it is not clean, and deserves more cleaning, but this is a distinct
work :)

> 
> >  
> > +	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;
> >  }  

Thanks,
Miquèl

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