| Message ID | 20200514121145.28737-6-jagan@amarulasolutions.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | sf: Cleanup | expand |
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() 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 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? 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 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 ]
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
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; }
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(-)