[PATCH 5/5] mtd: spi: Use IS_ENABLED to prevent ifdef

[Rasmus Villemoes]

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 at chromium.org>
>>> Cc: Vignesh R <vigneshr at ti.com>
>>> Cc: Daniel Schwierzeck <daniel.schwierzeck at gmail.com>
>>> Signed-off-by: Jagan Teki <jagan at 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

]