2022-02-03 13:32:36

by Michael Walle

[permalink] [raw]
Subject: [PATCH v1 12/14] mtd: spi-nor: move all spansion specifics into spansion.c

The clear status register flags is only available on spansion flashes.
Move all the functions around that into the spanion module.

Signed-off-by: Michael Walle <[email protected]>
---
drivers/mtd/spi-nor/core.c | 52 +------------------------
drivers/mtd/spi-nor/spansion.c | 70 ++++++++++++++++++++++++++++++++++
include/linux/mtd/spi-nor.h | 1 -
3 files changed, 72 insertions(+), 51 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index be65aaa954ca..5b00dfab77a6 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -554,33 +554,6 @@ int spi_nor_write_ear(struct spi_nor *nor, u8 ear)
return ret;
}

-/**
- * spi_nor_clear_sr() - Clear the Status Register.
- * @nor: pointer to 'struct spi_nor'.
- */
-static void spi_nor_clear_sr(struct spi_nor *nor)
-{
- int ret;
-
- if (nor->spimem) {
- struct spi_mem_op op =
- SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_CLSR, 0),
- SPI_MEM_OP_NO_ADDR,
- SPI_MEM_OP_NO_DUMMY,
- SPI_MEM_OP_NO_DATA);
-
- spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
-
- ret = spi_mem_exec_op(nor->spimem, &op);
- } else {
- ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_CLSR,
- NULL, 0);
- }
-
- if (ret)
- dev_dbg(nor->dev, "error %d clearing SR\n", ret);
-}
-
/**
* spi_nor_sr_ready() - Query the Status Register to see if the flash is ready
* for new commands.
@@ -590,33 +563,12 @@ static void spi_nor_clear_sr(struct spi_nor *nor)
*/
int spi_nor_sr_ready(struct spi_nor *nor)
{
- int ret = spi_nor_read_sr(nor, nor->bouncebuf);
+ int ret;

+ ret = spi_nor_read_sr(nor, nor->bouncebuf);
if (ret)
return ret;

- if (nor->flags & SNOR_F_USE_CLSR &&
- nor->bouncebuf[0] & (SR_E_ERR | SR_P_ERR)) {
- if (nor->bouncebuf[0] & SR_E_ERR)
- dev_err(nor->dev, "Erase Error occurred\n");
- else
- dev_err(nor->dev, "Programming Error occurred\n");
-
- spi_nor_clear_sr(nor);
-
- /*
- * WEL bit remains set to one when an erase or page program
- * error occurs. Issue a Write Disable command to protect
- * against inadvertent writes that can possibly corrupt the
- * contents of the memory.
- */
- ret = spi_nor_write_disable(nor);
- if (ret)
- return ret;
-
- return -EIO;
- }
-
return !(nor->bouncebuf[0] & SR_WIP);
}

diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index dedc2de90cb8..4756fb88eab2 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -8,6 +8,7 @@

#include "core.h"

+#define SPINOR_OP_CLSR 0x30 /* Clear status register 1 */
#define SPINOR_OP_RD_ANY_REG 0x65 /* Read any register */
#define SPINOR_OP_WR_ANY_REG 0x71 /* Write any register */
#define SPINOR_REG_CYPRESS_CFR2V 0x00800003
@@ -294,6 +295,72 @@ static const struct flash_info spansion_parts[] = {
},
};

