| Message ID | 20210401195012.4009166-2-yaliang.wang@windriver.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | [1/3] mtd: spi-nor: core: Add manuafacturer/chip specific operations | expand |
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. > > 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
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.
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. >
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