Subject: [PATCH v4 1/2] mtd: spinand: micron: Generalize the function and structure names

Rename the oob structure and read/write/update function names to
a generic names

Signed-off-by: Thirumalesha Narasimhappa <[email protected]>
---
drivers/mtd/nand/spi/micron.c | 80 +++++++++++++++++------------------
1 file changed, 40 insertions(+), 40 deletions(-)

diff --git a/drivers/mtd/nand/spi/micron.c b/drivers/mtd/nand/spi/micron.c
index 5d370cfcdaaa..d1b1073d1a55 100644
--- a/drivers/mtd/nand/spi/micron.c
+++ b/drivers/mtd/nand/spi/micron.c
@@ -28,7 +28,7 @@

#define MICRON_SELECT_DIE(x) ((x) << 6)

-static SPINAND_OP_VARIANTS(read_cache_variants,
+static SPINAND_OP_VARIANTS(quadio_read_cache_variants,
SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2, NULL, 0),
SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
SPINAND_PAGE_READ_FROM_CACHE_DUALIO_OP(0, 1, NULL, 0),
@@ -36,11 +36,11 @@ static SPINAND_OP_VARIANTS(read_cache_variants,
SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0),
SPINAND_PAGE_READ_FROM_CACHE_OP(false, 0, 1, NULL, 0));

