[PATCH] MMC: move regulator handling closer to core v3

[PATCH] MMC: move regulator handling closer to core v3

[Linus Walleij]

After discovering a problem in regulator reference counting I
took Mark Brown's advice to move the reference count into the
MMC core by making the regulator status a member of
struct mmc_host.

I took this opportunity to also implement NULL versions of
the regulator functions so as to rid the driver code from
some ugly #ifdef CONFIG_REGULATOR clauses.
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Liam Girdwood <lrg@slimlogic.co.uk>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Adrian Hunter <adrian.hunter@nokia.com>
Cc: Robert Jarzmik <robert.jarzmik@free.fr>
Cc: Sundar Iyer <sundar.iyer@stericsson.com>
Cc: Daniel Mack <daniel@caiaq.de>
Cc: Pierre Ossman <pierre@ossman.eu>
Cc: Matt Fleming <matt@console-pimps.org>
Cc: David Brownell <dbrownell@users.sourceforge.net>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Eric Miao <eric.y.miao@gmail.com>
Cc: Cliff Brake <cbrake@bec-systems.com>
Cc: Jarkko Lavinen <jarkko.lavinen@nokia.com>
Cc: linux-mmc@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Reviewed-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
Changes v2->v3:
- One of Adrians contractor friends found a non-consistent way of
  disabling the regulator in mmci.c.
---
 drivers/mmc/core/core.c       |   26 ++++++++++++++++----------
 drivers/mmc/host/mmci.c       |   15 ++++++---------
 drivers/mmc/host/omap_hsmmc.c |   21 +++++++++++++--------
 drivers/mmc/host/pxamci.c     |   18 ++++++++++++------
 include/linux/mmc/host.h      |   22 +++++++++++++++++++++-
 5 files changed, 68 insertions(+), 34 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 5db49b1..2d47467 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -771,8 +771,9 @@ EXPORT_SYMBOL(mmc_regulator_get_ocrmask);
 
 /**
  * mmc_regulator_set_ocr - set regulator to match host->ios voltage
- * @vdd_bit: zero for power off, else a bit number (host->ios.vdd)
+ * @mmc: the host to regulate
  * @supply: regulator to use
+ * @vdd_bit: zero for power off, else a bit number (host->ios.vdd)
  *
  * Returns zero on success, else negative errno.
  *
@@ -780,15 +781,12 @@ EXPORT_SYMBOL(mmc_regulator_get_ocrmask);
  * a particular 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 *mmc,
+			struct regulator *supply,
+			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 (vdd_bit) {
 		int		tmp;
@@ -819,17 +817,25 @@ int mmc_regulator_set_ocr(struct regulator *supply, unsigned short vdd_bit)
 		else
 			result = 0;
 
-		if (result == 0 && !enabled)
+		if (result == 0 && !mmc->regulator_enabled) {
 			result = regulator_enable(supply);
-	} else if (enabled) {
+			if (!result)
+				mmc->regulator_enabled = true;
+		}
+	} else if (mmc->regulator_enabled) {
 		result = regulator_disable(supply);
+		if (result == 0)
+			mmc->regulator_enabled = false;
 	}
 
+	if (result)
+		dev_err(mmc_dev(mmc),
+			"could not set regulator OCR (%d)\n", result);
 	return result;
 }
 EXPORT_SYMBOL(mmc_regulator_set_ocr);
 
-#endif
+#endif /* CONFIG_REGULATOR */
 
 /*
  * Mask off any voltages we don't support and select
diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 840b301..041cbca 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -507,19 +507,16 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	struct mmci_host *host = mmc_priv(mmc);
 	u32 pwr = 0;
 	unsigned long flags;
+	int ret;
 
 	switch (ios->power_mode) {
 	case MMC_POWER_OFF:
-		if(host->vcc &&
-		   regulator_is_enabled(host->vcc))
-			regulator_disable(host->vcc);
+		if (host->vcc)
+			ret = mmc_regulator_set_ocr(mmc, host->vcc, 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);
-#endif
+			ret = mmc_regulator_set_ocr(mmc, host->vcc, ios->vdd);
 		if (host->plat->vdd_handler)
 			pwr |= host->plat->vdd_handler(mmc_dev(mmc), ios->vdd,
 						       ios->power_mode);
@@ -826,8 +823,8 @@ 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);
+		if (host->vcc)
+			mmc_regulator_set_ocr(mmc, host->vcc, 0);
 		regulator_put(host->vcc);
 
 		mmc_free_host(mmc);
diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 4a8776f..14598ca 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -250,9 +250,9 @@ static int omap_hsmmc_1_set_power(struct device *dev, int slot, int power_on,
 		mmc_slot(host).before_set_reg(dev, slot, power_on, vdd);
 
 	if (power_on)
-		ret = mmc_regulator_set_ocr(host->vcc, vdd);
+		ret = mmc_regulator_set_ocr(host->mmc, host->vcc, vdd);
 	else
-		ret = mmc_regulator_set_ocr(host->vcc, 0);
+		ret = mmc_regulator_set_ocr(host->mmc, host->vcc, 0);
 
 	if (mmc_slot(host).after_set_reg)
 		mmc_slot(host).after_set_reg(dev, slot, power_on, vdd);
@@ -291,18 +291,23 @@ 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 = mmc_regulator_set_ocr(host->mmc, host->vcc, vdd);
 		/* Enable interface voltage rail, if needed */
 		if (ret == 0 && host->vcc_aux) {
 			ret = regulator_enable(host->vcc_aux);
 			if (ret < 0)
-				ret = mmc_regulator_set_ocr(host->vcc, 0);
+				ret = mmc_regulator_set_ocr(host->mmc,
+							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) {
+			/* Then proceed to shut down the local regulator */
+			ret = mmc_regulator_set_ocr(host->mmc,
+						host->vcc, 0);
+		}
 	}
 
 	if (mmc_slot(host).after_set_reg)
@@ -343,9 +348,9 @@ 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);
+			err = mmc_regulator_set_ocr(host->mmc, host->vcc, 0);
 		else
