Re: [PATCH 2/3] mtd: spi-nor: core: reuse spi_nor_clear_sr function

Pratyush Yadav
2021-04-08T16:24:02Z

On 06/04/21 10:52PM, Yaliang.Wang wrote:
>
> On 4/6/21 7:07 PM, Pratyush Yadav wrote:
> > [Please note: This e-mail is from an EXTERNAL e-mail address]
> >
> > On 02/04/21 03:50AM,yaliang.wang@windriver.com wrote:
> > > From: Yaliang Wang<Yaliang.Wang@windriver.com>
> > >
> > > spi_nor_clear_fsr() and spi_nor_clear_sr() are almost the same, except
> > > the opcode they used, the codes can be easily reused without much
> > > changing.
> > Honestly, I am not sure how I feel about this. Yes, it would reduce some
> > code duplication but at the same time reduces the readability slightly.
> > spi_nor_clear_fsr(nor) is easier to read than spi_nor_clear_sr(nor, OP_CLFSR).
> > I won't blame someone for missing that 'F' and thinking that it actually
> > simply clears the SR. Dunno...
> >
> > Anyway, if we do decide to go through with this change, the code changes
> > look good to me.
> The reason why am I doing this is quite simple, the contents of the
> function  are relatively  common, no other manufacturer specific codes
> involved, and it can be easily expanded to other manufacturers.  Actually
> there is an instant example, "is25wp256" chip needs to clear error bits with
> using CLEAR EXTENDED READ REGISTER OPERATION (CLERP, 82h) [1].

Ok.

>
> With that said, the name of the function do can be revised, maybe the name
> "spi_nor_clear_status()" is more proper?

Yes, this is certainly better. While you're doing this then you might
also want to look into spi_nor_read_sr() and spi_nor_read_fsr() and see
if we can exploit similarities there as well.

>
> [1]: https://www.issi.com/WW/pdf/25LP-WP256.pdf ; Page21,Page22 ;
> > > Signed-off-by: Yaliang Wang<Yaliang.Wang@windriver.com>
> > > ---
> > > drivers/mtd/spi-nor/core.c | 40 +++++++-------------------------------
> > > 1 file changed, 7 insertions(+), 33 deletions(-)
> > >
> > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > > index 6dc8bd0a6bd4..dbd6adb6aa0b 100644
> > > --- a/drivers/mtd/spi-nor/core.c
> > > +++ b/drivers/mtd/spi-nor/core.c
> > > @@ -652,14 +652,15 @@ static int spi_nor_xsr_ready(struct spi_nor *nor)
> > > /**
> > > * spi_nor_clear_sr() - Clear the Status Register.
> > > * @nor: pointer to 'struct spi_nor'.
> > > + * @opcode: the SPI command op code to clear status register.
> > > */
> > > -static void spi_nor_clear_sr(struct spi_nor *nor)
> > > +static void spi_nor_clear_sr(struct spi_nor *nor, u8 opcode)
> > > {
> > > int ret;
> > >
> > > if (nor->spimem) {
> > > struct spi_mem_op op =
> > > - SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_CLSR, 0),
> > > + SPI_MEM_OP(SPI_MEM_OP_CMD(opcode, 0),
> > > SPI_MEM_OP_NO_ADDR,
> > > SPI_MEM_OP_NO_DUMMY,
> > > SPI_MEM_OP_NO_DATA);
> > > @@ -668,12 +669,12 @@ static void spi_nor_clear_sr(struct spi_nor *nor)
> > >
> > > ret = spi_mem_exec_op(nor->spimem, &op);
> > > } else {
> > > - ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_CLSR,
> > > + ret = spi_nor_controller_ops_write_reg(nor, opcode,
> > > NULL, 0);
> > > }
> > >
> > > if (ret)
> > > - dev_dbg(nor->dev, "error %d clearing SR\n", ret);
> > > + dev_dbg(nor->dev, "error %d clearing status\n", ret);
> > > }
> > >
> > > /**
> > > @@ -697,7 +698,7 @@ static int spi_nor_sr_ready(struct spi_nor *nor)
> > > else
> > > dev_err(nor->dev, "Programming Error occurred\n");
> > >
> > > - spi_nor_clear_sr(nor);
> > > + spi_nor_clear_sr(nor, SPINOR_OP_CLSR);
> > >
> > > /*
> > > * WEL bit remains set to one when an erase or page program
> > > @@ -715,33 +716,6 @@ static int spi_nor_sr_ready(struct spi_nor *nor)
> > > return !(nor->bouncebuf[0] & SR_WIP);
> > > }
> > >
> > > -/**
> > > - * spi_nor_clear_fsr() - Clear the Flag Status Register.
> > > - * @nor: pointer to 'struct spi_nor'.
> > > - */
> > > -static void spi_nor_clear_fsr(struct spi_nor *nor)
> > > -{
> > > - int ret;
> > > -
> > > - if (nor->spimem) {
> > > - struct spi_mem_op op =
> > > - SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_CLFSR, 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_CLFSR,
> > > - NULL, 0);
> > > - }
> > > -
> > > - if (ret)
> > > - dev_dbg(nor->dev, "error %d clearing FSR\n", ret);
> > > -}
> > > -
> > > /**
> > > * spi_nor_fsr_ready() - Query the Flag Status Register to see if the flash is
> > > * ready for new commands.
> > > @@ -766,7 +740,7 @@ static int spi_nor_fsr_ready(struct spi_nor *nor)
> > > dev_err(nor->dev,
> > > "Attempted to modify a protected sector.\n");
> > >
> > > - spi_nor_clear_fsr(nor);
> > > + spi_nor_clear_sr(nor, SPINOR_OP_CLFSR);
> > >
> > > /*
> > > * WEL bit remains set to one when an erase or page program
> > --
> > Regards,
> > Pratyush Yadav
> > Texas Instruments Inc.
>

--
Regards,
Pratyush Yadav
Texas Instruments Inc.