[PATCH v3] mmc: Poll CD in case cyclic framework is enabled
Marek Vasut
marek.vasut at mailbox.org
Wed Dec 13 15:07:59 CET 2023
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 .
More information about the U-Boot
mailing list