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
]