diff mbox series

[14/42] mmc: dw_mmc: Use CONFIG_IS_ENABLED() to check config options

Message ID 20240522233135.26835-15-semen.protsenko@linaro.org
State New
Headers show
Series mmc: dw_mmc: Enable eMMC on E850-96 board | expand

Commit Message

Sam Protsenko May 22, 2024, 11:31 p.m. UTC
Use CONFIG_IS_ENABLED() macro to check config options as recommended by
checkpatch, instead of checking those with just #ifdef CONFIG_...

No functional change.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 drivers/mmc/dw_mmc.c | 6 +++---
 include/dwmmc.h      | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

Comments

Quentin Schulz May 23, 2024, 3:03 p.m. UTC | #1
Hi Sam,

On 5/23/24 1:31 AM, Sam Protsenko wrote:
> Use CONFIG_IS_ENABLED() macro to check config options as recommended by
> checkpatch, instead of checking those with just #ifdef CONFIG_...
> 
> No functional change.
> 

There are actual functional changes in here.

defined(CONFIG_DM_MMC) != CONFIG_IS_ENABLED(DM_MMC)

Currently, if one has SPL_MMC and MMC_DW_ROCKCHIP defined, it'll build 
the SPL variant of MMC_DW_ROCKCHIP but guarding only with the U-Boot 
proper symbols, meaning, essentially the SPL and proper variant of 
rockchip_dw_mmc.o are more or less identical.

I think we can argue this isn't proper and should be fixed. I think we 
need to migrate all the MMC_DW drivers to use $(SPL_TPL_) in there and 
create the symbols in Kconfig with the appropriate dependencies. We'll 
likely also need to modify a bunch of defconfigs or make 
SPL_MMC_DW_ROCKCHIP default y if MMC_DW_ROCKCHIP for example, so that we 
don't break current support (it's pretty much expected to have MMC 
support in SPL).

I may have misinterpreted this, so please let me know where I am wrong 
in my assumptions here, but this does look like more work than just this.

Cheers,
Quentin
Sam Protsenko June 9, 2024, 10:33 p.m. UTC | #2
On Thu, May 23, 2024 at 10:03 AM Quentin Schulz
<quentin.schulz@cherry.de> wrote:
>
> Hi Sam,
>
> On 5/23/24 1:31 AM, Sam Protsenko wrote:
> > Use CONFIG_IS_ENABLED() macro to check config options as recommended by
> > checkpatch, instead of checking those with just #ifdef CONFIG_...
> >
> > No functional change.
> >
>
> There are actual functional changes in here.
>
> defined(CONFIG_DM_MMC) != CONFIG_IS_ENABLED(DM_MMC)
>
> Currently, if one has SPL_MMC and MMC_DW_ROCKCHIP defined, it'll build
> the SPL variant of MMC_DW_ROCKCHIP but guarding only with the U-Boot
> proper symbols, meaning, essentially the SPL and proper variant of
> rockchip_dw_mmc.o are more or less identical.
>
> I think we can argue this isn't proper and should be fixed. I think we
> need to migrate all the MMC_DW drivers to use $(SPL_TPL_) in there and
> create the symbols in Kconfig with the appropriate dependencies. We'll
> likely also need to modify a bunch of defconfigs or make
> SPL_MMC_DW_ROCKCHIP default y if MMC_DW_ROCKCHIP for example, so that we
> don't break current support (it's pretty much expected to have MMC
> support in SPL).
>
> I may have misinterpreted this, so please let me know where I am wrong
> in my assumptions here, but this does look like more work than just this.
>

Thanks for noticing this! I completely forgot about SPL as it's not
built for my board. I tried to check with buildman and it seems like
replacing #ifdefs with CONFIG_IS_ENABLED() doesn't change anything,
but I'm not sure if buildman actually checks SPL at all (it probably
only checks the U-Boot proper binary?).

In my v2 I'll stick to #ifdef and avoid using CONFIG_IS_ENABLED(). As
you said, correct rework probably takes much more than just replacing
#ifdefs. It can be done later in a separate series.

> Cheers,
> Quentin
diff mbox series

Patch

diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c
index a1f06931b7ac..ebe239547a7d 100644
--- a/drivers/mmc/dw_mmc.c
+++ b/drivers/mmc/dw_mmc.c
@@ -573,7 +573,7 @@  static int dwmci_setup_bus(struct dwmci_host *host, u32 freq)
 	return 0;
 }
 
-#ifdef CONFIG_DM_MMC
+#if CONFIG_IS_ENABLED(DM_MMC)
 static int dwmci_set_ios(struct udevice *dev)
 {
 	struct mmc *mmc = mmc_get_mmc_dev(dev);
@@ -713,7 +713,7 @@  static int dwmci_init(struct mmc *mmc)
 	return 0;
 }
 
-#ifdef CONFIG_DM_MMC
+#if CONFIG_IS_ENABLED(DM_MMC)
 int dwmci_probe(struct udevice *dev)
 {
 	struct mmc *mmc = mmc_get_mmc_dev(dev);
@@ -738,7 +738,7 @@  void dwmci_setup_cfg(struct mmc_config *cfg, struct dwmci_host *host,
 		u32 max_clk, u32 min_clk)
 {
 	cfg->name = host->name;
-#ifndef CONFIG_DM_MMC
+#if !CONFIG_IS_ENABLED(DM_MMC)
 	cfg->ops = &dwmci_ops;
 #endif
 	cfg->f_min = min_clk;
diff --git a/include/dwmmc.h b/include/dwmmc.h
index 77c8989148a1..9252bef24329 100644
--- a/include/dwmmc.h
+++ b/include/dwmmc.h
@@ -268,7 +268,7 @@  static inline u8 dwmci_readb(struct dwmci_host *host, int reg)
 	return readb(host->ioaddr + reg);
 }
 
-#ifdef CONFIG_BLK
+#if CONFIG_IS_ENABLED(BLK)
 /**
  * dwmci_setup_cfg() - Set up the configuration for DWMMC
  * @cfg:	Configuration structure to fill in (generally &plat->mmc)
@@ -334,7 +334,7 @@  int dwmci_bind(struct udevice *dev, struct mmc *mmc, struct mmc_config *cfg);
 int add_dwmci(struct dwmci_host *host, u32 max_clk, u32 min_clk);
 #endif /* !CONFIG_BLK */
 
-#ifdef CONFIG_DM_MMC
+#if CONFIG_IS_ENABLED(DM_MMC)
 /* Export the operations to drivers */
 int dwmci_probe(struct udevice *dev);
 extern const struct dm_mmc_ops dm_dwmci_ops;