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

[Pratyush Yadav <p.yadav@ti.com>]

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.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/