| Message ID | 20240522233135.26835-15-semen.protsenko@linaro.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Jaehoon Chung |
| Headers | show |
| Series | mmc: dw_mmc: Enable eMMC on E850-96 board | expand |
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
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 --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;
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(-)