Re: [PATCH 5/5] mmc: sdhci: Tidy together LED code

[Adrian Hunter <adrian.hunter@intel.com>]

On 13/04/16 15:15, Ulf Hansson wrote:
> On 13 April 2016 at 13:45, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 13/04/16 14:42, Ulf Hansson wrote:
>>> On 12 April 2016 at 13:25, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>> ifdef's make the code more complicated and harder to read.
>>>> Move all the LED code together to reduce the ifdef's to
>>>> one place.
>>>>
>>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>>> ---
>>>>  drivers/mmc/host/sdhci.c | 96 +++++++++++++++++++++++++++++++-----------------
>>>>  1 file changed, 63 insertions(+), 33 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>>> index be5b5c95138c..13f3dd8992d5 100644
>>>> --- a/drivers/mmc/host/sdhci.c
>>>> +++ b/drivers/mmc/host/sdhci.c
>>>> @@ -38,11 +38,6 @@
>>>>  #define DBG(f, x...) \
>>>>         pr_debug(DRIVER_NAME " [%s()]: " f, __func__,## x)
>>>>
>>>> -#if defined(CONFIG_LEDS_CLASS) || (defined(CONFIG_LEDS_CLASS_MODULE) && \
>>>> -       defined(CONFIG_MMC_SDHCI_MODULE))
>>>> -#define SDHCI_USE_LEDS_CLASS
>>>> -#endif
>>>> -
>>>>  #define MAX_TUNING_LOOP 40
>>>>
>>>>  static unsigned int debug_quirks = 0;
>>>> @@ -246,7 +241,7 @@ static void sdhci_reinit(struct sdhci_host *host)
>>>>         sdhci_enable_card_detection(host);
>>>>  }
>>>>
>>>> -static void sdhci_activate_led(struct sdhci_host *host)
>>>> +static void __sdhci_led_activate(struct sdhci_host *host)
>>>
>>> The renaming here seems a bit unnecessary, but more importantly why
>>> can't you move these functions within the "if def sdhci leds" instead?
>>
>> They get used either way.  Either we use the LEDS subsystem (and mmc core
>> controls them) or we turn them on/off directly from sdhci_request() etc.
> 
> That seems wrong. Moreover it changes the current behaviour.

How does it change the current behaviour?  It was meant to be exactly the
same.  Perhaps looking closely at whether the original code was 'ifdef' or
'ifndef' will help.

> 
> I am not sure why you want to use the leds in case of when the LED
> subsystem isn't available?

I am attempting to preserve the existing behaviour, so that is just how it was.