-static SPINAND_OP_VARIANTS(write_cache_variants,
+static SPINAND_OP_VARIANTS(x4_write_cache_variants,
SPINAND_PROG_LOAD_X4(true, 0, NULL, 0),
SPINAND_PROG_LOAD(true, 0, NULL, 0));

-static SPINAND_OP_VARIANTS(update_cache_variants,
+static SPINAND_OP_VARIANTS(x4_update_cache_variants,
SPINAND_PROG_LOAD_X4(false, 0, NULL, 0),
SPINAND_PROG_LOAD(false, 0, NULL, 0));

@@ -69,7 +69,7 @@ static int micron_8_ooblayout_free(struct mtd_info *mtd, int section,
return 0;
}

-static const struct mtd_ooblayout_ops micron_8_ooblayout = {
+static const struct mtd_ooblayout_ops micron_grouped_ooblayout = {
.ecc = micron_8_ooblayout_ecc,
.free = micron_8_ooblayout_free,
};
@@ -120,55 +120,55 @@ static const struct spinand_info micron_spinand_table[] = {
SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x24),
NAND_MEMORG(1, 2048, 128, 64, 2048, 40, 2, 1, 1),
NAND_ECCREQ(8, 512),
- SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
- &write_cache_variants,
- &update_cache_variants),
+ SPINAND_INFO_OP_VARIANTS(&quadio_read_cache_variants,
+ &x4_write_cache_variants,
+ &x4_update_cache_variants),
0,
- SPINAND_ECCINFO(&micron_8_ooblayout,
+ SPINAND_ECCINFO(&micron_grouped_ooblayout,
micron_8_ecc_get_status)),
/* M79A 2Gb 1.8V */
SPINAND_INFO("MT29F2G01ABBGD",
SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x25),
NAND_MEMORG(1, 2048, 128, 64, 2048, 40, 2, 1, 1),
NAND_ECCREQ(8, 512),
- SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
- &write_cache_variants,
- &update_cache_variants),
+ SPINAND_INFO_OP_VARIANTS(&quadio_read_cache_variants,
+ &x4_write_cache_variants,
+ &x4_update_cache_variants),
0,
- SPINAND_ECCINFO(&micron_8_ooblayout,
+ SPINAND_ECCINFO(&micron_grouped_ooblayout,
micron_8_ecc_get_status)),
/* M78A 1Gb 3.3V */
SPINAND_INFO("MT29F1G01ABAFD",
SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x14),
NAND_MEMORG(1, 2048, 128, 64, 1024, 20, 1, 1, 1),
NAND_ECCREQ(8, 512),
- SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
- &write_cache_variants,
- &update_cache_variants),
+ SPINAND_INFO_OP_VARIANTS(&quadio_read_cache_variants,
+ &x4_write_cache_variants,
+ &x4_update_cache_variants),
0,
- SPINAND_ECCINFO(&micron_8_ooblayout,
+ SPINAND_ECCINFO(&micron_grouped_ooblayout,
micron_8_ecc_get_status)),
/* M78A 1Gb 1.8V */
SPINAND_INFO("MT29F1G01ABAFD",
SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x15),
NAND_MEMORG(1, 2048, 128, 64, 1024, 20, 1, 1, 1),
NAND_ECCREQ(8, 512),
- SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
- &write_cache_variants,
- &update_cache_variants),
+ SPINAND_INFO_OP_VARIANTS(&quadio_read_cache_variants,
+ &x4_write_cache_variants,
+ &x4_update_cache_variants),
0,
- SPINAND_ECCINFO(&micron_8_ooblayout,
+ SPINAND_ECCINFO(&micron_grouped_ooblayout,
micron_8_ecc_get_status)),
/* M79A 4Gb 3.3V */
SPINAND_INFO("MT29F4G01ADAGD",
SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x36),
NAND_MEMORG(1, 2048, 128, 64, 2048, 80, 2, 1, 2),
NAND_ECCREQ(8, 512),
- SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
- &write_cache_variants,
- &update_cache_variants),
+ SPINAND_INFO_OP_VARIANTS(&quadio_read_cache_variants,
+ &x4_write_cache_variants,
+ &x4_update_cache_variants),
0,
- SPINAND_ECCINFO(&micron_8_ooblayout,
+ SPINAND_ECCINFO(&micron_grouped_ooblayout,
micron_8_ecc_get_status),
SPINAND_SELECT_TARGET(micron_select_target)),
/* M70A 4Gb 3.3V */
@@ -176,33 +176,33 @@ static const struct spinand_info micron_spinand_table[] = {
SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x34),
NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1),
NAND_ECCREQ(8, 512),
- SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
- &write_cache_variants,
- &update_cache_variants),
+ SPINAND_INFO_OP_VARIANTS(&quadio_read_cache_variants,
+ &x4_write_cache_variants,
+ &x4_update_cache_variants),
SPINAND_HAS_CR_FEAT_BIT,
- SPINAND_ECCINFO(&micron_8_ooblayout,
+ SPINAND_ECCINFO(&micron_grouped_ooblayout,
micron_8_ecc_get_status)),
/* M70A 4Gb 1.8V */
SPINAND_INFO("MT29F4G01ABBFD",
SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x35),
NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1),
NAND_ECCREQ(8, 512),
- SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
- &write_cache_variants,
- &update_cache_variants),
+ SPINAND_INFO_OP_VARIANTS(&quadio_read_cache_variants,
+ &x4_write_cache_variants,
+ &x4_update_cache_variants),
SPINAND_HAS_CR_FEAT_BIT,
- SPINAND_ECCINFO(&micron_8_ooblayout,
+ SPINAND_ECCINFO(&micron_grouped_ooblayout,
micron_8_ecc_get_status)),
/* M70A 8Gb 3.3V */
SPINAND_INFO("MT29F8G01ADAFD",
SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x46),
NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 2),
NAND_ECCREQ(8, 512),
- SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
- &write_cache_variants,
- &update_cache_variants),
+ SPINAND_INFO_OP_VARIANTS(&quadio_read_cache_variants,
+ &x4_write_cache_variants,
+ &x4_update_cache_variants),
SPINAND_HAS_CR_FEAT_BIT,
- SPINAND_ECCINFO(&micron_8_ooblayout,
+ SPINAND_ECCINFO(&micron_grouped_ooblayout,
micron_8_ecc_get_status),
SPINAND_SELECT_TARGET(micron_select_target)),
/* M70A 8Gb 1.8V */
@@ -210,11 +210,11 @@ static const struct spinand_info micron_spinand_table[] = {
SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x47),
NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 2),
NAND_ECCREQ(8, 512),
- SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
- &write_cache_variants,
- &update_cache_variants),
+ SPINAND_INFO_OP_VARIANTS(&quadio_read_cache_variants,
+ &x4_write_cache_variants,
+ &x4_update_cache_variants),
SPINAND_HAS_CR_FEAT_BIT,
- SPINAND_ECCINFO(&micron_8_ooblayout,
+ SPINAND_ECCINFO(&micron_grouped_ooblayout,
micron_8_ecc_get_status),
SPINAND_SELECT_TARGET(micron_select_target)),
};
--
2.25.1


2020-09-15 08:17:31

by Miquel Raynal

[permalink] [raw]
Subject: Re: [PATCH v4 1/2] mtd: spinand: micron: Generalize the function and structure names

Hi Thirumalesha,

Thirumalesha Narasimhappa <[email protected]> wrote on Mon, 14
Sep 2020 00:15:32 +0800:

> Rename the oob structure and read/write/update function names to
> a generic names
>
> Signed-off-by: Thirumalesha Narasimhappa <[email protected]>
> ---
> drivers/mtd/nand/spi/micron.c | 80 +++++++++++++++++------------------
> 1 file changed, 40 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/mtd/nand/spi/micron.c b/drivers/mtd/nand/spi/micron.c
> index 5d370cfcdaaa..d1b1073d1a55 100644
> --- a/drivers/mtd/nand/spi/micron.c
> +++ b/drivers/mtd/nand/spi/micron.c
> @@ -28,7 +28,7 @@
>
> #define MICRON_SELECT_DIE(x) ((x) << 6)
>
> -static SPINAND_OP_VARIANTS(read_cache_variants,
> +static SPINAND_OP_VARIANTS(quadio_read_cache_variants,
> SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2, NULL, 0),
> SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
> SPINAND_PAGE_READ_FROM_CACHE_DUALIO_OP(0, 1, NULL, 0),
> @@ -36,11 +36,11 @@ static SPINAND_OP_VARIANTS(read_cache_variants,
> SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0),
> SPINAND_PAGE_READ_FROM_CACHE_OP(false, 0, 1, NULL, 0));
>
> -static SPINAND_OP_VARIANTS(write_cache_variants,
> +static SPINAND_OP_VARIANTS(x4_write_cache_variants,

Why quadio_read and x4_write? I don't get the logic.

> SPINAND_PROG_LOAD_X4(true, 0, NULL, 0),
> SPINAND_PROG_LOAD(true, 0, NULL, 0));
>
> -static SPINAND_OP_VARIANTS(update_cache_variants,
> +static SPINAND_OP_VARIANTS(x4_update_cache_variants,
> SPINAND_PROG_LOAD_X4(false, 0, NULL, 0),
> SPINAND_PROG_LOAD(false, 0, NULL, 0));
>
> @@ -69,7 +69,7 @@ static int micron_8_ooblayout_free(struct mtd_info *mtd, int section,
> return 0;
> }
>
> -static const struct mtd_ooblayout_ops micron_8_ooblayout = {
> +static const struct mtd_ooblayout_ops micron_grouped_ooblayout = {

Is this necessary? What does "grouped" means? Should we rename all
functions with _8_ in it to make sense?

> .ecc = micron_8_ooblayout_ecc,
> .free = micron_8_ooblayout_free,
> };
> @@ -120,55 +120,55 @@ static const struct spinand_info micron_spinand_table[] = {
> SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x24),
> NAND_MEMORG(1, 2048, 128, 64, 2048, 40, 2, 1, 1),
> NAND_ECCREQ(8, 512),
> - SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> - &write_cache_variants,
> - &update_cache_variants),
> + SPINAND_INFO_OP_VARIANTS(&quadio_read_cache_variants,
> + &x4_write_cache_variants,
> + &x4_update_cache_variants),
> 0,
> - SPINAND_ECCINFO(&micron_8_ooblayout,
> + SPINAND_ECCINFO(&micron_grouped_ooblayout,
> micron_8_ecc_get_status)),
> /* M79A 2Gb 1.8V */
> SPINAND_INFO("MT29F2G01ABBGD",
> SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x25),
> NAND_MEMORG(1, 2048, 128, 64, 2048, 40, 2, 1, 1),
> NAND_ECCREQ(8, 512),
> - SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> - &write_cache_variants,
> - &update_cache_variants),
> + SPINAND_INFO_OP_VARIANTS(&quadio_read_cache_variants,
> + &x4_write_cache_variants,
> + &x4_update_cache_variants),
> 0,
> - SPINAND_ECCINFO(&micron_8_ooblayout,
> + SPINAND_ECCINFO(&micron_grouped_ooblayout,
> micron_8_ecc_get_status)),
> /* M78A 1Gb 3.3V */
> SPINAND_INFO("MT29F1G01ABAFD",
> SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x14),
> NAND_MEMORG(1, 2048, 128, 64, 1024, 20, 1, 1, 1),
> NAND_ECCREQ(8, 512),
> - SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> - &write_cache_variants,
> - &update_cache_variants),
> + SPINAND_INFO_OP_VARIANTS(&quadio_read_cache_variants,
> + &x4_write_cache_variants,
> + &x4_update_cache_variants),
> 0,
> - SPINAND_ECCINFO(&micron_8_ooblayout,
> + SPINAND_ECCINFO(&micron_grouped_ooblayout,
> micron_8_ecc_get_status)),
> /* M78A 1Gb 1.8V */
> SPINAND_INFO("MT29F1G01ABAFD",
> SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x15),
> NAND_MEMORG(1, 2048, 128, 64, 1024, 20, 1, 1, 1),
> NAND_ECCREQ(8, 512),
> - SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> - &write_cache_variants,
> - &update_cache_variants),
> + SPINAND_INFO_OP_VARIANTS(&quadio_read_cache_variants,
> + &x4_write_cache_variants,
> + &x4_update_cache_variants),
> 0,
> - SPINAND_ECCINFO(&micron_8_ooblayout,
> + SPINAND_ECCINFO(&micron_grouped_ooblayout,
> micron_8_ecc_get_status)),
> /* M79A 4Gb 3.3V */
> SPINAND_INFO("MT29F4G01ADAGD",
> SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x36),
> NAND_MEMORG(1, 2048, 128, 64, 2048, 80, 2, 1, 2),
> NAND_ECCREQ(8, 512),
> - SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> - &write_cache_variants,
> - &update_cache_variants),
> + SPINAND_INFO_OP_VARIANTS(&quadio_read_cache_variants,
> + &x4_write_cache_variants,
> + &x4_update_cache_variants),
> 0,
> - SPINAND_ECCINFO(&micron_8_ooblayout,
> + SPINAND_ECCINFO(&micron_grouped_ooblayout,
> micron_8_ecc_get_status),
> SPINAND_SELECT_TARGET(micron_select_target)),
> /* M70A 4Gb 3.3V */
> @@ -176,33 +176,33 @@ static const struct spinand_info micron_spinand_table[] = {
> SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x34),
> NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1),
> NAND_ECCREQ(8, 512),
> - SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> - &write_cache_variants,
> - &update_cache_variants),
> + SPINAND_INFO_OP_VARIANTS(&quadio_read_cache_variants,
> + &x4_write_cache_variants,
> + &x4_update_cache_variants),
> SPINAND_HAS_CR_FEAT_BIT,
> - SPINAND_ECCINFO(&micron_8_ooblayout,
> + SPINAND_ECCINFO(&micron_grouped_ooblayout,
> micron_8_ecc_get_status)),
> /* M70A 4Gb 1.8V */
> SPINAND_INFO("MT29F4G01ABBFD",
> SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x35),
> NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1),
> NAND_ECCREQ(8, 512),
> - SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> - &write_cache_variants,
> - &update_cache_variants),
> + SPINAND_INFO_OP_VARIANTS(&quadio_read_cache_variants,
> + &x4_write_cache_variants,
> + &x4_update_cache_variants),
> SPINAND_HAS_CR_FEAT_BIT,
> - SPINAND_ECCINFO(&micron_8_ooblayout,
> + SPINAND_ECCINFO(&micron_grouped_ooblayout,
> micron_8_ecc_get_status)),
> /* M70A 8Gb 3.3V */
> SPINAND_INFO("MT29F8G01ADAFD",
> SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x46),
> NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 2),
> NAND_ECCREQ(8, 512),
> - SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> - &write_cache_variants,
> - &update_cache_variants),
> + SPINAND_INFO_OP_VARIANTS(&quadio_read_cache_variants,
> + &x4_write_cache_variants,
> + &x4_update_cache_variants),
> SPINAND_HAS_CR_FEAT_BIT,
> - SPINAND_ECCINFO(&micron_8_ooblayout,
> + SPINAND_ECCINFO(&micron_grouped_ooblayout,
> micron_8_ecc_get_status),
> SPINAND_SELECT_TARGET(micron_select_target)),
> /* M70A 8Gb 1.8V */
> @@ -210,11 +210,11 @@ static const struct spinand_info micron_spinand_table[] = {
> SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x47),
> NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 2),
> NAND_ECCREQ(8, 512),
> - SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> - &write_cache_variants,
> - &update_cache_variants),
> + SPINAND_INFO_OP_VARIANTS(&quadio_read_cache_variants,
> + &x4_write_cache_variants,
> + &x4_update_cache_variants),
> SPINAND_HAS_CR_FEAT_BIT,
> - SPINAND_ECCINFO(&micron_8_ooblayout,
> + SPINAND_ECCINFO(&micron_grouped_ooblayout,
> micron_8_ecc_get_status),
> SPINAND_SELECT_TARGET(micron_select_target)),
> };

