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

[Marek Vasut]

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.