+/**
+ * spi_nor_clear_sr() - Clear the Status Register.
+ * @nor: pointer to 'struct spi_nor'.
+ */
+static void spi_nor_clear_sr(struct spi_nor *nor)
+{
+ int ret;
+
+ if (nor->spimem) {
+ struct spi_mem_op op =
+ SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_CLSR, 0),
+ SPI_MEM_OP_NO_ADDR,
+ SPI_MEM_OP_NO_DUMMY,
+ SPI_MEM_OP_NO_DATA);
+
+ spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
+
+ ret = spi_mem_exec_op(nor->spimem, &op);
+ } else {
+ ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_CLSR,
+ NULL, 0);
+ }
+
+ if (ret)
+ dev_dbg(nor->dev, "error %d clearing SR\n", ret);
+}
+
+/**
+ * spi_nor_sr_ready_and_clear() - Query the Status Register to see if the flash
+ * is ready for new commands and clear it.
+ * @nor: pointer to 'struct spi_nor'.
+ *
+ * Return: 1 if ready, 0 if not ready, -errno on errors.
+ */
+int spi_nor_sr_ready_and_clear(struct spi_nor *nor)
+{
+ int ret;
+
+ ret = spi_nor_read_sr(nor, nor->bouncebuf);
+ if (ret)
+ return ret;
+
+ if (nor->bouncebuf[0] & (SR_E_ERR | SR_P_ERR)) {
+ if (nor->bouncebuf[0] & SR_E_ERR)
+ dev_err(nor->dev, "Erase Error occurred\n");
+ else
+ dev_err(nor->dev, "Programming Error occurred\n");
+
+ spi_nor_clear_sr(nor);
+
+ /*
+ * WEL bit remains set to one when an erase or page program
+ * error occurs. Issue a Write Disable command to protect
+ * against inadvertent writes that can possibly corrupt the
+ * contents of the memory.
+ */
+ ret = spi_nor_write_disable(nor);
+ if (ret)
+ return ret;
+
+ return -EIO;
+ }
+
+ return !(nor->bouncebuf[0] & SR_WIP);
+}
+
static void spansion_late_init(struct spi_nor *nor)
{
if (nor->params->size > SZ_16M) {
@@ -302,6 +369,9 @@ static void spansion_late_init(struct spi_nor *nor)
nor->erase_opcode = SPINOR_OP_SE;
nor->mtd.erasesize = nor->info->sector_size;
}
+
+ if (nor->flags & SNOR_F_USE_CLSR)
+ nor->params->ready = spi_nor_sr_ready_and_clear;
}

static const struct spi_nor_fixups spansion_fixups = {
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 4622251a79ff..5e25a7b75ae2 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -90,7 +90,6 @@

/* Used for Spansion flashes only. */
#define SPINOR_OP_BRWR 0x17 /* Bank register write */
-#define SPINOR_OP_CLSR 0x30 /* Clear status register 1 */

/* Used for Micron flashes only. */
#define SPINOR_OP_RD_EVCR 0x65 /* Read EVCR register */
--
2.30.2


2022-02-03 18:16:28

by kernel test robot

[permalink] [raw]
Subject: [RFC PATCH] mtd: spi-nor: spi_nor_sr_ready_and_clear() can be static

drivers/mtd/spi-nor/spansion.c:332:5: warning: symbol 'spi_nor_sr_ready_and_clear' was not declared. Should it be static?

Reported-by: kernel test robot <[email protected]>
Signed-off-by: kernel test robot <[email protected]>
---
spansion.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index 4756fb88eab232..242d55e043f08d 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -329,7 +329,7 @@ static void spi_nor_clear_sr(struct spi_nor *nor)
*
* Return: 1 if ready, 0 if not ready, -errno on errors.
*/
-int spi_nor_sr_ready_and_clear(struct spi_nor *nor)
+static int spi_nor_sr_ready_and_clear(struct spi_nor *nor)
{
int ret;

2022-02-10 04:16:11

by Tudor Ambarus

[permalink] [raw]
Subject: Re: [PATCH v1 12/14] mtd: spi-nor: move all spansion specifics into spansion.c

On 2/2/22 16:58, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> The clear status register flags is only available on spansion flashes.
> Move all the functions around that into the spanion module.
>
> Signed-off-by: Michael Walle <[email protected]>
> ---
> drivers/mtd/spi-nor/core.c | 52 +------------------------
> drivers/mtd/spi-nor/spansion.c | 70 ++++++++++++++++++++++++++++++++++
> include/linux/mtd/spi-nor.h | 1 -
> 3 files changed, 72 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index be65aaa954ca..5b00dfab77a6 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -554,33 +554,6 @@ int spi_nor_write_ear(struct spi_nor *nor, u8 ear)
> return ret;
> }
>
> -/**
> - * spi_nor_clear_sr() - Clear the Status Register.
> - * @nor: pointer to 'struct spi_nor'.
> - */
> -static void spi_nor_clear_sr(struct spi_nor *nor)
> -{
> - int ret;
> -
> - if (nor->spimem) {
> - struct spi_mem_op op =
> - SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_CLSR, 0),
> - SPI_MEM_OP_NO_ADDR,
> - SPI_MEM_OP_NO_DUMMY,
> - SPI_MEM_OP_NO_DATA);
> -
> - spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> -
> - ret = spi_mem_exec_op(nor->spimem, &op);
> - } else {
> - ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_CLSR,
> - NULL, 0);
> - }
> -
> - if (ret)
> - dev_dbg(nor->dev, "error %d clearing SR\n", ret);
> -}
> -
> /**
> * spi_nor_sr_ready() - Query the Status Register to see if the flash is ready
> * for new commands.
> @@ -590,33 +563,12 @@ static void spi_nor_clear_sr(struct spi_nor *nor)
> */
> int spi_nor_sr_ready(struct spi_nor *nor)
> {
> - int ret = spi_nor_read_sr(nor, nor->bouncebuf);
> + int ret;
>
> + ret = spi_nor_read_sr(nor, nor->bouncebuf);
> if (ret)
> return ret;

:) don't change style for no reason. What's wrong with the previous version?

