Re: [PATCH 08/41] mtd: spi-nor: default .n_banks to 1

From: Miquel Raynal
Date: Mon Aug 07 2023 - 09:51:26 EST


Hi Michael,

mwalle@xxxxxxxxxx wrote on Mon, 07 Aug 2023 15:21:02 +0200:

> If .n_banks is not set in the flash_info database, the default value
> should be 1. This way, we don't have to always set the .n_banks
> parameter in flash_info.
>
> Signed-off-by: Michael Walle <mwalle@xxxxxxxxxx>
> ---
> drivers/mtd/spi-nor/core.c | 3 +--
> drivers/mtd/spi-nor/core.h | 8 ++++----
> drivers/mtd/spi-nor/xilinx.c | 1 -
> 3 files changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index ea94fb0da1e5..015152ba8973 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2016,7 +2016,6 @@ static const struct spi_nor_manufacturer *manufacturers[] = {
>
> static const struct flash_info spi_nor_generic_flash = {
> .name = "spi-nor-generic",
> - .n_banks = 1,
> .parse_sfdp = true,
> };
>
> @@ -2996,7 +2995,7 @@ static void spi_nor_init_default_params(struct spi_nor *nor)
> params->size = info->size;
> params->bank_size = params->size;
> params->page_size = info->page_size ?: SPI_NOR_DEFAULT_PAGE_SIZE;
> - params->n_banks = info->n_banks;
> + params->n_banks = info->n_banks ?: SPI_NOR_DEFAULT_N_BANKS;

Same here, I'm not against ternary operators at all, but the

? /* nothing */ : <something>

form is strange.

>
> 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 fe1ce232a6c8..c90445e186c0 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -15,6 +15,7 @@
> * have the page size defined within their SFDP tables.
> */
> #define SPI_NOR_DEFAULT_PAGE_SIZE 256
> +#define SPI_NOR_DEFAULT_N_BANKS 1
>
> /* Standard SPI NOR flash operations. */
> #define SPI_NOR_READID_OP(naddr, ndummy, buf, len) \
> @@ -453,7 +454,7 @@ struct spi_nor_fixups {
> * @size: the size of the flash in bytes.
> * @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.
> + * @n_banks: (optional) the number of banks. Defaults to 1.
> * @page_size: (optional) the flash's page size. Defaults to 256.
> * @addr_nbytes: number of address bytes to send.
> *
> @@ -570,7 +571,7 @@ struct flash_info {
> /* Used when the "_ext_id" is two bytes at most */
> #define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors) \
> SPI_NOR_ID((_jedec_id), (_ext_id)), \
> - SPI_NOR_GEOMETRY((_sector_size), (_n_sectors), 1),
> + SPI_NOR_GEOMETRY((_sector_size), (_n_sectors), 0),
>
> #define INFOB(_jedec_id, _ext_id, _sector_size, _n_sectors, _n_banks) \
> SPI_NOR_ID((_jedec_id), (_ext_id)), \
> @@ -578,13 +579,12 @@ struct flash_info {
>
> #define INFO6(_jedec_id, _ext_id, _sector_size, _n_sectors) \
> SPI_NOR_ID6((_jedec_id), (_ext_id)), \
> - SPI_NOR_GEOMETRY((_sector_size), (_n_sectors), 1),
> + SPI_NOR_GEOMETRY((_sector_size), (_n_sectors), 0),
>
> #define CAT25_INFO(_sector_size, _n_sectors, _page_size, _addr_nbytes) \
> .size = (_sector_size) * (_n_sectors), \
> .sector_size = (_sector_size), \
> .page_size = (_page_size), \
> - .n_banks = 1, \
> .addr_nbytes = (_addr_nbytes), \
> .flags = SPI_NOR_NO_ERASE | SPI_NOR_NO_FR, \
>
> diff --git a/drivers/mtd/spi-nor/xilinx.c b/drivers/mtd/spi-nor/xilinx.c
> index 284e2e4970ab..8d4539e32dfe 100644
> --- a/drivers/mtd/spi-nor/xilinx.c
> +++ b/drivers/mtd/spi-nor/xilinx.c
> @@ -26,7 +26,6 @@
> .size = 8 * (_page_size) * (_n_sectors), \
> .sector_size = (8 * (_page_size)), \
> .page_size = (_page_size), \
> - .n_banks = 1, \
> .flags = SPI_NOR_NO_FR
>
> /* Xilinx S3AN share MFR with Atmel SPI NOR */
>

But otherwise

Reviewed-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>


Thanks,
Miquèl