Re: [PATCH v6] mmc: Poll CD in case cyclic framework is enabled

[Rasmus Villemoes <rasmus.villemoes@prevas.dk>]

Marek Vasut <marek.vasut+renesas@mailbox.org> writes:

> In case the cyclic framework is enabled, poll the card detect of already
> initialized cards and deinitialize them in case they are removed. Since
> the card initialization is a longer process and card initialization is
> done on first access to an uninitialized card anyway, avoid initializing
> newly detected uninitialized cards in the cyclic callback.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> ---
> Cc: Jaehoon Chung <jh80.chung@samsung.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Simon Glass <sjg@chromium.org>
> ---
> V2: Move the cyclic registration/unregistration into mmc init/deinit
> V3: Replace if (CONFIG_IS_ENABLED(CYCLIC)...) with #if as the former
>     does not work with structure members
> V4: Stuff the code with CONFIG_IS_ENABLED() variants to avoid #ifdefs
> V5: Rebase on u-boot/next
> V6: Rebase on u-boot/next
> ---
>  drivers/mmc/mmc.c | 25 +++++++++++++++++++++++++
>  include/mmc.h     |  3 +++
>  2 files changed, 28 insertions(+)
>

[rearranging hunks for easier reading]

> diff --git a/include/mmc.h b/include/mmc.h
> index f508cd15700..0044ff8bef7 100644
> --- a/include/mmc.h
> +++ b/include/mmc.h
> @@ -14,6 +14,7 @@
>  #include <linux/sizes.h>
>  #include <linux/compiler.h>
>  #include <linux/dma-direction.h>
> +#include <cyclic.h>
>  #include <part.h>
>  
>  struct bd_info;
> @@ -757,6 +758,8 @@ struct mmc {
>  	bool hs400_tuning:1;
>  
>  	enum bus_mode user_speed_mode; /* input speed mode from user */
> +
> +	CONFIG_IS_ENABLED(CYCLIC, (struct cyclic_info cyclic));


I think that you can simplify this quite a lot by dropping all of the
the CONFIG_IS_ENABLED stuff. If CYCLIC is not enabled, struct
cyclic_info is an empty struct, so takes up no space, but it still
exists in struct mmc, allowing it to be referenced in C code that need
not be guarded.

>  };
>  
>  #if CONFIG_IS_ENABLED(DM_MMC)

> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
> index 0b881f11b4a..c787ff6bc49 100644
> --- a/drivers/mmc/mmc.c
> +++ b/drivers/mmc/mmc.c
> @@ -3039,6 +3039,20 @@ static int mmc_complete_init(struct mmc *mmc)
>  	return err;
>  }
>  
> +static void __maybe_unused mmc_cyclic_cd_poll(struct cyclic_info *c)
> +{

You can drop __maybe_unused.

> +	struct mmc *m = CONFIG_IS_ENABLED(CYCLIC, (container_of(c, struct mmc, cyclic)), (NULL));
> +

No need for the CONFIG_IS_ENABLED(), container_of works just fine when
the cyclic member exists unconditionally.

> +	if (!m->has_init)
> +		return;
> +

(I'd assume that in the !CYCLIC case the compiler might warn about this
unconditional NULL deref; that you avoid with the above.)

> +	if (mmc_getcd(m))
> +		return;
> +
> +	mmc_deinit(m);
> +	m->has_init = 0;
> +}
> +
>  int mmc_init(struct mmc *mmc)
>  {
>  	int err = 0;
> @@ -3061,6 +3075,14 @@ int mmc_init(struct mmc *mmc)
>  	if (err)
>  		pr_info("%s: %d, time %lu\n", __func__, err, get_timer(start));
>  
> +	if (CONFIG_IS_ENABLED(CYCLIC, (!mmc->cyclic.func), (NULL))) {
> +		/* Register cyclic function for card detect polling */
> +		CONFIG_IS_ENABLED(CYCLIC, (cyclic_register(&mmc->cyclic,
> +							   mmc_cyclic_cd_poll,
> +							   100 * 1000,
> +							   mmc->cfg->name)));
> +	}
> +

No need for any of the CONFIG_IS_ENABLED nesting. Just do
cyclic_register() - that's a no-op when !CYCLIC, and the compiler will
see the reference to mmc_cyclic_cd_poll, so not warn about it being
unused, yet also see that it is not actually called, so elide it from
the compiled code.

>  	return err;
>  }
>  
> @@ -3068,6 +3090,9 @@ int mmc_deinit(struct mmc *mmc)
>  {
>  	u32 caps_filtered;
>  
> +	if (CONFIG_IS_ENABLED(CYCLIC, (mmc->cyclic.func), (NULL)))
> +		CONFIG_IS_ENABLED(CYCLIC, (cyclic_unregister(&mmc->cyclic)));
> +

Again, just do cyclic_unregister() unconditionally.

Rasmus