Anyway, with the reports fixed and no hidden style changes, it looks good to me.

>
> - if (nor->flags & SNOR_F_USE_CLSR &&
> - nor->bouncebuf[0] & (SR_E_ERR | SR_P_ERR)) {
> - if (nor->bouncebuf[0] & SR_E_ERR)
> - dev_err(nor->dev, "Erase Error occurred\n");
> - else
> - dev_err(nor->dev, "Programming Error occurred\n");
> -
> - spi_nor_clear_sr(nor);
> -
> - /*
> - * WEL bit remains set to one when an erase or page program
> - * error occurs. Issue a Write Disable command to protect
> - * against inadvertent writes that can possibly corrupt the
> - * contents of the memory.
> - */
> - ret = spi_nor_write_disable(nor);
> - if (ret)
> - return ret;
> -
> - return -EIO;
> - }
> -
> return !(nor->bouncebuf[0] & SR_WIP);
> }
>
> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> index dedc2de90cb8..4756fb88eab2 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c
> @@ -8,6 +8,7 @@
>
> #include "core.h"
>
> +#define SPINOR_OP_CLSR 0x30 /* Clear status register 1 */
> #define SPINOR_OP_RD_ANY_REG 0x65 /* Read any register */
> #define SPINOR_OP_WR_ANY_REG 0x71 /* Write any register */
> #define SPINOR_REG_CYPRESS_CFR2V 0x00800003
> @@ -294,6 +295,72 @@ static const struct flash_info spansion_parts[] = {
> },
> };
>
> +/**
> + * spi_nor_clear_sr() - Clear the Status Register.
> + * @nor: pointer to 'struct spi_nor'.
> + */
> +static void spi_nor_clear_sr(struct spi_nor *nor)
> +{
> + int ret;
> +
> + if (nor->spimem) {
> + struct spi_mem_op op =
> + SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_CLSR, 0),
> + SPI_MEM_OP_NO_ADDR,
> + SPI_MEM_OP_NO_DUMMY,
> + SPI_MEM_OP_NO_DATA);
> +
> + spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> +
> + ret = spi_mem_exec_op(nor->spimem, &op);
> + } else {
> + ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_CLSR,
> + NULL, 0);
> + }
> +
> + if (ret)
> + dev_dbg(nor->dev, "error %d clearing SR\n", ret);
> +}
> +
> +/**
> + * spi_nor_sr_ready_and_clear() - Query the Status Register to see if the flash
> + * is ready for new commands and clear it.
> + * @nor: pointer to 'struct spi_nor'.
> + *
> + * Return: 1 if ready, 0 if not ready, -errno on errors.
> + */
> +int spi_nor_sr_ready_and_clear(struct spi_nor *nor)
> +{
> + int ret;
> +
> + ret = spi_nor_read_sr(nor, nor->bouncebuf);
> + if (ret)
> + return ret;
> +
> + if (nor->bouncebuf[0] & (SR_E_ERR | SR_P_ERR)) {
> + if (nor->bouncebuf[0] & SR_E_ERR)
> + dev_err(nor->dev, "Erase Error occurred\n");
> + else
> + dev_err(nor->dev, "Programming Error occurred\n");
> +
> + spi_nor_clear_sr(nor);
> +
> + /*
> + * WEL bit remains set to one when an erase or page program
> + * error occurs. Issue a Write Disable command to protect
> + * against inadvertent writes that can possibly corrupt the
> + * contents of the memory.
> + */
> + ret = spi_nor_write_disable(nor);
> + if (ret)
> + return ret;
> +
> + return -EIO;
> + }
> +
> + return !(nor->bouncebuf[0] & SR_WIP);
> +}
> +
> static void spansion_late_init(struct spi_nor *nor)
> {
> if (nor->params->size > SZ_16M) {
> @@ -302,6 +369,9 @@ static void spansion_late_init(struct spi_nor *nor)
> nor->erase_opcode = SPINOR_OP_SE;
> nor->mtd.erasesize = nor->info->sector_size;
> }
> +
> + if (nor->flags & SNOR_F_USE_CLSR)
> + nor->params->ready = spi_nor_sr_ready_and_clear;
> }
>
> static const struct spi_nor_fixups spansion_fixups = {
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index 4622251a79ff..5e25a7b75ae2 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -90,7 +90,6 @@
>
> /* Used for Spansion flashes only. */
> #define SPINOR_OP_BRWR 0x17 /* Bank register write */
> -#define SPINOR_OP_CLSR 0x30 /* Clear status register 1 */
>
> /* Used for Micron flashes only. */
> #define SPINOR_OP_RD_EVCR 0x65 /* Read EVCR register */
> --
> 2.30.2
>

2022-02-15 19:48:19

by Pratyush Yadav

[permalink] [raw]
Subject: Re: [PATCH v1 12/14] mtd: spi-nor: move all spansion specifics into spansion.c

On 10/02/22 03:32AM, [email protected] wrote:
> On 2/2/22 16:58, Michael Walle wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > The clear status register flags is only available on spansion flashes.
> > Move all the functions around that into the spanion module.
> >
> > Signed-off-by: Michael Walle <[email protected]>
> > ---
> > drivers/mtd/spi-nor/core.c | 52 +------------------------
> > drivers/mtd/spi-nor/spansion.c | 70 ++++++++++++++++++++++++++++++++++
> > include/linux/mtd/spi-nor.h | 1 -
> > 3 files changed, 72 insertions(+), 51 deletions(-)
> >
> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > index be65aaa954ca..5b00dfab77a6 100644
> > --- a/drivers/mtd/spi-nor/core.c
> > +++ b/drivers/mtd/spi-nor/core.c
> > @@ -554,33 +554,6 @@ int spi_nor_write_ear(struct spi_nor *nor, u8 ear)
> > return ret;
> > }
> >
> > -/**
> > - * spi_nor_clear_sr() - Clear the Status Register.
> > - * @nor: pointer to 'struct spi_nor'.
> > - */
> > -static void spi_nor_clear_sr(struct spi_nor *nor)
> > -{
> > - int ret;
> > -
> > - if (nor->spimem) {
> > - struct spi_mem_op op =
> > - SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_CLSR, 0),
> > - SPI_MEM_OP_NO_ADDR,
> > - SPI_MEM_OP_NO_DUMMY,
> > - SPI_MEM_OP_NO_DATA);
> > -
> > - spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> > -
> > - ret = spi_mem_exec_op(nor->spimem, &op);
> > - } else {
> > - ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_CLSR,
> > - NULL, 0);
> > - }
> > -
> > - if (ret)
> > - dev_dbg(nor->dev, "error %d clearing SR\n", ret);
> > -}
> > -
> > /**
> > * spi_nor_sr_ready() - Query the Status Register to see if the flash is ready
> > * for new commands.
> > @@ -590,33 +563,12 @@ static void spi_nor_clear_sr(struct spi_nor *nor)
> > */
> > int spi_nor_sr_ready(struct spi_nor *nor)
> > {
> > - int ret = spi_nor_read_sr(nor, nor->bouncebuf);
> > + int ret;
> >
> > + ret = spi_nor_read_sr(nor, nor->bouncebuf);
> > if (ret)
> > return ret;
>
> :) don't change style for no reason. What's wrong with the previous version?

