[PATCH v2 0/6] mtd: spi-nor: Random cleanups

[PATCH v2 0/6] mtd: spi-nor: Random cleanups

[Boris Brezillon]

Hi,

The patch series is a collection of minor cleanups I have accumulated
while working on the "SPI NOR SFDP fixups" infrastructure.

I'd really like to have those patches reviewed and merged quickly
because the 'soon to be posted' "SPI NOR SFDP fixups" patchset depends
on it.

Regards,

Boris

Changes in v2:
- Drop
  "mtd: spi-nor: Set SPI_NOR_4B_OPCODES on all >16MB Spansion NORs",
  "mtd: spi-nor: Create a ->set_4byte() method",
  "mtd: spi-nor: Explicitly flag all Micron NORs as supporting locking"
  "mtd: spi-nor: Move Spansion erase related fixup to a dedicated function"
- Add a patch adding the SPDX tag
- Add a patch moving nor->info assignment earlier in the spi_nor_scan()
  function

Boris Brezillon (6):
  mtd: spi-nor: Drop inline on all internal helpers
  mtd: spi-nor: Avoid forward declaration of internal functions
  mtd: spi-nor: Stop passing info to set_4byte()
  mtd: spi-nor: Make the enable argument passed to set_byte() a bool
  mtd: spi-nor: Add an SPDX tag to spi-nor.c
  mtd: spi-nor: Move the nor->info assignment earlier in the scan func

 drivers/mtd/spi-nor/spi-nor.c | 264 +++++++++++++++++-----------------
 1 file changed, 129 insertions(+), 135 deletions(-)

-- 
2.17.1

[PATCH v2 1/6] mtd: spi-nor: Drop inline on all internal helpers

[Boris Brezillon]

gcc should be smart enough to decide when inlining a function makes
sense. Drop all inline specifiers.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
Changes in v2:
- None
---
 drivers/mtd/spi-nor/spi-nor.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 828d03edc0a3..0aa1b1035b4b 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -159,7 +159,7 @@ static int read_cr(struct spi_nor *nor)
  * Write status register 1 byte
  * Returns negative if error occurred.
  */
-static inline int write_sr(struct spi_nor *nor, u8 val)
+static int write_sr(struct spi_nor *nor, u8 val)
 {
 	nor->cmd_buf[0] = val;
 	return nor->write_reg(nor, SPINOR_OP_WRSR, nor->cmd_buf, 1);
@@ -169,7 +169,7 @@ static inline int write_sr(struct spi_nor *nor, u8 val)
  * Set write enable latch with Write Enable command.
  * Returns negative if error occurred.
  */
-static inline int write_enable(struct spi_nor *nor)
+static int write_enable(struct spi_nor *nor)
 {
 	return nor->write_reg(nor, SPINOR_OP_WREN, NULL, 0);
 }
@@ -177,12 +177,12 @@ static inline int write_enable(struct spi_nor *nor)
 /*
  * Send write disable instruction to the chip.
  */
-static inline int write_disable(struct spi_nor *nor)
+static int write_disable(struct spi_nor *nor)
 {
 	return nor->write_reg(nor, SPINOR_OP_WRDI, NULL, 0);
 }
 
-static inline struct spi_nor *mtd_to_spi_nor(struct mtd_info *mtd)
+static struct spi_nor *mtd_to_spi_nor(struct mtd_info *mtd)
 {
 	return mtd->priv;
 }
@@ -200,7 +200,7 @@ static u8 spi_nor_convert_opcode(u8 opcode, const u8 table[][2], size_t size)
 	return opcode;
 }
 
-static inline u8 spi_nor_convert_3to4_read(u8 opcode)
+static u8 spi_nor_convert_3to4_read(u8 opcode)
 {
 	static const u8 spi_nor_3to4_read[][2] = {
 		{ SPINOR_OP_READ,	SPINOR_OP_READ_4B },
@@ -219,7 +219,7 @@ static inline u8 spi_nor_convert_3to4_read(u8 opcode)
 				      ARRAY_SIZE(spi_nor_3to4_read));
 }
 
-static inline u8 spi_nor_convert_3to4_program(u8 opcode)
+static u8 spi_nor_convert_3to4_program(u8 opcode)
 {
 	static const u8 spi_nor_3to4_program[][2] = {
 		{ SPINOR_OP_PP,		SPINOR_OP_PP_4B },
@@ -231,7 +231,7 @@ static inline u8 spi_nor_convert_3to4_program(u8 opcode)
 				      ARRAY_SIZE(spi_nor_3to4_program));
 }
 
