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

[Marek Vasut]

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 .