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 <s...@chromium.org>
>>> Cc: Vignesh R <vigne...@ti.com>
>>> Cc: Daniel Schwierzeck <daniel.schwierz...@gmail.com>
>>> Signed-off-by: Jagan Teki <ja...@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
]