-static inline u8 spi_nor_convert_3to4_erase(u8 opcode)
+static u8 spi_nor_convert_3to4_erase(u8 opcode)
 {
 	static const u8 spi_nor_3to4_erase[][2] = {
 		{ SPINOR_OP_BE_4K,	SPINOR_OP_BE_4K_4B },
@@ -276,8 +276,8 @@ static void spi_nor_set_4byte_opcodes(struct spi_nor *nor,
 }
 
 /* Enable/disable 4-byte addressing mode. */
-static inline int set_4byte(struct spi_nor *nor, const struct flash_info *info,
-			    int enable)
+static int set_4byte(struct spi_nor *nor, const struct flash_info *info,
+		     int enable)
 {
 	int status;
 	bool need_wren = false;
@@ -335,7 +335,7 @@ static int s3an_sr_ready(struct spi_nor *nor)
 	return !!(val & XSR_RDY);
 }
 
-static inline int spi_nor_sr_ready(struct spi_nor *nor)
+static int spi_nor_sr_ready(struct spi_nor *nor)
 {
 	int sr = read_sr(nor);
 	if (sr < 0)
@@ -354,7 +354,7 @@ static inline int spi_nor_sr_ready(struct spi_nor *nor)
 	return !(sr & SR_WIP);
 }
 
-static inline int spi_nor_fsr_ready(struct spi_nor *nor)
+static int spi_nor_fsr_ready(struct spi_nor *nor)
 {
 	int fsr = read_fsr(nor);
 	if (fsr < 0)
@@ -2372,7 +2372,7 @@ struct sfdp_bfpt {
 
 /* Fast Read settings. */
 
-static inline void
+static void
 spi_nor_set_read_settings_from_bfpt(struct spi_nor_read_command *read,
 				    u16 half,
 				    enum spi_nor_protocol proto)
-- 
2.17.1

Re: [PATCH v2 1/6] mtd: spi-nor: Drop inline on all internal helpers

[Sverdlin, Alexander (Nokia - DE/Ulm)]

On 29/11/2018 15:10, Boris Brezillon wrote:
> gcc should be smart enough to decide when inlining a function makes
> sense. Drop all inline specifiers.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>

Reviewed-by: Aleaxander Sverdlin <alexander.sverdlin@nokia.com>

> ---
> Changes in v2:
> - None
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 828d03edc0a3..0aa1b1035b4b 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -159,7 +159,7 @@ static int read_cr(struct spi_nor *nor)
>   * Write status register 1 byte
>   * Returns negative if error occurred.
>   */
> -static inline int write_sr(struct spi_nor *nor, u8 val)
> +static int write_sr(struct spi_nor *nor, u8 val)
>  {
>  	nor->cmd_buf[0] = val;
>  	return nor->write_reg(nor, SPINOR_OP_WRSR, nor->cmd_buf, 1);
> @@ -169,7 +169,7 @@ static inline int write_sr(struct spi_nor *nor, u8 val)
>   * Set write enable latch with Write Enable command.
>   * Returns negative if error occurred.
>   */
> -static inline int write_enable(struct spi_nor *nor)
> +static int write_enable(struct spi_nor *nor)
>  {
>  	return nor->write_reg(nor, SPINOR_OP_WREN, NULL, 0);
>  }
> @@ -177,12 +177,12 @@ static inline int write_enable(struct spi_nor *nor)
>  /*
>   * Send write disable instruction to the chip.
>   */
> -static inline int write_disable(struct spi_nor *nor)
> +static int write_disable(struct spi_nor *nor)
>  {
>  	return nor->write_reg(nor, SPINOR_OP_WRDI, NULL, 0);
>  }
>  
> -static inline struct spi_nor *mtd_to_spi_nor(struct mtd_info *mtd)
> +static struct spi_nor *mtd_to_spi_nor(struct mtd_info *mtd)
>  {
>  	return mtd->priv;
>  }
> @@ -200,7 +200,7 @@ static u8 spi_nor_convert_opcode(u8 opcode, const u8 table[][2], size_t size)
>  	return opcode;
>  }
>  
> -static inline u8 spi_nor_convert_3to4_read(u8 opcode)
> +static u8 spi_nor_convert_3to4_read(u8 opcode)
>  {
>  	static const u8 spi_nor_3to4_read[][2] = {
>  		{ SPINOR_OP_READ,	SPINOR_OP_READ_4B },
> @@ -219,7 +219,7 @@ static inline u8 spi_nor_convert_3to4_read(u8 opcode)
>  				      ARRAY_SIZE(spi_nor_3to4_read));
>  }
>  
> -static inline u8 spi_nor_convert_3to4_program(u8 opcode)
> +static u8 spi_nor_convert_3to4_program(u8 opcode)
>  {
>  	static const u8 spi_nor_3to4_program[][2] = {
>  		{ SPINOR_OP_PP,		SPINOR_OP_PP_4B },
> @@ -231,7 +231,7 @@ static inline u8 spi_nor_convert_3to4_program(u8 opcode)
>  				      ARRAY_SIZE(spi_nor_3to4_program));
>  }
>  
> -static inline u8 spi_nor_convert_3to4_erase(u8 opcode)
> +static u8 spi_nor_convert_3to4_erase(u8 opcode)
>  {
>  	static const u8 spi_nor_3to4_erase[][2] = {
>  		{ SPINOR_OP_BE_4K,	SPINOR_OP_BE_4K_4B },
> @@ -276,8 +276,8 @@ static void spi_nor_set_4byte_opcodes(struct spi_nor *nor,
>  }
>  
>  /* Enable/disable 4-byte addressing mode. */
> -static inline int set_4byte(struct spi_nor *nor, const struct flash_info *info,
> -			    int enable)
> +static int set_4byte(struct spi_nor *nor, const struct flash_info *info,
> +		     int enable)
>  {
>  	int status;
>  	bool need_wren = false;
> @@ -335,7 +335,7 @@ static int s3an_sr_ready(struct spi_nor *nor)
>  	return !!(val & XSR_RDY);
>  }
>  
> -static inline int spi_nor_sr_ready(struct spi_nor *nor)
> +static int spi_nor_sr_ready(struct spi_nor *nor)
>  {
>  	int sr = read_sr(nor);
>  	if (sr < 0)
> @@ -354,7 +354,7 @@ static inline int spi_nor_sr_ready(struct spi_nor *nor)
>  	return !(sr & SR_WIP);
>  }
>  
> -static inline int spi_nor_fsr_ready(struct spi_nor *nor)
> +static int spi_nor_fsr_ready(struct spi_nor *nor)
>  {
>  	int fsr = read_fsr(nor);
>  	if (fsr < 0)
> @@ -2372,7 +2372,7 @@ struct sfdp_bfpt {
>  
>  /* Fast Read settings. */
>  
> -static inline void
> +static void
>  spi_nor_set_read_settings_from_bfpt(struct spi_nor_read_command *read,
>  				    u16 half,
>  				    enum spi_nor_protocol proto)

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH v2 1/6] mtd: spi-nor: Drop inline on all internal helpers

[Tudor.Ambarus]

On 11/29/2018 04:10 PM, Boris Brezillon wrote:
> gcc should be smart enough to decide when inlining a function makes
> sense. Drop all inline specifiers.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>

Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

> ---
> Changes in v2:
> - None
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 828d03edc0a3..0aa1b1035b4b 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -159,7 +159,7 @@ static int read_cr(struct spi_nor *nor)
>   * Write status register 1 byte
>   * Returns negative if error occurred.
>   */
> -static inline int write_sr(struct spi_nor *nor, u8 val)
> +static int write_sr(struct spi_nor *nor, u8 val)
>  {
>  	nor->cmd_buf[0] = val;
>  	return nor->write_reg(nor, SPINOR_OP_WRSR, nor->cmd_buf, 1);
> @@ -169,7 +169,7 @@ static inline int write_sr(struct spi_nor *nor, u8 val)
>   * Set write enable latch with Write Enable command.
>   * Returns negative if error occurred.
>   */
> -static inline int write_enable(struct spi_nor *nor)
> +static int write_enable(struct spi_nor *nor)
>  {
>  	return nor->write_reg(nor, SPINOR_OP_WREN, NULL, 0);
>  }
> @@ -177,12 +177,12 @@ static inline int write_enable(struct spi_nor *nor)
>  /*
>   * Send write disable instruction to the chip.
>   */
> -static inline int write_disable(struct spi_nor *nor)
> +static int write_disable(struct spi_nor *nor)
>  {
>  	return nor->write_reg(nor, SPINOR_OP_WRDI, NULL, 0);
>  }
>  
> -static inline struct spi_nor *mtd_to_spi_nor(struct mtd_info *mtd)
> +static struct spi_nor *mtd_to_spi_nor(struct mtd_info *mtd)
>  {
>  	return mtd->priv;
>  }
> @@ -200,7 +200,7 @@ static u8 spi_nor_convert_opcode(u8 opcode, const u8 table[][2], size_t size)
>  	return opcode;
>  }
>  
> -static inline u8 spi_nor_convert_3to4_read(u8 opcode)
> +static u8 spi_nor_convert_3to4_read(u8 opcode)
>  {
>  	static const u8 spi_nor_3to4_read[][2] = {
>  		{ SPINOR_OP_READ,	SPINOR_OP_READ_4B },
> @@ -219,7 +219,7 @@ static inline u8 spi_nor_convert_3to4_read(u8 opcode)
>  				      ARRAY_SIZE(spi_nor_3to4_read));
>  }
>  
> -static inline u8 spi_nor_convert_3to4_program(u8 opcode)
> +static u8 spi_nor_convert_3to4_program(u8 opcode)
>  {
>  	static const u8 spi_nor_3to4_program[][2] = {
>  		{ SPINOR_OP_PP,		SPINOR_OP_PP_4B },
> @@ -231,7 +231,7 @@ static inline u8 spi_nor_convert_3to4_program(u8 opcode)
>  				      ARRAY_SIZE(spi_nor_3to4_program));
>  }
>  
> -static inline u8 spi_nor_convert_3to4_erase(u8 opcode)
> +static u8 spi_nor_convert_3to4_erase(u8 opcode)
>  {
>  	static const u8 spi_nor_3to4_erase[][2] = {
>  		{ SPINOR_OP_BE_4K,	SPINOR_OP_BE_4K_4B },
> @@ -276,8 +276,8 @@ static void spi_nor_set_4byte_opcodes(struct spi_nor *nor,
>  }
>  
>  /* Enable/disable 4-byte addressing mode. */
> -static inline int set_4byte(struct spi_nor *nor, const struct flash_info *info,
> -			    int enable)
> +static int set_4byte(struct spi_nor *nor, const struct flash_info *info,
> +		     int enable)
>  {
>  	int status;
>  	bool need_wren = false;
> @@ -335,7 +335,7 @@ static int s3an_sr_ready(struct spi_nor *nor)
>  	return !!(val & XSR_RDY);
>  }
>  
> -static inline int spi_nor_sr_ready(struct spi_nor *nor)
> +static int spi_nor_sr_ready(struct spi_nor *nor)
>  {
>  	int sr = read_sr(nor);
>  	if (sr < 0)
> @@ -354,7 +354,7 @@ static inline int spi_nor_sr_ready(struct spi_nor *nor)
>  	return !(sr & SR_WIP);
>  }
>  
> -static inline int spi_nor_fsr_ready(struct spi_nor *nor)
> +static int spi_nor_fsr_ready(struct spi_nor *nor)
>  {
>  	int fsr = read_fsr(nor);
>  	if (fsr < 0)
> @@ -2372,7 +2372,7 @@ struct sfdp_bfpt {
>  
>  /* Fast Read settings. */
>  
> -static inline void
> +static void
>  spi_nor_set_read_settings_from_bfpt(struct spi_nor_read_command *read,
>  				    u16 half,
>  				    enum spi_nor_protocol proto)
> 

[PATCH v2 2/6] mtd: spi-nor: Avoid forward declaration of internal functions

[Boris Brezillon]

Reorganize the code to kill forward declarations of spi_nor_match_id()
macronix_quad_enable() and spi_nor_hwcaps_read2cmd().

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
Changes in v2:
- None
---
 drivers/mtd/spi-nor/spi-nor.c | 202 +++++++++++++++++-----------------
 1 file changed, 98 insertions(+), 104 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 0aa1b1035b4b..e1eaabf98d08 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -96,8 +96,6 @@ struct flash_info {
 
 #define JEDEC_MFR(info)	((info)->id[0])
 
-static const struct flash_info *spi_nor_match_id(const char *name);
-
 /*
  * Read the status register, returning its value in the location
  * Return the status register value.
@@ -1202,7 +1200,42 @@ static int spi_nor_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 	return ret;
 }
 
-static int macronix_quad_enable(struct spi_nor *nor);
+/**
+ * macronix_quad_enable() - set QE bit in Status Register.
+ * @nor:	pointer to a 'struct spi_nor'
+ *
+ * Set the Quad Enable (QE) bit in the Status Register.
+ *
+ * bit 6 of the Status Register is the QE bit for Macronix like QSPI memories.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int macronix_quad_enable(struct spi_nor *nor)
+{
+	int ret, val;
+
+	val = read_sr(nor);
+	if (val < 0)
+		return val;
+	if (val & SR_QUAD_EN_MX)
+		return 0;
+
+	write_enable(nor);
+
+	write_sr(nor, val | SR_QUAD_EN_MX);
+
+	ret = spi_nor_wait_till_ready(nor);
+	if (ret)
+		return ret;
+
+	ret = read_sr(nor);
+	if (!(ret > 0 && (ret & SR_QUAD_EN_MX))) {
+		dev_err(nor->dev, "Macronix Quad bit not set\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
 
 /* Used when the "_ext_id" is two bytes at most */
 #define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags)	\
@@ -1778,43 +1811,6 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
 	return ret;
 }
 
-/**
- * macronix_quad_enable() - set QE bit in Status Register.
- * @nor:	pointer to a 'struct spi_nor'
- *
- * Set the Quad Enable (QE) bit in the Status Register.
- *
- * bit 6 of the Status Register is the QE bit for Macronix like QSPI memories.
- *
- * Return: 0 on success, -errno otherwise.
- */
-static int macronix_quad_enable(struct spi_nor *nor)
-{
-	int ret, val;
-
-	val = read_sr(nor);
-	if (val < 0)
-		return val;
-	if (val & SR_QUAD_EN_MX)
-		return 0;
-
-	write_enable(nor);
-
-	write_sr(nor, val | SR_QUAD_EN_MX);
-
-	ret = spi_nor_wait_till_ready(nor);
-	if (ret)
-		return ret;
-
-	ret = read_sr(nor);
-	if (!(ret > 0 && (ret & SR_QUAD_EN_MX))) {
-		dev_err(nor->dev, "Macronix Quad bit not set\n");
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
 /*
  * Write status Register and configuration register with 2 bytes
  * The first byte will be written to the status register, while the
@@ -2479,7 +2475,56 @@ static const struct sfdp_bfpt_erase sfdp_bfpt_erases[] = {
 	{BFPT_DWORD(9), 16},
 };
 
-static int spi_nor_hwcaps_read2cmd(u32 hwcaps);
+static int spi_nor_hwcaps2cmd(u32 hwcaps, const int table[][2], size_t size)
+{
+	size_t i;
+
+	for (i = 0; i < size; i++)
+		if (table[i][0] == (int)hwcaps)
+			return table[i][1];
+
+	return -EINVAL;
+}
+
+static int spi_nor_hwcaps_read2cmd(u32 hwcaps)
+{
+	static const int hwcaps_read2cmd[][2] = {
+		{ SNOR_HWCAPS_READ,		SNOR_CMD_READ },
+		{ SNOR_HWCAPS_READ_FAST,	SNOR_CMD_READ_FAST },
+		{ SNOR_HWCAPS_READ_1_1_1_DTR,	SNOR_CMD_READ_1_1_1_DTR },
+		{ SNOR_HWCAPS_READ_1_1_2,	SNOR_CMD_READ_1_1_2 },
+		{ SNOR_HWCAPS_READ_1_2_2,	SNOR_CMD_READ_1_2_2 },
+		{ SNOR_HWCAPS_READ_2_2_2,	SNOR_CMD_READ_2_2_2 },
+		{ SNOR_HWCAPS_READ_1_2_2_DTR,	SNOR_CMD_READ_1_2_2_DTR },
+		{ SNOR_HWCAPS_READ_1_1_4,	SNOR_CMD_READ_1_1_4 },
+		{ SNOR_HWCAPS_READ_1_4_4,	SNOR_CMD_READ_1_4_4 },
+		{ SNOR_HWCAPS_READ_4_4_4,	SNOR_CMD_READ_4_4_4 },
+		{ SNOR_HWCAPS_READ_1_4_4_DTR,	SNOR_CMD_READ_1_4_4_DTR },
+		{ SNOR_HWCAPS_READ_1_1_8,	SNOR_CMD_READ_1_1_8 },
+		{ SNOR_HWCAPS_READ_1_8_8,	SNOR_CMD_READ_1_8_8 },
+		{ SNOR_HWCAPS_READ_8_8_8,	SNOR_CMD_READ_8_8_8 },
+		{ SNOR_HWCAPS_READ_1_8_8_DTR,	SNOR_CMD_READ_1_8_8_DTR },
+	};
+
+	return spi_nor_hwcaps2cmd(hwcaps, hwcaps_read2cmd,
+				  ARRAY_SIZE(hwcaps_read2cmd));
+}
+
+static int spi_nor_hwcaps_pp2cmd(u32 hwcaps)
+{
+	static const int hwcaps_pp2cmd[][2] = {
+		{ SNOR_HWCAPS_PP,		SNOR_CMD_PP },
+		{ SNOR_HWCAPS_PP_1_1_4,		SNOR_CMD_PP_1_1_4 },
+		{ SNOR_HWCAPS_PP_1_4_4,		SNOR_CMD_PP_1_4_4 },
+		{ SNOR_HWCAPS_PP_4_4_4,		SNOR_CMD_PP_4_4_4 },
+		{ SNOR_HWCAPS_PP_1_1_8,		SNOR_CMD_PP_1_1_8 },
+		{ SNOR_HWCAPS_PP_1_8_8,		SNOR_CMD_PP_1_8_8 },
+		{ SNOR_HWCAPS_PP_8_8_8,		SNOR_CMD_PP_8_8_8 },
+	};
+
+	return spi_nor_hwcaps2cmd(hwcaps, hwcaps_pp2cmd,
+				  ARRAY_SIZE(hwcaps_pp2cmd));
+}
 
 /**
  * spi_nor_set_erase_type() - set a SPI NOR erase type
@@ -3279,57 +3324,6 @@ static int spi_nor_init_params(struct spi_nor *nor,
 	return 0;
 }
 
-static int spi_nor_hwcaps2cmd(u32 hwcaps, const int table[][2], size_t size)
-{
-	size_t i;
-
-	for (i = 0; i < size; i++)
-		if (table[i][0] == (int)hwcaps)
-			return table[i][1];
-
-	return -EINVAL;
-}
-
-static int spi_nor_hwcaps_read2cmd(u32 hwcaps)
-{
-	static const int hwcaps_read2cmd[][2] = {
-		{ SNOR_HWCAPS_READ,		SNOR_CMD_READ },
-		{ SNOR_HWCAPS_READ_FAST,	SNOR_CMD_READ_FAST },
-		{ SNOR_HWCAPS_READ_1_1_1_DTR,	SNOR_CMD_READ_1_1_1_DTR },
-		{ SNOR_HWCAPS_READ_1_1_2,	SNOR_CMD_READ_1_1_2 },
-		{ SNOR_HWCAPS_READ_1_2_2,	SNOR_CMD_READ_1_2_2 },
-		{ SNOR_HWCAPS_READ_2_2_2,	SNOR_CMD_READ_2_2_2 },
-		{ SNOR_HWCAPS_READ_1_2_2_DTR,	SNOR_CMD_READ_1_2_2_DTR },
-		{ SNOR_HWCAPS_READ_1_1_4,	SNOR_CMD_READ_1_1_4 },
-		{ SNOR_HWCAPS_READ_1_4_4,	SNOR_CMD_READ_1_4_4 },
-		{ SNOR_HWCAPS_READ_4_4_4,	SNOR_CMD_READ_4_4_4 },
-		{ SNOR_HWCAPS_READ_1_4_4_DTR,	SNOR_CMD_READ_1_4_4_DTR },
-		{ SNOR_HWCAPS_READ_1_1_8,	SNOR_CMD_READ_1_1_8 },
-		{ SNOR_HWCAPS_READ_1_8_8,	SNOR_CMD_READ_1_8_8 },
-		{ SNOR_HWCAPS_READ_8_8_8,	SNOR_CMD_READ_8_8_8 },
-		{ SNOR_HWCAPS_READ_1_8_8_DTR,	SNOR_CMD_READ_1_8_8_DTR },
-	};
-
-	return spi_nor_hwcaps2cmd(hwcaps, hwcaps_read2cmd,
-				  ARRAY_SIZE(hwcaps_read2cmd));
-}
-
-static int spi_nor_hwcaps_pp2cmd(u32 hwcaps)
-{
-	static const int hwcaps_pp2cmd[][2] = {
-		{ SNOR_HWCAPS_PP,		SNOR_CMD_PP },
-		{ SNOR_HWCAPS_PP_1_1_4,		SNOR_CMD_PP_1_1_4 },
-		{ SNOR_HWCAPS_PP_1_4_4,		SNOR_CMD_PP_1_4_4 },
-		{ SNOR_HWCAPS_PP_4_4_4,		SNOR_CMD_PP_4_4_4 },
-		{ SNOR_HWCAPS_PP_1_1_8,		SNOR_CMD_PP_1_1_8 },
-		{ SNOR_HWCAPS_PP_1_8_8,		SNOR_CMD_PP_1_8_8 },
-		{ SNOR_HWCAPS_PP_8_8_8,		SNOR_CMD_PP_8_8_8 },
-	};
-
-	return spi_nor_hwcaps2cmd(hwcaps, hwcaps_pp2cmd,
-				  ARRAY_SIZE(hwcaps_pp2cmd));
-}
-
 static int spi_nor_select_read(struct spi_nor *nor,
 			       const struct spi_nor_flash_parameter *params,
 			       u32 shared_hwcaps)
@@ -3610,6 +3604,18 @@ void spi_nor_restore(struct spi_nor *nor)
 }
 EXPORT_SYMBOL_GPL(spi_nor_restore);
 
+static const struct flash_info *spi_nor_match_id(const char *name)
+{
+	const struct flash_info *id = spi_nor_ids;
+
+	while (id->name) {
+		if (!strcmp(name, id->name))
+			return id;
+		id++;
+	}
+	return NULL;
+}
+
 int spi_nor_scan(struct spi_nor *nor, const char *name,
 		 const struct spi_nor_hwcaps *hwcaps)
 {
@@ -3809,18 +3815,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 }
 EXPORT_SYMBOL_GPL(spi_nor_scan);
 
-static const struct flash_info *spi_nor_match_id(const char *name)
-{
-	const struct flash_info *id = spi_nor_ids;
-
-	while (id->name) {
-		if (!strcmp(name, id->name))
-			return id;
-		id++;
-	}
-	return NULL;
-}
-
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Huang Shijie <shijie8@gmail.com>");
 MODULE_AUTHOR("Mike Lavender");
-- 
2.17.1

Re: [PATCH v2 2/6] mtd: spi-nor: Avoid forward declaration of internal functions

[Tudor.Ambarus]

Hi, Boris,

On 11/29/2018 04:10 PM, Boris Brezillon wrote:
> Reorganize the code to kill forward declarations of spi_nor_match_id()
> macronix_quad_enable() and spi_nor_hwcaps_read2cmd().
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
> Changes in v2:
> - None
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 202 +++++++++++++++++-----------------
>  1 file changed, 98 insertions(+), 104 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 0aa1b1035b4b..e1eaabf98d08 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -96,8 +96,6 @@ struct flash_info {
>  
>  #define JEDEC_MFR(info)	((info)->id[0])
>  
> -static const struct flash_info *spi_nor_match_id(const char *name);
> -
>  /*
>   * Read the status register, returning its value in the location
>   * Return the status register value.
> @@ -1202,7 +1200,42 @@ static int spi_nor_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>  	return ret;
>  }
>  
> -static int macronix_quad_enable(struct spi_nor *nor);
> +/**
> + * macronix_quad_enable() - set QE bit in Status Register.
> + * @nor:	pointer to a 'struct spi_nor'
> + *
> + * Set the Quad Enable (QE) bit in the Status Register.
> + *
> + * bit 6 of the Status Register is the QE bit for Macronix like QSPI memories.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int macronix_quad_enable(struct spi_nor *nor)
> +{
> +	int ret, val;
> +
> +	val = read_sr(nor);
> +	if (val < 0)
> +		return val;
> +	if (val & SR_QUAD_EN_MX)
> +		return 0;
> +
> +	write_enable(nor);
> +
> +	write_sr(nor, val | SR_QUAD_EN_MX);
> +
> +	ret = spi_nor_wait_till_ready(nor);
> +	if (ret)
> +		return ret;
> +
> +	ret = read_sr(nor);
> +	if (!(ret > 0 && (ret & SR_QUAD_EN_MX))) {
> +		dev_err(nor->dev, "Macronix Quad bit not set\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}

The *_quad_enable() functions are now spread in the file. Would it make sense to
move all the *_quad_enable() functions, to keep them together?

>  
>  /* Used when the "_ext_id" is two bytes at most */
>  #define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags)	\
> @@ -1778,43 +1811,6 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
>  	return ret;
>  }
>  
> -/**
> - * macronix_quad_enable() - set QE bit in Status Register.
> - * @nor:	pointer to a 'struct spi_nor'
> - *
> - * Set the Quad Enable (QE) bit in the Status Register.
> - *
> - * bit 6 of the Status Register is the QE bit for Macronix like QSPI memories.
> - *
> - * Return: 0 on success, -errno otherwise.
> - */
> -static int macronix_quad_enable(struct spi_nor *nor)
> -{
> -	int ret, val;
> -
> -	val = read_sr(nor);
> -	if (val < 0)
> -		return val;
> -	if (val & SR_QUAD_EN_MX)
> -		return 0;
> -
> -	write_enable(nor);
> -
> -	write_sr(nor, val | SR_QUAD_EN_MX);
> -
> -	ret = spi_nor_wait_till_ready(nor);
> -	if (ret)
> -		return ret;
> -
> -	ret = read_sr(nor);
> -	if (!(ret > 0 && (ret & SR_QUAD_EN_MX))) {
> -		dev_err(nor->dev, "Macronix Quad bit not set\n");
> -		return -EINVAL;
> -	}
> -
> -	return 0;
> -}
> -
>  /*
>   * Write status Register and configuration register with 2 bytes
>   * The first byte will be written to the status register, while the
> @@ -2479,7 +2475,56 @@ static const struct sfdp_bfpt_erase sfdp_bfpt_erases[] = {
>  	{BFPT_DWORD(9), 16},
>  };
>  
> -static int spi_nor_hwcaps_read2cmd(u32 hwcaps);
> +static int spi_nor_hwcaps2cmd(u32 hwcaps, const int table[][2], size_t size)
> +{
> +	size_t i;
> +
> +	for (i = 0; i < size; i++)
> +		if (table[i][0] == (int)hwcaps)
> +			return table[i][1];
> +
> +	return -EINVAL;
> +}
> +
> +static int spi_nor_hwcaps_read2cmd(u32 hwcaps)
> +{
> +	static const int hwcaps_read2cmd[][2] = {
> +		{ SNOR_HWCAPS_READ,		SNOR_CMD_READ },
> +		{ SNOR_HWCAPS_READ_FAST,	SNOR_CMD_READ_FAST },
> +		{ SNOR_HWCAPS_READ_1_1_1_DTR,	SNOR_CMD_READ_1_1_1_DTR },
> +		{ SNOR_HWCAPS_READ_1_1_2,	SNOR_CMD_READ_1_1_2 },
> +		{ SNOR_HWCAPS_READ_1_2_2,	SNOR_CMD_READ_1_2_2 },
> +		{ SNOR_HWCAPS_READ_2_2_2,	SNOR_CMD_READ_2_2_2 },
> +		{ SNOR_HWCAPS_READ_1_2_2_DTR,	SNOR_CMD_READ_1_2_2_DTR },
> +		{ SNOR_HWCAPS_READ_1_1_4,	SNOR_CMD_READ_1_1_4 },
> +		{ SNOR_HWCAPS_READ_1_4_4,	SNOR_CMD_READ_1_4_4 },
> +		{ SNOR_HWCAPS_READ_4_4_4,	SNOR_CMD_READ_4_4_4 },
> +		{ SNOR_HWCAPS_READ_1_4_4_DTR,	SNOR_CMD_READ_1_4_4_DTR },
> +		{ SNOR_HWCAPS_READ_1_1_8,	SNOR_CMD_READ_1_1_8 },
> +		{ SNOR_HWCAPS_READ_1_8_8,	SNOR_CMD_READ_1_8_8 },
> +		{ SNOR_HWCAPS_READ_8_8_8,	SNOR_CMD_READ_8_8_8 },
> +		{ SNOR_HWCAPS_READ_1_8_8_DTR,	SNOR_CMD_READ_1_8_8_DTR },
> +	};
> +
> +	return spi_nor_hwcaps2cmd(hwcaps, hwcaps_read2cmd,
> +				  ARRAY_SIZE(hwcaps_read2cmd));
> +}
> +
> +static int spi_nor_hwcaps_pp2cmd(u32 hwcaps)
> +{
> +	static const int hwcaps_pp2cmd[][2] = {
> +		{ SNOR_HWCAPS_PP,		SNOR_CMD_PP },
> +		{ SNOR_HWCAPS_PP_1_1_4,		SNOR_CMD_PP_1_1_4 },
> +		{ SNOR_HWCAPS_PP_1_4_4,		SNOR_CMD_PP_1_4_4 },
> +		{ SNOR_HWCAPS_PP_4_4_4,		SNOR_CMD_PP_4_4_4 },
> +		{ SNOR_HWCAPS_PP_1_1_8,		SNOR_CMD_PP_1_1_8 },
> +		{ SNOR_HWCAPS_PP_1_8_8,		SNOR_CMD_PP_1_8_8 },
> +		{ SNOR_HWCAPS_PP_8_8_8,		SNOR_CMD_PP_8_8_8 },
> +	};
> +
> +	return spi_nor_hwcaps2cmd(hwcaps, hwcaps_pp2cmd,
> +				  ARRAY_SIZE(hwcaps_pp2cmd));
> +}

Would it be a better place to put these immediately after
spi_nor_set_pp_settings()? It would be consistent with how they were declared
back in cfc5604c488ccd17936b69008af0c9ae050f4a08.

Cheers,
ta

Re: [PATCH v2 2/6] mtd: spi-nor: Avoid forward declaration of internal functions

[Boris Brezillon]

On Tue, 4 Dec 2018 19:09:04 +0000
<Tudor.Ambarus@microchip.com> wrote:

> Hi, Boris,
> 
> On 11/29/2018 04:10 PM, Boris Brezillon wrote:
> > Reorganize the code to kill forward declarations of spi_nor_match_id()
> > macronix_quad_enable() and spi_nor_hwcaps_read2cmd().
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > ---
> > Changes in v2:
> > - None
> > ---
> >  drivers/mtd/spi-nor/spi-nor.c | 202 +++++++++++++++++-----------------
> >  1 file changed, 98 insertions(+), 104 deletions(-)
> > 
> > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> > index 0aa1b1035b4b..e1eaabf98d08 100644
> > --- a/drivers/mtd/spi-nor/spi-nor.c
> > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > @@ -96,8 +96,6 @@ struct flash_info {
> >  
> >  #define JEDEC_MFR(info)	((info)->id[0])
> >  
> > -static const struct flash_info *spi_nor_match_id(const char *name);
> > -
> >  /*
> >   * Read the status register, returning its value in the location
> >   * Return the status register value.
> > @@ -1202,7 +1200,42 @@ static int spi_nor_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len)
> >  	return ret;
> >  }
> >  
> > -static int macronix_quad_enable(struct spi_nor *nor);
> > +/**
> > + * macronix_quad_enable() - set QE bit in Status Register.
> > + * @nor:	pointer to a 'struct spi_nor'
> > + *
> > + * Set the Quad Enable (QE) bit in the Status Register.
> > + *
> > + * bit 6 of the Status Register is the QE bit for Macronix like QSPI memories.
> > + *
> > + * Return: 0 on success, -errno otherwise.
> > + */
> > +static int macronix_quad_enable(struct spi_nor *nor)
> > +{
> > +	int ret, val;
> > +
> > +	val = read_sr(nor);
> > +	if (val < 0)
> > +		return val;
> > +	if (val & SR_QUAD_EN_MX)
> > +		return 0;
> > +
> > +	write_enable(nor);
> > +
> > +	write_sr(nor, val | SR_QUAD_EN_MX);
> > +
> > +	ret = spi_nor_wait_till_ready(nor);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = read_sr(nor);
> > +	if (!(ret > 0 && (ret & SR_QUAD_EN_MX))) {
> > +		dev_err(nor->dev, "Macronix Quad bit not set\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}  
> 
> The *_quad_enable() functions are now spread in the file. Would it make sense to
> move all the *_quad_enable() functions, to keep them together?

Yep, will do.

> 
> >  
> >  /* Used when the "_ext_id" is two bytes at most */
> >  #define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags)	\
> > @@ -1778,43 +1811,6 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
> >  	return ret;
> >  }
> >  
> > -/**
> > - * macronix_quad_enable() - set QE bit in Status Register.
> > - * @nor:	pointer to a 'struct spi_nor'
> > - *
> > - * Set the Quad Enable (QE) bit in the Status Register.
> > - *
> > - * bit 6 of the Status Register is the QE bit for Macronix like QSPI memories.
> > - *
> > - * Return: 0 on success, -errno otherwise.
> > - */
> > -static int macronix_quad_enable(struct spi_nor *nor)
> > -{
> > -	int ret, val;
> > -
> > -	val = read_sr(nor);
> > -	if (val < 0)
> > -		return val;
> > -	if (val & SR_QUAD_EN_MX)
> > -		return 0;
> > -
> > -	write_enable(nor);
> > -
> > -	write_sr(nor, val | SR_QUAD_EN_MX);
> > -
> > -	ret = spi_nor_wait_till_ready(nor);
> > -	if (ret)
> > -		return ret;
> > -
> > -	ret = read_sr(nor);
> > -	if (!(ret > 0 && (ret & SR_QUAD_EN_MX))) {
> > -		dev_err(nor->dev, "Macronix Quad bit not set\n");
> > -		return -EINVAL;
> > -	}
> > -
> > -	return 0;
> > -}
> > -
> >  /*
> >   * Write status Register and configuration register with 2 bytes
> >   * The first byte will be written to the status register, while the
> > @@ -2479,7 +2475,56 @@ static const struct sfdp_bfpt_erase sfdp_bfpt_erases[] = {
> >  	{BFPT_DWORD(9), 16},
> >  };
> >  
> > -static int spi_nor_hwcaps_read2cmd(u32 hwcaps);
> > +static int spi_nor_hwcaps2cmd(u32 hwcaps, const int table[][2], size_t size)
> > +{
> > +	size_t i;
> > +
> > +	for (i = 0; i < size; i++)
> > +		if (table[i][0] == (int)hwcaps)
> > +			return table[i][1];
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static int spi_nor_hwcaps_read2cmd(u32 hwcaps)
> > +{
> > +	static const int hwcaps_read2cmd[][2] = {
> > +		{ SNOR_HWCAPS_READ,		SNOR_CMD_READ },
> > +		{ SNOR_HWCAPS_READ_FAST,	SNOR_CMD_READ_FAST },
> > +		{ SNOR_HWCAPS_READ_1_1_1_DTR,	SNOR_CMD_READ_1_1_1_DTR },
> > +		{ SNOR_HWCAPS_READ_1_1_2,	SNOR_CMD_READ_1_1_2 },
> > +		{ SNOR_HWCAPS_READ_1_2_2,	SNOR_CMD_READ_1_2_2 },
> > +		{ SNOR_HWCAPS_READ_2_2_2,	SNOR_CMD_READ_2_2_2 },
> > +		{ SNOR_HWCAPS_READ_1_2_2_DTR,	SNOR_CMD_READ_1_2_2_DTR },
> > +		{ SNOR_HWCAPS_READ_1_1_4,	SNOR_CMD_READ_1_1_4 },
> > +		{ SNOR_HWCAPS_READ_1_4_4,	SNOR_CMD_READ_1_4_4 },
> > +		{ SNOR_HWCAPS_READ_4_4_4,	SNOR_CMD_READ_4_4_4 },
> > +		{ SNOR_HWCAPS_READ_1_4_4_DTR,	SNOR_CMD_READ_1_4_4_DTR },
> > +		{ SNOR_HWCAPS_READ_1_1_8,	SNOR_CMD_READ_1_1_8 },
> > +		{ SNOR_HWCAPS_READ_1_8_8,	SNOR_CMD_READ_1_8_8 },
> > +		{ SNOR_HWCAPS_READ_8_8_8,	SNOR_CMD_READ_8_8_8 },
> > +		{ SNOR_HWCAPS_READ_1_8_8_DTR,	SNOR_CMD_READ_1_8_8_DTR },
> > +	};
> > +
> > +	return spi_nor_hwcaps2cmd(hwcaps, hwcaps_read2cmd,
> > +				  ARRAY_SIZE(hwcaps_read2cmd));
> > +}
> > +
> > +static int spi_nor_hwcaps_pp2cmd(u32 hwcaps)
> > +{
> > +	static const int hwcaps_pp2cmd[][2] = {
> > +		{ SNOR_HWCAPS_PP,		SNOR_CMD_PP },
> > +		{ SNOR_HWCAPS_PP_1_1_4,		SNOR_CMD_PP_1_1_4 },
> > +		{ SNOR_HWCAPS_PP_1_4_4,		SNOR_CMD_PP_1_4_4 },
> > +		{ SNOR_HWCAPS_PP_4_4_4,		SNOR_CMD_PP_4_4_4 },
> > +		{ SNOR_HWCAPS_PP_1_1_8,		SNOR_CMD_PP_1_1_8 },
> > +		{ SNOR_HWCAPS_PP_1_8_8,		SNOR_CMD_PP_1_8_8 },
> > +		{ SNOR_HWCAPS_PP_8_8_8,		SNOR_CMD_PP_8_8_8 },
> > +	};
> > +
> > +	return spi_nor_hwcaps2cmd(hwcaps, hwcaps_pp2cmd,
> > +				  ARRAY_SIZE(hwcaps_pp2cmd));
> > +}  
> 
> Would it be a better place to put these immediately after
> spi_nor_set_pp_settings()? It would be consistent with how they were declared
> back in cfc5604c488ccd17936b69008af0c9ae050f4a08.

I thought it would be preferable to have the xx2cmd[] conversion tables
grouped together, but I can move this one next to
spi_nor_set_pp_settings() if you prefer.

Re: [PATCH v2 2/6] mtd: spi-nor: Avoid forward declaration of internal functions

[Tudor.Ambarus]

Hi,

On 12/05/2018 10:09 AM, Boris Brezillon wrote:
[cut]

>>> -static int spi_nor_hwcaps_read2cmd(u32 hwcaps);
>>> +static int spi_nor_hwcaps2cmd(u32 hwcaps, const int table[][2], size_t size)
>>> +{
>>> +	size_t i;
>>> +
>>> +	for (i = 0; i < size; i++)
>>> +		if (table[i][0] == (int)hwcaps)
>>> +			return table[i][1];
>>> +
>>> +	return -EINVAL;
>>> +}
>>> +
>>> +static int spi_nor_hwcaps_read2cmd(u32 hwcaps)
>>> +{
>>> +	static const int hwcaps_read2cmd[][2] = {
>>> +		{ SNOR_HWCAPS_READ,		SNOR_CMD_READ },
>>> +		{ SNOR_HWCAPS_READ_FAST,	SNOR_CMD_READ_FAST },
>>> +		{ SNOR_HWCAPS_READ_1_1_1_DTR,	SNOR_CMD_READ_1_1_1_DTR },
>>> +		{ SNOR_HWCAPS_READ_1_1_2,	SNOR_CMD_READ_1_1_2 },
>>> +		{ SNOR_HWCAPS_READ_1_2_2,	SNOR_CMD_READ_1_2_2 },
>>> +		{ SNOR_HWCAPS_READ_2_2_2,	SNOR_CMD_READ_2_2_2 },
>>> +		{ SNOR_HWCAPS_READ_1_2_2_DTR,	SNOR_CMD_READ_1_2_2_DTR },
>>> +		{ SNOR_HWCAPS_READ_1_1_4,	SNOR_CMD_READ_1_1_4 },
>>> +		{ SNOR_HWCAPS_READ_1_4_4,	SNOR_CMD_READ_1_4_4 },
>>> +		{ SNOR_HWCAPS_READ_4_4_4,	SNOR_CMD_READ_4_4_4 },
>>> +		{ SNOR_HWCAPS_READ_1_4_4_DTR,	SNOR_CMD_READ_1_4_4_DTR },
>>> +		{ SNOR_HWCAPS_READ_1_1_8,	SNOR_CMD_READ_1_1_8 },
>>> +		{ SNOR_HWCAPS_READ_1_8_8,	SNOR_CMD_READ_1_8_8 },
>>> +		{ SNOR_HWCAPS_READ_8_8_8,	SNOR_CMD_READ_8_8_8 },
>>> +		{ SNOR_HWCAPS_READ_1_8_8_DTR,	SNOR_CMD_READ_1_8_8_DTR },
>>> +	};
>>> +
>>> +	return spi_nor_hwcaps2cmd(hwcaps, hwcaps_read2cmd,
>>> +				  ARRAY_SIZE(hwcaps_read2cmd));
>>> +}
>>> +
>>> +static int spi_nor_hwcaps_pp2cmd(u32 hwcaps)
>>> +{
>>> +	static const int hwcaps_pp2cmd[][2] = {
>>> +		{ SNOR_HWCAPS_PP,		SNOR_CMD_PP },
>>> +		{ SNOR_HWCAPS_PP_1_1_4,		SNOR_CMD_PP_1_1_4 },
>>> +		{ SNOR_HWCAPS_PP_1_4_4,		SNOR_CMD_PP_1_4_4 },
>>> +		{ SNOR_HWCAPS_PP_4_4_4,		SNOR_CMD_PP_4_4_4 },
>>> +		{ SNOR_HWCAPS_PP_1_1_8,		SNOR_CMD_PP_1_1_8 },
>>> +		{ SNOR_HWCAPS_PP_1_8_8,		SNOR_CMD_PP_1_8_8 },
>>> +		{ SNOR_HWCAPS_PP_8_8_8,		SNOR_CMD_PP_8_8_8 },
>>> +	};
>>> +
>>> +	return spi_nor_hwcaps2cmd(hwcaps, hwcaps_pp2cmd,
>>> +				  ARRAY_SIZE(hwcaps_pp2cmd));
>>> +}  
>>
>> Would it be a better place to put these immediately after
>> spi_nor_set_pp_settings()? It would be consistent with how they were declared
>> back in cfc5604c488ccd17936b69008af0c9ae050f4a08.
> 
> I thought it would be preferable to have the xx2cmd[] conversion tables
> grouped together, but I can move this one next to
> spi_nor_set_pp_settings() if you prefer.
> 

I thought of moving the entire chunk (_hwcaps2cmd(), _read2cmd(), _pp2cmd())
just after spi_nor_set_pp_settings(). You have in that section of code the
spi_nor_read/pp_command_index enums followed by spi_nor_set_read/pp_settings().
We can have all the read/pp stuff grouped together.

Cheers,
ta

[PATCH v2 3/6] mtd: spi-nor: Stop passing info to set_4byte()

[Boris Brezillon]

info can be extracted from nor->info, no need to pass it as an
argument.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
Changes in v2:
- None
---
 drivers/mtd/spi-nor/spi-nor.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index e1eaabf98d08..6da50684f303 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -274,14 +274,13 @@ static void spi_nor_set_4byte_opcodes(struct spi_nor *nor,
 }
 
 /* Enable/disable 4-byte addressing mode. */
-static int set_4byte(struct spi_nor *nor, const struct flash_info *info,
-		     int enable)
+static int set_4byte(struct spi_nor *nor, int enable)
 {
 	int status;
 	bool need_wren = false;
 	u8 cmd;
 
-	switch (JEDEC_MFR(info)) {
+	switch (JEDEC_MFR(nor->info)) {
 	case SNOR_MFR_ST:
 	case SNOR_MFR_MICRON:
 		/* Some Micron need WREN command; all will accept it */
@@ -298,7 +297,7 @@ static int set_4byte(struct spi_nor *nor, const struct flash_info *info,
 			write_disable(nor);
 
 		if (!status && !enable &&
-		    JEDEC_MFR(info) == SNOR_MFR_WINBOND) {
+		    JEDEC_MFR(nor->info) == SNOR_MFR_WINBOND) {
 			/*
 			 * On Winbond W25Q256FV, leaving 4byte mode causes
 			 * the Extended Address Register to be set to 1, so all
@@ -3574,7 +3573,7 @@ static int spi_nor_init(struct spi_nor *nor)
 		 */
 		WARN_ONCE(nor->flags & SNOR_F_BROKEN_RESET,
 			  "enabling reset hack; may not recover from unexpected reboots\n");
-		set_4byte(nor, nor->info, 1);
+		set_4byte(nor, 1);
 	}
 
 	return 0;
@@ -3600,7 +3599,7 @@ void spi_nor_restore(struct spi_nor *nor)
 	    (JEDEC_MFR(nor->info) != SNOR_MFR_SPANSION) &&
 	    !(nor->info->flags & SPI_NOR_4B_OPCODES) &&
 	    (nor->flags & SNOR_F_BROKEN_RESET))
-		set_4byte(nor, nor->info, 0);
+		set_4byte(nor, 0);
 }
 EXPORT_SYMBOL_GPL(spi_nor_restore);
 
-- 
2.17.1

Re: [PATCH v2 3/6] mtd: spi-nor: Stop passing info to set_4byte()

[Sverdlin, Alexander (Nokia - DE/Ulm)]

On 29/11/2018 15:10, Boris Brezillon wrote:
> info can be extracted from nor->info, no need to pass it as an
> argument.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>

Reviewed-by: Aleaxander Sverdlin <alexander.sverdlin@nokia.com>

> ---
> Changes in v2:
> - None
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index e1eaabf98d08..6da50684f303 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -274,14 +274,13 @@ static void spi_nor_set_4byte_opcodes(struct spi_nor *nor,
>  }
>  
>  /* Enable/disable 4-byte addressing mode. */
> -static int set_4byte(struct spi_nor *nor, const struct flash_info *info,
> -		     int enable)
> +static int set_4byte(struct spi_nor *nor, int enable)
>  {
>  	int status;
>  	bool need_wren = false;
>  	u8 cmd;
>  
> -	switch (JEDEC_MFR(info)) {
> +	switch (JEDEC_MFR(nor->info)) {
>  	case SNOR_MFR_ST:
>  	case SNOR_MFR_MICRON:
>  		/* Some Micron need WREN command; all will accept it */
> @@ -298,7 +297,7 @@ static int set_4byte(struct spi_nor *nor, const struct flash_info *info,
>  			write_disable(nor);
>  
>  		if (!status && !enable &&
> -		    JEDEC_MFR(info) == SNOR_MFR_WINBOND) {
> +		    JEDEC_MFR(nor->info) == SNOR_MFR_WINBOND) {
>  			/*
>  			 * On Winbond W25Q256FV, leaving 4byte mode causes
>  			 * the Extended Address Register to be set to 1, so all
> @@ -3574,7 +3573,7 @@ static int spi_nor_init(struct spi_nor *nor)
>  		 */
>  		WARN_ONCE(nor->flags & SNOR_F_BROKEN_RESET,
>  			  "enabling reset hack; may not recover from unexpected reboots\n");
> -		set_4byte(nor, nor->info, 1);
> +		set_4byte(nor, 1);
>  	}
>  
>  	return 0;
> @@ -3600,7 +3599,7 @@ void spi_nor_restore(struct spi_nor *nor)
>  	    (JEDEC_MFR(nor->info) != SNOR_MFR_SPANSION) &&
>  	    !(nor->info->flags & SPI_NOR_4B_OPCODES) &&
>  	    (nor->flags & SNOR_F_BROKEN_RESET))
> -		set_4byte(nor, nor->info, 0);
> +		set_4byte(nor, 0);
>  }
>  EXPORT_SYMBOL_GPL(spi_nor_restore);

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH v2 3/6] mtd: spi-nor: Stop passing info to set_4byte()

[Tudor.Ambarus]

On 11/29/2018 04:10 PM, Boris Brezillon wrote:
> info can be extracted from nor->info, no need to pass it as an
> argument.

Nice! If you move the assignment nor->info = info; earlier in spi_nor_scan(),
you'll be able to spare more functions to pass info as argument.

Cheers,
ta

Re: [PATCH v2 3/6] mtd: spi-nor: Stop passing info to set_4byte()

[Boris Brezillon]

On Tue, 4 Dec 2018 19:35:15 +0000
<Tudor.Ambarus@microchip.com> wrote:

> On 11/29/2018 04:10 PM, Boris Brezillon wrote:
> > info can be extracted from nor->info, no need to pass it as an
> > argument.  
> 
> Nice! If you move the assignment nor->info = info; earlier in spi_nor_scan(),
> you'll be able to spare more functions to pass info as argument.

You mean like in patch 6? ;)

[PATCH v2 4/6] mtd: spi-nor: Make the enable argument passed to set_byte() a bool

[Boris Brezillon]

No need to use an integer when the value is either true or false.
Make it a boolean.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
Changes in v2:
- None
---
 drivers/mtd/spi-nor/spi-nor.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 6da50684f303..ed1d7ad2dcbb 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -274,7 +274,7 @@ static void spi_nor_set_4byte_opcodes(struct spi_nor *nor,
 }
 
 /* Enable/disable 4-byte addressing mode. */
-static int set_4byte(struct spi_nor *nor, int enable)
+static int set_4byte(struct spi_nor *nor, bool enable)
 {
 	int status;
 	bool need_wren = false;
@@ -3573,7 +3573,7 @@ static int spi_nor_init(struct spi_nor *nor)
 		 */
 		WARN_ONCE(nor->flags & SNOR_F_BROKEN_RESET,
 			  "enabling reset hack; may not recover from unexpected reboots\n");
-		set_4byte(nor, 1);
+		set_4byte(nor, true);
 	}
 
 	return 0;
@@ -3599,7 +3599,7 @@ void spi_nor_restore(struct spi_nor *nor)
 	    (JEDEC_MFR(nor->info) != SNOR_MFR_SPANSION) &&
 	    !(nor->info->flags & SPI_NOR_4B_OPCODES) &&
 	    (nor->flags & SNOR_F_BROKEN_RESET))
-		set_4byte(nor, 0);
+		set_4byte(nor, false);
 }
 EXPORT_SYMBOL_GPL(spi_nor_restore);
 
-- 
2.17.1

Re: [PATCH v2 4/6] mtd: spi-nor: Make the enable argument passed to set_byte() a bool

[Tudor.Ambarus]

On 11/29/2018 04:10 PM, Boris Brezillon wrote:
> No need to use an integer when the value is either true or false.
> Make it a boolean.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>

Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

> ---
> Changes in v2:
> - None
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 6da50684f303..ed1d7ad2dcbb 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -274,7 +274,7 @@ static void spi_nor_set_4byte_opcodes(struct spi_nor *nor,
>  }
>  
>  /* Enable/disable 4-byte addressing mode. */
> -static int set_4byte(struct spi_nor *nor, int enable)
> +static int set_4byte(struct spi_nor *nor, bool enable)
>  {
>  	int status;
>  	bool need_wren = false;
> @@ -3573,7 +3573,7 @@ static int spi_nor_init(struct spi_nor *nor)
>  		 */
>  		WARN_ONCE(nor->flags & SNOR_F_BROKEN_RESET,
>  			  "enabling reset hack; may not recover from unexpected reboots\n");
> -		set_4byte(nor, 1);
> +		set_4byte(nor, true);
>  	}
>  
>  	return 0;
> @@ -3599,7 +3599,7 @@ void spi_nor_restore(struct spi_nor *nor)
>  	    (JEDEC_MFR(nor->info) != SNOR_MFR_SPANSION) &&
>  	    !(nor->info->flags & SPI_NOR_4B_OPCODES) &&
>  	    (nor->flags & SNOR_F_BROKEN_RESET))
> -		set_4byte(nor, 0);
> +		set_4byte(nor, false);
>  }
>  EXPORT_SYMBOL_GPL(spi_nor_restore);
>  
> 

[PATCH v2 5/6] mtd: spi-nor: Add an SPDX tag to spi-nor.c

[Boris Brezillon]

Add an SPDX tag to replace the license boiler-plate and fix the
MODULE_LICENSE() definition to match the the license (GPL v2).

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
Changes in v2:
- New patch
---
 drivers/mtd/spi-nor/spi-nor.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index ed1d7ad2dcbb..d2b09f52b1fb 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1,13 +1,10 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * Based on m25p80.c, by Mike Lavender (mike@steroidmicros.com), with
  * influence from lart.c (Abraham Van Der Merwe) and mtd_dataflash.c
  *
  * Copyright (C) 2005, Intec Automation Inc.
  * Copyright (C) 2014, Freescale Semiconductor, Inc.
- *
- * This code is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
  */
 
 #include <linux/err.h>
@@ -3814,7 +3811,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 }
 EXPORT_SYMBOL_GPL(spi_nor_scan);
 
-MODULE_LICENSE("GPL");
+MODULE_LICENSE("GPL v2");
 MODULE_AUTHOR("Huang Shijie <shijie8@gmail.com>");
 MODULE_AUTHOR("Mike Lavender");
 MODULE_DESCRIPTION("framework for SPI NOR");
-- 
2.17.1

Re: [PATCH v2 5/6] mtd: spi-nor: Add an SPDX tag to spi-nor.c

[Sverdlin, Alexander (Nokia - DE/Ulm)]

On 29/11/2018 15:10, Boris Brezillon wrote:
> Add an SPDX tag to replace the license boiler-plate and fix the
> MODULE_LICENSE() definition to match the the license (GPL v2).
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>

Reviewed-by: Aleaxander Sverdlin <alexander.sverdlin@nokia.com>

> ---
> Changes in v2:
> - New patch
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index ed1d7ad2dcbb..d2b09f52b1fb 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -1,13 +1,10 @@
> +// SPDX-License-Identifier: GPL-2.0
>  /*
>   * Based on m25p80.c, by Mike Lavender (mike@steroidmicros.com), with
>   * influence from lart.c (Abraham Van Der Merwe) and mtd_dataflash.c
>   *
>   * Copyright (C) 2005, Intec Automation Inc.
>   * Copyright (C) 2014, Freescale Semiconductor, Inc.
> - *
> - * This code is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
>   */
>  
>  #include <linux/err.h>
> @@ -3814,7 +3811,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>  }
>  EXPORT_SYMBOL_GPL(spi_nor_scan);
>  
> -MODULE_LICENSE("GPL");
> +MODULE_LICENSE("GPL v2");
>  MODULE_AUTHOR("Huang Shijie <shijie8@gmail.com>");
>  MODULE_AUTHOR("Mike Lavender");
>  MODULE_DESCRIPTION("framework for SPI NOR");

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH v2 5/6] mtd: spi-nor: Add an SPDX tag to spi-nor.c

[Tudor.Ambarus]

On 11/29/2018 04:10 PM, Boris Brezillon wrote:
> Add an SPDX tag to replace the license boiler-plate and fix the
> MODULE_LICENSE() definition to match the the license (GPL v2).
> 

Can we add the spdx tag for include/linux/mtd/spi-nor.h in the same patch?

Cheers,
ta

Re: [PATCH v2 5/6] mtd: spi-nor: Add an SPDX tag to spi-nor.c

[Boris Brezillon]

On Tue, 4 Dec 2018 19:45:51 +0000
<Tudor.Ambarus@microchip.com> wrote:

> On 11/29/2018 04:10 PM, Boris Brezillon wrote:
> > Add an SPDX tag to replace the license boiler-plate and fix the
> > MODULE_LICENSE() definition to match the the license (GPL v2).
> >   
> 
> Can we add the spdx tag for include/linux/mtd/spi-nor.h in the same patch?

Absolutely. I'll fix that.

[PATCH v2 6/6] mtd: spi-nor: Move the nor->info assignment earlier in the scan func

[Boris Brezillon]

Some functions called from spi_nor_scan() need a flash_info object.
Let's assign nor->info early on to avoid passing info as an extra
argument to each of these sub-functions.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
Changes in v2:
- New patch
---
 drivers/mtd/spi-nor/spi-nor.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index d2b09f52b1fb..c1d9c2e50bee 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -238,9 +238,10 @@ static u8 spi_nor_convert_3to4_erase(u8 opcode)
 				      ARRAY_SIZE(spi_nor_3to4_erase));
 }
 
-static void spi_nor_set_4byte_opcodes(struct spi_nor *nor,
-				      const struct flash_info *info)
+static void spi_nor_set_4byte_opcodes(struct spi_nor *nor)
 {
+	const struct flash_info *info = nor->info;
+
 	/* Do some manufacturer fixups first */
 	switch (JEDEC_MFR(info)) {
 	case SNOR_MFR_SPANSION:
@@ -2023,8 +2024,9 @@ static int spi_nor_check(struct spi_nor *nor)
 	return 0;
 }
 
-static int s3an_nor_scan(const struct flash_info *info, struct spi_nor *nor)
+static int s3an_nor_scan(struct spi_nor *nor)
 {
+	const struct flash_info *info = nor->info;
 	int ret;
 	u8 val;
 
@@ -3204,10 +3206,10 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
 }
 
 static int spi_nor_init_params(struct spi_nor *nor,
-			       const struct flash_info *info,
 			       struct spi_nor_flash_parameter *params)
 {
 	struct spi_nor_erase_map *map = &nor->erase_map;
+	const struct flash_info *info = nor->info;
 	u8 i, erase_mask;
 
 	/* Set legacy flash parameters as default. */
@@ -3472,10 +3474,11 @@ static int spi_nor_select_erase(struct spi_nor *nor, u32 wanted_size)
 	return 0;
 }
 
-static int spi_nor_setup(struct spi_nor *nor, const struct flash_info *info,
+static int spi_nor_setup(struct spi_nor *nor,
 			 const struct spi_nor_flash_parameter *params,
 			 const struct spi_nor_hwcaps *hwcaps)
 {
+	const struct flash_info *info = nor->info;
 	u32 ignored_mask, shared_mask;
 	bool enable_quad_io;
 	int err;
@@ -3664,6 +3667,8 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 		}
 	}
 
+	nor->info = info;
+
 	mutex_init(&nor->lock);
 
 	/*
@@ -3675,7 +3680,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 		nor->flags |=  SNOR_F_READY_XSR_RDY;
 
 	/* Parse the Serial Flash Discoverable Parameters table. */
-	ret = spi_nor_init_params(nor, info, &params);
+	ret = spi_nor_init_params(nor, &params);
 	if (ret)
 		return ret;
 
@@ -3752,7 +3757,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 	 * - set the SPI protocols for register and memory accesses.
 	 * - set the Quad Enable bit if needed (required by SPI x-y-4 protos).
 	 */
-	ret = spi_nor_setup(nor, info, &params, hwcaps);
+	ret = spi_nor_setup(nor, &params, hwcaps);
 	if (ret)
 		return ret;
 
@@ -3765,7 +3770,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 		nor->addr_width = 4;
 		if (JEDEC_MFR(info) == SNOR_MFR_SPANSION ||
 		    info->flags & SPI_NOR_4B_OPCODES)
-			spi_nor_set_4byte_opcodes(nor, info);
+			spi_nor_set_4byte_opcodes(nor);
 	} else {
 		nor->addr_width = 3;
 	}
@@ -3777,13 +3782,12 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 	}
 
 	if (info->flags & SPI_S3AN) {
-		ret = s3an_nor_scan(info, nor);
+		ret = s3an_nor_scan(nor);
 		if (ret)
 			return ret;
 	}
 
 	/* Send all the required SPI flash commands to initialize device */
-	nor->info = info;
 	ret = spi_nor_init(nor);
 	if (ret)
 		return ret;
-- 
2.17.1

Re: [PATCH v2 6/6] mtd: spi-nor: Move the nor->info assignment earlier in the scan func

[Sverdlin, Alexander (Nokia - DE/Ulm)]

On 29/11/2018 15:10, Boris Brezillon wrote:
> Some functions called from spi_nor_scan() need a flash_info object.
> Let's assign nor->info early on to avoid passing info as an extra
> argument to each of these sub-functions.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>

Reviewed-by: Aleaxander Sverdlin <alexander.sverdlin@nokia.com>

> ---
> Changes in v2:
> - New patch
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 24 ++++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index d2b09f52b1fb..c1d9c2e50bee 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -238,9 +238,10 @@ static u8 spi_nor_convert_3to4_erase(u8 opcode)
>  				      ARRAY_SIZE(spi_nor_3to4_erase));
>  }
>  
> -static void spi_nor_set_4byte_opcodes(struct spi_nor *nor,
> -				      const struct flash_info *info)
> +static void spi_nor_set_4byte_opcodes(struct spi_nor *nor)
>  {
> +	const struct flash_info *info = nor->info;
> +
>  	/* Do some manufacturer fixups first */
>  	switch (JEDEC_MFR(info)) {
>  	case SNOR_MFR_SPANSION:
> @@ -2023,8 +2024,9 @@ static int spi_nor_check(struct spi_nor *nor)
>  	return 0;
>  }
>  
> -static int s3an_nor_scan(const struct flash_info *info, struct spi_nor *nor)
> +static int s3an_nor_scan(struct spi_nor *nor)
>  {
> +	const struct flash_info *info = nor->info;
>  	int ret;
>  	u8 val;
>  
> @@ -3204,10 +3206,10 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
>  }
>  
>  static int spi_nor_init_params(struct spi_nor *nor,
> -			       const struct flash_info *info,
>  			       struct spi_nor_flash_parameter *params)
>  {
>  	struct spi_nor_erase_map *map = &nor->erase_map;
> +	const struct flash_info *info = nor->info;
>  	u8 i, erase_mask;
>  
>  	/* Set legacy flash parameters as default. */
> @@ -3472,10 +3474,11 @@ static int spi_nor_select_erase(struct spi_nor *nor, u32 wanted_size)
>  	return 0;
>  }
>  
> -static int spi_nor_setup(struct spi_nor *nor, const struct flash_info *info,
> +static int spi_nor_setup(struct spi_nor *nor,
>  			 const struct spi_nor_flash_parameter *params,
>  			 const struct spi_nor_hwcaps *hwcaps)
>  {
> +	const struct flash_info *info = nor->info;
>  	u32 ignored_mask, shared_mask;
>  	bool enable_quad_io;
>  	int err;
> @@ -3664,6 +3667,8 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>  		}
>  	}
>  
> +	nor->info = info;
> +
>  	mutex_init(&nor->lock);
>  
>  	/*
> @@ -3675,7 +3680,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>  		nor->flags |=  SNOR_F_READY_XSR_RDY;
>  
>  	/* Parse the Serial Flash Discoverable Parameters table. */
> -	ret = spi_nor_init_params(nor, info, &params);
> +	ret = spi_nor_init_params(nor, &params);
>  	if (ret)
>  		return ret;
>  
> @@ -3752,7 +3757,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>  	 * - set the SPI protocols for register and memory accesses.
>  	 * - set the Quad Enable bit if needed (required by SPI x-y-4 protos).
>  	 */
> -	ret = spi_nor_setup(nor, info, &params, hwcaps);
> +	ret = spi_nor_setup(nor, &params, hwcaps);
>  	if (ret)
>  		return ret;
>  
> @@ -3765,7 +3770,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>  		nor->addr_width = 4;
>  		if (JEDEC_MFR(info) == SNOR_MFR_SPANSION ||
>  		    info->flags & SPI_NOR_4B_OPCODES)
> -			spi_nor_set_4byte_opcodes(nor, info);
> +			spi_nor_set_4byte_opcodes(nor);
>  	} else {
>  		nor->addr_width = 3;
>  	}
> @@ -3777,13 +3782,12 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>  	}
>  
>  	if (info->flags & SPI_S3AN) {
> -		ret = s3an_nor_scan(info, nor);
> +		ret = s3an_nor_scan(nor);
>  		if (ret)
>  			return ret;
>  	}
>  
>  	/* Send all the required SPI flash commands to initialize device */
> -	nor->info = info;
>  	ret = spi_nor_init(nor);
>  	if (ret)
>  		return ret;

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH v2 6/6] mtd: spi-nor: Move the nor->info assignment earlier in the scan func

[Tudor.Ambarus]

Hi, Alexander,

On 11/29/2018 06:35 PM, Sverdlin, Alexander (Nokia - DE/Ulm) wrote:
> Reviewed-by: Aleaxander Sverdlin <alexander.sverdlin@nokia.com>

there's a typo in your name, here and in all the other rb tags from this patch set.

Cheers,
ta

Re: [PATCH v2 6/6] mtd: spi-nor: Move the nor->info assignment earlier in the scan func

[Tudor.Ambarus]

On 11/29/2018 04:10 PM, Boris Brezillon wrote:
> Some functions called from spi_nor_scan() need a flash_info object.
> Let's assign nor->info early on to avoid passing info as an extra
> argument to each of these sub-functions.

Why not to squash it with patch 3?

> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
> Changes in v2:
> - New patch
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 24 ++++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index d2b09f52b1fb..c1d9c2e50bee 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -238,9 +238,10 @@ static u8 spi_nor_convert_3to4_erase(u8 opcode)
>  				      ARRAY_SIZE(spi_nor_3to4_erase));
>  }
>  
> -static void spi_nor_set_4byte_opcodes(struct spi_nor *nor,
> -				      const struct flash_info *info)
> +static void spi_nor_set_4byte_opcodes(struct spi_nor *nor)
>  {
> +	const struct flash_info *info = nor->info;
> +
>  	/* Do some manufacturer fixups first */
>  	switch (JEDEC_MFR(info)) {
>  	case SNOR_MFR_SPANSION:
> @@ -2023,8 +2024,9 @@ static int spi_nor_check(struct spi_nor *nor)
>  	return 0;
>  }
>  
> -static int s3an_nor_scan(const struct flash_info *info, struct spi_nor *nor)
> +static int s3an_nor_scan(struct spi_nor *nor)
>  {
> +	const struct flash_info *info = nor->info;

info is used in this function just to get info->n_sectors. We can dereference
n_sectors directly.

>  	int ret;
>  	u8 val;
>  
> @@ -3204,10 +3206,10 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
>  }
>  
>  static int spi_nor_init_params(struct spi_nor *nor,
> -			       const struct flash_info *info,
>  			       struct spi_nor_flash_parameter *params)
>  {
>  	struct spi_nor_erase_map *map = &nor->erase_map;
> +	const struct flash_info *info = nor->info;
>  	u8 i, erase_mask;
>  
>  	/* Set legacy flash parameters as default. */
> @@ -3472,10 +3474,11 @@ static int spi_nor_select_erase(struct spi_nor *nor, u32 wanted_size)
>  	return 0;
>  }
>  
> -static int spi_nor_setup(struct spi_nor *nor, const struct flash_info *info,
> +static int spi_nor_setup(struct spi_nor *nor,
>  			 const struct spi_nor_flash_parameter *params,
>  			 const struct spi_nor_hwcaps *hwcaps)
>  {
> +	const struct flash_info *info = nor->info;

info is used in this function just to pass the info->sector_size to
spi_nor_select_erase(). We can dereference sector_size directly,  without even
declaring a local variable.

Looks good. Cheers,
ta

Re: [PATCH v2 6/6] mtd: spi-nor: Move the nor->info assignment earlier in the scan func

[Boris Brezillon]

On Tue, 4 Dec 2018 20:21:36 +0000
<Tudor.Ambarus@microchip.com> wrote:

> On 11/29/2018 04:10 PM, Boris Brezillon wrote:
> > Some functions called from spi_nor_scan() need a flash_info object.
> > Let's assign nor->info early on to avoid passing info as an extra
> > argument to each of these sub-functions.  
> 
> Why not to squash it with patch 3?

Makes sense. I'll squash this patch in patch 3 and reword the commit
message accordingly.

> 
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > ---
> > Changes in v2:
> > - New patch
> > ---
> >  drivers/mtd/spi-nor/spi-nor.c | 24 ++++++++++++++----------
> >  1 file changed, 14 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> > index d2b09f52b1fb..c1d9c2e50bee 100644
> > --- a/drivers/mtd/spi-nor/spi-nor.c
> > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > @@ -238,9 +238,10 @@ static u8 spi_nor_convert_3to4_erase(u8 opcode)
> >  				      ARRAY_SIZE(spi_nor_3to4_erase));
> >  }
> >  
> > -static void spi_nor_set_4byte_opcodes(struct spi_nor *nor,
> > -				      const struct flash_info *info)
> > +static void spi_nor_set_4byte_opcodes(struct spi_nor *nor)
> >  {
> > +	const struct flash_info *info = nor->info;
> > +
> >  	/* Do some manufacturer fixups first */
> >  	switch (JEDEC_MFR(info)) {
> >  	case SNOR_MFR_SPANSION:
> > @@ -2023,8 +2024,9 @@ static int spi_nor_check(struct spi_nor *nor)
> >  	return 0;
> >  }
> >  
> > -static int s3an_nor_scan(const struct flash_info *info, struct spi_nor *nor)
> > +static int s3an_nor_scan(struct spi_nor *nor)
> >  {
> > +	const struct flash_info *info = nor->info;  
> 
> info is used in this function just to get info->n_sectors. We can dereference
> n_sectors directly.

Sure.

> 
> >  	int ret;
> >  	u8 val;
> >  
> > @@ -3204,10 +3206,10 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
> >  }
> >  
> >  static int spi_nor_init_params(struct spi_nor *nor,
> > -			       const struct flash_info *info,
> >  			       struct spi_nor_flash_parameter *params)
> >  {
> >  	struct spi_nor_erase_map *map = &nor->erase_map;
> > +	const struct flash_info *info = nor->info;
> >  	u8 i, erase_mask;
> >  
> >  	/* Set legacy flash parameters as default. */
> > @@ -3472,10 +3474,11 @@ static int spi_nor_select_erase(struct spi_nor *nor, u32 wanted_size)
> >  	return 0;
> >  }
> >  
> > -static int spi_nor_setup(struct spi_nor *nor, const struct flash_info *info,
> > +static int spi_nor_setup(struct spi_nor *nor,
> >  			 const struct spi_nor_flash_parameter *params,
> >  			 const struct spi_nor_hwcaps *hwcaps)
> >  {
> > +	const struct flash_info *info = nor->info;  
> 
> info is used in this function just to pass the info->sector_size to
> spi_nor_select_erase(). We can dereference sector_size directly,  without even
> declaring a local variable.

Ditto.

Thanks for the review.

Boris