| Message ID | 20210727045222.905056-23-tudor.ambarus@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mtd: spi-nor: Handle ID collisions and clean params init | expand |
On 27/07/21 07:52AM, Tudor Ambarus wrote: > 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: Pratyush Yadav <p.yadav@ti.com> > --- > drivers/mtd/spi-nor/core.c | 6 ++---- > drivers/mtd/spi-nor/core.h | 4 ++-- > drivers/mtd/spi-nor/otp.c | 2 +- > drivers/mtd/spi-nor/swp.c | 2 +- > 4 files changed, 6 insertions(+), 8 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 26acfc9901db..72dfe6274041 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -3129,10 +3129,8 @@ static void spi_nor_set_mtd_info(struct spi_nor *nor) > struct mtd_info *mtd = &nor->mtd; > struct device *dev = nor->dev; > > - spi_nor_register_locking_ops(nor); > - > - /* Configure OTP parameters and ops */ > - spi_nor_otp_init(nor); > + spi_nor_set_mtd_locking_ops(nor); > + spi_nor_set_mtd_otp_ops(nor); > > mtd->dev.parent = dev; > if (!mtd->name) > diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h > index 8fddc685d2d3..7fb0cfabe85a 100644 > --- a/drivers/mtd/spi-nor/core.h > +++ b/drivers/mtd/spi-nor/core.h > @@ -552,8 +552,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 struct spi_nor __maybe_unused *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 >
Am 2021-07-27 06:52, schrieb Tudor Ambarus: > The functions names are self explanatory, get rid of the comment > for the OTP function. Mhh. I see, this partly addresses my comments to the previous patch. Maybe it would have been better to have this squashed into one commit :p But my main concern remains: what if we need more that just the mtd callbacks assigments. Basically we loose the otp_init() call. -michael
On 10/22/21 2:57 PM, Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Am 2021-07-27 06:52, schrieb Tudor Ambarus: >> The functions names are self explanatory, get rid of the comment >> for the OTP function. > > Mhh. I see, this partly addresses my comments to the previous patch. > Maybe it would have been better to have this squashed into one > commit :p one thing per patch > > But my main concern remains: what if we need more that just > the mtd callbacks assigments. Basically we loose the otp_init() > call. if (WARN_ON(!is_power_of_2(spi_nor_otp_region_len(nor)))) return; is there just to avoid setting the mtd function pointers when OTP region length is not power of 2. This is not an init, just a check, so for the moment I see it ok to have it in spi_nor_set_mtd_otp_ops(). If some OTP init is required we'll introduce it.
Am 2021-10-22 14:51, schrieb Tudor.Ambarus@microchip.com: > On 10/22/21 2:57 PM, Michael Walle wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know >> the content is safe >> >> Am 2021-07-27 06:52, schrieb Tudor Ambarus: >>> The functions names are self explanatory, get rid of the comment >>> for the OTP function. >> >> Mhh. I see, this partly addresses my comments to the previous patch. >> Maybe it would have been better to have this squashed into one >> commit :p > > one thing per patch well it is really just one thing, you're moving the function. but then the function name doesn't make sense anymore. So the patch on its own is wrong or at least it makes things worse. Also, I'm reviewing that just to see this function was renamed (correctly) in the next patch. >> But my main concern remains: what if we need more that just >> the mtd callbacks assigments. Basically we loose the otp_init() >> call. > > if (WARN_ON(!is_power_of_2(spi_nor_otp_region_len(nor)))) > return; > > is there just to avoid setting the mtd function pointers when OTP > region length is not power of 2. This is not an init, just a check, > so for the moment I see it ok to have it in spi_nor_set_mtd_otp_ops(). It is a sanity check which actually has nothing to do with the mtd ops registration. Just to see if there was a mistake in the otp info flags. Of course, if that is not the case, we don't set the ops. But that doesn't mean it is part of the mtd op assigment. I said stricly speaking, I'm ok with having that in the (now renamed) "otp set mtd ops" function. -michael
On 10/22/21 4:08 PM, Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Am 2021-10-22 14:51, schrieb Tudor.Ambarus@microchip.com: >> On 10/22/21 2:57 PM, Michael Walle wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know >>> the content is safe >>> >>> Am 2021-07-27 06:52, schrieb Tudor Ambarus: >>>> The functions names are self explanatory, get rid of the comment >>>> for the OTP function. >>> >>> Mhh. I see, this partly addresses my comments to the previous patch. >>> Maybe it would have been better to have this squashed into one >>> commit :p >> >> one thing per patch > > well it is really just one thing, you're moving the function. but then > the function name doesn't make sense anymore. So the patch on its own > is wrong or at least it makes things worse. Also, I'm reviewing that > just to see this function was renamed (correctly) in the next patch. I'll squash it then. > >>> But my main concern remains: what if we need more that just >>> the mtd callbacks assigments. Basically we loose the otp_init() >>> call. >> >> if (WARN_ON(!is_power_of_2(spi_nor_otp_region_len(nor)))) >> return; >> >> is there just to avoid setting the mtd function pointers when OTP >> region length is not power of 2. This is not an init, just a check, >> so for the moment I see it ok to have it in spi_nor_set_mtd_otp_ops(). > > It is a sanity check which actually has nothing to do with the mtd ops > registration. Just to see if there was a mistake in the otp info flags. > > Of course, if that is not the case, we don't set the ops. But that > doesn't > mean it is part of the mtd op assigment. > > I said stricly speaking, I'm ok with having that in the (now renamed) > "otp set mtd ops" function. ok, I'll keep it. If condition is true, you don't set the mtd ops. I find it part of the mtd op assignment.
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index 26acfc9901db..72dfe6274041 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -3129,10 +3129,8 @@ static void spi_nor_set_mtd_info(struct spi_nor *nor) struct mtd_info *mtd = &nor->mtd; struct device *dev = nor->dev; - spi_nor_register_locking_ops(nor); - - /* Configure OTP parameters and ops */ - spi_nor_otp_init(nor); + spi_nor_set_mtd_locking_ops(nor); + spi_nor_set_mtd_otp_ops(nor); mtd->dev.parent = dev; if (!mtd->name) diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h index 8fddc685d2d3..7fb0cfabe85a 100644 --- a/drivers/mtd/spi-nor/core.h +++ b/drivers/mtd/spi-nor/core.h @@ -552,8 +552,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 struct spi_nor __maybe_unused *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;
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 | 6 ++---- drivers/mtd/spi-nor/core.h | 4 ++-- drivers/mtd/spi-nor/otp.c | 2 +- drivers/mtd/spi-nor/swp.c | 2 +- 4 files changed, 6 insertions(+), 8 deletions(-)