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

From: Miquel Raynal <[email-masked]>
Date: Wed, 15 Jan 2020 21:58:40 +0100

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-arm-kernel mailing list
[email-masked]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel