| Message ID | 20190304222841.13899-8-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce the generic ECC engine abstraction | expand |
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. > > enum nand_ecc_algo { > NAND_ECC_UNKNOWN, > @@ -340,7 +340,7 @@ static const struct nand_ecc_caps __name = { \ > * @write_oob: function to write chip OOB data > */ > struct nand_ecc_ctrl { > - nand_ecc_modes_t mode; > + enum nand_ecc_mode mode; > enum nand_ecc_algo algo; > int steps; > int size; > diff --git a/include/linux/platform_data/mtd-davinci.h b/include/linux/platform_data/mtd-davinci.h > index 1bbfa27cccb4..e7457be12b8f 100644 > --- a/include/linux/platform_data/mtd-davinci.h > +++ b/include/linux/platform_data/mtd-davinci.h > @@ -81,7 +81,7 @@ struct davinci_nand_pdata { /* platform_data */ > * Newer ones also support 4-bit ECC, but are awkward > * using it with large page chips. > */ > - nand_ecc_modes_t ecc_mode; > + enum nand_ecc_mode ecc_mode; > u8 ecc_bits; > > /* e.g. NAND_BUSWIDTH_16 */ > diff --git a/include/linux/platform_data/mtd-nand-s3c2410.h b/include/linux/platform_data/mtd-nand-s3c2410.h > index f8c553f92655..ff6501c51244 100644 > --- a/include/linux/platform_data/mtd-nand-s3c2410.h > +++ b/include/linux/platform_data/mtd-nand-s3c2410.h > @@ -52,7 +52,7 @@ struct s3c2410_platform_nand { > > unsigned int ignore_unset_ecc:1; > > - nand_ecc_modes_t ecc_mode; > + enum nand_ecc_mode ecc_mode; > > int nr_sets; > struct s3c2410_nand_set *sets;
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. Thanks, Miquèl
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.
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
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; +}; enum nand_ecc_algo { NAND_ECC_UNKNOWN, @@ -340,7 +340,7 @@ static const struct nand_ecc_caps __name = { \ * @write_oob: function to write chip OOB data */ struct nand_ecc_ctrl { - nand_ecc_modes_t mode; + enum nand_ecc_mode mode; enum nand_ecc_algo algo; int steps; int size; diff --git a/include/linux/platform_data/mtd-davinci.h b/include/linux/platform_data/mtd-davinci.h index 1bbfa27cccb4..e7457be12b8f 100644 --- a/include/linux/platform_data/mtd-davinci.h +++ b/include/linux/platform_data/mtd-davinci.h @@ -81,7 +81,7 @@ struct davinci_nand_pdata { /* platform_data */ * Newer ones also support 4-bit ECC, but are awkward * using it with large page chips. */ - nand_ecc_modes_t ecc_mode; + enum nand_ecc_mode ecc_mode; u8 ecc_bits; /* e.g. NAND_BUSWIDTH_16 */ diff --git a/include/linux/platform_data/mtd-nand-s3c2410.h b/include/linux/platform_data/mtd-nand-s3c2410.h index f8c553f92655..ff6501c51244 100644 --- a/include/linux/platform_data/mtd-nand-s3c2410.h +++ b/include/linux/platform_data/mtd-nand-s3c2410.h @@ -52,7 +52,7 @@ struct s3c2410_platform_nand { unsigned int ignore_unset_ecc:1; - nand_ecc_modes_t ecc_mode; + enum nand_ecc_mode ecc_mode; int nr_sets; struct s3c2410_nand_set *sets;
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(-)