Re: [PATCH] MMC: remove regulator refcount fiddling in mmc core

From: Adrian Hunter
Date: Mon Jun 28 2010 - 03:26:25 EST

Linus Walleij wrote:
Currently the mmc_regulator_set_ocr() fiddles with the regulator
refcount by selectively calling regulator_[enable|disable]
depending on the state of the regulator. This will confuse the
reference count if case the regulator is for example shared with
other MMC slots or user for other stuff than the MMC card.

Push regulator_[enable|disable] out into the MMC host drivers
and remove this from the MMC core so the reference count can be
trusted.

That is not a technical reason. It would be better to provide
more regulator support in core so there is less duplication in
the drivers.

At least it should be a separate patch.


Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
Cc: Liam Girdwood <lrg@xxxxxxxxxxxxxxx>
Cc: Mark Brown <broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx>
Cc: Tony Lindgren <tony@xxxxxxxxxxx>
Cc: Adrian Hunter <adrian.hunter@xxxxxxxxx>
Cc: Robert Jarzmik <robert.jarzmik@xxxxxxx>
Cc: Sundar Iyer <sundar.iyer@xxxxxxxxxxxxxx>
Cc: Bengt Jonsson <bengt.jonsson@xxxxxxxxxxxxxx>
Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxxxxxx>
---
This has been regression compiled for U300, OMAP3, PXA3XX
defconfigs, tested on U300.

