[PATCH v3 03/25] mtd: spi-nor: Introduce spi_nor_set_mtd_info()
Tudor.Ambarus at microchip.com
Tudor.Ambarus at microchip.com
Tue Nov 16 06:25:48 PST 2021
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 at 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.
>
More information about the linux-arm-kernel
mailing list