[PATCH 0/5] mmc: sdhci: Misc patches

[PATCH 0/5] mmc: sdhci: Misc patches

[Adrian Hunter]

Hi

Here are 5 miscellaneous patches for sdhci.


Adrian Hunter (5):
      mmc: sdhci-pci: Set MMC_CAP_AGGRESSIVE_PM for Broxton controllers
      mmc: sdhci-acpi: Set MMC_CAP_AGGRESSIVE_PM for Broxton controllers
      mmc: sdhci: Remove redundant condition
      mmc: sdhci: Fix error paths in sdhci_add_host()
      mmc: sdhci: Tidy together LED code


Regards
Adrian

[PATCH 1/5] mmc: sdhci-pci: Set MMC_CAP_AGGRESSIVE_PM for Broxton controllers

[Adrian Hunter]

Set MMC_CAP_AGGRESSIVE_PM for Broxton host controllers.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/host/sdhci-pci-core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
index e5c6a4917682..97d4eebd6bf5 100644
--- a/drivers/mmc/host/sdhci-pci-core.c
+++ b/drivers/mmc/host/sdhci-pci-core.c
@@ -386,8 +386,10 @@ static int byt_sd_probe_slot(struct sdhci_pci_slot *slot)
 	slot->cd_override_level = true;
 	if (slot->chip->pdev->device == PCI_DEVICE_ID_INTEL_BXT_SD ||
 	    slot->chip->pdev->device == PCI_DEVICE_ID_INTEL_BXTM_SD ||
-	    slot->chip->pdev->device == PCI_DEVICE_ID_INTEL_APL_SD)
+	    slot->chip->pdev->device == PCI_DEVICE_ID_INTEL_APL_SD) {
 		slot->host->mmc_host_ops.get_cd = bxt_get_cd;
+		slot->host->mmc->caps |= MMC_CAP_AGGRESSIVE_PM;
+	}
 
 	return 0;
 }
-- 
1.9.1

[PATCH 2/5] mmc: sdhci-acpi: Set MMC_CAP_AGGRESSIVE_PM for Broxton controllers

[Adrian Hunter]

Set MMC_CAP_AGGRESSIVE_PM for Broxton host controllers.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/host/sdhci-acpi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
index 1ed8ea6ff155..6106354c6c17 100644
--- a/drivers/mmc/host/sdhci-acpi.c
+++ b/drivers/mmc/host/sdhci-acpi.c
@@ -188,8 +188,10 @@ static int sdhci_acpi_sd_probe_slot(struct platform_device *pdev,
 
 	/* Platform specific code during sd probe slot goes here */
 
-	if (hid && !strcmp(hid, "80865ACA"))
+	if (hid && !strcmp(hid, "80865ACA")) {
 		host->mmc_host_ops.get_cd = bxt_get_cd;
+		host->mmc->caps |= MMC_CAP_AGGRESSIVE_PM;
+	}
 
 	return 0;
 }
-- 
1.9.1

[PATCH 3/5] mmc: sdhci: Remove redundant condition

[Adrian Hunter]

The logic '!mmc.f_max || (mmc.f_max && mmc.f_max > max_clk)'
is equivalent to '!mmc.f_max || mmc.f_max > max_clk'.

Reported-by: David Binderman <dcb314@hotmail.com>
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/host/sdhci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index c6dbe65ada20..3e221e9a7b52 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2992,7 +2992,7 @@ int sdhci_add_host(struct sdhci_host *host)
 	} else
 		mmc->f_min = host->max_clk / SDHCI_MAX_DIV_SPEC_200;
 
-	if (!mmc->f_max || (mmc->f_max && (mmc->f_max > max_clk)))
+	if (!mmc->f_max || mmc->f_max > max_clk)
 		mmc->f_max = max_clk;
 
 	if (!(host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK)) {
-- 
1.9.1

[PATCH 4/5] mmc: sdhci: Fix error paths in sdhci_add_host()

[Adrian Hunter]

Some error paths in sdhci_add_host() simply returned without
cleaning up.  Also the return value from mmc_add_host()
was not being checked.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/host/sdhci.c | 36 +++++++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 3e221e9a7b52..be5b5c95138c 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2955,7 +2955,8 @@ int sdhci_add_host(struct sdhci_host *host)
 		if (!host->ops->get_max_clock) {
 			pr_err("%s: Hardware doesn't specify base clock frequency.\n",
 			       mmc_hostname(mmc));
-			return -ENODEV;
+			ret = -ENODEV;
+			goto undma;
 		}
 		host->max_clk = host->ops->get_max_clock(host);
 	}
