Re: [PATCH v3 03/25] mtd: spi-nor: Introduce spi_nor_set_mtd_info()

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

On 29/10/21 08:26PM, Tudor Ambarus wrote:
> Used to init all the mtd_info fields. Move the mtd_info init
> the last thing in the spi_nor_scan(), so that we avoid superfluous
> initialization of the mtd_info fields in case of errors.
> 
> While here use common naming scheme for functions that are setting
> mtd_info fields:
> s/spi_nor_register_locking_ops/spi_nor_set_mtd_locking_ops
> s/spi_nor_otp_init/spi_nor_set_mtd_otp_ops
> The functions names are self explanatory, get rid of the comment
> for the OTP function.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi-nor/core.c | 54 +++++++++++++++++++++-----------------
>  drivers/mtd/spi-nor/core.h |  4 +--
>  drivers/mtd/spi-nor/otp.c  |  2 +-
>  drivers/mtd/spi-nor/swp.c  |  2 +-
>  4 files changed, 34 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 277d1fde84c8..ae971c773334 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3071,6 +3071,35 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
>  	return info;
>  }
>  
> +static void spi_nor_set_mtd_info(struct spi_nor *nor)
> +{
> +	struct mtd_info *mtd = &nor->mtd;
> +	struct device *dev = nor->dev;
> +
> +	spi_nor_set_mtd_locking_ops(nor);
> +	spi_nor_set_mtd_otp_ops(nor);
> +
> +	mtd->dev.parent = dev;
> +	if (!mtd->name)
> +		mtd->name = dev_name(dev);
> +	mtd->type = MTD_NORFLASH;
> +	mtd->flags = MTD_CAP_NORFLASH;
> +	if (nor->info->flags & SPI_NOR_NO_ERASE)
> +		mtd->flags |= MTD_NO_ERASE;
> +	mtd->writesize = nor->params->writesize;
> +	mtd->writebufsize = nor->page_size;
> +	mtd->size = nor->params->size;
> +	mtd->_erase = spi_nor_erase;
> +	mtd->_read = spi_nor_read;
> +	/* Might be already set by some SST flashes. */
> +	if (!mtd->_write)
> +		mtd->_write = spi_nor_write;
> +	mtd->_suspend = spi_nor_suspend;
> +	mtd->_resume = spi_nor_resume;
> +	mtd->_get_device = spi_nor_get_device;
> +	mtd->_put_device = spi_nor_put_device;

You should also merge in spi_nor_debugfs_init() in here which 
initializes mtd->dbg.

> +}
> +
>  int spi_nor_scan(struct spi_nor *nor, const char *name,
>  		 const struct spi_nor_hwcaps *hwcaps)
>  {
> @@ -3125,26 +3154,11 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>  	if (info->flags & SPI_NOR_HAS_LOCK)
>  		nor->flags |= SNOR_F_HAS_LOCK;
>  
> -	mtd->_write = spi_nor_write;
> -
>  	/* Init flash parameters based on flash_info struct and SFDP */
>  	ret = spi_nor_init_params(nor);
>  	if (ret)
>  		return ret;
>  
> -	if (!mtd->name)
> -		mtd->name = dev_name(dev);
> -	mtd->type = MTD_NORFLASH;
> -	mtd->writesize = nor->params->writesize;
> -	mtd->flags = MTD_CAP_NORFLASH;
> -	mtd->size = nor->params->size;
> -	mtd->_erase = spi_nor_erase;
> -	mtd->_read = spi_nor_read;
> -	mtd->_suspend = spi_nor_suspend;
> -	mtd->_resume = spi_nor_resume;
> -	mtd->_get_device = spi_nor_get_device;
> -	mtd->_put_device = spi_nor_put_device;
> -
>  	if (info->flags & USE_FSR)
>  		nor->flags |= SNOR_F_USE_FSR;
>  	if (info->flags & SPI_NOR_HAS_TB) {
> @@ -3166,12 +3180,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>  			nor->flags |= SNOR_F_HAS_SR_BP3_BIT6;
>  	}
>  
> -	if (info->flags & SPI_NOR_NO_ERASE)
> -		mtd->flags |= MTD_NO_ERASE;
> -
> -	mtd->dev.parent = dev;
>  	nor->page_size = nor->params->page_size;
> -	mtd->writebufsize = nor->page_size;
>  
>  	if (of_property_read_bool(np, "broken-flash-reset"))
>  		nor->flags |= SNOR_F_BROKEN_RESET;
> @@ -3196,15 +3205,12 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>  	if (ret)
>  		return ret;
>  
> -	spi_nor_register_locking_ops(nor);
> -
>  	/* Send all the required SPI flash commands to initialize device */
>  	ret = spi_nor_init(nor);
>  	if (ret)
>  		return ret;
>  
> -	/* Configure OTP parameters and ops */
> -	spi_nor_otp_init(nor);
> +	spi_nor_set_mtd_info(nor);

This will break multiple things.

- spi_nor_set_addr_width(), which is called by spi_nor-setup(). It 
  checks for nor->mtd.size which has not been set yet.
- spi_nor_spimem_check_op(), which is called (indirectly) by 
  spi_nor_default_setup(). It check for nor->mtd.size.
- spi_nor_try_unlock_all(), which is called by spi_nor_init(). I don't 
  think it actually uses any values you initialize here but still worth 
  pointing out.

>  
>  	dev_info(dev, "%s (%lld Kbytes)\n", info->name,
>  			(long long)mtd->size >> 10);
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 223a03769950..48f26d3f1b3c 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -548,8 +548,8 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
>  
>  void spi_nor_init_default_locking_ops(struct spi_nor *nor);
>  void spi_nor_try_unlock_all(struct spi_nor *nor);
> -void spi_nor_register_locking_ops(struct spi_nor *nor);
> -void spi_nor_otp_init(struct spi_nor *nor);
> +void spi_nor_set_mtd_locking_ops(struct spi_nor *nor);
> +void spi_nor_set_mtd_otp_ops(struct spi_nor *nor);
>  
>  static inline struct spi_nor *mtd_to_spi_nor(struct mtd_info *mtd)
>  {
> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
> index 983e40b19134..fa63d8571218 100644
> --- a/drivers/mtd/spi-nor/otp.c
> +++ b/drivers/mtd/spi-nor/otp.c
> @@ -480,7 +480,7 @@ static int spi_nor_mtd_otp_lock(struct mtd_info *mtd, loff_t from, size_t len)
>  	return ret;
>  }
>  
> -void spi_nor_otp_init(struct spi_nor *nor)
> +void spi_nor_set_mtd_otp_ops(struct spi_nor *nor)
>  {
>  	struct mtd_info *mtd = &nor->mtd;
>  
> diff --git a/drivers/mtd/spi-nor/swp.c b/drivers/mtd/spi-nor/swp.c
> index 8594bcbb7dbe..1f178313ba8f 100644
> --- a/drivers/mtd/spi-nor/swp.c
> +++ b/drivers/mtd/spi-nor/swp.c
> @@ -414,7 +414,7 @@ void spi_nor_try_unlock_all(struct spi_nor *nor)
>  		dev_dbg(nor->dev, "Failed to unlock the entire flash memory array\n");
>  }
>  
> -void spi_nor_register_locking_ops(struct spi_nor *nor)
> +void spi_nor_set_mtd_locking_ops(struct spi_nor *nor)
>  {
>  	struct mtd_info *mtd = &nor->mtd;
>  
> -- 
> 2.25.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel