Re: [PATCH] mmc: move regulator handling to core

From: Adrian Hunter
Date: Thu Dec 03 2009 - 09:29:29 EST

gDaniel Mack wrote:
At the moment, regulator operations are done from individual mmc host
drivers. This is a problem because the regulators are not claimed
exclusively but the mmc core enables and disables them according to the
return value of regulator_is_enabled(). That can lead to a number of
problems and warnings when regulators are shared among multiple
consumers or if regulators are marked as 'always_on'.

Fix this by moving the some logic to the core, and put the regulator
reference to the mmc_host struct and let it do its own supply state
tracking so that the reference counting in the regulator won't get
confused.

[Note that I could only compile-test the mmci.c change]

Signed-off-by: Daniel Mack <daniel@xxxxxxxx>
Cc: Mark Brown <broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx>
Cc: Liam Girdwood <lrg@xxxxxxxxxxxxxxx>
Cc: Pierre Ossman <pierre@xxxxxxxxx>
Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
Cc: Matt Fleming <matt@xxxxxxxxxxxxxxxxx>
Cc: Adrian Hunter <adrian.hunter@xxxxxxxxx>
Cc: David Brownell <dbrownell@xxxxxxxxxxxxxxxxxxxxx>
Cc: Russell King <rmk+kernel@xxxxxxxxxxxxxxxx>
Cc: Linus Walleij <linus.walleij@xxxxxxxxxxxxxx>
Cc: Eric Miao <eric.y.miao@xxxxxxxxx>
Cc: Robert Jarzmik <robert.jarzmik@xxxxxxx>
Cc: Cliff Brake <cbrake@xxxxxxxxxxxxxxx>
Cc: Jarkko Lavinen <jarkko.lavinen@xxxxxxxxx>
Cc: linux-mmc@xxxxxxxxxxxxxxx
Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
---
drivers/mmc/core/core.c | 36 ++++++++++++++++++++----------------
drivers/mmc/core/host.c | 3 +++
drivers/mmc/host/mmci.c | 28 ++++++++++++----------------
drivers/mmc/host/mmci.h | 1 -
drivers/mmc/host/pxamci.c | 20 ++++++++------------
include/linux/mmc/host.h | 10 ++++++----

What about arch/arm/mach-omap2/mmc-twl4030.c ?