@@ -3005,7 +3006,8 @@ int sdhci_add_host(struct sdhci_host *host)
 			} else {
 				pr_err("%s: Hardware doesn't specify timeout clock frequency.\n",
 					mmc_hostname(mmc));
-				return -ENODEV;
+				ret = -ENODEV;
+				goto undma;
 			}
 		}
 
@@ -3059,8 +3061,9 @@ int sdhci_add_host(struct sdhci_host *host)
 		mmc->caps |= MMC_CAP_NEEDS_POLL;
 
 	/* If there are external regulators, get them */
-	if (mmc_regulator_get_supply(mmc) == -EPROBE_DEFER)
-		return -EPROBE_DEFER;
+	ret = mmc_regulator_get_supply(mmc);
+	if (ret == -EPROBE_DEFER)
+		goto undma;
 
 	/* If vqmmc regulator and no 1.8V signalling, then there's no UHS */
 	if (!IS_ERR(mmc->supply.vqmmc)) {
@@ -3217,7 +3220,8 @@ int sdhci_add_host(struct sdhci_host *host)
 	if (mmc->ocr_avail == 0) {
 		pr_err("%s: Hardware doesn't report any support voltages.\n",
 		       mmc_hostname(mmc));
-		return -ENODEV;
+		ret = -ENODEV;
+		goto unreg;
 	}
 
 	spin_lock_init(&host->lock);
@@ -3313,13 +3317,15 @@ int sdhci_add_host(struct sdhci_host *host)
 	if (ret) {
 		pr_err("%s: Failed to register LED device: %d\n",
 		       mmc_hostname(mmc), ret);
-		goto reset;
+		goto unirq;
 	}
 #endif
 
 	mmiowb();
 
-	mmc_add_host(mmc);
+	ret = mmc_add_host(mmc);
+	if (ret)
+		goto unled;
 
 	pr_info("%s: SDHCI controller on %s [%s] using %s\n",
 		mmc_hostname(mmc), host->hw_name, dev_name(mmc_dev(mmc)),
@@ -3331,15 +3337,27 @@ int sdhci_add_host(struct sdhci_host *host)
 
 	return 0;
 
+unled:
 #ifdef SDHCI_USE_LEDS_CLASS
-reset:
+	led_classdev_unregister(&host->led);
+unirq:
+#endif
 	sdhci_do_reset(host, SDHCI_RESET_ALL);
 	sdhci_writel(host, 0, SDHCI_INT_ENABLE);
 	sdhci_writel(host, 0, SDHCI_SIGNAL_ENABLE);
 	free_irq(host->irq, host);
-#endif
 untasklet:
 	tasklet_kill(&host->finish_tasklet);
+unreg:
+	if (!IS_ERR(mmc->supply.vqmmc))
+		regulator_disable(mmc->supply.vqmmc);
+undma:
+	if (host->align_buffer)
+		dma_free_coherent(mmc_dev(mmc), host->align_buffer_sz +
+				  host->adma_table_sz, host->align_buffer,
+				  host->align_addr);
+	host->adma_table = NULL;
+	host->align_buffer = NULL;
 
 	return ret;
 }
-- 
1.9.1

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

[Adrian Hunter]

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)
 {
 	u8 ctrl;
 
@@ -255,7 +250,7 @@ static void sdhci_activate_led(struct sdhci_host *host)
 	sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
 }
 
-static void sdhci_deactivate_led(struct sdhci_host *host)
+static void __sdhci_led_deactivate(struct sdhci_host *host)
 {
 	u8 ctrl;
 
@@ -264,9 +259,11 @@ static void sdhci_deactivate_led(struct sdhci_host *host)
 	sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
 }
 
