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

[Simon Glass]

Hi Marek,

On Wed, 13 Dec 2023 at 07:08, Marek Vasut <marek.vasut at mailbox.org> wrote:
>
> On 12/12/23 15:05, Simon Glass wrote:
> > Hi Marek,
> >
> > On Mon, 11 Dec 2023 at 12:24, Marek Vasut <marek.vasut at mailbox.org> wrote:
> >>
> >> On 12/11/23 18:52, Simon Glass wrote:
> >>> Hi Marek,
> >>>
> >>> On Sun, 10 Dec 2023 at 08:03, Marek Vasut
> >>> <marek.vasut+renesas at mailbox.org> 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 <marek.vasut+renesas at mailbox.org>
> >>>> ---
> >>>> Cc: Jaehoon Chung <jh80.chung at samsung.com>
> >>>> Cc: Peng Fan <peng.fan at nxp.com>
> >>>> Cc: Simon Glass <sjg at 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
> >>>> ---
> >>>>    drivers/mmc/mmc.c | 36 ++++++++++++++++++++++++++++++++++++
> >>>>    include/mmc.h     |  5 +++++
> >>>>    2 files changed, 41 insertions(+)
> >>>>
> >>>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
> >>>> index eb5010c1465..a5686dbc12e 100644
> >>>> --- a/drivers/mmc/mmc.c
> >>>> +++ b/drivers/mmc/mmc.c
> >>>> @@ -2985,6 +2985,22 @@ static int mmc_complete_init(struct mmc *mmc)
> >>>>           return err;
> >>>>    }
> >>>>
> >>>> +#if CONFIG_IS_ENABLED(CYCLIC)
> >>>> +static void mmc_cyclic_cd_poll(void *ctx)
> >>>> +{
> >>>> +       struct mmc *m = ctx;
> >>>> +
> >>>> +       if (!m->has_init)
> >>>> +               return;
> >>>> +
> >>>> +       if (mmc_getcd(m))
> >>>> +               return;
> >>>> +
> >>>> +       mmc_deinit(m);
> >>>> +       m->has_init = 0;
> >>>> +}
> >>>> +#endif
> >>>> +
> >>>>    int mmc_init(struct mmc *mmc)
> >>>>    {
> >>>>           int err = 0;
> >>>> @@ -3007,6 +3023,19 @@ 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)
> >>>
> >>> We really shouldn't be adding new #ifdefs to the code.
> >>>
> >>> If you really want to make put ->cyclic behind an #ifdef then how
> >>> about creating an accessor as is done in global_data.h ?
> >>
> >> That's really just working around the underlying issue, which is that
> >> this does not compile:
> >>
> >> struct foo {
> >> #if CONFIG_IS_ENABLED(STUFF)
> >>     type member;
> >> #endif
> >> ...
> >> };
> >>
> >> type fn() {
> >> ...
> >> if (CONFIG_IS_ENABLED(STUFF))
> >>     access struct->member;
> >> ...
> >> }
> >>
> >> Accessor won't make it any better, would it ? It would only attempt to
> >> hide the error and make the code more fragile, i.e. this:
> >>
> >> accessor(struct)
> >> {
> >> type fn() {
> >> if (CONFIG_IS_ENABLED(STUFF))
> >>     return access struct->member;
> >> else
> >>     return 0;
> >> }
> >>
> >> }
> >>
> >> type fn() {
> >> ...
> >>     accessor(struct);
> >> ...
> >> }
> >>
> >> I suspect it also won't compile for one thing, and for the other, it
> >> really just hides the issue.
> >
> > No, I mean like this:
> >
> > #ifdef CONFIG_ACPI
> > #define gd_acpi_ctx() gd->acpi_ctx
> > #define gd_acpi_start() gd->acpi_start
> > #define gd_set_acpi_start(addr) gd->acpi_start = addr
> > #else
> > #define gd_acpi_ctx() NULL
> > #define gd_acpi_start() 0UL
> > #define gd_set_acpi_start(addr)
> > #endif
> >
> > The header file has #ifdefs but not the C files.
>
> Hmmm, I can't say I like that either, that just adds more indirection
> into the code and makes it harder to read in my opinion .

Sure, it isn't for everyone. But adding new #ifdefs is really grim
IMO. Maybe you will get used to it? Maybe there is a better approach?

Regards,
Simon