-			err = mmc_regulator_set_ocr(host->vcc, vdd);
+			err = mmc_regulator_set_ocr(host->mmc, 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..a219f1f 100644
--- a/drivers/mmc/host/pxamci.c
+++ b/drivers/mmc/host/pxamci.c
@@ -99,14 +99,20 @@ 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);
-#endif
+	if (host->vcc) {
+		int ret;
+
+		if (power_mode == MMC_POWER_UP)
+			ret = mmc_regulator_set_ocr(host->mmc, host->vcc, vdd);
+		else if (power_mode == MMC_POWER_OFF)
+			ret = mmc_regulator_set_ocr(host->mmc, host->vcc, 0);
+	}
 	if (!host->vcc && host->pdata &&
 	    gpio_is_valid(host->pdata->gpio_power)) {
 		on = ((1 << vdd) & host->pdata->ocr_mask);
@@ -492,7 +498,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;
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 1575b52..d009d73 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -212,6 +212,10 @@ struct mmc_host {
 	struct led_trigger	*led;		/* activity led */
 #endif
 
+#ifdef CONFIG_REGULATOR
+	bool			regulator_enabled; /* regulator state */
+#endif
+
 	struct dentry		*debugfs_root;
 
 	unsigned long		private[0] ____cacheline_aligned;
@@ -250,8 +254,24 @@ static inline void mmc_signal_sdio_irq(struct mmc_host *host)
 
 struct regulator;
 
+#ifdef CONFIG_REGULATOR
 int mmc_regulator_get_ocrmask(struct regulator *supply);
-int mmc_regulator_set_ocr(struct regulator *supply, unsigned short vdd_bit);
+int mmc_regulator_set_ocr(struct mmc_host *mmc,
+			struct regulator *supply,
+			unsigned short vdd_bit);
+#else
+int inline mmc_regulator_get_ocrmask(struct regulator *supply)
+{
+	return 0;
+}
+
+int inline mmc_regulator_set_ocr(struct mmc_host *mmc,
+				 struct regulator *supply,
+				 unsigned short vdd_bit)
+{
+	return 0;
+}
+#endif
 
 int mmc_card_awake(struct mmc_host *host);
 int mmc_card_sleep(struct mmc_host *host);
-- 
1.7.2.2

[PATCH] MMC: move regulator handling closer to core v3

[Linus Walleij]

After discovering a problem in regulator reference counting I
took Mark Brown's advice to move the reference count into the
MMC core by making the regulator status a member of
struct mmc_host.

I took this opportunity to also implement NULL versions of
the regulator functions so as to rid the driver code from
some ugly #ifdef CONFIG_REGULATOR clauses.
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Liam Girdwood <lrg@slimlogic.co.uk>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Adrian Hunter <adrian.hunter@nokia.com>
Cc: Robert Jarzmik <robert.jarzmik@free.fr>
Cc: Sundar Iyer <sundar.iyer@stericsson.com>
Cc: Daniel Mack <daniel@caiaq.de>
Cc: Pierre Ossman <pierre@ossman.eu>
Cc: Matt Fleming <matt@console-pimps.org>
Cc: David Brownell <dbrownell@users.sourceforge.net>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Eric Miao <eric.y.miao@gmail.com>
Cc: Cliff Brake <cbrake@bec-systems.com>
Cc: Jarkko Lavinen <jarkko.lavinen@nokia.com>
Cc: linux-mmc at vger.kernel.org
Cc: linux-arm-kernel at lists.infradead.org
Reviewed-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
Changes v2->v3:
- One of Adrians contractor friends found a non-consistent way of
  disabling the regulator in mmci.c.
---
 drivers/mmc/core/core.c       |   26 ++++++++++++++++----------
 drivers/mmc/host/mmci.c       |   15 ++++++---------
 drivers/mmc/host/omap_hsmmc.c |   21 +++++++++++++--------
 drivers/mmc/host/pxamci.c     |   18 ++++++++++++------
 include/linux/mmc/host.h      |   22 +++++++++++++++++++++-
 5 files changed, 68 insertions(+), 34 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 5db49b1..2d47467 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -771,8 +771,9 @@ EXPORT_SYMBOL(mmc_regulator_get_ocrmask);
 
 /**
  * mmc_regulator_set_ocr - set regulator to match host->ios voltage
- * @vdd_bit: zero for power off, else a bit number (host->ios.vdd)
+ * @mmc: the host to regulate
  * @supply: regulator to use
+ * @vdd_bit: zero for power off, else a bit number (host->ios.vdd)
  *
  * Returns zero on success, else negative errno.
  *
@@ -780,15 +781,12 @@ EXPORT_SYMBOL(mmc_regulator_get_ocrmask);
  * a particular 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 *mmc,
+			struct regulator *supply,
+			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 (vdd_bit) {
 		int		tmp;
@@ -819,17 +817,25 @@ int mmc_regulator_set_ocr(struct regulator *supply, unsigned short vdd_bit)
 		else
 			result = 0;
 
-		if (result == 0 && !enabled)
+		if (result == 0 && !mmc->regulator_enabled) {
 			result = regulator_enable(supply);
-	} else if (enabled) {
+			if (!result)
+				mmc->regulator_enabled = true;
+		}
+	} else if (mmc->regulator_enabled) {
 		result = regulator_disable(supply);
+		if (result == 0)
+			mmc->regulator_enabled = false;
 	}
 
+	if (result)
+		dev_err(mmc_dev(mmc),
+			"could not set regulator OCR (%d)\n", result);
 	return result;
 }
 EXPORT_SYMBOL(mmc_regulator_set_ocr);
 
-#endif
+#endif /* CONFIG_REGULATOR */
 
 /*
  * Mask off any voltages we don't support and select
diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 840b301..041cbca 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -507,19 +507,16 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	struct mmci_host *host = mmc_priv(mmc);
 	u32 pwr = 0;
 	unsigned long flags;
+	int ret;
 
 	switch (ios->power_mode) {
 	case MMC_POWER_OFF:
-		if(host->vcc &&
-		   regulator_is_enabled(host->vcc))
-			regulator_disable(host->vcc);
+		if (host->vcc)
+			ret = mmc_regulator_set_ocr(mmc, host->vcc, 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);
-#endif
+			ret = mmc_regulator_set_ocr(mmc, host->vcc, ios->vdd);
 		if (host->plat->vdd_handler)
 			pwr |= host->plat->vdd_handler(mmc_dev(mmc), ios->vdd,
 						       ios->power_mode);
@@ -826,8 +823,8 @@ 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);
+		if (host->vcc)
+			mmc_regulator_set_ocr(mmc, host->vcc, 0);
 		regulator_put(host->vcc);
 
 		mmc_free_host(mmc);
diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 4a8776f..14598ca 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -250,9 +250,9 @@ static int omap_hsmmc_1_set_power(struct device *dev, int slot, int power_on,
 		mmc_slot(host).before_set_reg(dev, slot, power_on, vdd);
 
 	if (power_on)
-		ret = mmc_regulator_set_ocr(host->vcc, vdd);
+		ret = mmc_regulator_set_ocr(host->mmc, host->vcc, vdd);
 	else
-		ret = mmc_regulator_set_ocr(host->vcc, 0);
+		ret = mmc_regulator_set_ocr(host->mmc, host->vcc, 0);
 
 	if (mmc_slot(host).after_set_reg)
 		mmc_slot(host).after_set_reg(dev, slot, power_on, vdd);
@@ -291,18 +291,23 @@ 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 = mmc_regulator_set_ocr(host->mmc, host->vcc, vdd);
 		/* Enable interface voltage rail, if needed */
 		if (ret == 0 && host->vcc_aux) {
 			ret = regulator_enable(host->vcc_aux);
 			if (ret < 0)
-				ret = mmc_regulator_set_ocr(host->vcc, 0);
+				ret = mmc_regulator_set_ocr(host->mmc,
+							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) {
+			/* Then proceed to shut down the local regulator */
+			ret = mmc_regulator_set_ocr(host->mmc,
+						host->vcc, 0);
+		}
 	}
 
 	if (mmc_slot(host).after_set_reg)
@@ -343,9 +348,9 @@ 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);
+			err = mmc_regulator_set_ocr(host->mmc, host->vcc, 0);
 		else
-			err = mmc_regulator_set_ocr(host->vcc, vdd);
+			err = mmc_regulator_set_ocr(host->mmc, 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..a219f1f 100644
--- a/drivers/mmc/host/pxamci.c
+++ b/drivers/mmc/host/pxamci.c
@@ -99,14 +99,20 @@ 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);
-#endif
+	if (host->vcc) {
+		int ret;
+
+		if (power_mode == MMC_POWER_UP)
+			ret = mmc_regulator_set_ocr(host->mmc, host->vcc, vdd);
+		else if (power_mode == MMC_POWER_OFF)
+			ret = mmc_regulator_set_ocr(host->mmc, host->vcc, 0);
+	}
 	if (!host->vcc && host->pdata &&
 	    gpio_is_valid(host->pdata->gpio_power)) {
 		on = ((1 << vdd) & host->pdata->ocr_mask);
@@ -492,7 +498,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;
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 1575b52..d009d73 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -212,6 +212,10 @@ struct mmc_host {
 	struct led_trigger	*led;		/* activity led */
 #endif
 
+#ifdef CONFIG_REGULATOR
+	bool			regulator_enabled; /* regulator state */
+#endif
+
 	struct dentry		*debugfs_root;
 
 	unsigned long		private[0] ____cacheline_aligned;
@@ -250,8 +254,24 @@ static inline void mmc_signal_sdio_irq(struct mmc_host *host)
 
 struct regulator;
 
+#ifdef CONFIG_REGULATOR
 int mmc_regulator_get_ocrmask(struct regulator *supply);
-int mmc_regulator_set_ocr(struct regulator *supply, unsigned short vdd_bit);
+int mmc_regulator_set_ocr(struct mmc_host *mmc,
+			struct regulator *supply,
+			unsigned short vdd_bit);
+#else
+int inline mmc_regulator_get_ocrmask(struct regulator *supply)
+{
+	return 0;
+}
+
+int inline mmc_regulator_set_ocr(struct mmc_host *mmc,
+				 struct regulator *supply,
+				 unsigned short vdd_bit)
+{
+	return 0;
+}
+#endif
 
 int mmc_card_awake(struct mmc_host *host);
 int mmc_card_sleep(struct mmc_host *host);
-- 
1.7.2.2

Re: [PATCH] MMC: move regulator handling closer to core v3

[Andrew Morton]

On Sun,  5 Sep 2010 11:05:38 +0200
Linus Walleij <linus.walleij@stericsson.com> wrote:

> After discovering a problem in regulator reference counting I
> took Mark Brown's advice to move the reference count into the
> MMC core by making the regulator status a member of
> struct mmc_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);
> -#endif
> +	if (host->vcc) {
> +		int ret;
> +
> +		if (power_mode == MMC_POWER_UP)
> +			ret = mmc_regulator_set_ocr(host->mmc, host->vcc, vdd);
> +		else if (power_mode == MMC_POWER_OFF)
> +			ret = mmc_regulator_set_ocr(host->mmc, host->vcc, 0);
> +	}

There's no point in copying the return value into a local then ignoring
it.  mmc_regulator_set_ocr() can return a negative errno so we should
test for that, clean up and propagate the error.

If we really do deliberately ignore the error then there should be a
code comment which excuses this behaviour and perhaps a warning printk.

The same comments apply to mmci_set_ios().

omap_hsmmc_1_set_power() gets it right.

Why doesn't omap_hsmmc_23_set_sleep() run .before_set_reg() and
.after_set_reg()?

>
> ...
>

[PATCH] MMC: move regulator handling closer to core v3

[Andrew Morton]

On Sun,  5 Sep 2010 11:05:38 +0200
Linus Walleij <linus.walleij@stericsson.com> wrote:

> After discovering a problem in regulator reference counting I
> took Mark Brown's advice to move the reference count into the
> MMC core by making the regulator status a member of
> struct mmc_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);
> -#endif
> +	if (host->vcc) {
> +		int ret;
> +
> +		if (power_mode == MMC_POWER_UP)
> +			ret = mmc_regulator_set_ocr(host->mmc, host->vcc, vdd);
> +		else if (power_mode == MMC_POWER_OFF)
> +			ret = mmc_regulator_set_ocr(host->mmc, host->vcc, 0);
> +	}

There's no point in copying the return value into a local then ignoring
it.  mmc_regulator_set_ocr() can return a negative errno so we should
test for that, clean up and propagate the error.

If we really do deliberately ignore the error then there should be a
code comment which excuses this behaviour and perhaps a warning printk.

The same comments apply to mmci_set_ios().

omap_hsmmc_1_set_power() gets it right.

Why doesn't omap_hsmmc_23_set_sleep() run .before_set_reg() and
.after_set_reg()?

>
> ...
>

Re: [PATCH] MMC: move regulator handling closer to core v3

[Adrian Hunter]

ext Andrew Morton wrote:
> On Sun,  5 Sep 2010 11:05:38 +0200
> Linus Walleij <linus.walleij@stericsson.com> wrote:
> 
>> After discovering a problem in regulator reference counting I
>> took Mark Brown's advice to move the reference count into the
>> MMC core by making the regulator status a member of
>> struct mmc_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);
>> -#endif
>> +	if (host->vcc) {
>> +		int ret;
>> +
>> +		if (power_mode == MMC_POWER_UP)
>> +			ret = mmc_regulator_set_ocr(host->mmc, host->vcc, vdd);
>> +		else if (power_mode == MMC_POWER_OFF)
>> +			ret = mmc_regulator_set_ocr(host->mmc, host->vcc, 0);
>> +	}
> 
> There's no point in copying the return value into a local then ignoring
> it.  mmc_regulator_set_ocr() can return a negative errno so we should
> test for that, clean up and propagate the error.
> 
> If we really do deliberately ignore the error then there should be a
> code comment which excuses this behaviour and perhaps a warning printk.
> 
> The same comments apply to mmci_set_ios().
> 
> omap_hsmmc_1_set_power() gets it right.
> 
> Why doesn't omap_hsmmc_23_set_sleep() run .before_set_reg() and
> .after_set_reg()?

