Re: [PATCH v3 05/40] mtd: rawnand: Create a new enumeration to describe properly ECC types

[Miquel Raynal <miquel.raynal@xxxxxxxxxxx>]

Hi Boris,

> > +/**
> > + * enum nand_ecc_engine_type - NAND ECC engine type/provider
> > + * @NAND_INVALID_ECC_ENGINE: Invalid value
> > + * @NAND_NO_ECC_ENGINE: No ECC correction
> > + * @NAND_SOFT_ECC_ENGINE: Software ECC correction
> > + * @NAND_HW_ECC_ENGINE: Hardware (controller side) ECC correction
> > + * @NAND_ON_DIE_ECC_ENGINE: Hardware (chip side) ECC correction
> > + */
> > +enum nand_ecc_engine_type {
> > +	NAND_INVALID_ECC_ENGINE,  
> 
> Same comment as for the NAND_ECC_INVALID addition: if you don't have an
> entry in nand_ecc_engine_providers for this INVALID case, it's probably
> better to define it to -1.
> 
> > +	NAND_NO_ECC_ENGINE,
> > +	NAND_SOFT_ECC_ENGINE,
> > +	NAND_HW_ECC_ENGINE,  
> 
> I'd rename that one into
> 
> 	NAND_CONTROLLER_ECC_ENGINE,
> 
> HW is a bit too vague.
> 
> > +	NAND_ON_DIE_ECC_ENGINE,  
> 
> I also find it clearer when the same prefix is used:
> 
> 	NAND_ECC_ENGINE_INVALID = -1,
> 	NAND_ECC_ENGINE_NONE = 0,
> 	NAND_ECC_ENGINE_SOFT,
> 	NAND_ECC_ENGINE_CONTROLLER,
> 	NAND_ECC_ENGINE_ON_DIE,
> 
> Looks good otherwise. Feel free to ignore my comments if you disagree.

Same prefix it is!

However, I don't want to start as -1 as otherwise using a for-loop to
loop over each ECC engine would be less straightforward.

Thanks,
Miquèl

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