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

[Simon Glass]

Hi Marek,

On Wed, 13 Dec 2023 at 14:00, Marek Vasut <marek.vasut at mailbox.org> wrote:
>
> On 12/13/23 20:49, Simon Glass wrote:
> > 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?
>
> I don't really want to get used to such workarounds which only obfuscate
> the code and decrease readability.
>
> > Maybe there is a better approach?
>
> I am open to suggestions . If there are none, I would say go with the
> ifdefs .

OK

Regards,
Simon