Mostly because the voltage does not change.  The voltage regulator
switches to a low current mode that reduces leakage but the voltage
does not change.

As an aside, I would like to enhance the regulator framework to
enable boards to hook their code directly into the regulator.
Arguably this is essential to allow pbias configuration (without
which the board may be damaged) so that regulator_enable/disable
can be used independently of the board, or for example to allow
the regulator core to turn the regulator on/off at initialisation.

Anyway, if that was done .before/.after_set_reg would not be
needed anymore.

> 
>> ...
>>
> 

Re: [PATCH] MMC: move regulator handling closer to core v3

[Adrian Hunter]

ext Andrew Morton wrote:
> On Sun,  5 Sep 2010 11:05:38 +0200
> Linus Walleij <linus.walleij@stericsson.com> wrote:
> 
>> After discovering a problem in regulator reference counting I
>> took Mark Brown's advice to move the reference count into the
>> MMC core by making the regulator status a member of
>> struct mmc_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);
>> -#endif
>> +	if (host->vcc) {
>> +		int ret;
>> +
>> +		if (power_mode == MMC_POWER_UP)
>> +			ret = mmc_regulator_set_ocr(host->mmc, host->vcc, vdd);
>> +		else if (power_mode == MMC_POWER_OFF)
>> +			ret = mmc_regulator_set_ocr(host->mmc, host->vcc, 0);
>> +	}
> 
> There's no point in copying the return value into a local then ignoring
> it.  mmc_regulator_set_ocr() can return a negative errno so we should
> test for that, clean up and propagate the error.
> 
> If we really do deliberately ignore the error then there should be a
> code comment which excuses this behaviour and perhaps a warning printk.
> 
> The same comments apply to mmci_set_ios().
> 
> omap_hsmmc_1_set_power() gets it right.
> 
> Why doesn't omap_hsmmc_23_set_sleep() run .before_set_reg() and
> .after_set_reg()?

