| Message ID | 20230807-mtd-flash-info-db-rework-v1-6-3d3d5bef4ba4@kernel.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | mtd: spi-nor: clean the flash_info database up | expand |
Hi Michael, mwalle@kernel.org wrote on Mon, 07 Aug 2023 15:21:00 +0200: > The INFO() macro always set the page_size to 256 bytes. Make that an > optinal parameter. This default is a sane one for all older flashes, optional > newer ones will set the page size by its SFDP tables anyway. > > Signed-off-by: Michael Walle <mwalle@kernel.org> > --- > drivers/mtd/spi-nor/core.c | 7 +------ > drivers/mtd/spi-nor/core.h | 8 ++++++-- > 2 files changed, 7 insertions(+), 8 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index c504a5af4032..138bc1e0a67c 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -2017,11 +2017,6 @@ static const struct spi_nor_manufacturer *manufacturers[] = { > static const struct flash_info spi_nor_generic_flash = { > .name = "spi-nor-generic", > .n_banks = 1, > - /* > - * JESD216 rev A doesn't specify the page size, therefore we need a > - * sane default. > - */ > - .page_size = 256, > .parse_sfdp = true, > }; > > @@ -3000,7 +2995,7 @@ static void spi_nor_init_default_params(struct spi_nor *nor) > params->writesize = 1; > params->size = info->size; > params->bank_size = params->size; > - params->page_size = info->page_size; > + params->page_size = info->page_size ?: SPI_NOR_DEFAULT_PAGE_SIZE; Would you mind clarifying this? It does not look right, while perhaps it is. But TBH, I have no idea what params->page_size will be after this assignment. > > if (!(info->flags & SPI_NOR_NO_FR)) { > /* Default to Fast Read for DT and non-DT platform devices. */ > diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h > index 12c35409493b..25bc18197614 100644 > --- a/drivers/mtd/spi-nor/core.h > +++ b/drivers/mtd/spi-nor/core.h > @@ -10,6 +10,11 @@ > #include "sfdp.h" > > #define SPI_NOR_MAX_ID_LEN 6 > +/* > + * 256 bytes is a sane default for most older flashes. Newer flashes will > + * have the page size defined within their SFDP tables. > + */ > +#define SPI_NOR_DEFAULT_PAGE_SIZE 256 > > /* Standard SPI NOR flash operations. */ > #define SPI_NOR_READID_OP(naddr, ndummy, buf, len) \ > @@ -447,7 +452,7 @@ struct spi_nor_fixups { > * @sector_size: the size listed here is what works with SPINOR_OP_SE, which > * isn't necessarily called a "sector" by the vendor. > * @n_banks: the number of banks. > - * @page_size: the flash's page size. > + * @page_size: (optional) the flash's page size. Defaults to 256. > * @addr_nbytes: number of address bytes to send. > * > * @parse_sfdp: true when flash supports SFDP tables. The false value has no > @@ -558,7 +563,6 @@ struct flash_info { > #define SPI_NOR_GEOMETRY(_sector_size, _n_sectors, _n_banks) \ > .size = (_sector_size) * (_n_sectors), \ > .sector_size = (_sector_size), \ > - .page_size = 256, \ > .n_banks = (_n_banks) > > /* Used when the "_ext_id" is two bytes at most */ > Otherwise, lgtm. Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com> Thanks, Miquèl
Hi Miquel, >> + params->page_size = info->page_size ?: SPI_NOR_DEFAULT_PAGE_SIZE; > > Would you mind clarifying this? It does not look right, while > perhaps it is. But TBH, I have no idea what params->page_size will be > after this assignment. It's info->page_size, it's a shortcut [1]. I'm not the first one using it (in fact I was explicitly asked to use it, e.g. in gpio subsys): $ grep -r "?:" **/*.c |wc -l 1268 -michael [1] https://en.wikipedia.org/wiki/Elvis_operator
Hi Michael, michael@walle.cc wrote on Mon, 07 Aug 2023 16:29:38 +0200: > Hi Miquel, > > >> + params->page_size = info->page_size ?: SPI_NOR_DEFAULT_PAGE_SIZE; > > > > Would you mind clarifying this? It does not look right, while > > perhaps it is. But TBH, I have no idea what params->page_size will be > > after this assignment. > > It's info->page_size, it's a shortcut [1]. Elvis operator :') > I'm not the first one using it (in fact I was explicitly asked to use it, > e.g. in gpio subsys): > > $ grep -r "?:" **/*.c |wc -l > 1268 Okay then... I didn't know this was encouraged. > -michael > > [1] https://en.wikipedia.org/wiki/Elvis_operator Thanks, Miquèl
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index c504a5af4032..138bc1e0a67c 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -2017,11 +2017,6 @@ static const struct spi_nor_manufacturer *manufacturers[] = { static const struct flash_info spi_nor_generic_flash = { .name = "spi-nor-generic", .n_banks = 1, - /* - * JESD216 rev A doesn't specify the page size, therefore we need a - * sane default. - */ - .page_size = 256, .parse_sfdp = true, }; @@ -3000,7 +2995,7 @@ static void spi_nor_init_default_params(struct spi_nor *nor) params->writesize = 1; params->size = info->size; params->bank_size = params->size; - params->page_size = info->page_size; + params->page_size = info->page_size ?: SPI_NOR_DEFAULT_PAGE_SIZE; if (!(info->flags & SPI_NOR_NO_FR)) { /* Default to Fast Read for DT and non-DT platform devices. */ diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h index 12c35409493b..25bc18197614 100644 --- a/drivers/mtd/spi-nor/core.h +++ b/drivers/mtd/spi-nor/core.h @@ -10,6 +10,11 @@ #include "sfdp.h" #define SPI_NOR_MAX_ID_LEN 6 +/* + * 256 bytes is a sane default for most older flashes. Newer flashes will + * have the page size defined within their SFDP tables. + */ +#define SPI_NOR_DEFAULT_PAGE_SIZE 256 /* Standard SPI NOR flash operations. */ #define SPI_NOR_READID_OP(naddr, ndummy, buf, len) \ @@ -447,7 +452,7 @@ struct spi_nor_fixups { * @sector_size: the size listed here is what works with SPINOR_OP_SE, which * isn't necessarily called a "sector" by the vendor. * @n_banks: the number of banks. - * @page_size: the flash's page size. + * @page_size: (optional) the flash's page size. Defaults to 256. * @addr_nbytes: number of address bytes to send. * * @parse_sfdp: true when flash supports SFDP tables. The false value has no @@ -558,7 +563,6 @@ struct flash_info { #define SPI_NOR_GEOMETRY(_sector_size, _n_sectors, _n_banks) \ .size = (_sector_size) * (_n_sectors), \ .sector_size = (_sector_size), \ - .page_size = 256, \ .n_banks = (_n_banks) /* Used when the "_ext_id" is two bytes at most */
The INFO() macro always set the page_size to 256 bytes. Make that an optinal parameter. This default is a sane one for all older flashes, newer ones will set the page size by its SFDP tables anyway. Signed-off-by: Michael Walle <mwalle@kernel.org> --- drivers/mtd/spi-nor/core.c | 7 +------ drivers/mtd/spi-nor/core.h | 8 ++++++-- 2 files changed, 7 insertions(+), 8 deletions(-)