Re: [PATCH v2 07/36] mtd: rawnand: Avoid a typedef

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

Hi Boris,

Boris Brezillon <boris.brezillon@collabora.com> wrote on Fri, 3 May
2019 14:50:06 +0200:

> On Fri, 3 May 2019 14:40:30 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Hi Boris,
> > 
> > Boris Brezillon <boris.brezillon@collabora.com> wrote on Sun, 31 Mar
> > 2019 13:55:13 +0200:
> >   
> > > On Mon,  4 Mar 2019 23:28:12 +0100
> > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >     
> > > > In new code, the use of typedef is discouraged. Before moving this
> > > > section out of the raw NAND base, let's switch the nand_ecc_modes_t
> > > > type into a regular nand_ecc_mode enumeration.
> > > > 
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > ---
> > > >  drivers/mtd/nand/raw/nand_base.c               | 4 ++--
> > > >  include/linux/mtd/rawnand.h                    | 6 +++---
> > > >  include/linux/platform_data/mtd-davinci.h      | 2 +-
> > > >  include/linux/platform_data/mtd-nand-s3c2410.h | 2 +-
> > > >  4 files changed, 7 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> > > > index e14f02a01efd..05174c6a3099 100644
> > > > --- a/drivers/mtd/nand/raw/nand_base.c
> > > > +++ b/drivers/mtd/nand/raw/nand_base.c
> > > > @@ -4881,8 +4881,8 @@ static int of_get_nand_ecc_mode(struct device_node *np)
> > > >  
> > > >  	/*
> > > >  	 * For backward compatibility we support few obsoleted values that don't
> > > > -	 * have their mappings into nand_ecc_modes_t anymore (they were merged
> > > > -	 * with other enums).
> > > > +	 * have their mappings into the nand_ecc_mode enum anymore (they were
> > > > +	 * merged with other enums).
> > > >  	 */
> > > >  	if (!strcasecmp(pm, "soft_bch"))
> > > >  		return NAND_ECC_SOFT;
> > > > diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> > > > index 14748183508b..c5bf6bb49329 100644
> > > > --- a/include/linux/mtd/rawnand.h
> > > > +++ b/include/linux/mtd/rawnand.h
> > > > @@ -87,14 +87,14 @@ struct nand_chip;
> > > >  /*
> > > >   * Constants for ECC_MODES
> > > >   */
> > > > -typedef enum {
> > > > +enum nand_ecc_mode {
> > > >  	NAND_ECC_NONE,
> > > >  	NAND_ECC_SOFT,
> > > >  	NAND_ECC_HW,
> > > >  	NAND_ECC_HW_SYNDROME,
> > > >  	NAND_ECC_HW_OOB_FIRST,
> > > >  	NAND_ECC_ON_DIE,
> > > > -} nand_ecc_modes_t;
> > > > +};      
> > > 
> > > Hm, I'm really not a big fan of this enum because it's mixing 2
> > > different concepts: the type of ECC engine to use (on-die,
> > > hw-controller-side, software, no-ECC) and the layout of
> > > ECC/FREE bytes (_SYNDROME, _OOB_FIRST).
> > > 
> > > I'd recommend creating a nand_ecc_engine_type enum:
> > > 
> > > enum nand_ecc_engine_type {
> > > 	NAND_NO_ECC_ENGINE,
> > > 	NAND_SOFT_ECC_ENGINE,
> > > 	NAND_HW_ECC_ENGINE,
> > > 	NAND_ON_DIE_ECC_ENGINE,
> > > };
> > > 
> > > and then convert the raw NAND layer to this enum when the time comes.    
> > 
> > I started something but this goes way too far from what I want to
> > achieve. I know it would be nice to have it but it has an
> > increasingly number of side effects which scared me. The way the
> > series is organized does not allow to easily ignore the raw NAND layer
> > first and then convert it. I am giving up on this one for now, sorry.  
> 
> I don't think that's a wise choice. Why not focusing on
> nand_device/spinand with a clean implementation that does not try to
> convert the rawnand layer to this approach?

I know, and that was probably the best thing to do, unfortunately I
started bigger because there was so much things to change, I got a bit
lost at the beginning. I know this is not a valid reason but in
practice I could not come with something lighter, ie. focused on
SPI-NAND only.

> The reason I initially
> started over with a new generic NAND layer instead of copying things
> from nand_base is that I wanted to avoid having to deal with stuff that
> were not so great in there, and clearly nand_ecc_mode is one of them.

I understand that so I think I managed to clean the situation with a
preliminary series which I will send soon.

Thanks,
Miquèl

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