Mostly because the voltage does not change.  The voltage regulator
switches to a low current mode that reduces leakage but the voltage
does not change.

As an aside, I would like to enhance the regulator framework to
enable boards to hook their code directly into the regulator.
Arguably this is essential to allow pbias configuration (without
which the board may be damaged) so that regulator_enable/disable
can be used independently of the board, or for example to allow
the regulator core to turn the regulator on/off at initialisation.

Anyway, if that was done .before/.after_set_reg would not be
needed anymore.

> 
>> ...
>>
> 

[PATCH] MMC: move regulator handling closer to core v3

[Adrian Hunter]

ext Andrew Morton wrote:
> On Sun,  5 Sep 2010 11:05:38 +0200
> Linus Walleij <linus.walleij@stericsson.com> wrote:
> 
>> After discovering a problem in regulator reference counting I
>> took Mark Brown's advice to move the reference count into the
>> MMC core by making the regulator status a member of
>> struct mmc_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);
>> -#endif
>> +	if (host->vcc) {
>> +		int ret;
>> +
>> +		if (power_mode == MMC_POWER_UP)
>> +			ret = mmc_regulator_set_ocr(host->mmc, host->vcc, vdd);
>> +		else if (power_mode == MMC_POWER_OFF)
>> +			ret = mmc_regulator_set_ocr(host->mmc, host->vcc, 0);
>> +	}
> 
> There's no point in copying the return value into a local then ignoring
> it.  mmc_regulator_set_ocr() can return a negative errno so we should
> test for that, clean up and propagate the error.
> 
> If we really do deliberately ignore the error then there should be a
> code comment which excuses this behaviour and perhaps a warning printk.
> 
> The same comments apply to mmci_set_ios().
> 
> omap_hsmmc_1_set_power() gets it right.
> 
> Why doesn't omap_hsmmc_23_set_sleep() run .before_set_reg() and
> .after_set_reg()?

Mostly because the voltage does not change.  The voltage regulator
switches to a low current mode that reduces leakage but the voltage
does not change.

As an aside, I would like to enhance the regulator framework to
enable boards to hook their code directly into the regulator.
Arguably this is essential to allow pbias configuration (without
which the board may be damaged) so that regulator_enable/disable
can be used independently of the board, or for example to allow
the regulator core to turn the regulator on/off at initialisation.

Anyway, if that was done .before/.after_set_reg would not be
needed anymore.

> 
>> ...
>>
> 

Re: [PATCH] MMC: move regulator handling closer to core v3

[Linus Walleij]

2010/9/9 Andrew Morton <akpm@linux-foundation.org>:
> On Sun,  5 Sep 2010 11:05:38 +0200
> Linus Walleij <linus.walleij@stericsson.com> wrote:
>> +     if (host->vcc) {
>> +             int ret;
>> +
>> +             if (power_mode == MMC_POWER_UP)
>> +                     ret = mmc_regulator_set_ocr(host->mmc, host->vcc, vdd);
>> +             else if (power_mode == MMC_POWER_OFF)
>> +                     ret = mmc_regulator_set_ocr(host->mmc, host->vcc, 0);
>> +     }
>
> There's no point in copying the return value into a local then ignoring
> it.  mmc_regulator_set_ocr() can return a negative errno so we should
> test for that, clean up and propagate the error.

OK I'll fix.

> The same comments apply to mmci_set_ios().

OK.

Yours,
Linus Walleij

[PATCH] MMC: move regulator handling closer to core v3

[Linus Walleij]

2010/9/9 Andrew Morton <akpm@linux-foundation.org>:
> On Sun, ?5 Sep 2010 11:05:38 +0200
> Linus Walleij <linus.walleij@stericsson.com> wrote:
>> + ? ? if (host->vcc) {
>> + ? ? ? ? ? ? int ret;
>> +
>> + ? ? ? ? ? ? if (power_mode == MMC_POWER_UP)
>> + ? ? ? ? ? ? ? ? ? ? ret = mmc_regulator_set_ocr(host->mmc, host->vcc, vdd);
>> + ? ? ? ? ? ? else if (power_mode == MMC_POWER_OFF)
>> + ? ? ? ? ? ? ? ? ? ? ret = mmc_regulator_set_ocr(host->mmc, host->vcc, 0);
>> + ? ? }
>
> There's no point in copying the return value into a local then ignoring
> it. ?mmc_regulator_set_ocr() can return a negative errno so we should
> test for that, clean up and propagate the error.

OK I'll fix.

> The same comments apply to mmci_set_ios().

OK.

Yours,
Linus Walleij

Re: [PATCH] MMC: move regulator handling closer to core v3

[Mark Brown]

On Thu, Sep 09, 2010 at 09:01:27AM +0300, Adrian Hunter wrote:

> As an aside, I would like to enhance the regulator framework to
> enable boards to hook their code directly into the regulator.

What do you mean when you say that you would like boards to "hook
directly into the regulator" - what do you want to be able to do?
Is this the notifiers?  Do you need more of them?

> Arguably this is essential to allow pbias configuration (without
> which the board may be damaged) so that regulator_enable/disable
> can be used independently of the board, or for example to allow

I can't parse this at all I'm afraid.  Could you be more specific about
what you mean by using enable and disable independantly of the board -
clearly the consumer drivers are already able to be board independant?

> the regulator core to turn the regulator on/off at initialisation.

The regulator core already supports enabling or disabling regulators
from the machine constraints which I *think* is what you're looking for
but since I can't follow what you're saying above I'm not sure.

Re: [PATCH] MMC: move regulator handling closer to core v3

[Mark Brown]

On Thu, Sep 09, 2010 at 09:01:27AM +0300, Adrian Hunter wrote:

> As an aside, I would like to enhance the regulator framework to
> enable boards to hook their code directly into the regulator.

What do you mean when you say that you would like boards to "hook
directly into the regulator" - what do you want to be able to do?
Is this the notifiers?  Do you need more of them?

> Arguably this is essential to allow pbias configuration (without
> which the board may be damaged) so that regulator_enable/disable
> can be used independently of the board, or for example to allow

I can't parse this at all I'm afraid.  Could you be more specific about
what you mean by using enable and disable independantly of the board -
clearly the consumer drivers are already able to be board independant?

