On Mon, Sep 09, 2024 at 10:46:21AM +0200, Rasmus Villemoes wrote: > Marek Vasut <[email protected]> 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 <[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(+) > > > > [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.
The challenge here is that Simon asked for all of this as part of feedback for v3. What are your thoughts here, Simon? -- Tom
signature.asc
Description: PGP signature
