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

[<Tudor.Ambarus@microchip.com>]

On 11/15/21 8:52 PM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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.
> 

I was thinking of getting rid of debugfs entries since they duplicate
those on sysfs.

>> +}
>> +
>>  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.

this is gold :), thanks
> 
> - 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.

Let's use NOR parameters in SPI NOR core, thus nor->params->size instead,
and let the mtd fields for mtd. This will spare us of these problems.
And it's cleaner too.

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

we are safe here, the pointer to mtd is used just to get the pointer to
nor.

Thanks again.
ta

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