> the regulator core to turn the regulator on/off at initialisation.

The regulator core already supports enabling or disabling regulators
from the machine constraints which I *think* is what you're looking for
but since I can't follow what you're saying above I'm not sure.

[PATCH] MMC: move regulator handling closer to core v3

[Mark Brown]

On Thu, Sep 09, 2010 at 09:01:27AM +0300, Adrian Hunter wrote:

> As an aside, I would like to enhance the regulator framework to
> enable boards to hook their code directly into the regulator.

What do you mean when you say that you would like boards to "hook
directly into the regulator" - what do you want to be able to do?
Is this the notifiers?  Do you need more of them?

> Arguably this is essential to allow pbias configuration (without
> which the board may be damaged) so that regulator_enable/disable
> can be used independently of the board, or for example to allow

I can't parse this at all I'm afraid.  Could you be more specific about
what you mean by using enable and disable independantly of the board -
clearly the consumer drivers are already able to be board independant?

> the regulator core to turn the regulator on/off at initialisation.

The regulator core already supports enabling or disabling regulators
from the machine constraints which I *think* is what you're looking for
but since I can't follow what you're saying above I'm not sure.

Re: [PATCH] MMC: move regulator handling closer to core v3

[Adrian Hunter]

Mark Brown wrote:
> On Thu, Sep 09, 2010 at 09:01:27AM +0300, Adrian Hunter wrote:
> 
>> As an aside, I would like to enhance the regulator framework to
>> enable boards to hook their code directly into the regulator.
> 
> What do you mean when you say that you would like boards to "hook
> directly into the regulator" - what do you want to be able to do?
> Is this the notifiers?  Do you need more of them?

There would need to be notifiers before and after any change to the
voltage.  However the notifiers would have to be set up before or
during regulator initialisation because initialisation already can
change the voltage e.g. in set_machine_constraints.

> 
>> Arguably this is essential to allow pbias configuration (without
>> which the board may be damaged) so that regulator_enable/disable
>> can be used independently of the board, or for example to allow
> 
> I can't parse this at all I'm afraid.  Could you be more specific about
> what you mean by using enable and disable independantly of the board -
> clearly the consumer drivers are already able to be board independant?

For OMAP3 (and OMAP2 in some cases) the 1st SD/SDIO/MMC controller
can operate at either 1.8V or 3V, however at 3V the board must apply
voltage level shifting (PBIAS configuation).  Currently the
level-shifting is done by board code that must be called before and
after the voltage changes.  The omap_hsmmc driver calls the board
code via callbacks in platform data (.before_set_reg and
.after_set_reg).

> 
>> the regulator core to turn the regulator on/off at initialisation.
> 
> The regulator core already supports enabling or disabling regulators
> from the machine constraints which I *think* is what you're looking for
> but since I can't follow what you're saying above I'm not sure.

Yes that is what I would like, but it won't work at present because
the core does not do the PBIAS configuration.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] MMC: move regulator handling closer to core v3

[Adrian Hunter]

Mark Brown wrote:
> On Thu, Sep 09, 2010 at 09:01:27AM +0300, Adrian Hunter wrote:
> 
>> As an aside, I would like to enhance the regulator framework to
>> enable boards to hook their code directly into the regulator.
> 
> What do you mean when you say that you would like boards to "hook
> directly into the regulator" - what do you want to be able to do?
> Is this the notifiers?  Do you need more of them?

There would need to be notifiers before and after any change to the
voltage.  However the notifiers would have to be set up before or
during regulator initialisation because initialisation already can
change the voltage e.g. in set_machine_constraints.

> 
>> Arguably this is essential to allow pbias configuration (without
>> which the board may be damaged) so that regulator_enable/disable
>> can be used independently of the board, or for example to allow
> 
> I can't parse this at all I'm afraid.  Could you be more specific about
> what you mean by using enable and disable independantly of the board -
> clearly the consumer drivers are already able to be board independant?

For OMAP3 (and OMAP2 in some cases) the 1st SD/SDIO/MMC controller
can operate at either 1.8V or 3V, however at 3V the board must apply
voltage level shifting (PBIAS configuation).  Currently the
level-shifting is done by board code that must be called before and
after the voltage changes.  The omap_hsmmc driver calls the board
code via callbacks in platform data (.before_set_reg and
.after_set_reg).

> 
>> the regulator core to turn the regulator on/off at initialisation.
> 
> The regulator core already supports enabling or disabling regulators
> from the machine constraints which I *think* is what you're looking for
> but since I can't follow what you're saying above I'm not sure.

Yes that is what I would like, but it won't work at present because
the core does not do the PBIAS configuration.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH] MMC: move regulator handling closer to core v3

[Adrian Hunter]

Mark Brown wrote:
> On Thu, Sep 09, 2010 at 09:01:27AM +0300, Adrian Hunter wrote:
> 
>> As an aside, I would like to enhance the regulator framework to
>> enable boards to hook their code directly into the regulator.
> 
> What do you mean when you say that you would like boards to "hook
> directly into the regulator" - what do you want to be able to do?
> Is this the notifiers?  Do you need more of them?

There would need to be notifiers before and after any change to the
voltage.  However the notifiers would have to be set up before or
during regulator initialisation because initialisation already can
change the voltage e.g. in set_machine_constraints.

> 
>> Arguably this is essential to allow pbias configuration (without
>> which the board may be damaged) so that regulator_enable/disable
>> can be used independently of the board, or for example to allow
> 
> I can't parse this at all I'm afraid.  Could you be more specific about
> what you mean by using enable and disable independantly of the board -
> clearly the consumer drivers are already able to be board independant?

For OMAP3 (and OMAP2 in some cases) the 1st SD/SDIO/MMC controller
can operate at either 1.8V or 3V, however at 3V the board must apply
voltage level shifting (PBIAS configuation).  Currently the
level-shifting is done by board code that must be called before and
after the voltage changes.  The omap_hsmmc driver calls the board
code via callbacks in platform data (.before_set_reg and
.after_set_reg).

> 
>> the regulator core to turn the regulator on/off at initialisation.
> 
> The regulator core already supports enabling or disabling regulators
> from the machine constraints which I *think* is what you're looking for
> but since I can't follow what you're saying above I'm not sure.

Yes that is what I would like, but it won't work at present because
the core does not do the PBIAS configuration.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] MMC: move regulator handling closer to core v3

[Mark Brown]

On Thu, Sep 09, 2010 at 03:56:31PM +0300, Adrian Hunter wrote:
> Mark Brown wrote:

> >What do you mean when you say that you would like boards to "hook
> >directly into the regulator" - what do you want to be able to do?
> >Is this the notifiers?  Do you need more of them?

> There would need to be notifiers before and after any change to the
> voltage.  However the notifiers would have to be set up before or
> during regulator initialisation because initialisation already can
> change the voltage e.g. in set_machine_constraints.

