Messages in this thread | | | Date | Wed, 6 Sep 2023 07:12:12 +0100 | Subject | Re: [PATCH v2 13/41] mtd: spi-nor: move the .id and .id_len into an own structure | From | Tudor Ambarus <> |
| |
On 8/22/23 08:09, Michael Walle wrote: > Create a new structure to hold a flash ID and its length. The goal is to > have a new macro SNOR_ID() which can have a flexible id length. This way > we can get rid of all the individual INFOx() macros. > > Signed-off-by: Michael Walle <mwalle@kernel.org> > --- > drivers/mtd/spi-nor/core.c | 6 +++--- > drivers/mtd/spi-nor/core.h | 33 ++++++++++++++++++++++++--------- > drivers/mtd/spi-nor/micron-st.c | 4 ++-- > drivers/mtd/spi-nor/spansion.c | 4 ++-- > drivers/mtd/spi-nor/sysfs.c | 6 +++--- > drivers/mtd/spi-nor/winbond.c | 1 - > 6 files changed, 34 insertions(+), 20 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 4ba1778eda4b..80c340c7863a 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -2028,8 +2028,8 @@ static const struct flash_info *spi_nor_match_id(struct spi_nor *nor, > for (i = 0; i < ARRAY_SIZE(manufacturers); i++) { > for (j = 0; j < manufacturers[i]->nparts; j++) { > part = &manufacturers[i]->parts[j]; > - if (part->id_len && > - !memcmp(part->id, id, part->id_len)) { > + if (part->id && > + !memcmp(part->id->bytes, id, part->id->len)) { > nor->manufacturer = manufacturers[i]; > return part; > } > @@ -3370,7 +3370,7 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor, > * If caller has specified name of flash model that can normally be > * detected using JEDEC, let's verify it. > */ > - if (name && info->id_len) { > + if (name && info->id) { > const struct flash_info *jinfo; > > jinfo = spi_nor_detect(nor); > diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h > index c42b65623da7..81535f31907f 100644 > --- a/drivers/mtd/spi-nor/core.h > +++ b/drivers/mtd/spi-nor/core.h > @@ -446,12 +446,22 @@ struct spi_nor_fixups { > int (*late_init)(struct spi_nor *nor); > }; > > +/** > + * struct spi_nor_id - SPI NOR flash ID. > + * > + * @bytes: the flash's ID bytes. The first three bytes are the JEDIC ID.
typo, JEDIC. But are there 3 bytes of ID specified by JEDEC? I remeber there's only the MFR ID specified, the rest are flash specific, which are not mentioned by JEDEC.
> + * @len: the number of bytes of ID. > + */ > +struct spi_nor_id { > + const u8 *bytes; > + u8 len; > +}; > + > /** > * struct flash_info - SPI NOR flash_info entry. > + * @id: pointer to struct spi_nor_id or NULL, which means "no ID" (mostly > + * older chips). > * @name: the name of the flash. > - * @id: the flash's ID bytes. The first three bytes are the > - * JEDIC ID. JEDEC ID zero means "no ID" (mostly older chips). > - * @id_len: the number of bytes of ID. > * @size: the size of the flash in bytes. > * @sector_size: (optional) the size listed here is what works with > * SPINOR_OP_SE, which isn't necessarily called a "sector" by > @@ -510,8 +520,7 @@ struct spi_nor_fixups { > */ > struct flash_info { > char *name; > - u8 id[SPI_NOR_MAX_ID_LEN]; > - u8 id_len; > + const struct spi_nor_id *id; > size_t size; > unsigned sector_size; > u16 page_size; > @@ -554,12 +563,18 @@ struct flash_info { > #define SPI_NOR_ID_3ITEMS(_id) ((_id) >> 16) & 0xff, SPI_NOR_ID_2ITEMS(_id) > > #define SPI_NOR_ID(_jedec_id, _ext_id) \ > - .id = { SPI_NOR_ID_3ITEMS(_jedec_id), SPI_NOR_ID_2ITEMS(_ext_id) }, \ > - .id_len = !(_jedec_id) ? 0 : (3 + ((_ext_id) ? 2 : 0)) > + .id = &(const struct spi_nor_id){ \ > + .bytes = (const u8[]){ SPI_NOR_ID_3ITEMS(_jedec_id), \ > + SPI_NOR_ID_2ITEMS(_ext_id) }, \ > + .len = !(_jedec_id) ? 0 : (3 + ((_ext_id) ? 2 : 0)), \ > + } > > #define SPI_NOR_ID6(_jedec_id, _ext_id) \ > - .id = { SPI_NOR_ID_3ITEMS(_jedec_id), SPI_NOR_ID_3ITEMS(_ext_id) }, \ > - .id_len = 6 > + .id = &(const struct spi_nor_id){ \ > + .bytes = (const u8[]){ SPI_NOR_ID_3ITEMS(_jedec_id), \ > + SPI_NOR_ID_3ITEMS(_ext_id) }, \ > + .len = 6, \ > + } > > #define SPI_NOR_GEOMETRY(_sector_size, _n_sectors, _n_banks) \ > .size = (_sector_size) * (_n_sectors), \ > diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c > index 5406a3af2ce0..229c951efcce 100644 > --- a/drivers/mtd/spi-nor/micron-st.c > +++ b/drivers/mtd/spi-nor/micron-st.c > @@ -78,7 +78,7 @@ static int micron_st_nor_octal_dtr_en(struct spi_nor *nor) > return ret; > } > > - if (memcmp(buf, nor->info->id, nor->info->id_len)) > + if (memcmp(buf, nor->info->id->bytes, nor->info->id->len)) > return -EINVAL; > > return 0; > @@ -114,7 +114,7 @@ static int micron_st_nor_octal_dtr_dis(struct spi_nor *nor) > return ret; > } > > - if (memcmp(buf, nor->info->id, nor->info->id_len)) > + if (memcmp(buf, nor->info->id->bytes, nor->info->id->len)) > return -EINVAL; > > return 0; > diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c > index e6468569f178..d7012ab3de2c 100644 > --- a/drivers/mtd/spi-nor/spansion.c > +++ b/drivers/mtd/spi-nor/spansion.c > @@ -228,7 +228,7 @@ static int cypress_nor_octal_dtr_en(struct spi_nor *nor) > return ret; > } > > - if (memcmp(buf, nor->info->id, nor->info->id_len)) > + if (memcmp(buf, nor->info->id->bytes, nor->info->id->len)) > return -EINVAL; > > return 0; > @@ -272,7 +272,7 @@ static int cypress_nor_octal_dtr_dis(struct spi_nor *nor) > return ret; > } > > - if (memcmp(buf, nor->info->id, nor->info->id_len)) > + if (memcmp(buf, nor->info->id->bytes, nor->info->id->len)) > return -EINVAL; > > return 0; > diff --git a/drivers/mtd/spi-nor/sysfs.c b/drivers/mtd/spi-nor/sysfs.c > index c09bb832b3b9..2dfdc555a69f 100644 > --- a/drivers/mtd/spi-nor/sysfs.c > +++ b/drivers/mtd/spi-nor/sysfs.c > @@ -35,8 +35,8 @@ static ssize_t jedec_id_show(struct device *dev, > struct spi_device *spi = to_spi_device(dev); > struct spi_mem *spimem = spi_get_drvdata(spi); > struct spi_nor *nor = spi_mem_get_drvdata(spimem); > - const u8 *id = nor->info->id_len ? nor->info->id : nor->id; > - u8 id_len = nor->info->id_len ?: SPI_NOR_MAX_ID_LEN; > + const u8 *id = nor->info->id ? nor->info->id->bytes : nor->id; > + u8 id_len = nor->info->id ? nor->info->id->len : SPI_NOR_MAX_ID_LEN; > > return sysfs_emit(buf, "%*phN\n", id_len, id); > } > @@ -78,7 +78,7 @@ static umode_t spi_nor_sysfs_is_visible(struct kobject *kobj, > > if (attr == &dev_attr_manufacturer.attr && !nor->manufacturer) > return 0; > - if (attr == &dev_attr_jedec_id.attr && !nor->info->id_len && !nor->id) > + if (attr == &dev_attr_jedec_id.attr && !nor->info->id && !nor->id) > return 0; > > return 0444; > diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c > index c21fed842762..7873cc394f07 100644 > --- a/drivers/mtd/spi-nor/winbond.c > +++ b/drivers/mtd/spi-nor/winbond.c > @@ -121,7 +121,6 @@ static const struct flash_info winbond_nor_parts[] = { > { "w25q80bl", INFO(0xef4014, 0, 64 * 1024, 16) > NO_SFDP_FLAGS(SECT_4K) }, > { "w25q128", INFO(0xef4018, 0, 0, 0) > - PARSE_SFDP
this is a leftover that should be squashed in a previous commit.
I'm fine with the overal idea, but please fix the comment about the 3 bytes of jedec id.
> FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) }, > { "w25q256", INFO(0xef4019, 0, 64 * 1024, 512) > NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) >
| |