FWIW, I like the newer style better. But that should come in a separate
patch in either case.

>
> Anyway, with the reports fixed and no hidden style changes, it looks good to me.
>
> >
> > - if (nor->flags & SNOR_F_USE_CLSR &&
> > - nor->bouncebuf[0] & (SR_E_ERR | SR_P_ERR)) {
> > - if (nor->bouncebuf[0] & SR_E_ERR)
> > - dev_err(nor->dev, "Erase Error occurred\n");
> > - else
> > - dev_err(nor->dev, "Programming Error occurred\n");
> > -
> > - spi_nor_clear_sr(nor);
> > -
> > - /*
> > - * WEL bit remains set to one when an erase or page program
> > - * error occurs. Issue a Write Disable command to protect
> > - * against inadvertent writes that can possibly corrupt the
> > - * contents of the memory.
> > - */
> > - ret = spi_nor_write_disable(nor);
> > - if (ret)
> > - return ret;
> > -
> > - return -EIO;
> > - }
> > -
> > return !(nor->bouncebuf[0] & SR_WIP);
> > }
> >
> > diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> > index dedc2de90cb8..4756fb88eab2 100644
> > --- a/drivers/mtd/spi-nor/spansion.c
> > +++ b/drivers/mtd/spi-nor/spansion.c
> > @@ -8,6 +8,7 @@
> >
> > #include "core.h"
> >
> > +#define SPINOR_OP_CLSR 0x30 /* Clear status register 1 */
> > #define SPINOR_OP_RD_ANY_REG 0x65 /* Read any register */
> > #define SPINOR_OP_WR_ANY_REG 0x71 /* Write any register */
> > #define SPINOR_REG_CYPRESS_CFR2V 0x00800003
> > @@ -294,6 +295,72 @@ static const struct flash_info spansion_parts[] = {
> > },
> > };
> >
> > +/**
> > + * spi_nor_clear_sr() - Clear the Status Register.
> > + * @nor: pointer to 'struct spi_nor'.
> > + */
> > +static void spi_nor_clear_sr(struct spi_nor *nor)
> > +{
> > + int ret;
> > +
> > + if (nor->spimem) {
> > + struct spi_mem_op op =
> > + SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_CLSR, 0),
> > + SPI_MEM_OP_NO_ADDR,
> > + SPI_MEM_OP_NO_DUMMY,
> > + SPI_MEM_OP_NO_DATA);
> > +
> > + spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> > +
> > + ret = spi_mem_exec_op(nor->spimem, &op);
> > + } else {
> > + ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_CLSR,
> > + NULL, 0);
> > + }
> > +
> > + if (ret)
> > + dev_dbg(nor->dev, "error %d clearing SR\n", ret);
> > +}
> > +
> > +/**
> > + * spi_nor_sr_ready_and_clear() - Query the Status Register to see if the flash
> > + * is ready for new commands and clear it.
> > + * @nor: pointer to 'struct spi_nor'.
> > + *
> > + * Return: 1 if ready, 0 if not ready, -errno on errors.
> > + */
> > +int spi_nor_sr_ready_and_clear(struct spi_nor *nor)

Make it static.

[...]

--
Regards,
Pratyush Yadav
Texas Instruments Inc.