[PATCH v3 03/25] mtd: spi-nor: Introduce spi_nor_set_mtd_info()
Pratyush Yadav
p.yadav at ti.com
Tue Nov 16 10:11:39 PST 2021
On 16/11/21 02:25PM, Tudor.Ambarus at microchip.com wrote:
> 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.
Okay. Fine by me either way.
>
> >> +}
> >> +
> >> 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.
I agree.
>
> > - 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.
Yeah, but who knows if that might change some time later. I would prefer
we don't use a member we haven't initialized yet.
>
> 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.
> >
>
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
More information about the linux-mtd
mailing list