Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210624061418.30361-1-tykwon@m2i.co.kr>
In-Reply-To: <20210624061418.30361-1-tykwon@m2i.co.kr>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Thu, 24 Jun 2021 14:21:00 +0200
Message-ID: <CAPDyKFrh9azyXNwFHxoe1svDQKRWbr5FpWz3V_JOD+Zv0cHcug@mail.gmail.com>
Subject: Re: [PATCH] mmc: core: Added support for LED trigger only when SD
 card is connected
To:     Kwon Tae-young <tykwon@m2i.co.kr>
Cc:     linux-mmc <linux-mmc@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, 24 Jun 2021 at 08:14, Kwon Tae-young <tykwon@m2i.co.kr> wrote:
>
> led_trigger_event() is always called.
> In this case, if the LED trigger is set to the SD Card, the trigger
> will occur even when the SD card is not connected and the LED will blink.
>
> In case of SD Card, it is judged based on Card Detection information and
> changes to generate LED trigger only when SD Card is connected.
>
> Board tested: NXP i.MX 8M board
>
> Signed-off-by: Kwon Tae-young <tykwon@m2i.co.kr>
> ---
>  drivers/mmc/core/core.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index f194940c5974..b3156f6c5cfa 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -352,7 +352,11 @@ int mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
>         if (err)
>                 return err;
>
> -       led_trigger_event(host->led, LED_FULL);
> +       if (host->ops->get_cd)

No, this is not the right thing to do. Invoking the ->get_cd()
callback, for every request is suboptimal and would likely have
effects on performance.

Moreover, I wonder how big an issue it is to use the led here. If the
card is being removed, the request will fail anyway, so the led should
soon stop flashing anyway, right?

> +               host->ops->get_cd(host) ? led_trigger_event(host->led, LED_FULL) : NULL;
> +       else
> +               led_trigger_event(host->led, LED_FULL);
> +
>         __mmc_start_request(host, mrq);
>
>         return 0;
> --
> 2.17.1
>

Kind regards
Uffe