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

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

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

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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