[PATCH 0/5] sf: Cleanup
Cleanup of SF, no precise functionality changes.
Any inputs? Jagan.
Jagan Teki (5): mtd: spi: Call sst_write in _write ops cmd: sf Drop reassignment of new into flash env: sf: Preserve and free the previous flash mtd: sf: Drop plat from sf_probe mtd: spi: Use IS_ENABLED to prevent ifdef
cmd/sf.c | 3 --- drivers/mtd/spi/sf_internal.h | 10 ++++++++++ drivers/mtd/spi/sf_probe.c | 19 ++++++++----------- drivers/mtd/spi/spi-nor-core.c | 13 +++++++------ env/sf.c | 18 ++++++++++-------- 5 files changed, 35 insertions(+), 28 deletions(-)
Currently spi-nor code is assigning _write ops for SST and other flashes separately.
Just call the sst_write from generic write ops and return if SST flash found, this way it avoids the confusion of multiple write ops assignment during the scan and makes it more feasible for code readability.
No functionality changes.
Cc: Simon Glass sjg@chromium.org Cc: Vignesh R vigneshr@ti.com Signed-off-by: Jagan Teki jagan@amarulasolutions.com --- drivers/mtd/spi/spi-nor-core.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index 3d4361493e..984cece0b0 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -1233,6 +1233,12 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len, size_t page_offset, page_remain, i; ssize_t ret;
+#ifdef CONFIG_SPI_FLASH_SST + /* sst nor chips use AAI word program */ + if (nor->info->flags & SST_WRITE) + return sst_write(mtd, to, len, retlen, buf); +#endif + dev_dbg(nor->dev, "to 0x%08x, len %zd\n", (u32)to, len);
if (!len) @@ -2528,6 +2534,7 @@ int spi_nor_scan(struct spi_nor *nor) mtd->size = params.size; mtd->_erase = spi_nor_erase; mtd->_read = spi_nor_read; + mtd->_write = spi_nor_write;
#if defined(CONFIG_SPI_FLASH_STMICRO) || defined(CONFIG_SPI_FLASH_SST) /* NOR protection support for STmicro/Micron chips and similar */ @@ -2551,13 +2558,7 @@ int spi_nor_scan(struct spi_nor *nor) nor->flash_unlock = sst26_unlock; nor->flash_is_locked = sst26_is_locked; } - - /* sst nor chips use AAI word program */ - if (info->flags & SST_WRITE) - mtd->_write = sst_write; - else #endif - mtd->_write = spi_nor_write;
if (info->flags & USE_FSR) nor->flags |= SNOR_F_USE_FSR;
On Thu, May 14, 2020 at 5:41 PM Jagan Teki jagan@amarulasolutions.com wrote:
Currently spi-nor code is assigning _write ops for SST and other flashes separately.
Just call the sst_write from generic write ops and return if SST flash found, this way it avoids the confusion of multiple write ops assignment during the scan and makes it more feasible for code readability.
No functionality changes.
Cc: Simon Glass sjg@chromium.org Cc: Vignesh R vigneshr@ti.com Signed-off-by: Jagan Teki jagan@amarulasolutions.com
Applied to u-boot-spi/master
The new pointer points to flash found and that would assign it to global 'flash' pointer for further flash operations and also keep track of old flash pointer.
This would happen if the probe is successful or even failed, but current code assigning new into flash before and after checking the new.
So, drop the assignment after new checks so flash always latest new pointer even if probe failed or succeed.
Cc: Simon Glass sjg@chromium.org Cc: Vignesh R vigneshr@ti.com Signed-off-by: Jagan Teki jagan@amarulasolutions.com --- cmd/sf.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/cmd/sf.c b/cmd/sf.c index e993b3e5ad..302201c2b0 100644 --- a/cmd/sf.c +++ b/cmd/sf.c @@ -141,13 +141,10 @@ static int do_spi_flash_probe(int argc, char * const argv[])
new = spi_flash_probe(bus, cs, speed, mode); flash = new; - if (!new) { printf("Failed to initialize SPI flash at %u:%u\n", bus, cs); return 1; } - - flash = new; #endif
return 0;
On Thu, May 14, 2020 at 5:42 PM Jagan Teki jagan@amarulasolutions.com wrote:
The new pointer points to flash found and that would assign it to global 'flash' pointer for further flash operations and also keep track of old flash pointer.
This would happen if the probe is successful or even failed, but current code assigning new into flash before and after checking the new.
So, drop the assignment after new checks so flash always latest new pointer even if probe failed or succeed.
Cc: Simon Glass sjg@chromium.org Cc: Vignesh R vigneshr@ti.com Signed-off-by: Jagan Teki jagan@amarulasolutions.com
Applied to u-boot-spi/master
env_flash is a global flash pointer, and the probe would happen only if env_flash is NULL, but there is no checking and free the pointer if is not NULL.
So, this patch frees the env_flash if it's not NULL, and get the probed flash in new flash pointer and finally assign into env_flash.
Note: Similar approach has been followed and tested in cmd/sf.c
Cc: Simon Glass sjg@chromium.org Cc: Vignesh R vigneshr@ti.com Signed-off-by: Jagan Teki jagan@amarulasolutions.com --- env/sf.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/env/sf.c b/env/sf.c index 64c57f2cdf..af59c8375c 100644 --- a/env/sf.c +++ b/env/sf.c @@ -50,15 +50,17 @@ static int setup_flash_device(void)
env_flash = dev_get_uclass_priv(new); #else + struct spi_flash *new;
- if (!env_flash) { - env_flash = spi_flash_probe(CONFIG_ENV_SPI_BUS, - CONFIG_ENV_SPI_CS, - CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE); - if (!env_flash) { - env_set_default("spi_flash_probe() failed", 0); - return -EIO; - } + if (env_flash) + spi_flash_free(env_flash); + + new = spi_flash_probe(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS, + CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE); + env_flash = new; + if (!new) { + env_set_default("spi_flash_probe() failed", 0); + return -EIO; } #endif return 0;
Hi Jagan,
On 14/05/20 05:41PM, Jagan Teki wrote:
env_flash is a global flash pointer, and the probe would happen only if env_flash is NULL, but there is no checking and free the pointer if is not NULL.
So, this patch frees the env_flash if it's not NULL, and get the probed flash in new flash pointer and finally assign into env_flash.
Note: Similar approach has been followed and tested in cmd/sf.c
Cc: Simon Glass sjg@chromium.org Cc: Vignesh R vigneshr@ti.com Signed-off-by: Jagan Teki jagan@amarulasolutions.com
env/sf.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/env/sf.c b/env/sf.c index 64c57f2cdf..af59c8375c 100644 --- a/env/sf.c +++ b/env/sf.c @@ -50,15 +50,17 @@ static int setup_flash_device(void)
env_flash = dev_get_uclass_priv(new); #else
- struct spi_flash *new;
- if (!env_flash) {
env_flash = spi_flash_probe(CONFIG_ENV_SPI_BUS,CONFIG_ENV_SPI_CS,CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE);if (!env_flash) {env_set_default("spi_flash_probe() failed", 0);return -EIO;}
- if (env_flash)
spi_flash_free(env_flash);- new = spi_flash_probe(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS,
Why not assign env_flash directly here? You do it in the very next statement anyway. I don't think the extra 'new' variable is needed.
CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE);- env_flash = new;
- if (!new) {
env_set_default("spi_flash_probe() failed", 0);return -EIO;#endif return 0; -- 2.20.1
On Fri, May 15, 2020 at 12:54 PM Pratyush Yadav p.yadav@ti.com wrote:
Hi Jagan,
On 14/05/20 05:41PM, Jagan Teki wrote:
env_flash is a global flash pointer, and the probe would happen only if env_flash is NULL, but there is no checking and free the pointer if is not NULL.
So, this patch frees the env_flash if it's not NULL, and get the probed flash in new flash pointer and finally assign into env_flash.
Note: Similar approach has been followed and tested in cmd/sf.c
Cc: Simon Glass sjg@chromium.org Cc: Vignesh R vigneshr@ti.com Signed-off-by: Jagan Teki jagan@amarulasolutions.com
env/sf.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/env/sf.c b/env/sf.c index 64c57f2cdf..af59c8375c 100644 --- a/env/sf.c +++ b/env/sf.c @@ -50,15 +50,17 @@ static int setup_flash_device(void)
env_flash = dev_get_uclass_priv(new);#else
struct spi_flash *new;
if (!env_flash) {env_flash = spi_flash_probe(CONFIG_ENV_SPI_BUS,CONFIG_ENV_SPI_CS,CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE);if (!env_flash) {env_set_default("spi_flash_probe() failed", 0);return -EIO;}
if (env_flash)spi_flash_free(env_flash);new = spi_flash_probe(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS,Why not assign env_flash directly here? You do it in the very next statement anyway. I don't think the extra 'new' variable is needed.
If flash pointer is used free it, before probing a new flash and storing it in flash. In this scenario it requires local new pointer, it was known observation we have faced in cmd/sf.c
Jagan.
On 15/05/20 01:14PM, Jagan Teki wrote:
On Fri, May 15, 2020 at 12:54 PM Pratyush Yadav p.yadav@ti.com wrote:
Hi Jagan,
On 14/05/20 05:41PM, Jagan Teki wrote:
env_flash is a global flash pointer, and the probe would happen only if env_flash is NULL, but there is no checking and free the pointer if is not NULL.
So, this patch frees the env_flash if it's not NULL, and get the probed flash in new flash pointer and finally assign into env_flash.
Note: Similar approach has been followed and tested in cmd/sf.c
Cc: Simon Glass sjg@chromium.org Cc: Vignesh R vigneshr@ti.com Signed-off-by: Jagan Teki jagan@amarulasolutions.com
env/sf.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/env/sf.c b/env/sf.c index 64c57f2cdf..af59c8375c 100644 --- a/env/sf.c +++ b/env/sf.c @@ -50,15 +50,17 @@ static int setup_flash_device(void)
env_flash = dev_get_uclass_priv(new);#else
struct spi_flash *new;
if (!env_flash) {env_flash = spi_flash_probe(CONFIG_ENV_SPI_BUS,CONFIG_ENV_SPI_CS,CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE);if (!env_flash) {env_set_default("spi_flash_probe() failed", 0);return -EIO;}
if (env_flash)spi_flash_free(env_flash);new = spi_flash_probe(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS,Why not assign env_flash directly here? You do it in the very next statement anyway. I don't think the extra 'new' variable is needed.
If flash pointer is used free it, before probing a new flash and storing it in flash.
Understood.
In this scenario it requires local new pointer, it was known observation we have faced in cmd/sf.c
What difference does using the local pointer make though? We don't do anything with it. We just assign it to env_flash in the next statement. This is similar to doing:
a = foo(); b = a; if (a) ...
The below snippet is equivalent:
b = foo(); if (b) ...
We can get rid of 'a' this way.
On Fri, May 15, 2020 at 2:19 PM Pratyush Yadav p.yadav@ti.com wrote:
On 15/05/20 01:14PM, Jagan Teki wrote:
On Fri, May 15, 2020 at 12:54 PM Pratyush Yadav p.yadav@ti.com wrote:
Hi Jagan,
On 14/05/20 05:41PM, Jagan Teki wrote:
env_flash is a global flash pointer, and the probe would happen only if env_flash is NULL, but there is no checking and free the pointer if is not NULL.
So, this patch frees the env_flash if it's not NULL, and get the probed flash in new flash pointer and finally assign into env_flash.
Note: Similar approach has been followed and tested in cmd/sf.c
Cc: Simon Glass sjg@chromium.org Cc: Vignesh R vigneshr@ti.com Signed-off-by: Jagan Teki jagan@amarulasolutions.com
env/sf.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/env/sf.c b/env/sf.c index 64c57f2cdf..af59c8375c 100644 --- a/env/sf.c +++ b/env/sf.c @@ -50,15 +50,17 @@ static int setup_flash_device(void)
env_flash = dev_get_uclass_priv(new);#else
struct spi_flash *new;
if (!env_flash) {env_flash = spi_flash_probe(CONFIG_ENV_SPI_BUS,CONFIG_ENV_SPI_CS,CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE);if (!env_flash) {env_set_default("spi_flash_probe() failed", 0);return -EIO;}
if (env_flash)spi_flash_free(env_flash);new = spi_flash_probe(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS,Why not assign env_flash directly here? You do it in the very next statement anyway. I don't think the extra 'new' variable is needed.
If flash pointer is used free it, before probing a new flash and storing it in flash.
Understood.
In this scenario it requires local new pointer, it was known observation we have faced in cmd/sf.c
What difference does using the local pointer make though? We don't do anything with it. We just assign it to env_flash in the next statement. This is similar to doing:
a = foo(); b = a; if (a) ...
The below snippet is equivalent:
b = foo(); if (b) ...
We can get rid of 'a' this way.
The scenario here, seems different here we need to take care of free the old pointer and update new pointer with a global pointer like below.
if (b) free(b); a = foo(); b = a; if (!a) fail;
So, you mean to say as below?
if (b) free(b); b = foo(); if (!b) fail;
Jagan.
On 18/05/20 05:36PM, Jagan Teki wrote:
On Fri, May 15, 2020 at 2:19 PM Pratyush Yadav p.yadav@ti.com wrote:
On 15/05/20 01:14PM, Jagan Teki wrote:
On Fri, May 15, 2020 at 12:54 PM Pratyush Yadav p.yadav@ti.com wrote:
Hi Jagan,
On 14/05/20 05:41PM, Jagan Teki wrote:
env_flash is a global flash pointer, and the probe would happen only if env_flash is NULL, but there is no checking and free the pointer if is not NULL.
So, this patch frees the env_flash if it's not NULL, and get the probed flash in new flash pointer and finally assign into env_flash.
Note: Similar approach has been followed and tested in cmd/sf.c
Cc: Simon Glass sjg@chromium.org Cc: Vignesh R vigneshr@ti.com Signed-off-by: Jagan Teki jagan@amarulasolutions.com
env/sf.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/env/sf.c b/env/sf.c index 64c57f2cdf..af59c8375c 100644 --- a/env/sf.c +++ b/env/sf.c @@ -50,15 +50,17 @@ static int setup_flash_device(void)
env_flash = dev_get_uclass_priv(new);#else
struct spi_flash *new;
if (!env_flash) {env_flash = spi_flash_probe(CONFIG_ENV_SPI_BUS,CONFIG_ENV_SPI_CS,CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE);if (!env_flash) {env_set_default("spi_flash_probe() failed", 0);return -EIO;}
if (env_flash)spi_flash_free(env_flash);new = spi_flash_probe(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS,Why not assign env_flash directly here? You do it in the very next statement anyway. I don't think the extra 'new' variable is needed.
If flash pointer is used free it, before probing a new flash and storing it in flash.
Understood.
In this scenario it requires local new pointer, it was known observation we have faced in cmd/sf.c
What difference does using the local pointer make though? We don't do anything with it. We just assign it to env_flash in the next statement. This is similar to doing:
a = foo(); b = a; if (a) ...
The below snippet is equivalent:
b = foo(); if (b) ...
We can get rid of 'a' this way.
The scenario here, seems different here we need to take care of free the old pointer and update new pointer with a global pointer like below.
if (b) free(b); a = foo(); b = a; if (!a) fail;
So, you mean to say as below?
if (b) free(b); b = foo(); if (!b) fail;
Yes, I mean this. Since the pointer b is freed/unassigned, the old pointer value is of no use to us. Getting rid of a (or new in case of this patch) makes the code a bit cleaner.
dm_spi_slave_platdata used in sf_probe for printing plat->cs value and there is no relevant usage apart from this.
We have enouch debug messages available in SPI and SF areas so drop this plat get and associated bug statement.
Cc: Simon Glass sjg@chromium.org Cc: Vignesh R vigneshr@ti.com Signed-off-by: Jagan Teki jagan@amarulasolutions.com --- drivers/mtd/spi/sf_probe.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c index 72b6ee702d..89e384901c 100644 --- a/drivers/mtd/spi/sf_probe.c +++ b/drivers/mtd/spi/sf_probe.c @@ -140,13 +140,11 @@ static int spi_flash_std_get_sw_write_prot(struct udevice *dev) int spi_flash_std_probe(struct udevice *dev) { struct spi_slave *slave = dev_get_parent_priv(dev); - struct dm_spi_slave_platdata *plat = dev_get_parent_platdata(dev); struct spi_flash *flash;
flash = dev_get_uclass_priv(dev); flash->dev = dev; flash->spi = slave; - debug("%s: slave=%p, cs=%d\n", __func__, slave, plat->cs); return spi_flash_probe_slave(flash); }
On 14/05/20 05:41PM, Jagan Teki wrote:
dm_spi_slave_platdata used in sf_probe for printing plat->cs value and there is no relevant usage apart from this.
We have enouch debug messages available in SPI and SF
s/enouch/enough/
areas so drop this plat get and associated bug statement.
Cc: Simon Glass sjg@chromium.org Cc: Vignesh R vigneshr@ti.com Signed-off-by: Jagan Teki jagan@amarulasolutions.com
On Fri, May 15, 2020 at 12:47 PM Pratyush Yadav p.yadav@ti.com wrote:
On 14/05/20 05:41PM, Jagan Teki wrote:
dm_spi_slave_platdata used in sf_probe for printing plat->cs value and there is no relevant usage apart from this.
We have enouch debug messages available in SPI and SF
s/enouch/enough/
Applied to u-boot-spi/master
Use IS_ENABLED to prevent ifdef in sf_probe.c
Cc: Simon Glass sjg@chromium.org Cc: Vignesh R vigneshr@ti.com Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com Signed-off-by: Jagan Teki jagan@amarulasolutions.com --- drivers/mtd/spi/sf_internal.h | 10 ++++++++++ drivers/mtd/spi/sf_probe.c | 17 ++++++++--------- 2 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/drivers/mtd/spi/sf_internal.h b/drivers/mtd/spi/sf_internal.h index 940b2e4c9e..544ed74a5f 100644 --- a/drivers/mtd/spi/sf_internal.h +++ b/drivers/mtd/spi/sf_internal.h @@ -81,5 +81,15 @@ int spi_flash_cmd_get_sw_write_prot(struct spi_flash *flash); #if CONFIG_IS_ENABLED(SPI_FLASH_MTD) int spi_flash_mtd_register(struct spi_flash *flash); void spi_flash_mtd_unregister(void); +#else +static inline int spi_flash_mtd_register(struct spi_flash *flash) +{ + return 0; +} + +static inline void spi_flash_mtd_unregister(void) +{ +} #endif + #endif /* _SF_INTERNAL_H_ */ diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c index 89e384901c..1e8744896c 100644 --- a/drivers/mtd/spi/sf_probe.c +++ b/drivers/mtd/spi/sf_probe.c @@ -44,9 +44,8 @@ static int spi_flash_probe_slave(struct spi_flash *flash) if (ret) goto err_read_id;
-#if CONFIG_IS_ENABLED(SPI_FLASH_MTD) - ret = spi_flash_mtd_register(flash); -#endif + if (IS_ENABLED(CONFIG_SPI_FLASH_MTD)) + ret = spi_flash_mtd_register(flash);
err_read_id: spi_release_bus(spi); @@ -83,9 +82,9 @@ struct spi_flash *spi_flash_probe(unsigned int busnum, unsigned int cs,
void spi_flash_free(struct spi_flash *flash) { -#if CONFIG_IS_ENABLED(SPI_FLASH_MTD) - spi_flash_mtd_unregister(); -#endif + if (IS_ENABLED(CONFIG_SPI_FLASH_MTD)) + spi_flash_mtd_unregister(); + spi_free_slave(flash->spi); free(flash); } @@ -150,9 +149,9 @@ int spi_flash_std_probe(struct udevice *dev)
static int spi_flash_std_remove(struct udevice *dev) { -#if CONFIG_IS_ENABLED(SPI_FLASH_MTD) - spi_flash_mtd_unregister(); -#endif + if (IS_ENABLED(CONFIG_SPI_FLASH_MTD)) + spi_flash_mtd_unregister(); + return 0; }
Am 14.05.20 um 14:11 schrieb Jagan Teki:
Use IS_ENABLED to prevent ifdef in sf_probe.c
Cc: Simon Glass sjg@chromium.org Cc: Vignesh R vigneshr@ti.com Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com Signed-off-by: Jagan Teki jagan@amarulasolutions.com
drivers/mtd/spi/sf_internal.h | 10 ++++++++++ drivers/mtd/spi/sf_probe.c | 17 ++++++++--------- 2 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/drivers/mtd/spi/sf_internal.h b/drivers/mtd/spi/sf_internal.h index 940b2e4c9e..544ed74a5f 100644 --- a/drivers/mtd/spi/sf_internal.h +++ b/drivers/mtd/spi/sf_internal.h @@ -81,5 +81,15 @@ int spi_flash_cmd_get_sw_write_prot(struct spi_flash *flash); #if CONFIG_IS_ENABLED(SPI_FLASH_MTD) int spi_flash_mtd_register(struct spi_flash *flash); void spi_flash_mtd_unregister(void); +#else +static inline int spi_flash_mtd_register(struct spi_flash *flash) +{
- return 0;
+}
+static inline void spi_flash_mtd_unregister(void) +{ +} #endif
#endif /* _SF_INTERNAL_H_ */ diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c index 89e384901c..1e8744896c 100644 --- a/drivers/mtd/spi/sf_probe.c +++ b/drivers/mtd/spi/sf_probe.c @@ -44,9 +44,8 @@ static int spi_flash_probe_slave(struct spi_flash *flash) if (ret) goto err_read_id;
-#if CONFIG_IS_ENABLED(SPI_FLASH_MTD)
- ret = spi_flash_mtd_register(flash);
-#endif
- if (IS_ENABLED(CONFIG_SPI_FLASH_MTD))
ret = spi_flash_mtd_register(flash);
you have to use CONFIG_IS_ENABLED() instead of IS_ENABLED() to also consider the existing CONFIG_SPL_SPI_FLASH_MTD option
$ git grep -n SPI_FLASH_MTD -- drivers/mtd/spi/ drivers/mtd/spi/Kconfig:187:config SPI_FLASH_MTD drivers/mtd/spi/Kconfig:199:config SPL_SPI_FLASH_MTD
err_read_id: spi_release_bus(spi); @@ -83,9 +82,9 @@ struct spi_flash *spi_flash_probe(unsigned int busnum, unsigned int cs,
void spi_flash_free(struct spi_flash *flash) { -#if CONFIG_IS_ENABLED(SPI_FLASH_MTD)
- spi_flash_mtd_unregister();
-#endif
- if (IS_ENABLED(CONFIG_SPI_FLASH_MTD))
spi_flash_mtd_unregister();- spi_free_slave(flash->spi); free(flash);
} @@ -150,9 +149,9 @@ int spi_flash_std_probe(struct udevice *dev)
static int spi_flash_std_remove(struct udevice *dev) { -#if CONFIG_IS_ENABLED(SPI_FLASH_MTD)
- spi_flash_mtd_unregister();
-#endif
- if (IS_ENABLED(CONFIG_SPI_FLASH_MTD))
spi_flash_mtd_unregister();- return 0;
}
Hi Daniel,
On 14.05.20 18:31, Daniel Schwierzeck wrote:
Am 14.05.20 um 14:11 schrieb Jagan Teki:
Use IS_ENABLED to prevent ifdef in sf_probe.c
Cc: Simon Glass sjg@chromium.org Cc: Vignesh R vigneshr@ti.com Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com Signed-off-by: Jagan Teki jagan@amarulasolutions.com
drivers/mtd/spi/sf_internal.h | 10 ++++++++++ drivers/mtd/spi/sf_probe.c | 17 ++++++++--------- 2 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/drivers/mtd/spi/sf_internal.h b/drivers/mtd/spi/sf_internal.h index 940b2e4c9e..544ed74a5f 100644 --- a/drivers/mtd/spi/sf_internal.h +++ b/drivers/mtd/spi/sf_internal.h @@ -81,5 +81,15 @@ int spi_flash_cmd_get_sw_write_prot(struct spi_flash *flash); #if CONFIG_IS_ENABLED(SPI_FLASH_MTD) int spi_flash_mtd_register(struct spi_flash *flash); void spi_flash_mtd_unregister(void); +#else +static inline int spi_flash_mtd_register(struct spi_flash *flash) +{
- return 0;
+}
+static inline void spi_flash_mtd_unregister(void) +{ +} #endif
- #endif /* _SF_INTERNAL_H_ */
diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c index 89e384901c..1e8744896c 100644 --- a/drivers/mtd/spi/sf_probe.c +++ b/drivers/mtd/spi/sf_probe.c @@ -44,9 +44,8 @@ static int spi_flash_probe_slave(struct spi_flash *flash) if (ret) goto err_read_id;
-#if CONFIG_IS_ENABLED(SPI_FLASH_MTD)
- ret = spi_flash_mtd_register(flash);
-#endif
- if (IS_ENABLED(CONFIG_SPI_FLASH_MTD))
ret = spi_flash_mtd_register(flash);you have to use CONFIG_IS_ENABLED() instead of IS_ENABLED()
Just curious: I thought that those two are equivalent:
IS_ENABLED(CONFIG_FOO) and CONFIG_IS_ENABLED(FOO)
Is this not the case? From a functional point of view? I personally prefer IS_ENABLED(CONFIG_FOO), as the Kconfig symbol is completely visible this way.
Thanks, Stefan
to also consider the existing CONFIG_SPL_SPI_FLASH_MTD option
$ git grep -n SPI_FLASH_MTD -- drivers/mtd/spi/ drivers/mtd/spi/Kconfig:187:config SPI_FLASH_MTD drivers/mtd/spi/Kconfig:199:config SPL_SPI_FLASH_MTD
err_read_id: spi_release_bus(spi); @@ -83,9 +82,9 @@ struct spi_flash *spi_flash_probe(unsigned int busnum, unsigned int cs,
void spi_flash_free(struct spi_flash *flash) { -#if CONFIG_IS_ENABLED(SPI_FLASH_MTD)
- spi_flash_mtd_unregister();
-#endif
- if (IS_ENABLED(CONFIG_SPI_FLASH_MTD))
spi_flash_mtd_unregister();- spi_free_slave(flash->spi); free(flash); }
@@ -150,9 +149,9 @@ int spi_flash_std_probe(struct udevice *dev)
static int spi_flash_std_remove(struct udevice *dev) { -#if CONFIG_IS_ENABLED(SPI_FLASH_MTD)
- spi_flash_mtd_unregister();
-#endif
- if (IS_ENABLED(CONFIG_SPI_FLASH_MTD))
spi_flash_mtd_unregister();- return 0; }
On 15/05/2020 06.27, Stefan Roese wrote:
Hi Daniel,
On 14.05.20 18:31, Daniel Schwierzeck wrote:
Am 14.05.20 um 14:11 schrieb Jagan Teki:
Use IS_ENABLED to prevent ifdef in sf_probe.c
Cc: Simon Glass sjg@chromium.org Cc: Vignesh R vigneshr@ti.com Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com Signed-off-by: Jagan Teki jagan@amarulasolutions.com
drivers/mtd/spi/sf_internal.h | 10 ++++++++++ drivers/mtd/spi/sf_probe.c | 17 ++++++++--------- 2 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/drivers/mtd/spi/sf_internal.h b/drivers/mtd/spi/sf_internal.h index 940b2e4c9e..544ed74a5f 100644 --- a/drivers/mtd/spi/sf_internal.h +++ b/drivers/mtd/spi/sf_internal.h @@ -81,5 +81,15 @@ int spi_flash_cmd_get_sw_write_prot(struct spi_flash *flash); #if CONFIG_IS_ENABLED(SPI_FLASH_MTD) int spi_flash_mtd_register(struct spi_flash *flash); void spi_flash_mtd_unregister(void); +#else +static inline int spi_flash_mtd_register(struct spi_flash *flash) +{ + return 0; +}
+static inline void spi_flash_mtd_unregister(void) +{ +} #endif
#endif /* _SF_INTERNAL_H_ */ diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c index 89e384901c..1e8744896c 100644 --- a/drivers/mtd/spi/sf_probe.c +++ b/drivers/mtd/spi/sf_probe.c @@ -44,9 +44,8 @@ static int spi_flash_probe_slave(struct spi_flash *flash) if (ret) goto err_read_id; -#if CONFIG_IS_ENABLED(SPI_FLASH_MTD) - ret = spi_flash_mtd_register(flash); -#endif + if (IS_ENABLED(CONFIG_SPI_FLASH_MTD)) + ret = spi_flash_mtd_register(flash);
you have to use CONFIG_IS_ENABLED() instead of IS_ENABLED()
Just curious: I thought that those two are equivalent:
IS_ENABLED(CONFIG_FOO) and CONFIG_IS_ENABLED(FOO)
Is this not the case?
No. The latter must be used for the symbols FOO that also exist in a separate SPL_FOO variant, while the former must be used where the same Kconfig symbol is supposed to cover both SPL and U-Boot proper.
The former "morally" expands to (morally, there's some some preprocessor trickery to deal with the fact that defined() doesn't work outside preprocessor conditionals)
(defined(CONFIG_FOO) && CONFIG_FOO == 1) ? 1 : 0
If FOO is a proper boolean Kconfig symbol, CONFIG_FOO is either defined as 1 or undefined. But the above also works for the legacy things set in a board header, where CONFIG_FOO could be explicitly defined as 0 or 1.
The CONFIG_IS_ENABLED(FOO) expands to exactly the same thing when building U-Boot proper. But for SPL, the expansion is instead (again, morally)
(defined(CONFIG_SPL_FOO) && CONFIG_SPL_FOO == 1) ? CONFIG_SPL_FOO : 0
That should make it obvious why one needs a variant that doesn't want the CONFIG_ included in the argument; the CONFIG_IS_ENABLED macro needs to do token-pasting with either CONFIG_SPL_ or just CONFIG_.
[Implementation-wise, the trick to make the above work both for macros that are not defined and those that are defined with the value 1 is to have helpers
#define FIRST_ARGUMENT_1 blargh, #define second_arg(one, two, ...) two
macro, then with the appropriate amount of indirections to make sure macros get expanded and tokens get concatenated in the right order, one does
second_arg(EAT_AN_ARGUMENT_##${CONFIG_FOO} 1, 0)
If CONFIG_FOO is a macro with the value 1, the above becomes
second_arg(FIRST_ARGUMENT_1 1, 0) -> second_arg(blarg, 1, 0) -> 1
while if CONFIG_FOO is not defined, the token just "expands" to itself, so we get
second_arg(FIRST_ARGUMENT_CONFIG_FOO 1, 0)
where, since FIRST_ARGUMENT_CONFIG_FOO also doesn't expand further, we get the 0. The same works if CONFIG_FOO is defined to any value other than 1, as long as its expansion is something that is valid for token concatenation; in particular, if it has the value 0, one just gets second_arg(FIRST_ARGUMENT_0 1, 0) where again FIRST_ARGUMENT_0 doesn't expand further and provide another comma, so the 0 gets picked out.]
second_arg(blarg, 1, 0) -> 1
]
On 15/05/2020 09.22, Rasmus Villemoes wrote:
That should make it obvious why one needs a variant that doesn't want the CONFIG_ included in the argument; the CONFIG_IS_ENABLED macro needs to do token-pasting with either CONFIG_SPL_ or just CONFIG_.
On that note, I think all hits from
git grep 'CONFIG_IS_ENABLED(CONFIG_'
are bugs.
Another "script" that might be worth doing is finding instances of CONFIG_IS_ENABLED(FOO) where SPL_FOO doesn't exist as a Kconfig symbol (or perhaps in the whitelist), and conversely finding IS_ENABLED(FOO) when there actually is a separate SPL_FOO symbol.
Rasmus
participants (5)
-
Daniel Schwierzeck -
Jagan Teki -
Pratyush Yadav -
Rasmus Villemoes -
Stefan Roese