| Message ID | 20211029172633.886453-4-tudor.ambarus@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mtd: spi-nor: Clean params init | expand |
Am 2021-10-29 19:26, schrieb Tudor Ambarus: > 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> Reviewed-by: Michael Walle <michael@walle.cc>
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 >
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. >
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. > > >
On 11/16/21 8:11 PM, Pratyush Yadav wrote: >> >>> - 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. If it weren't for the SPI NOR controller drivers that use spi_nor_scan(), I would put the spi_nor_set_mtd_info() just above the mtd_device_register(). It will indicate that no mtd_info field is used up to that point, less things to worry about. spi_nor_try_unlock_all() calls spi_nor_unlock(&nor->mtd, 0, nor->params->size); I can't see for now if we will ever need some specific mtd_info parameter. I would say that we won't, we're just unlocking the full flash, every info we would need we can obtain from NOR. The discussion would be different if it were about mtd partitions, but it isn't, we're dealing with the entire flash. Would you accept the place where I put spi_nor_set_mtd_info() if I add a comment before calling it? Something like: /* No mtd_info fields are used up to this point. */ spi_nor_set_mtd_info(); Cheers, ta
On 17/11/21 02:36PM, Tudor.Ambarus@microchip.com wrote: > On 11/16/21 8:11 PM, Pratyush Yadav wrote: > > >> > >>> - 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. > > If it weren't for the SPI NOR controller drivers that use > spi_nor_scan(), I would put the spi_nor_set_mtd_info() just > above the mtd_device_register(). It will indicate that no mtd_info > field is used up to that point, less things to worry about. > spi_nor_try_unlock_all() calls > spi_nor_unlock(&nor->mtd, 0, nor->params->size); > I can't see for now if we will ever need some specific mtd_info > parameter. I would say that we won't, we're just unlocking the full > flash, every info we would need we can obtain from NOR. The discussion > would be different if it were about mtd partitions, but it isn't, we're > dealing with the entire flash. > > Would you accept the place where I put spi_nor_set_mtd_info() if I add > a comment before calling it? Something like: > /* No mtd_info fields are used up to this point. */ > spi_nor_set_mtd_info(); I see that everything that spi_nor_set_mtd_info() needs is set by the time spi_nor_init_params() is finished. Everything after that is concerned about selecting the protocol and sending the init commands to the flash. So why can't you call it right after spi_nor_init_params()? That and updating spi_nor_spimem_check_op() and spi_nor_set_addr_width() to use nor->params->size instead of nor->mtd.size should do the trick. I think that it is implied that mtd_info fields are not being used until they are initialized so I don't think the comment itself is of much use, but I don't care much about it either way.
Hi, Pratyush, On 11/19/21 8:23 PM, Pratyush Yadav wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 17/11/21 02:36PM, Tudor.Ambarus@microchip.com wrote: >> On 11/16/21 8:11 PM, Pratyush Yadav wrote: >> >>>> >>>>> - 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. >> >> If it weren't for the SPI NOR controller drivers that use >> spi_nor_scan(), I would put the spi_nor_set_mtd_info() just >> above the mtd_device_register(). It will indicate that no mtd_info >> field is used up to that point, less things to worry about. >> spi_nor_try_unlock_all() calls >> spi_nor_unlock(&nor->mtd, 0, nor->params->size); >> I can't see for now if we will ever need some specific mtd_info >> parameter. I would say that we won't, we're just unlocking the full >> flash, every info we would need we can obtain from NOR. The discussion >> would be different if it were about mtd partitions, but it isn't, we're >> dealing with the entire flash. >> >> Would you accept the place where I put spi_nor_set_mtd_info() if I add >> a comment before calling it? Something like: >> /* No mtd_info fields are used up to this point. */ >> spi_nor_set_mtd_info(); > > I see that everything that spi_nor_set_mtd_info() needs is set by the > time spi_nor_init_params() is finished. Everything after that is > concerned about selecting the protocol and sending the init commands to > the flash. So why can't you call it right after spi_nor_init_params()? Because I would like to move it just above mtd_device_register() in the future. If unlock_all() will need some mtd fields in the future, we can introduce a spi_nor_prepare_mtd_for_unlock_all(). I don't want the mtd fields init to be scattered through the SPI NOR core. They shouldn't be used in the NOR's probe sequence of calls anyway, keeping them closer to mtd_device_register() makes the code easier to grasp I think. I will respin the series soon and wanted to let you know why I kept spi_nor_set_mtd_info() where it is in this patch set. Cheers, ta > That and updating spi_nor_spimem_check_op() and spi_nor_set_addr_width() > to use nor->params->size instead of nor->mtd.size should do the trick. > > I think that it is implied that mtd_info fields are not being used until > they are initialized so I don't think the comment itself is of much use, > but I don't care much about it either way. > > -- > Regards, > Pratyush Yadav > Texas Instruments Inc. >
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; +} + 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); 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;
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(-)