Hi Marek, On Fri, 6 Sept 2024 at 11:11, Marek Vasut <[email protected]> wrote: > > 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 <[email protected]> > --- > Cc: Jaehoon Chung <[email protected]> > Cc: Peng Fan <[email protected]> > Cc: Simon Glass <[email protected]> > --- > 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(+) >
Reviewed-by: Simon Glass <[email protected]> Some things below to do now or later. > 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) Add a comment here to explain what is going on - e.g. stuff from your comment message. > +{ > + struct mmc *m = CONFIG_IS_ENABLED(CYCLIC, (container_of(c, struct > mmc, cyclic)), (NULL)); > + > + if (!m->has_init) > + return; > + > + 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))); > + } > + This seems a bit confusing. How about: if (CONFIG_IS_ENABLED(CYCLIC) && !cyclic_mmc->cyclic.func) then in cyclic.h something like static inline bool cyclic_valid(struct cyclic *cyc) { #if CONFIG_IS_ENABLED(CYCLIC) return cyc->func; #else return false; #endif } We really should not have clients looking at the 'func' to decide if something is active. You can also turn cyclic_register into a macro (empty if cyclic not enabled) to avoid the problem of ->cyclic not existing. > 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))); Same comment here. > + > if (!CONFIG_IS_ENABLED(MMC_UHS_SUPPORT) && > !CONFIG_IS_ENABLED(MMC_HS200_SUPPORT) && > !CONFIG_IS_ENABLED(MMC_HS400_SUPPORT)) > 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)); > }; > > #if CONFIG_IS_ENABLED(DM_MMC) > -- > 2.45.2 > Regards, Simon