We're facing problems with our regulator reference counters if
this is not fixed.
---
drivers/mmc/core/core.c | 66 ++++++++++++++++++-----------------------
drivers/mmc/host/mmci.c | 10 ++++--
drivers/mmc/host/omap_hsmmc.c | 40 ++++++++++++++++++------
drivers/mmc/host/pxamci.c | 17 ++++++++--
4 files changed, 78 insertions(+), 55 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 569e94d..904f245 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -784,47 +784,39 @@ int mmc_regulator_set_ocr(struct regulator *supply, unsigned short vdd_bit)
{
int result = 0;
int min_uV, max_uV;
- int enabled;
+ int tmp;
+ int voltage;
- enabled = regulator_is_enabled(supply);
- if (enabled < 0)
- return enabled;
-
- if (vdd_bit) {
- int tmp;
- int voltage;
-
- /* REVISIT mmc_vddrange_to_ocrmask() may have set some
- * bits this regulator doesn't quite support ... don't
- * be too picky, most cards and regulators are OK with
- * a 0.1V range goof (it's a small error percentage).
- */
- tmp = vdd_bit - ilog2(MMC_VDD_165_195);
- if (tmp == 0) {
- min_uV = 1650 * 1000;
- max_uV = 1950 * 1000;
- } else {
- min_uV = 1900 * 1000 + tmp * 100 * 1000;
- max_uV = min_uV + 100 * 1000;
- }
-
- /* avoid needless changes to this voltage; the regulator
- * might not allow this operation
- */
- voltage = regulator_get_voltage(supply);
- if (voltage < 0)
- result = voltage;
- else if (voltage < min_uV || voltage > max_uV)
- result = regulator_set_voltage(supply, min_uV, max_uV);
- else
- result = 0;
+ if (!vdd_bit)
+ return 0;
- if (result == 0 && !enabled)
- result = regulator_enable(supply);
- } else if (enabled) {
- result = regulator_disable(supply);
+ /*
+ * REVISIT mmc_vddrange_to_ocrmask() may have set some
+ * bits this regulator doesn't quite support ... don't
+ * be too picky, most cards and regulators are OK with
+ * a 0.1V range goof (it's a small error percentage).
+ */
+ tmp = vdd_bit - ilog2(MMC_VDD_165_195);
+ if (tmp == 0) {
+ min_uV = 1650 * 1000;
+ max_uV = 1950 * 1000;
+ } else {
+ min_uV = 1900 * 1000 + tmp * 100 * 1000;
+ max_uV = min_uV + 100 * 1000;
}
+ /*
+ * Avoid needless changes to this voltage; the regulator
+ * might not allow this operation
+ */
+ voltage = regulator_get_voltage(supply);
+ if (voltage < 0)
+ result = voltage;
+ else if (voltage < min_uV || voltage > max_uV)
+ result = regulator_set_voltage(supply, min_uV, max_uV);
+ else
+ result = 0;
+
return result;
}
EXPORT_SYMBOL(mmc_regulator_set_ocr);
diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 4917af9..5f530b1 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -467,15 +467,17 @@ 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))
+ if (host->vcc)
regulator_disable(host->vcc);
break;
case MMC_POWER_UP:
#ifdef CONFIG_REGULATOR
- if (host->vcc)
- /* This implicitly enables the regulator */
+ if (host->vcc) {
+ if (regulator_enable(host->vcc))
+ dev_err(mmc_dev(mmc),
+ "could not enable regulator\n");
mmc_regulator_set_ocr(host->vcc, ios->vdd);
+ }
#endif
/*
* The translate_vdd function is not used if you have
diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index b032828..d8d07e1 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -247,10 +247,17 @@ static int omap_hsmmc_1_set_power(struct device *dev, int slot, int power_on,
if (mmc_slot(host).before_set_reg)
mmc_slot(host).before_set_reg(dev, slot, power_on, vdd);
- if (power_on)
- ret = mmc_regulator_set_ocr(host->vcc, vdd);
- else
- ret = mmc_regulator_set_ocr(host->vcc, 0);
+ if (power_on) {
+ ret = regulator_enable(host->vcc);
+ if (ret)
+ dev_err(dev, "could not enable regulator\n");
+ else
+ ret = mmc_regulator_set_ocr(host->vcc, vdd);
+ } else {
+ ret = regulator_disable(host->vcc);
+ if (ret)
+ dev_err(dev, "could not disable regulator\n");
+ }

Enabling the regulator before setting the voltage seems like it could
create problems, for example, enabling at an invalid voltage before
ramping to the correct voltage. Also, while regulator_enable typically
waits for the regulator to stablize, regulator_set_voltage typically
does not.

The original code was the other way around.

if (mmc_slot(host).after_set_reg)
mmc_slot(host).after_set_reg(dev, slot, power_on, vdd);
@@ -289,7 +296,11 @@ static int omap_hsmmc_23_set_power(struct device *dev, int slot, int power_on,
* chips/cards need an interface voltage rail too.
*/
if (power_on) {
- ret = mmc_regulator_set_ocr(host->vcc, vdd);
+ ret = regulator_enable(host->vcc);
+ if (ret < 0)
+ dev_err(dev, "could not enable regulator\n");
+ else
+ ret = mmc_regulator_set_ocr(host->vcc, vdd);
/* Enable interface voltage rail, if needed */
if (ret == 0 && host->vcc_aux) {
ret = regulator_enable(host->vcc_aux);
@@ -297,10 +308,15 @@ static int omap_hsmmc_23_set_power(struct device *dev, int slot, int power_on,
ret = mmc_regulator_set_ocr(host->vcc, 0);
}
} else {
+ /* Shut down the rail */
if (host->vcc_aux)
ret = regulator_disable(host->vcc_aux);
- if (ret == 0)
- ret = mmc_regulator_set_ocr(host->vcc, 0);
+ if (ret == 0) {
+ /* Then proeeed to shut down the local regulator */
+ ret = regulator_disable(host->vcc);
+ if (ret == 0)
+ ret = mmc_regulator_set_ocr(host->vcc, 0);
+ }
}
if (mmc_slot(host).after_set_reg)
@@ -340,10 +356,14 @@ static int omap_hsmmc_23_set_sleep(struct device *dev, int slot, int sleep,
if (cardsleep) {
/* VCC can be turned off if card is asleep */
- if (sleep)
- err = mmc_regulator_set_ocr(host->vcc, 0);
- else
+ if (sleep) {
+ err = regulator_disable(host->vcc);
+ } else {
+ err = regulator_enable(host->vcc);
+ if (err)
+ return err;
err = mmc_regulator_set_ocr(host->vcc, vdd);
+ }
} else
err = regulator_set_mode(host->vcc, mode);
if (err)
diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
index 0a4e43f..b7e0216 100644
--- a/drivers/mmc/host/pxamci.c
+++ b/drivers/mmc/host/pxamci.c
@@ -99,13 +99,22 @@ static inline void pxamci_init_ocr(struct pxamci_host *host)
}
}
-static inline void pxamci_set_power(struct pxamci_host *host, unsigned int vdd)
+static inline void pxamci_set_power(struct pxamci_host *host,
+ unsigned char power_mode,
+ unsigned int vdd)
{
int on;
#ifdef CONFIG_REGULATOR
- if (host->vcc)
- mmc_regulator_set_ocr(host->vcc, vdd);
+ if (host->vcc) {
+ if (power_mode == MMC_POWER_UP) {
+ if (regulator_enable(host->vcc))
+ dev_err(mmc_dev(host->mmc),
+ "could not enable regulator\n");
+ mmc_regulator_set_ocr(host->vcc, vdd);
+ } else if (power_mode == MMC_POWER_OFF)
+ regulator_disable(host->vcc);
+ }
#endif
if (!host->vcc && host->pdata &&
gpio_is_valid(host->pdata->gpio_power)) {
@@ -492,7 +501,7 @@ static void pxamci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
if (host->power_mode != ios->power_mode) {
host->power_mode = ios->power_mode;
- pxamci_set_power(host, ios->vdd);
+ pxamci_set_power(host, ios->power_mode, ios->vdd);
if (ios->power_mode == MMC_POWER_ON)
host->cmdat |= CMDAT_INIT;

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