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

From: Boris Brezillon <boris.brezillon@collabora.com>
To: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Mason Yang <masonccyang@mxic.com.tw>, Vignesh R <vigneshr@ti.com>,
	Tudor Ambarus <Tudor.Ambarus@microchip.com>,
	Julien Su <juliensu@mxic.com.tw>,
	Richard Weinberger <richard@nod.at>,
	Boris Brezillon <bbrezillon@kernel.org>,
	Schrempf Frieder <frieder.schrempf@kontron.de>,
	Paul Cercueil <paul@crapouillou.net>,
	Marek Vasut <marek.vasut@gmail.com>,
	linux-mtd@lists.infradead.org,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Brian Norris <computersforpeace@gmail.com>,
	David Woodhouse <dwmw2@infradead.org>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v2 07/36] mtd: rawnand: Avoid a typedef
Date: Fri, 3 May 2019 14:50:06 +0200	[thread overview]
Message-ID: <20190503145006.574af442@collabora.com> (raw)
In-Reply-To: <20190503144030.7fbbc0d4@xps13>

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? 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.

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