-#ifdef SDHCI_USE_LEDS_CLASS
+#if defined(CONFIG_LEDS_CLASS) || (defined(CONFIG_LEDS_CLASS_MODULE) && \
+				   defined(CONFIG_MMC_SDHCI_MODULE))
+
 static void sdhci_led_control(struct led_classdev *led,
-	enum led_brightness brightness)
+			      enum led_brightness brightness)
 {
 	struct sdhci_host *host = container_of(led, struct sdhci_host, led);
 	unsigned long flags;
@@ -277,12 +274,62 @@ static void sdhci_led_control(struct led_classdev *led,
 		goto out;
 
 	if (brightness == LED_OFF)
-		sdhci_deactivate_led(host);
+		__sdhci_led_deactivate(host);
 	else
-		sdhci_activate_led(host);
+		__sdhci_led_activate(host);
 out:
 	spin_unlock_irqrestore(&host->lock, flags);
 }
+
+static int sdhci_led_register(struct sdhci_host *host)
+{
+	struct mmc_host *mmc = host->mmc;
+
+	snprintf(host->led_name, sizeof(host->led_name),
+		 "%s::", mmc_hostname(mmc));
+
+	host->led.name = host->led_name;
+	host->led.brightness = LED_OFF;
+	host->led.default_trigger = mmc_hostname(mmc);
+	host->led.brightness_set = sdhci_led_control;
+
+	return led_classdev_register(mmc_dev(mmc), &host->led);
+}
+
+static void sdhci_led_unregister(struct sdhci_host *host)
+{
+	led_classdev_unregister(&host->led);
+}
+
+static inline void sdhci_led_activate(struct sdhci_host *host)
+{
+}
+
+static inline void sdhci_led_deactivate(struct sdhci_host *host)
+{
+}
+
+#else
+
+static inline int sdhci_led_register(struct sdhci_host *host)
+{
+	return 0;
+}
+
+static inline void sdhci_led_unregister(struct sdhci_host *host)
+{
+}
+
+static inline void sdhci_led_activate(struct sdhci_host *host)
+{
+	__sdhci_led_activate(host);
+}
+
+static inline void sdhci_led_deactivate(struct sdhci_host *host)
+{
+	__sdhci_led_deactivate(host);
+}
+
 #endif
 
 /*****************************************************************************\
@@ -1320,9 +1367,7 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 
 	WARN_ON(host->mrq != NULL);
 
-#ifndef SDHCI_USE_LEDS_CLASS
-	sdhci_activate_led(host);
-#endif
+	sdhci_led_activate(host);
 
 	/*
 	 * Ensure we don't send the STOP for non-SET_BLOCK_COUNTED
@@ -2183,9 +2228,7 @@ static void sdhci_tasklet_finish(unsigned long param)
 	host->cmd = NULL;
 	host->data = NULL;
 
-#ifndef SDHCI_USE_LEDS_CLASS
-	sdhci_deactivate_led(host);
-#endif
+	sdhci_led_deactivate(host);
 
 	mmiowb();
 	spin_unlock_irqrestore(&host->lock, flags);
@@ -3305,21 +3348,12 @@ int sdhci_add_host(struct sdhci_host *host)
 	sdhci_dumpregs(host);
 #endif
 
-#ifdef SDHCI_USE_LEDS_CLASS
-	snprintf(host->led_name, sizeof(host->led_name),
-		"%s::", mmc_hostname(mmc));
-	host->led.name = host->led_name;
-	host->led.brightness = LED_OFF;
-	host->led.default_trigger = mmc_hostname(mmc);
-	host->led.brightness_set = sdhci_led_control;
-
-	ret = led_classdev_register(mmc_dev(mmc), &host->led);
+	ret = sdhci_led_register(host);
 	if (ret) {
 		pr_err("%s: Failed to register LED device: %d\n",
 		       mmc_hostname(mmc), ret);
 		goto unirq;
 	}
-#endif
 
 	mmiowb();
 
@@ -3338,10 +3372,8 @@ int sdhci_add_host(struct sdhci_host *host)
 	return 0;
 
 unled:
-#ifdef SDHCI_USE_LEDS_CLASS
-	led_classdev_unregister(&host->led);
+	sdhci_led_unregister(host);
 unirq:
-#endif
 	sdhci_do_reset(host, SDHCI_RESET_ALL);
 	sdhci_writel(host, 0, SDHCI_INT_ENABLE);
 	sdhci_writel(host, 0, SDHCI_SIGNAL_ENABLE);
@@ -3389,9 +3421,7 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
 
 	mmc_remove_host(mmc);
 
-#ifdef SDHCI_USE_LEDS_CLASS
-	led_classdev_unregister(&host->led);
-#endif
+	sdhci_led_unregister(host);
 
 	if (!dead)
 		sdhci_do_reset(host, SDHCI_RESET_ALL);
-- 
1.9.1

Re: [PATCH 4/5] mmc: sdhci: Fix error paths in sdhci_add_host()

[Adrian Hunter]

On 13/04/16 14:41, Ulf Hansson wrote:
> [...]
> 
>>
>>         pr_info("%s: SDHCI controller on %s [%s] using %s\n",
>>                 mmc_hostname(mmc), host->hw_name, dev_name(mmc_dev(mmc)),
>> @@ -3331,15 +3337,27 @@ int sdhci_add_host(struct sdhci_host *host)
>>
>>         return 0;
>>
>> +unled:
>>  #ifdef SDHCI_USE_LEDS_CLASS
> 
> Shouldn't this ifdef be removed in the earlier patch?

It gets removed in the next patch.  Is that what you meant?

> 
>> -reset:
>> +       led_classdev_unregister(&host->led);
>> +unirq:
>> +#endif
>>         sdhci_do_reset(host, SDHCI_RESET_ALL);
>>         sdhci_writel(host, 0, SDHCI_INT_ENABLE);
>>         sdhci_writel(host, 0, SDHCI_SIGNAL_ENABLE);
>>         free_irq(host->irq, host);
>> -#endif
>>  untasklet:
>>         tasklet_kill(&host->finish_tasklet);
>> +unreg:
>> +       if (!IS_ERR(mmc->supply.vqmmc))
>> +               regulator_disable(mmc->supply.vqmmc);
>> +undma:
>> +       if (host->align_buffer)
>> +               dma_free_coherent(mmc_dev(mmc), host->align_buffer_sz +
>> +                                 host->adma_table_sz, host->align_buffer,
>> +                                 host->align_addr);
>> +       host->adma_table = NULL;
>> +       host->align_buffer = NULL;
>>
>>         return ret;
>>  }
>> --
>> 1.9.1
>>
> 
> Kind regards
> Uffe
> 

Re: [PATCH 4/5] mmc: sdhci: Fix error paths in sdhci_add_host()

[Ulf Hansson]

[...]

>
>         pr_info("%s: SDHCI controller on %s [%s] using %s\n",
>                 mmc_hostname(mmc), host->hw_name, dev_name(mmc_dev(mmc)),
> @@ -3331,15 +3337,27 @@ int sdhci_add_host(struct sdhci_host *host)
>
>         return 0;
>
> +unled:
>  #ifdef SDHCI_USE_LEDS_CLASS

Shouldn't this ifdef be removed in the earlier patch?

> -reset:
> +       led_classdev_unregister(&host->led);
> +unirq:
> +#endif
>         sdhci_do_reset(host, SDHCI_RESET_ALL);
>         sdhci_writel(host, 0, SDHCI_INT_ENABLE);
>         sdhci_writel(host, 0, SDHCI_SIGNAL_ENABLE);
>         free_irq(host->irq, host);
> -#endif
>  untasklet:
>         tasklet_kill(&host->finish_tasklet);
> +unreg:
> +       if (!IS_ERR(mmc->supply.vqmmc))
> +               regulator_disable(mmc->supply.vqmmc);
> +undma:
> +       if (host->align_buffer)
> +               dma_free_coherent(mmc_dev(mmc), host->align_buffer_sz +
> +                                 host->adma_table_sz, host->align_buffer,
> +                                 host->align_addr);
> +       host->adma_table = NULL;
> +       host->align_buffer = NULL;
>
>         return ret;
>  }
> --
> 1.9.1
>

Kind regards
Uffe

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

[Ulf Hansson]

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?

>  {
>         u8 ctrl;
>
> @@ -255,7 +250,7 @@ static void sdhci_activate_led(struct sdhci_host *host)
>         sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
>  }
>
> -static void sdhci_deactivate_led(struct sdhci_host *host)
> +static void __sdhci_led_deactivate(struct sdhci_host *host)
>  {
>         u8 ctrl;
>
> @@ -264,9 +259,11 @@ static void sdhci_deactivate_led(struct sdhci_host *host)
>         sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
>  }
>
> -#ifdef SDHCI_USE_LEDS_CLASS
> +#if defined(CONFIG_LEDS_CLASS) || (defined(CONFIG_LEDS_CLASS_MODULE) && \
> +                                  defined(CONFIG_MMC_SDHCI_MODULE))
> +
>  static void sdhci_led_control(struct led_classdev *led,
> -       enum led_brightness brightness)
> +                             enum led_brightness brightness)
>  {
>         struct sdhci_host *host = container_of(led, struct sdhci_host, led);
>         unsigned long flags;
> @@ -277,12 +274,62 @@ static void sdhci_led_control(struct led_classdev *led,
>                 goto out;
>
>         if (brightness == LED_OFF)
> -               sdhci_deactivate_led(host);
> +               __sdhci_led_deactivate(host);
>         else
> -               sdhci_activate_led(host);
> +               __sdhci_led_activate(host);
>  out:
>         spin_unlock_irqrestore(&host->lock, flags);
>  }
> +
> +static int sdhci_led_register(struct sdhci_host *host)
> +{
> +       struct mmc_host *mmc = host->mmc;
> +
> +       snprintf(host->led_name, sizeof(host->led_name),
> +                "%s::", mmc_hostname(mmc));
> +
> +       host->led.name = host->led_name;
> +       host->led.brightness = LED_OFF;
> +       host->led.default_trigger = mmc_hostname(mmc);
> +       host->led.brightness_set = sdhci_led_control;
> +
> +       return led_classdev_register(mmc_dev(mmc), &host->led);
> +}
> +
> +static void sdhci_led_unregister(struct sdhci_host *host)
> +{
> +       led_classdev_unregister(&host->led);
> +}
> +
> +static inline void sdhci_led_activate(struct sdhci_host *host)
> +{
> +}
> +
> +static inline void sdhci_led_deactivate(struct sdhci_host *host)
> +{
> +}

This seems wrong. I assume you actually want to control the registered
leds within this ifdef and not have the two functions above being
stubs?

> +
> +#else
> +
> +static inline int sdhci_led_register(struct sdhci_host *host)
> +{
> +       return 0;
> +}
> +
> +static inline void sdhci_led_unregister(struct sdhci_host *host)
> +{
> +}
> +
> +static inline void sdhci_led_activate(struct sdhci_host *host)
> +{
> +       __sdhci_led_activate(host);
> +}
> +
> +static inline void sdhci_led_deactivate(struct sdhci_host *host)
> +{
> +       __sdhci_led_deactivate(host);
> +}

See my comment above. Shouldn't these two functions be stubs?

> +
>  #endif
>
>  /*****************************************************************************\
> @@ -1320,9 +1367,7 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>
>         WARN_ON(host->mrq != NULL);
>
> -#ifndef SDHCI_USE_LEDS_CLASS
> -       sdhci_activate_led(host);
> -#endif
> +       sdhci_led_activate(host);
>
>         /*
>          * Ensure we don't send the STOP for non-SET_BLOCK_COUNTED
> @@ -2183,9 +2228,7 @@ static void sdhci_tasklet_finish(unsigned long param)
>         host->cmd = NULL;
>         host->data = NULL;
>
> -#ifndef SDHCI_USE_LEDS_CLASS
> -       sdhci_deactivate_led(host);
> -#endif
> +       sdhci_led_deactivate(host);
>
>         mmiowb();
>         spin_unlock_irqrestore(&host->lock, flags);
> @@ -3305,21 +3348,12 @@ int sdhci_add_host(struct sdhci_host *host)
>         sdhci_dumpregs(host);
>  #endif
>
> -#ifdef SDHCI_USE_LEDS_CLASS
> -       snprintf(host->led_name, sizeof(host->led_name),
> -               "%s::", mmc_hostname(mmc));
> -       host->led.name = host->led_name;
> -       host->led.brightness = LED_OFF;
> -       host->led.default_trigger = mmc_hostname(mmc);
> -       host->led.brightness_set = sdhci_led_control;
> -
> -       ret = led_classdev_register(mmc_dev(mmc), &host->led);
> +       ret = sdhci_led_register(host);
>         if (ret) {
>                 pr_err("%s: Failed to register LED device: %d\n",
>                        mmc_hostname(mmc), ret);
>                 goto unirq;
>         }
> -#endif
>
>         mmiowb();
>
> @@ -3338,10 +3372,8 @@ int sdhci_add_host(struct sdhci_host *host)
>         return 0;
>
>  unled:
> -#ifdef SDHCI_USE_LEDS_CLASS
> -       led_classdev_unregister(&host->led);
> +       sdhci_led_unregister(host);
>  unirq:
> -#endif
>         sdhci_do_reset(host, SDHCI_RESET_ALL);
>         sdhci_writel(host, 0, SDHCI_INT_ENABLE);
>         sdhci_writel(host, 0, SDHCI_SIGNAL_ENABLE);
> @@ -3389,9 +3421,7 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
>
>         mmc_remove_host(mmc);
>
> -#ifdef SDHCI_USE_LEDS_CLASS
> -       led_classdev_unregister(&host->led);
> -#endif
> +       sdhci_led_unregister(host);
>
>         if (!dead)
>                 sdhci_do_reset(host, SDHCI_RESET_ALL);
> --
> 1.9.1
>

Kind regards
Uffe

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

[Adrian Hunter]

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.

> 
>>  {
>>         u8 ctrl;
>>
>> @@ -255,7 +250,7 @@ static void sdhci_activate_led(struct sdhci_host *host)
>>         sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
>>  }
>>
>> -static void sdhci_deactivate_led(struct sdhci_host *host)
>> +static void __sdhci_led_deactivate(struct sdhci_host *host)
>>  {
>>         u8 ctrl;
>>
>> @@ -264,9 +259,11 @@ static void sdhci_deactivate_led(struct sdhci_host *host)
>>         sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
>>  }
>>
>> -#ifdef SDHCI_USE_LEDS_CLASS
>> +#if defined(CONFIG_LEDS_CLASS) || (defined(CONFIG_LEDS_CLASS_MODULE) && \
>> +                                  defined(CONFIG_MMC_SDHCI_MODULE))
>> +
>>  static void sdhci_led_control(struct led_classdev *led,
>> -       enum led_brightness brightness)
>> +                             enum led_brightness brightness)
>>  {
>>         struct sdhci_host *host = container_of(led, struct sdhci_host, led);
>>         unsigned long flags;
>> @@ -277,12 +274,62 @@ static void sdhci_led_control(struct led_classdev *led,
>>                 goto out;
>>
>>         if (brightness == LED_OFF)
>> -               sdhci_deactivate_led(host);
>> +               __sdhci_led_deactivate(host);
>>         else
>> -               sdhci_activate_led(host);
>> +               __sdhci_led_activate(host);
>>  out:
>>         spin_unlock_irqrestore(&host->lock, flags);
>>  }
>> +
>> +static int sdhci_led_register(struct sdhci_host *host)
>> +{
>> +       struct mmc_host *mmc = host->mmc;
>> +
>> +       snprintf(host->led_name, sizeof(host->led_name),
>> +                "%s::", mmc_hostname(mmc));
>> +
>> +       host->led.name = host->led_name;
>> +       host->led.brightness = LED_OFF;
>> +       host->led.default_trigger = mmc_hostname(mmc);
>> +       host->led.brightness_set = sdhci_led_control;
>> +
>> +       return led_classdev_register(mmc_dev(mmc), &host->led);
>> +}
>> +
>> +static void sdhci_led_unregister(struct sdhci_host *host)
>> +{
>> +       led_classdev_unregister(&host->led);
>> +}
>> +
>> +static inline void sdhci_led_activate(struct sdhci_host *host)
>> +{
>> +}
>> +
>> +static inline void sdhci_led_deactivate(struct sdhci_host *host)
>> +{
>> +}
> 
> This seems wrong. I assume you actually want to control the registered
> leds within this ifdef and not have the two functions above being
> stubs?

No, because this is the case where mmc core controls the LED and these are
the functions we call directly.

> 
>> +
>> +#else
>> +
>> +static inline int sdhci_led_register(struct sdhci_host *host)
>> +{
>> +       return 0;
>> +}
>> +
>> +static inline void sdhci_led_unregister(struct sdhci_host *host)
>> +{
>> +}
>> +
>> +static inline void sdhci_led_activate(struct sdhci_host *host)
>> +{
>> +       __sdhci_led_activate(host);
>> +}
>> +
>> +static inline void sdhci_led_deactivate(struct sdhci_host *host)
>> +{
>> +       __sdhci_led_deactivate(host);
>> +}
> 
> See my comment above. Shouldn't these two functions be stubs?
> 
>> +
>>  #endif
>>
>>  /*****************************************************************************\
>> @@ -1320,9 +1367,7 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>>
>>         WARN_ON(host->mrq != NULL);
>>
>> -#ifndef SDHCI_USE_LEDS_CLASS
>> -       sdhci_activate_led(host);
>> -#endif
>> +       sdhci_led_activate(host);
>>
>>         /*
>>          * Ensure we don't send the STOP for non-SET_BLOCK_COUNTED
>> @@ -2183,9 +2228,7 @@ static void sdhci_tasklet_finish(unsigned long param)
>>         host->cmd = NULL;
>>         host->data = NULL;
>>
>> -#ifndef SDHCI_USE_LEDS_CLASS
>> -       sdhci_deactivate_led(host);
>> -#endif
>> +       sdhci_led_deactivate(host);
>>
>>         mmiowb();
>>         spin_unlock_irqrestore(&host->lock, flags);
>> @@ -3305,21 +3348,12 @@ int sdhci_add_host(struct sdhci_host *host)
>>         sdhci_dumpregs(host);
>>  #endif
>>
>> -#ifdef SDHCI_USE_LEDS_CLASS
>> -       snprintf(host->led_name, sizeof(host->led_name),
>> -               "%s::", mmc_hostname(mmc));
>> -       host->led.name = host->led_name;
>> -       host->led.brightness = LED_OFF;
>> -       host->led.default_trigger = mmc_hostname(mmc);
>> -       host->led.brightness_set = sdhci_led_control;
>> -
>> -       ret = led_classdev_register(mmc_dev(mmc), &host->led);
>> +       ret = sdhci_led_register(host);
>>         if (ret) {
>>                 pr_err("%s: Failed to register LED device: %d\n",
>>                        mmc_hostname(mmc), ret);
>>                 goto unirq;
>>         }
>> -#endif
>>
>>         mmiowb();
>>
>> @@ -3338,10 +3372,8 @@ int sdhci_add_host(struct sdhci_host *host)
>>         return 0;
>>
>>  unled:
>> -#ifdef SDHCI_USE_LEDS_CLASS
>> -       led_classdev_unregister(&host->led);
>> +       sdhci_led_unregister(host);
>>  unirq:
>> -#endif
>>         sdhci_do_reset(host, SDHCI_RESET_ALL);
>>         sdhci_writel(host, 0, SDHCI_INT_ENABLE);
>>         sdhci_writel(host, 0, SDHCI_SIGNAL_ENABLE);
>> @@ -3389,9 +3421,7 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
>>
>>         mmc_remove_host(mmc);
>>
>> -#ifdef SDHCI_USE_LEDS_CLASS
>> -       led_classdev_unregister(&host->led);
>> -#endif
>> +       sdhci_led_unregister(host);
>>
>>         if (!dead)
>>                 sdhci_do_reset(host, SDHCI_RESET_ALL);
>> --
>> 1.9.1
>>
> 
> Kind regards
> Uffe
> 

Re: [PATCH 4/5] mmc: sdhci: Fix error paths in sdhci_add_host()

[Ulf Hansson]

On 13 April 2016 at 13:41, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 13/04/16 14:41, Ulf Hansson wrote:
>> [...]
>>
>>>
>>>         pr_info("%s: SDHCI controller on %s [%s] using %s\n",
>>>                 mmc_hostname(mmc), host->hw_name, dev_name(mmc_dev(mmc)),
>>> @@ -3331,15 +3337,27 @@ int sdhci_add_host(struct sdhci_host *host)
>>>
>>>         return 0;
>>>
>>> +unled:
>>>  #ifdef SDHCI_USE_LEDS_CLASS
>>
>> Shouldn't this ifdef be removed in the earlier patch?
>
> It gets removed in the next patch.  Is that what you meant?

Ahh, yes. I reviewed them in the wrong order. :-)

Kind regards
Uffe

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

[Ulf Hansson]

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.

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

[...]

Kind regards
Uffe

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

[Adrian Hunter]

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.

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

[Ulf Hansson]

[...]

>>>>>
>>>>> -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.

Huh. Yes, indeed you are right! Sorry for noise, again.

>
>>
>> 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.
>

I have applied all five patches for next! Thanks!

Still, perhaps a future clean-up can adopt to my proposal above!?

Kind regards
Uffe