6 files changed, 49 insertions(+), 49 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 7dab2e5..9acb655 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -727,13 +727,13 @@ EXPORT_SYMBOL(mmc_vddrange_to_ocrmask);
* regulator. This would normally be called before registering the
* MMC host adapter.
*/
-int mmc_regulator_get_ocrmask(struct regulator *supply)
+int mmc_regulator_get_ocrmask(struct mmc_host *host)
{
int result = 0;
int count;
int i;
- count = regulator_count_voltages(supply);
+ count = regulator_count_voltages(host->vcc);
if (count < 0)
return count;
@@ -741,7 +741,7 @@ int mmc_regulator_get_ocrmask(struct regulator *supply)
int vdd_uV;
int vdd_mV;
- vdd_uV = regulator_list_voltage(supply, i);
+ vdd_uV = regulator_list_voltage(host->vcc, i);
if (vdd_uV <= 0)
continue;
@@ -755,24 +755,22 @@ EXPORT_SYMBOL(mmc_regulator_get_ocrmask);
/**
* mmc_regulator_set_ocr - set regulator to match host->ios voltage
+ * @host: the mmc_host
* @vdd_bit: zero for power off, else a bit number (host->ios.vdd)
- * @supply: regulator to use
*
* Returns zero on success, else negative errno.
*
* MMC host drivers may use this to enable or disable a regulator using
- * a particular supply voltage. This would normally be called from the
+ * the registered supply voltage. This would normally be called from the
* set_ios() method.
*/
-int mmc_regulator_set_ocr(struct regulator *supply, unsigned short vdd_bit)
+int mmc_regulator_set_ocr(struct mmc_host *host, unsigned short vdd_bit)
{
int result = 0;
int min_uV, max_uV;
- int enabled;
- enabled = regulator_is_enabled(supply);
- if (enabled < 0)
- return enabled;
+ if (!host->vcc || IS_ERR(host->vcc))
+ return -EINVAL;
if (vdd_bit) {
int tmp;
@@ -795,18 +793,24 @@ int mmc_regulator_set_ocr(struct regulator *supply, unsigned short vdd_bit)
/* avoid needless changes to this voltage; the regulator
* might not allow this operation
*/
- voltage = regulator_get_voltage(supply);
+ voltage = regulator_get_voltage(host->vcc);
if (voltage < 0)
result = voltage;
else if (voltage < min_uV || voltage > max_uV)
- result = regulator_set_voltage(supply, min_uV, max_uV);
+ result = regulator_set_voltage(host->vcc, min_uV, max_uV);
else
result = 0;
- if (result == 0 && !enabled)
- result = regulator_enable(supply);
- } else if (enabled) {
- result = regulator_disable(supply);
+ if (result == 0 && !host->vcc_enabled) {
+ result = regulator_enable(host->vcc);
+
+ if (result == 0)
+ host->vcc_enabled = 1;
+ }
+ } else if (host->vcc_enabled) {
+ result = regulator_disable(host->vcc);
+ if (result == 0)
+ host->vcc_enabled = 0;
}
return result;
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index a268d12..f422d1f 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -18,6 +18,7 @@
#include <linux/leds.h>
#include <linux/mmc/host.h>
+#include <linux/regulator/consumer.h>
#include "core.h"
#include "host.h"
@@ -154,6 +155,8 @@ void mmc_remove_host(struct mmc_host *host)
mmc_remove_host_debugfs(host);
#endif
+ regulator_put(host->vcc);
+

If the core is doing a 'regulator_put()' shouldn't it also be doing
a 'regulator_get()'? Why not leave it to the drivers?

device_del(&host->class_dev);
led_trigger_unregister_simple(host->led);
diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 705a589..eea9cde 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -455,15 +455,15 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
switch (ios->power_mode) {
case MMC_POWER_OFF:
- if(host->vcc &&
- regulator_is_enabled(host->vcc))
- regulator_disable(host->vcc);
+ if(mmc->vcc && mmc->vcc_enabled) {
+ regulator_disable(mmc->vcc);
+ mmc->vcc_enabled = 0;
+ }
break;
case MMC_POWER_UP:
#ifdef CONFIG_REGULATOR
- if (host->vcc)
- /* This implicitly enables the regulator */
- mmc_regulator_set_ocr(host->vcc, ios->vdd);
+ /* This implicitly enables the regulator */
+ mmc_regulator_set_ocr(mmc, ios->vdd);
#endif
/*
* The translate_vdd function is not used if you have
@@ -473,7 +473,7 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
* a regulator, we might have some other platform specific
* power control behind this translate function.
*/
- if (!host->vcc && host->plat->translate_vdd)
+ if (!mmc->vcc && host->plat->translate_vdd)
pwr |= host->plat->translate_vdd(mmc_dev(mmc), ios->vdd);
/* The ST version does not have this, fall through to POWER_ON */
if (host->hw_designer != AMBA_VENDOR_ST) {
@@ -621,11 +621,11 @@ static int __devinit mmci_probe(struct amba_device *dev, struct amba_id *id)
mmc->f_max = min(host->mclk, fmax);
#ifdef CONFIG_REGULATOR
/* If we're using the regulator framework, try to fetch a regulator */
- host->vcc = regulator_get(&dev->dev, "vmmc");
- if (IS_ERR(host->vcc))
- host->vcc = NULL;
+ mmc->vcc = regulator_get(&dev->dev, "vmmc");
+ if (IS_ERR(mmc->vcc))
+ mmc->vcc = NULL;
else {
- int mask = mmc_regulator_get_ocrmask(host->vcc);
+ int mask = mmc_regulator_get_ocrmask(mmc);
if (mask < 0)
dev_err(&dev->dev, "error getting OCR mask (%d)\n",
@@ -640,7 +640,7 @@ static int __devinit mmci_probe(struct amba_device *dev, struct amba_id *id)
}
#endif
/* Fall back to platform data if no regulator is found */
- if (host->vcc == NULL)
+ if (mmc->vcc == NULL)
mmc->ocr_avail = plat->ocr_mask;
mmc->caps = plat->capabilities;
@@ -777,10 +777,6 @@ static int __devexit mmci_remove(struct amba_device *dev)
clk_disable(host->clk);
clk_put(host->clk);
- if (regulator_is_enabled(host->vcc))
- regulator_disable(host->vcc);
- regulator_put(host->vcc);
-
mmc_free_host(mmc);
amba_release_regions(dev);
diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
index 1ceb9a9..a7f9a51 100644
--- a/drivers/mmc/host/mmci.h
+++ b/drivers/mmc/host/mmci.h
@@ -175,7 +175,6 @@ struct mmci_host {
struct scatterlist *sg_ptr;
unsigned int sg_off;
unsigned int size;
- struct regulator *vcc;
};
static inline void mmci_init_sg(struct mmci_host *host, struct mmc_data *data)
diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
index fb0978c..25d2367 100644
--- a/drivers/mmc/host/pxamci.c
+++ b/drivers/mmc/host/pxamci.c
@@ -69,25 +69,23 @@ struct pxamci_host {
unsigned int dma_dir;
unsigned int dma_drcmrrx;
unsigned int dma_drcmrtx;
-
- struct regulator *vcc;
};
static inline void pxamci_init_ocr(struct pxamci_host *host)
{
#ifdef CONFIG_REGULATOR
- host->vcc = regulator_get(mmc_dev(host->mmc), "vmmc");
+ host->mmc->vcc = regulator_get(mmc_dev(host->mmc), "vmmc");
- if (IS_ERR(host->vcc))
- host->vcc = NULL;
+ if (IS_ERR(host->mmc->vcc))
+ host->mmc->vcc = NULL;
else {
- host->mmc->ocr_avail = mmc_regulator_get_ocrmask(host->vcc);
+ host->mmc->ocr_avail = mmc_regulator_get_ocrmask(host->mmc);
if (host->pdata && host->pdata->ocr_mask)
dev_warn(mmc_dev(host->mmc),
"given ocr_mask will not be used\n");
}
#endif
- if (host->vcc == NULL) {
+ if (host->mmc->vcc == NULL) {
/* fall-back to platform data */
host->mmc->ocr_avail = host->pdata ?
host->pdata->ocr_mask :
@@ -100,10 +98,10 @@ static inline void pxamci_set_power(struct pxamci_host *host, unsigned int vdd)
int on;
#ifdef CONFIG_REGULATOR
- if (host->vcc)
- mmc_regulator_set_ocr(host->vcc, vdd);
+ if (host->mmc->vcc)
+ mmc_regulator_set_ocr(host->mmc, vdd);
#endif
- if (!host->vcc && host->pdata &&
+ if (!host->mmc->vcc && host->pdata &&
gpio_is_valid(host->pdata->gpio_power)) {
on = ((1 << vdd) & host->pdata->ocr_mask);
gpio_set_value(host->pdata->gpio_power,
@@ -775,8 +773,6 @@ static int pxamci_remove(struct platform_device *pdev)
gpio_free(gpio_ro);
if (gpio_is_valid(gpio_power))
gpio_free(gpio_power);
- if (host->vcc)
- regulator_put(host->vcc);
if (host->pdata && host->pdata->exit)
host->pdata->exit(&pdev->dev, mmc);
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index eaf3636..2c1b079 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -111,6 +111,7 @@ struct mmc_host_ops {
struct mmc_card;
struct device;
+struct regulator;
struct mmc_host {
struct device *parent;
@@ -203,6 +204,9 @@ struct mmc_host {
struct dentry *debugfs_root;
+ struct regulator *vcc;
+ unsigned int vcc_enabled:1;
+
unsigned long private[0] ____cacheline_aligned;
};
@@ -237,10 +241,8 @@ static inline void mmc_signal_sdio_irq(struct mmc_host *host)
wake_up_process(host->sdio_irq_thread);
}
-struct regulator;
-
-int mmc_regulator_get_ocrmask(struct regulator *supply);
-int mmc_regulator_set_ocr(struct regulator *supply, unsigned short vdd_bit);
+int mmc_regulator_get_ocrmask(struct mmc_host *host);
+int mmc_regulator_set_ocr(struct mmc_host *host, unsigned short vdd_bit);
int mmc_card_awake(struct mmc_host *host);
int mmc_card_sleep(struct mmc_host *host);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/