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

["Yaliang.Wang" <Yaliang.Wang@windriver.com>]

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].

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

[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.


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