Thanks,
Miquèl

2020-09-28 14:59:27

by Miquel Raynal

[permalink] [raw]
Subject: Re: [PATCH v4 1/2] mtd: spinand: micron: Generalize the function and structure names

Hi Thirumalesha,

Thirumalesha N <[email protected]> wrote on Sun, 20 Sep 2020
22:38:53 +0800:

> Hi Miquèl,
>
> I adopted these changes as per the Boris Brezillon comments
>
> On Tue, Sep 15, 2020 at 4:13 PM Miquel Raynal <[email protected]>
> wrote:
>
> > Hi Thirumalesha,
> >
> > Thirumalesha Narasimhappa <[email protected]> wrote on Mon, 14
> > Sep 2020 00:15:32 +0800:
> >
> > > Rename the oob structure and read/write/update function names to
> > > a generic names
> > >
> > > Signed-off-by: Thirumalesha Narasimhappa <[email protected]>
> > > ---
> > > drivers/mtd/nand/spi/micron.c | 80 +++++++++++++++++------------------
> > > 1 file changed, 40 insertions(+), 40 deletions(-)
> > >
> > > diff --git a/drivers/mtd/nand/spi/micron.c
> > b/drivers/mtd/nand/spi/micron.c
> > > index 5d370cfcdaaa..d1b1073d1a55 100644
> > > --- a/drivers/mtd/nand/spi/micron.c
> > > +++ b/drivers/mtd/nand/spi/micron.c
> > > @@ -28,7 +28,7 @@
> > >
> > > #define MICRON_SELECT_DIE(x) ((x) << 6)
> > >
> > > -static SPINAND_OP_VARIANTS(read_cache_variants,
> > > +static SPINAND_OP_VARIANTS(quadio_read_cache_variants,
> > > SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2, NULL, 0),
> > > SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
> > > SPINAND_PAGE_READ_FROM_CACHE_DUALIO_OP(0, 1, NULL, 0),
> > > @@ -36,11 +36,11 @@ static SPINAND_OP_VARIANTS(read_cache_variants,
> > > SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0),
> > > SPINAND_PAGE_READ_FROM_CACHE_OP(false, 0, 1, NULL, 0));
> > >
> > > -static SPINAND_OP_VARIANTS(write_cache_variants,
> > > +static SPINAND_OP_VARIANTS(x4_write_cache_variants,
> >
> > Why quadio_read and x4_write? I don't get the logic.
> >
>
> IMHO, quadio_read is derived from SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP &
> x4_write is derived from SPINAND_PROG_LOAD_X4
>
> Boris, Please correct me, if my understanding is wrong

I see.

Then please split this change, one updating the helpers with an "x4"
prefix when relevant and another one for 8 vs. grouped.

>
>
> > > SPINAND_PROG_LOAD_X4(true, 0, NULL, 0),
> > > SPINAND_PROG_LOAD(true, 0, NULL, 0));
> > >
> > > -static SPINAND_OP_VARIANTS(update_cache_variants,
> > > +static SPINAND_OP_VARIANTS(x4_update_cache_variants,
> > > SPINAND_PROG_LOAD_X4(false, 0, NULL, 0),
> > > SPINAND_PROG_LOAD(false, 0, NULL, 0));
> > >
> > > @@ -69,7 +69,7 @@ static int micron_8_ooblayout_free(struct mtd_info
> > *mtd, int section,
> > > return 0;
> > > }
> > >
> > > -static const struct mtd_ooblayout_ops micron_8_ooblayout = {
> > > +static const struct mtd_ooblayout_ops micron_grouped_ooblayout = {
> >
> > Is this necessary? What does "grouped" means? Should we rename all
> > functions with _8_ in it to make sense?
> >
>
> IMHO, grouped means, ecc bytes are at continuous address, where as
> interleaved means ecc bytes splitted into multiple addresses

I don't like the name. Interleaved means that there are OOB bytes
stored in the data section, which is not the case here. The way OOB
bytes are organized do not seem relevant to me, I think i prefer the
"_4_/_8_" naming,even if it's not very explicit.

Also please be consistent, do not mix "_4_" and "interleaved" (see the
other patch).

Any feedback from Micron to validate these changes will be appreciated.

Thanks,
Miquèl

2020-09-28 15:46:43

by Boris Brezillon

[permalink] [raw]
Subject: Re: [PATCH v4 1/2] mtd: spinand: micron: Generalize the function and structure names

On Mon, 28 Sep 2020 16:55:28 +0200
Miquel Raynal <[email protected]> wrote:

> > IMHO, grouped means, ecc bytes are at continuous address, where as
> > interleaved means ecc bytes splitted into multiple addresses
>
> I don't like the name. Interleaved means that there are OOB bytes
> stored in the data section, which is not the case here.

Well, I would argue that the term interleaved alone doesn't say
anything about the things that are interleaved. But I guess
split/grouped would be fine too if you want to avoid re-using
interleaved here.

> The way OOB
> bytes are organized do not seem relevant to me, I think i prefer the
> "_4_/_8_" naming,even if it's not very explicit.

The ECC strength doesn't say anything about the scheme used for ECC
bytes placement, and you might end up with 2 different schemes
providing the same strength, or the same scheme used for 2 different
strengths.

2020-09-28 15:52:46

by Miquel Raynal

[permalink] [raw]
Subject: Re: [PATCH v4 1/2] mtd: spinand: micron: Generalize the function and structure names


Boris Brezillon <[email protected]> wrote on Mon, 28 Sep
2020 17:45:05 +0200:

> On Mon, 28 Sep 2020 16:55:28 +0200
> Miquel Raynal <[email protected]> wrote:
>
> > > IMHO, grouped means, ecc bytes are at continuous address, where as
> > > interleaved means ecc bytes splitted into multiple addresses
> >
> > I don't like the name. Interleaved means that there are OOB bytes
> > stored in the data section, which is not the case here.
>
> Well, I would argue that the term interleaved alone doesn't say
> anything about the things that are interleaved. But I guess

I should have said that interleaved, in this subsystem, generally refers
to in-band vs. out-of-band data.

> split/grouped would be fine too if you want to avoid re-using
> interleaved here.

split/grouped is fine by me.

>
> > The way OOB
> > bytes are organized do not seem relevant to me, I think i prefer the
> > "_4_/_8_" naming,even if it's not very explicit.
>
> The ECC strength doesn't say anything about the scheme used for ECC
> bytes placement, and you might end up with 2 different schemes
> providing the same strength, or the same scheme used for 2 different
> strengths.

So perhaps both should be present in the name?

2020-09-28 16:07:30

by Boris Brezillon

[permalink] [raw]
Subject: Re: [PATCH v4 1/2] mtd: spinand: micron: Generalize the function and structure names

On Mon, 28 Sep 2020 17:50:05 +0200
Miquel Raynal <[email protected]> wrote:

> > > The way OOB
> > > bytes are organized do not seem relevant to me, I think i prefer the
> > > "_4_/_8_" naming,even if it's not very explicit.
> >
> > The ECC strength doesn't say anything about the scheme used for ECC
> > bytes placement, and you might end up with 2 different schemes
> > providing the same strength, or the same scheme used for 2 different
> > strengths.
>
> So perhaps both should be present in the name?

No, the point was to re-use the same functions for various strengths if
they use the same ECC placement scheme.

2020-09-28 16:26:00

by Miquel Raynal

[permalink] [raw]
Subject: Re: [PATCH v4 1/2] mtd: spinand: micron: Generalize the function and structure names

Hi Boris,

Boris Brezillon <[email protected]> wrote on Mon, 28 Sep
2020 18:03:43 +0200:

> On Mon, 28 Sep 2020 17:50:05 +0200
> Miquel Raynal <[email protected]> wrote:
>
> > > > The way OOB
> > > > bytes are organized do not seem relevant to me, I think i prefer the
> > > > "_4_/_8_" naming,even if it's not very explicit.
> > >
> > > The ECC strength doesn't say anything about the scheme used for ECC
> > > bytes placement, and you might end up with 2 different schemes
> > > providing the same strength, or the same scheme used for 2 different
> > > strengths.
> >
> > So perhaps both should be present in the name?
>
> No, the point was to re-use the same functions for various strengths if
> they use the same ECC placement scheme.

I get the point, but is the current implementation generic enough? I
see hardcoded numbers, I have no idea if these numbers are common to
all strength given a specific layout, or if they only match for a given
strength?

+static int micron_4_ooblayout_ecc(struct mtd_info *mtd, int section,
+ struct mtd_oob_region *region)
+{
+ struct spinand_device *spinand = mtd_to_spinand(mtd);
+
+ if (section >= spinand->base.memorg.pagesize /
+ mtd->ecc_step_size)
+ return -ERANGE;
+
+ region->offset = (section * 16) + 8;
+ region->length = 8;
+
+ return 0;
+}

If possible, I would like to avoid several successive renaming.

Thanks,
Miquèl

2020-09-28 16:27:35

by Boris Brezillon

[permalink] [raw]
Subject: Re: [PATCH v4 1/2] mtd: spinand: micron: Generalize the function and structure names

On Mon, 28 Sep 2020 18:21:59 +0200
Miquel Raynal <[email protected]> wrote:

> Hi Boris,
>
> Boris Brezillon <[email protected]> wrote on Mon, 28 Sep
> 2020 18:03:43 +0200:
>
> > On Mon, 28 Sep 2020 17:50:05 +0200
> > Miquel Raynal <[email protected]> wrote:
> >
> > > > > The way OOB
> > > > > bytes are organized do not seem relevant to me, I think i prefer the
> > > > > "_4_/_8_" naming,even if it's not very explicit.
> > > >
> > > > The ECC strength doesn't say anything about the scheme used for ECC
> > > > bytes placement, and you might end up with 2 different schemes
> > > > providing the same strength, or the same scheme used for 2 different
> > > > strengths.
> > >
> > > So perhaps both should be present in the name?
> >
> > No, the point was to re-use the same functions for various strengths if
> > they use the same ECC placement scheme.
>
> I get the point, but is the current implementation generic enough? I
> see hardcoded numbers, I have no idea if these numbers are common to
> all strength given a specific layout, or if they only match for a given
> strength?
>
> +static int micron_4_ooblayout_ecc(struct mtd_info *mtd, int section,
> + struct mtd_oob_region *region)
> +{
> + struct spinand_device *spinand = mtd_to_spinand(mtd);
> +
> + if (section >= spinand->base.memorg.pagesize /
> + mtd->ecc_step_size)
> + return -ERANGE;
> +
> + region->offset = (section * 16) + 8;
> + region->length = 8;
> +
> + return 0;
> +}
>
> If possible, I would like to avoid several successive renaming.

Right, I thought those functions were patched to be generic, but that
doesn't seem to be the case, so I guess sticking to _<strength>_ makes
sense for now.