diff mbox series

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

Message ID 20200514121145.28737-6-jagan@amarulasolutions.com
State New
Headers
Series sf: Cleanup |

Commit Message

Jagan Teki May 14, 2020, 12:11 p.m. UTC 
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(-)

Comments

Daniel Schwierzeck May 14, 2020, 4:31 p.m. UTC | #1 
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;
>  }
>  
>
Rasmus Villemoes May 15, 2020, 7:22 a.m. UTC | #2 
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

]
diff mbox series

Patch

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;
 }