Why do you believe these notifiers are needed?  I would expect that
anything that needs pre and post change notifications like this is
actually changing the configuration itself (since if it's that sensitive
to changes it seems likely that it wouldn't be enthused about having the
configuration changed while it's trying to do stuff).

> >I can't parse this at all I'm afraid.  Could you be more specific about
> >what you mean by using enable and disable independantly of the board -
> >clearly the consumer drivers are already able to be board independant?

> For OMAP3 (and OMAP2 in some cases) the 1st SD/SDIO/MMC controller
> can operate at either 1.8V or 3V, however at 3V the board must apply
> voltage level shifting (PBIAS configuation).  Currently the
> level-shifting is done by board code that must be called before and
> after the voltage changes.  The omap_hsmmc driver calls the board
> code via callbacks in platform data (.before_set_reg and
> .after_set_reg).

This really sounds like something that's internal to the MMC controller
driver - based on your description I'm very surprised that boards even
get visibility of this.  Surely the MMC controller driver should be
taking care of configuring PBIAS itself, assuming it's something
internal to the chip?  Adding code to the regulator API so that the OMAP
MMC driver can be notified of the configuration it has applied to itself
(and any failures that ensue) feels baroque.

Re: [PATCH] MMC: move regulator handling closer to core v3

[Mark Brown]

On Thu, Sep 09, 2010 at 03:56:31PM +0300, Adrian Hunter wrote:
> Mark Brown wrote:

> >What do you mean when you say that you would like boards to "hook
> >directly into the regulator" - what do you want to be able to do?
> >Is this the notifiers?  Do you need more of them?

> There would need to be notifiers before and after any change to the
> voltage.  However the notifiers would have to be set up before or
> during regulator initialisation because initialisation already can
> change the voltage e.g. in set_machine_constraints.

Why do you believe these notifiers are needed?  I would expect that
anything that needs pre and post change notifications like this is
actually changing the configuration itself (since if it's that sensitive
to changes it seems likely that it wouldn't be enthused about having the
configuration changed while it's trying to do stuff).

> >I can't parse this at all I'm afraid.  Could you be more specific about
> >what you mean by using enable and disable independantly of the board -
> >clearly the consumer drivers are already able to be board independant?

> For OMAP3 (and OMAP2 in some cases) the 1st SD/SDIO/MMC controller
> can operate at either 1.8V or 3V, however at 3V the board must apply
> voltage level shifting (PBIAS configuation).  Currently the
> level-shifting is done by board code that must be called before and
> after the voltage changes.  The omap_hsmmc driver calls the board
> code via callbacks in platform data (.before_set_reg and
> .after_set_reg).

This really sounds like something that's internal to the MMC controller
driver - based on your description I'm very surprised that boards even
get visibility of this.  Surely the MMC controller driver should be
taking care of configuring PBIAS itself, assuming it's something
internal to the chip?  Adding code to the regulator API so that the OMAP
MMC driver can be notified of the configuration it has applied to itself
(and any failures that ensue) feels baroque.

[PATCH] MMC: move regulator handling closer to core v3

[Mark Brown]

On Thu, Sep 09, 2010 at 03:56:31PM +0300, Adrian Hunter wrote:
> Mark Brown wrote:

> >What do you mean when you say that you would like boards to "hook
> >directly into the regulator" - what do you want to be able to do?
> >Is this the notifiers?  Do you need more of them?

> There would need to be notifiers before and after any change to the
> voltage.  However the notifiers would have to be set up before or
> during regulator initialisation because initialisation already can
> change the voltage e.g. in set_machine_constraints.

Why do you believe these notifiers are needed?  I would expect that
anything that needs pre and post change notifications like this is
actually changing the configuration itself (since if it's that sensitive
to changes it seems likely that it wouldn't be enthused about having the
configuration changed while it's trying to do stuff).

> >I can't parse this at all I'm afraid.  Could you be more specific about
> >what you mean by using enable and disable independantly of the board -
> >clearly the consumer drivers are already able to be board independant?

> For OMAP3 (and OMAP2 in some cases) the 1st SD/SDIO/MMC controller
> can operate at either 1.8V or 3V, however at 3V the board must apply
> voltage level shifting (PBIAS configuation).  Currently the
> level-shifting is done by board code that must be called before and
> after the voltage changes.  The omap_hsmmc driver calls the board
> code via callbacks in platform data (.before_set_reg and
> .after_set_reg).

This really sounds like something that's internal to the MMC controller
driver - based on your description I'm very surprised that boards even
get visibility of this.  Surely the MMC controller driver should be
taking care of configuring PBIAS itself, assuming it's something
internal to the chip?  Adding code to the regulator API so that the OMAP
MMC driver can be notified of the configuration it has applied to itself
(and any failures that ensue) feels baroque.

Re: [PATCH] MMC: move regulator handling closer to core v3

[Adrian Hunter]

Mark Brown wrote:
> On Thu, Sep 09, 2010 at 03:56:31PM +0300, Adrian Hunter wrote:
>> Mark Brown wrote:
> 
>>> What do you mean when you say that you would like boards to "hook
>>> directly into the regulator" - what do you want to be able to do?
>>> Is this the notifiers?  Do you need more of them?
> 
>> There would need to be notifiers before and after any change to the
>> voltage.  However the notifiers would have to be set up before or
>> during regulator initialisation because initialisation already can
>> change the voltage e.g. in set_machine_constraints.
> 
> Why do you believe these notifiers are needed?

As I mentioned below, they would be needed to do PBIAS configuration
automatically using notifiers (notifiers would have been overkill
anyway).  However as you don't think that is a good idea, it is no
matter.

>                                                 I would expect that
> anything that needs pre and post change notifications like this is
> actually changing the configuration itself (since if it's that sensitive
> to changes it seems likely that it wouldn't be enthused about having the
> configuration changed while it's trying to do stuff).

Yes, except when it is the regulator core trying to set the initial power
state on or off.

> 
>>> I can't parse this at all I'm afraid.  Could you be more specific about
>>> what you mean by using enable and disable independantly of the board -
>>> clearly the consumer drivers are already able to be board independant?
> 
>> For OMAP3 (and OMAP2 in some cases) the 1st SD/SDIO/MMC controller
>> can operate at either 1.8V or 3V, however at 3V the board must apply
>> voltage level shifting (PBIAS configuation).  Currently the
>> level-shifting is done by board code that must be called before and
>> after the voltage changes.  The omap_hsmmc driver calls the board
>> code via callbacks in platform data (.before_set_reg and
>> .after_set_reg).
> 
> This really sounds like something that's internal to the MMC controller
> driver - based on your description I'm very surprised that boards even
> get visibility of this.  Surely the MMC controller driver should be
> taking care of configuring PBIAS itself, assuming it's something
> internal to the chip?

It is considered that the driver should know only about hsmmc ip not
all of OMAP.  Additionally the OMAP community does not allow the use
of the necessary system control registers outside of the board level
i.e. arch/arm/mach-omap2 etc.  Hence the driver calls down to the
board level.  That is ugly but not a problem.

>                        Adding code to the regulator API so that the OMAP
> MMC driver can be notified of the configuration it has applied to itself
> (and any failures that ensue) feels baroque.

As you wish.  That rules out using the core to turn off the regulator at
boot up, but that is OK, the driver can do it.  At present the driver
makes the mistake of just doing regulator_enable() / disable() to ensure the
regulator is off, without calling down to the board level for the PBIAS
configuration.  It also comments the code as an "ugly hack" whereas
it is really the only way to do it, if the regulator can't do the
PBIAS configuration automatically.  I will just fix the driver and add
better comments.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] MMC: move regulator handling closer to core v3

[Adrian Hunter]

Mark Brown wrote:
> On Thu, Sep 09, 2010 at 03:56:31PM +0300, Adrian Hunter wrote:
>> Mark Brown wrote:
> 
>>> What do you mean when you say that you would like boards to "hook
>>> directly into the regulator" - what do you want to be able to do?
>>> Is this the notifiers?  Do you need more of them?
> 
>> There would need to be notifiers before and after any change to the
>> voltage.  However the notifiers would have to be set up before or
>> during regulator initialisation because initialisation already can
>> change the voltage e.g. in set_machine_constraints.
> 
> Why do you believe these notifiers are needed?

As I mentioned below, they would be needed to do PBIAS configuration
automatically using notifiers (notifiers would have been overkill
anyway).  However as you don't think that is a good idea, it is no
matter.

>                                                 I would expect that
> anything that needs pre and post change notifications like this is
> actually changing the configuration itself (since if it's that sensitive
> to changes it seems likely that it wouldn't be enthused about having the
> configuration changed while it's trying to do stuff).

Yes, except when it is the regulator core trying to set the initial power
state on or off.

> 
>>> I can't parse this at all I'm afraid.  Could you be more specific about
>>> what you mean by using enable and disable independantly of the board -
>>> clearly the consumer drivers are already able to be board independant?
> 
>> For OMAP3 (and OMAP2 in some cases) the 1st SD/SDIO/MMC controller
>> can operate at either 1.8V or 3V, however at 3V the board must apply
>> voltage level shifting (PBIAS configuation).  Currently the
>> level-shifting is done by board code that must be called before and
>> after the voltage changes.  The omap_hsmmc driver calls the board
>> code via callbacks in platform data (.before_set_reg and
>> .after_set_reg).
> 
> This really sounds like something that's internal to the MMC controller
> driver - based on your description I'm very surprised that boards even
> get visibility of this.  Surely the MMC controller driver should be
> taking care of configuring PBIAS itself, assuming it's something
> internal to the chip?

It is considered that the driver should know only about hsmmc ip not
all of OMAP.  Additionally the OMAP community does not allow the use
of the necessary system control registers outside of the board level
i.e. arch/arm/mach-omap2 etc.  Hence the driver calls down to the
board level.  That is ugly but not a problem.

>                        Adding code to the regulator API so that the OMAP
> MMC driver can be notified of the configuration it has applied to itself
> (and any failures that ensue) feels baroque.

As you wish.  That rules out using the core to turn off the regulator at
boot up, but that is OK, the driver can do it.  At present the driver
makes the mistake of just doing regulator_enable() / disable() to ensure the
regulator is off, without calling down to the board level for the PBIAS
configuration.  It also comments the code as an "ugly hack" whereas
it is really the only way to do it, if the regulator can't do the
PBIAS configuration automatically.  I will just fix the driver and add
better comments.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH] MMC: move regulator handling closer to core v3

[Adrian Hunter]

Mark Brown wrote:
> On Thu, Sep 09, 2010 at 03:56:31PM +0300, Adrian Hunter wrote:
>> Mark Brown wrote:
> 
>>> What do you mean when you say that you would like boards to "hook
>>> directly into the regulator" - what do you want to be able to do?
>>> Is this the notifiers?  Do you need more of them?
> 
>> There would need to be notifiers before and after any change to the
>> voltage.  However the notifiers would have to be set up before or
>> during regulator initialisation because initialisation already can
>> change the voltage e.g. in set_machine_constraints.
> 
> Why do you believe these notifiers are needed?

As I mentioned below, they would be needed to do PBIAS configuration
automatically using notifiers (notifiers would have been overkill
anyway).  However as you don't think that is a good idea, it is no
matter.

>                                                 I would expect that
> anything that needs pre and post change notifications like this is
> actually changing the configuration itself (since if it's that sensitive
> to changes it seems likely that it wouldn't be enthused about having the
> configuration changed while it's trying to do stuff).

Yes, except when it is the regulator core trying to set the initial power
state on or off.

> 
>>> I can't parse this at all I'm afraid.  Could you be more specific about
>>> what you mean by using enable and disable independantly of the board -
>>> clearly the consumer drivers are already able to be board independant?
> 
>> For OMAP3 (and OMAP2 in some cases) the 1st SD/SDIO/MMC controller
>> can operate at either 1.8V or 3V, however at 3V the board must apply
>> voltage level shifting (PBIAS configuation).  Currently the
>> level-shifting is done by board code that must be called before and
>> after the voltage changes.  The omap_hsmmc driver calls the board
>> code via callbacks in platform data (.before_set_reg and
>> .after_set_reg).
> 
> This really sounds like something that's internal to the MMC controller
> driver - based on your description I'm very surprised that boards even
> get visibility of this.  Surely the MMC controller driver should be
> taking care of configuring PBIAS itself, assuming it's something
> internal to the chip?

It is considered that the driver should know only about hsmmc ip not
all of OMAP.  Additionally the OMAP community does not allow the use
of the necessary system control registers outside of the board level
i.e. arch/arm/mach-omap2 etc.  Hence the driver calls down to the
board level.  That is ugly but not a problem.

>                        Adding code to the regulator API so that the OMAP
> MMC driver can be notified of the configuration it has applied to itself
> (and any failures that ensue) feels baroque.

As you wish.  That rules out using the core to turn off the regulator at
boot up, but that is OK, the driver can do it.  At present the driver
makes the mistake of just doing regulator_enable() / disable() to ensure the
regulator is off, without calling down to the board level for the PBIAS
configuration.  It also comments the code as an "ugly hack" whereas
it is really the only way to do it, if the regulator can't do the
PBIAS configuration automatically.  I will just fix the driver and add
better comments.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] MMC: move regulator handling closer to core v3

[Mark Brown]

On Fri, Sep 10, 2010 at 01:05:26PM +0300, Adrian Hunter wrote:
> Mark Brown wrote:
> >On Thu, Sep 09, 2010 at 03:56:31PM +0300, Adrian Hunter wrote:

> >                                                I would expect that
> >anything that needs pre and post change notifications like this is
> >actually changing the configuration itself (since if it's that sensitive
> >to changes it seems likely that it wouldn't be enthused about having the
> >configuration changed while it's trying to do stuff).

> Yes, except when it is the regulator core trying to set the initial power
> state on or off.

Like I say, I'd not expect anything that needs this sort of fiddly
control in the hardware to be happy with doing this outside the driver
itself - this includes via the constraints.  If it's important that two
bits of hardware be updated in lockstep then I'd expect you to need to
probe both bits of hardware and control them from one place, which means
that using notifiers from constraints would be very limited due to probe
ordering issues.  You'd need to make sure that the notifiers had access
to whatever other hardware they need to fiddle with before we started
applying the constraints which just feels like asking for initialisation
ordering fun.

> >>For OMAP3 (and OMAP2 in some cases) the 1st SD/SDIO/MMC controller
> >>can operate at either 1.8V or 3V, however at 3V the board must apply
> >>voltage level shifting (PBIAS configuation).  Currently the

> >This really sounds like something that's internal to the MMC controller
> >driver - based on your description I'm very surprised that boards even
> >get visibility of this.  Surely the MMC controller driver should be
> >taking care of configuring PBIAS itself, assuming it's something
> >internal to the chip?

> It is considered that the driver should know only about hsmmc ip not
> all of OMAP.  Additionally the OMAP community does not allow the use
> of the necessary system control registers outside of the board level
> i.e. arch/arm/mach-omap2 etc.  Hence the driver calls down to the
> board level.  That is ugly but not a problem.

That sounds like you'll end up with a lot of cut'n'paste for this in
boards but if it works I guess it's OK.  Are there situations where
different boards need to do different things?

> >                       Adding code to the regulator API so that the OMAP
> >MMC driver can be notified of the configuration it has applied to itself
> >(and any failures that ensue) feels baroque.

> As you wish.  That rules out using the core to turn off the regulator at
> boot up, but that is OK, the driver can do it.  At present the driver
> makes the mistake of just doing regulator_enable() / disable() to ensure the
> regulator is off, without calling down to the board level for the PBIAS
> configuration.  It also comments the code as an "ugly hack" whereas
> it is really the only way to do it, if the regulator can't do the
> PBIAS configuration automatically.  I will just fix the driver and add
> better comments.

With the description you've given it really does feel like handling this
in the MMC driver is the best approach.

Re: [PATCH] MMC: move regulator handling closer to core v3

[Mark Brown]

On Fri, Sep 10, 2010 at 01:05:26PM +0300, Adrian Hunter wrote:
> Mark Brown wrote:
> >On Thu, Sep 09, 2010 at 03:56:31PM +0300, Adrian Hunter wrote:

> >                                                I would expect that
> >anything that needs pre and post change notifications like this is
> >actually changing the configuration itself (since if it's that sensitive
> >to changes it seems likely that it wouldn't be enthused about having the
> >configuration changed while it's trying to do stuff).

> Yes, except when it is the regulator core trying to set the initial power
> state on or off.

Like I say, I'd not expect anything that needs this sort of fiddly
control in the hardware to be happy with doing this outside the driver
itself - this includes via the constraints.  If it's important that two
bits of hardware be updated in lockstep then I'd expect you to need to
probe both bits of hardware and control them from one place, which means
that using notifiers from constraints would be very limited due to probe
ordering issues.  You'd need to make sure that the notifiers had access
to whatever other hardware they need to fiddle with before we started
applying the constraints which just feels like asking for initialisation
ordering fun.

> >>For OMAP3 (and OMAP2 in some cases) the 1st SD/SDIO/MMC controller
> >>can operate at either 1.8V or 3V, however at 3V the board must apply
> >>voltage level shifting (PBIAS configuation).  Currently the

> >This really sounds like something that's internal to the MMC controller
> >driver - based on your description I'm very surprised that boards even
> >get visibility of this.  Surely the MMC controller driver should be
> >taking care of configuring PBIAS itself, assuming it's something
> >internal to the chip?

> It is considered that the driver should know only about hsmmc ip not
> all of OMAP.  Additionally the OMAP community does not allow the use
> of the necessary system control registers outside of the board level
> i.e. arch/arm/mach-omap2 etc.  Hence the driver calls down to the
> board level.  That is ugly but not a problem.

That sounds like you'll end up with a lot of cut'n'paste for this in
boards but if it works I guess it's OK.  Are there situations where
different boards need to do different things?

> >                       Adding code to the regulator API so that the OMAP
> >MMC driver can be notified of the configuration it has applied to itself
> >(and any failures that ensue) feels baroque.

> As you wish.  That rules out using the core to turn off the regulator at
> boot up, but that is OK, the driver can do it.  At present the driver
> makes the mistake of just doing regulator_enable() / disable() to ensure the
> regulator is off, without calling down to the board level for the PBIAS
> configuration.  It also comments the code as an "ugly hack" whereas
> it is really the only way to do it, if the regulator can't do the
> PBIAS configuration automatically.  I will just fix the driver and add
> better comments.

With the description you've given it really does feel like handling this
in the MMC driver is the best approach.

[PATCH] MMC: move regulator handling closer to core v3

[Mark Brown]

On Fri, Sep 10, 2010 at 01:05:26PM +0300, Adrian Hunter wrote:
> Mark Brown wrote:
> >On Thu, Sep 09, 2010 at 03:56:31PM +0300, Adrian Hunter wrote:

> >                                                I would expect that
> >anything that needs pre and post change notifications like this is
> >actually changing the configuration itself (since if it's that sensitive
> >to changes it seems likely that it wouldn't be enthused about having the
> >configuration changed while it's trying to do stuff).

> Yes, except when it is the regulator core trying to set the initial power
> state on or off.

Like I say, I'd not expect anything that needs this sort of fiddly
control in the hardware to be happy with doing this outside the driver
itself - this includes via the constraints.  If it's important that two
bits of hardware be updated in lockstep then I'd expect you to need to
probe both bits of hardware and control them from one place, which means
that using notifiers from constraints would be very limited due to probe
ordering issues.  You'd need to make sure that the notifiers had access
to whatever other hardware they need to fiddle with before we started
applying the constraints which just feels like asking for initialisation
ordering fun.

> >>For OMAP3 (and OMAP2 in some cases) the 1st SD/SDIO/MMC controller
> >>can operate at either 1.8V or 3V, however at 3V the board must apply
> >>voltage level shifting (PBIAS configuation).  Currently the

> >This really sounds like something that's internal to the MMC controller
> >driver - based on your description I'm very surprised that boards even
> >get visibility of this.  Surely the MMC controller driver should be
> >taking care of configuring PBIAS itself, assuming it's something
> >internal to the chip?

> It is considered that the driver should know only about hsmmc ip not
> all of OMAP.  Additionally the OMAP community does not allow the use
> of the necessary system control registers outside of the board level
> i.e. arch/arm/mach-omap2 etc.  Hence the driver calls down to the
> board level.  That is ugly but not a problem.

That sounds like you'll end up with a lot of cut'n'paste for this in
boards but if it works I guess it's OK.  Are there situations where
different boards need to do different things?

> >                       Adding code to the regulator API so that the OMAP
> >MMC driver can be notified of the configuration it has applied to itself
> >(and any failures that ensue) feels baroque.

> As you wish.  That rules out using the core to turn off the regulator at
> boot up, but that is OK, the driver can do it.  At present the driver
> makes the mistake of just doing regulator_enable() / disable() to ensure the
> regulator is off, without calling down to the board level for the PBIAS
> configuration.  It also comments the code as an "ugly hack" whereas
> it is really the only way to do it, if the regulator can't do the
> PBIAS configuration automatically.  I will just fix the driver and add
> better comments.

With the description you've given it really does feel like handling this
in the MMC driver is the best approach.