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

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

On 16/11/21 02:25PM, Tudor.Ambarus@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@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.

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