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 <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Liam Girdwood <[email protected]>
Cc: Pierre Ossman <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Matt Fleming <[email protected]>
Cc: Adrian Hunter <[email protected]>
Cc: David Brownell <[email protected]>
Cc: Russell King <[email protected]>
Cc: Linus Walleij <[email protected]>
Cc: Eric Miao <[email protected]>
Cc: Robert Jarzmik <[email protected]>
Cc: Cliff Brake <[email protected]>
Cc: Jarkko Lavinen <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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 ++++++----
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);
+
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);
--
1.6.5.2
On Thu, Dec 03, 2009 at 01:46:30PM +0100, Daniel 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
This is historical, they can all be converted to regulator_get_exclusive()
so the move to the core (while good) isn't required for this reason.
> 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;
> + }
Can the MMC core actually tolerate the MMC power not getting killed when
expected? My understanding from previous discussion was that it wasn't
able to do so. If it is then conversion to using regulator_get_exclusive()
isn't desirable, of course.
On Thu, Dec 03, 2009 at 01:06:27PM +0000, Mark Brown wrote:
> On Thu, Dec 03, 2009 at 01:46:30PM +0100, Daniel 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
>
> This is historical, they can all be converted to regulator_get_exclusive()
> so the move to the core (while good) isn't required for this reason.
Is it? What if you share one regulator for two slots? While this isn't a
problem I have met in real life, this should still be considered.
The problem I _did_ see, however, was a warning when the regulator was
marked as always_on in its constraints. What happens then is that
regulator_is_enabled() will always return 1, causing the pxamci code to
- call regulator_disable() on power off, but
- _not_ call regulator_enable() in the oposite case
and that brings the regulator's reference count out of sync.
Making those drivers claim their regulators exclusively _does_ solve the
first problem, but not the latter.
> > 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;
> > + }
>
> Can the MMC core actually tolerate the MMC power not getting killed when
> expected? My understanding from previous discussion was that it wasn't
> able to do so. If it is then conversion to using regulator_get_exclusive()
> isn't desirable, of course.
I would expect the power to be killed when the last user stops using it.
Which should result in the same effect if you only have one host, one
regulator, and one user.
Daniel
On Thu, Dec 03, 2009 at 02:14:23PM +0100, Daniel Mack wrote:
> On Thu, Dec 03, 2009 at 01:06:27PM +0000, Mark Brown wrote:
> > This is historical, they can all be converted to regulator_get_exclusive()
> > so the move to the core (while good) isn't required for this reason.
> Is it? What if you share one regulator for two slots? While this isn't a
> problem I have met in real life, this should still be considered.
I agree, this is a configuration which I have also seen, but there was a
strong insistence that the power off had to function as expected. An
approach which allows shared regulators is generally always preferable
since it copes with a wider range of system designs.
> The problem I _did_ see, however, was a warning when the regulator was
> marked as always_on in its constraints. What happens then is that
> regulator_is_enabled() will always return 1, causing the pxamci code to
...
> Making those drivers claim their regulators exclusively _does_ solve the
> first problem, but not the latter.
Yeah, there's currently an assumption that the constraints will be
suitable for the driver there. A driver that can handle sharing should
always cope here, it's one reason to prefer them.
> > > 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;
> > > + }
> > Can the MMC core actually tolerate the MMC power not getting killed when
> > expected? My understanding from previous discussion was that it wasn't
> > able to do so. If it is then conversion to using regulator_get_exclusive()
> > isn't desirable, of course.
> I would expect the power to be killed when the last user stops using it.
> Which should result in the same effect if you only have one host, one
> regulator, and one user.
Yes, it's always fine in that case (modulo always_on and/or regulators
without power control). This goes back to the thing about using
regulator_get_exclusive(), the message given was that the MMC drivers
really needed to be able to guarantee that the power would be removed
when that was requested.
Like I say, if there isn't a *strict* requirement but it's only
desirable (possibly strongly desirable) then your approach is obviously
preferable.
On Thu, Dec 03, 2009 at 01:22:41PM +0000, Mark Brown wrote:
> On Thu, Dec 03, 2009 at 02:14:23PM +0100, Daniel Mack wrote:
[...]
> > I would expect the power to be killed when the last user stops using it.
> > Which should result in the same effect if you only have one host, one
> > regulator, and one user.
>
> Yes, it's always fine in that case (modulo always_on and/or regulators
> without power control).
Well, it didn't for me and always_on, though, due to the return values I
described.
> This goes back to the thing about using
> regulator_get_exclusive(), the message given was that the MMC drivers
> really needed to be able to guarantee that the power would be removed
> when that was requested.
>
> Like I say, if there isn't a *strict* requirement but it's only
> desirable (possibly strongly desirable) then your approach is obviously
> preferable.
The mmci people would need to answer that. To me, the code just looked
like a power saving feature.
If this driver needs it, the only tweak to my patch to let that
particular call site use regulator_get_exclusive, and the core will
still do the right thing. For this case, the behaviour should be exactly
the same than it currently is, correct?
Daniel
On Thu, Dec 03, 2009 at 02:32:00PM +0100, Daniel Mack wrote:
> On Thu, Dec 03, 2009 at 01:22:41PM +0000, Mark Brown wrote:
> > On Thu, Dec 03, 2009 at 02:14:23PM +0100, Daniel Mack wrote:
> > > I would expect the power to be killed when the last user stops using it.
> > > Which should result in the same effect if you only have one host, one
> > > regulator, and one user.
> > Yes, it's always fine in that case (modulo always_on and/or regulators
> > without power control).
> Well, it didn't for me and always_on, though, due to the return values I
> described.
I mean your new code is fine.
> > This goes back to the thing about using
> > regulator_get_exclusive(), the message given was that the MMC drivers
> > really needed to be able to guarantee that the power would be removed
> > when that was requested.
> > Like I say, if there isn't a *strict* requirement but it's only
> > desirable (possibly strongly desirable) then your approach is obviously
> > preferable.
> The mmci people would need to answer that. To me, the code just looked
> like a power saving feature.
> If this driver needs it, the only tweak to my patch to let that
> particular call site use regulator_get_exclusive, and the core will
> still do the right thing. For this case, the behaviour should be exactly
> the same than it currently is, correct?
No, you'll also need to update the way the driver bootstraps the
reference count since with regulator_get_exclusive() the reference count
is initialised to 1 if the regulator is enabled when it is claimed.
On Thu, Dec 03, 2009 at 01:40:32PM +0000, Mark Brown wrote:
> On Thu, Dec 03, 2009 at 02:32:00PM +0100, Daniel Mack wrote:
[...]
> > The mmci people would need to answer that. To me, the code just looked
> > like a power saving feature.
>
> > If this driver needs it, the only tweak to my patch to let that
> > particular call site use regulator_get_exclusive, and the core will
> > still do the right thing. For this case, the behaviour should be exactly
> > the same than it currently is, correct?
>
> No, you'll also need to update the way the driver bootstraps the
> reference count since with regulator_get_exclusive() the reference count
> is initialised to 1 if the regulator is enabled when it is claimed.
Ok, fine. It's still just the driver which would need amendment, not the
core. Thanks for explaining this.
Let's wait for the mmci maintainers to speak up on this particular
point.
Thanks,
Daniel
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 <[email protected]>
> Cc: Mark Brown <[email protected]>
> Cc: Liam Girdwood <[email protected]>
> Cc: Pierre Ossman <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Matt Fleming <[email protected]>
> Cc: Adrian Hunter <[email protected]>
> Cc: David Brownell <[email protected]>
> Cc: Russell King <[email protected]>
> Cc: Linus Walleij <[email protected]>
> Cc: Eric Miao <[email protected]>
> Cc: Robert Jarzmik <[email protected]>
> Cc: Cliff Brake <[email protected]>
> Cc: Jarkko Lavinen <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> 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);
On Thu, Dec 03, 2009 at 01:22:41PM +0000, Mark Brown wrote:
> On Thu, Dec 03, 2009 at 02:14:23PM +0100, Daniel Mack wrote:
> > I would expect the power to be killed when the last user stops using it.
> > Which should result in the same effect if you only have one host, one
> > regulator, and one user.
>
> Yes, it's always fine in that case (modulo always_on and/or regulators
> without power control). This goes back to the thing about using
> regulator_get_exclusive(), the message given was that the MMC drivers
> really needed to be able to guarantee that the power would be removed
> when that was requested.
If you take some cards through a series of steps and they stop responding,
it's normally because you've caused their internal state machine to
transition to "invalid" mode.
Further commands are ignored. The only recovery is to power cycle them.
As for the regulator support in mmci, I can't say anything about it - I
don't have anything which uses it (not that I have any desire at present
to bring a MMC or SD card near Linux after my relatively new 128MB MMC
card got totally buggered with pxamci - that's not to say that Linux is
at fault, but the utter crap that seemingly most MMC/SD cards are. Hence
why I'm not listed as maintainer of mmci.)
The regulator support in mmci is purely Linus' domain.
On Thu, Dec 03, 2009 at 02:58:25PM +0000, Russell King - ARM Linux wrote:
> On Thu, Dec 03, 2009 at 01:22:41PM +0000, Mark Brown wrote:
> > without power control). This goes back to the thing about using
> > regulator_get_exclusive(), the message given was that the MMC drivers
> > really needed to be able to guarantee that the power would be removed
> > when that was requested.
> If you take some cards through a series of steps and they stop responding,
> it's normally because you've caused their internal state machine to
> transition to "invalid" mode.
> Further commands are ignored. The only recovery is to power cycle them.
I assume that this is something it's desirable to be able to do in the
face of poor hardware rather than something that the Linux MMC stack is
actively using in normal operation (modulo issues with the general
quality of implementation of MMC hardware)?
On Thu, Dec 03, 2009 at 04:27:39PM +0200, Adrian Hunter wrote:
> gDaniel Mack wrote:
[...]
> > 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 ?
Argh, missed that one. And this particular case doesn't fit to my
modifications. I don't know the code well ... We would need to
have a struct mmc_host * in all the functions there calling
mmc_regulator_{set,get}_ocr. Any idea how to resolve that?
> >--- 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?
Yes, I can change the patch to do that, no problem. The major reason why
I didn't put the regulator_get() to the mmc core is that I need to have
the platform_device to obtain its name.
Daniel
Daniel Mack wrote:
> On Thu, Dec 03, 2009 at 04:27:39PM +0200, Adrian Hunter wrote:
>> gDaniel Mack wrote:
>
> [...]
>
>>> 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 ?
>
> Argh, missed that one. And this particular case doesn't fit to my
> modifications. I don't know the code well ... We would need to
> have a struct mmc_host * in all the functions there calling
> mmc_regulator_{set,get}_ocr. Any idea how to resolve that?
>
Pass it down from the omap_hsmmc driver.
>>> --- 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?
>
> Yes, I can change the patch to do that, no problem. The major reason why
> I didn't put the regulator_get() to the mmc core is that I need to have
> the platform_device to obtain its name.
>
> Daniel
>
>
On Thu, Dec 03, 2009 at 10:12:36PM +0200, Adrian Hunter wrote:
> Daniel Mack wrote:
> >On Thu, Dec 03, 2009 at 04:27:39PM +0200, Adrian Hunter wrote:
> >>What about arch/arm/mach-omap2/mmc-twl4030.c ?
> >
> >Argh, missed that one. And this particular case doesn't fit to my
> >modifications. I don't know the code well ... We would need to
> >have a struct mmc_host * in all the functions there calling
> >mmc_regulator_{set,get}_ocr. Any idea how to resolve that?
> >
>
> Pass it down from the omap_hsmmc driver.
It's not that easy, unfortunately, because this code does not conform to
all the other mmc host drivers in tree.
I don't understand why things are done the way it is currently
implemented. Why isn't there a mmc_host for each slot, and why is a
regulator reference acquired for each slot, and not once for the whole
device?
Even with the default 'vcc' supply factored out to the mmc core, the
'vmmc_aux' regulator would still need some extra attention, but I would
also do that from the omap_hsmmc driver rather than in the plaform
support code.
Moving the regulator handling to the mmc core would require a major
cleanup to all this code, but I don't have such hardware to test my
modifications. Can anyone help here?
Thanks,
Daniel
On Fri, Dec 04, 2009 at 12:58:05PM +0100, Daniel Mack wrote:
> > >On Thu, Dec 03, 2009 at 04:27:39PM +0200, Adrian Hunter wrote:
>
> > >>What about arch/arm/mach-omap2/mmc-twl4030.c ?
> > >
> > >Argh, missed that one. And this particular case doesn't fit to my
> > >modifications. I don't know the code well ... We would need to
> > >have a struct mmc_host * in all the functions there calling
> > >mmc_regulator_{set,get}_ocr. Any idea how to resolve that?
> > >
> >
> > Pass it down from the omap_hsmmc driver.
>
> It's not that easy, unfortunately, because this code does not conform to
> all the other mmc host drivers in tree.
>
> I don't understand why things are done the way it is currently
> implemented. Why isn't there a mmc_host for each slot, and why is a
> regulator reference acquired for each slot, and not once for the whole
> device?
>
> Even with the default 'vcc' supply factored out to the mmc core, the
> 'vmmc_aux' regulator would still need some extra attention, but I would
> also do that from the omap_hsmmc driver rather than in the plaform
> support code.
>
> Moving the regulator handling to the mmc core would require a major
> cleanup to all this code, but I don't have such hardware to test my
> modifications. Can anyone help here?
Can anyone of the OMAP people help sort this out?
Thanks,
Daniel
> -----Original Message-----
> From: Daniel Mack [mailto:[email protected]]
> Sent: Friday, December 11, 2009 6:59 PM
> To: Adrian Hunter
> Cc: Matt Fleming; David Brownell; Eric Miao; Linus Walleij; Lavinen Jarkko
> (Nokia-D/Helsinki); Mark Brown; [email protected]; linux-
> [email protected]; [email protected] >> Madhusudhan Chikkature; Cliff
> Brake; Russell King; Pierre Ossman; Robert Jarzmik; Andrew Morton; linux-
> [email protected]; Liam Girdwood
> Subject: Re: [PATCH] mmc: move regulator handling to core
>
> On Fri, Dec 04, 2009 at 12:58:05PM +0100, Daniel Mack wrote:
> > > >On Thu, Dec 03, 2009 at 04:27:39PM +0200, Adrian Hunter wrote:
> >
> > > >>What about arch/arm/mach-omap2/mmc-twl4030.c ?
> > > >
> > > >Argh, missed that one. And this particular case doesn't fit to my
> > > >modifications. I don't know the code well ... We would need to
> > > >have a struct mmc_host * in all the functions there calling
> > > >mmc_regulator_{set,get}_ocr. Any idea how to resolve that?
> > > >
> > >
> > > Pass it down from the omap_hsmmc driver.
> >
> > It's not that easy, unfortunately, because this code does not conform to
> > all the other mmc host drivers in tree.
> >
> > I don't understand why things are done the way it is currently
> > implemented. Why isn't there a mmc_host for each slot, and why is a
> > regulator reference acquired for each slot, and not once for the whole
> > device?
> >
> > Even with the default 'vcc' supply factored out to the mmc core, the
> > 'vmmc_aux' regulator would still need some extra attention, but I would
> > also do that from the omap_hsmmc driver rather than in the plaform
> > support code.
> >
> > Moving the regulator handling to the mmc core would require a major
> > cleanup to all this code, but I don't have such hardware to test my
> > modifications. Can anyone help here?
The mmc-twl4030 wrapper is being used by many
platforms(2430sdp,omap3_beagle,LDP,Overo,Omap3EVM,Pandora,3430SDP,Nokia_RX51
,Zoom2,Zoom3,3630SDP,CM_T35,IGEP0020).
I have access only to 3430Sdp, Zoom2 and Zoom3. I can test your changes on
these platforms. Would that help?
Regards,
Madhu
>
> Can anyone of the OMAP people help sort this out?
>
> Thanks,
> Daniel
On Friday 04 December 2009, Daniel Mack wrote:
> On Thu, Dec 03, 2009 at 10:12:36PM +0200, Adrian Hunter wrote:
> > Daniel Mack wrote:
> > >On Thu, Dec 03, 2009 at 04:27:39PM +0200, Adrian Hunter wrote:
>
> > >>What about arch/arm/mach-omap2/mmc-twl4030.c ?
> > >
> > >Argh, missed that one. And this particular case doesn't fit to my
> > >modifications. I don't know the code well ... We would need to
> > >have a struct mmc_host * in all the functions there calling
> > >mmc_regulator_{set,get}_ocr. Any idea how to resolve that?
Finish separating the HSMMC setup from the OMAP2 setup, then the
HSMMC stuff will be a lot easier to cope with. The HSMMC stuff
is never used with the mfd/menelaus transceiver/slot switch stuff;
including those data structures caused lots of hassles.
The only real issue with the HSMMC stuff is that it's got to continue
working with the many wiring options now in use.
> Moving the regulator handling to the mmc core would require a major
> cleanup to all this code, but I don't have such hardware to test my
> modifications. Can anyone help here?
Simplest to borrow a Beagle. Make sure whatever you do
can handle both standard SDHC cards, and also 8-bit HSMMC.
Once that works, the rest will fall out pretty easily.
- Dave
Hi Daniel,
On Thu, Dec 03, 2009 at 01:46:30PM +0100, Daniel 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.
Looks like this patch got dropped because of the missing modifications
to arch/arm/mach-omap2/mmc-twl4030.c. Are we still interested in the
patch otherwise, and can anyone help with that?
--
Chris Ball <[email protected]> <http://printf.net/>
One Laptop Per Child
2010/8/27 Chris Ball <[email protected]>:
> Looks like this patch got dropped because of the missing modifications
> to arch/arm/mach-omap2/mmc-twl4030.c. ?Are we still interested in the
> patch otherwise, and can anyone help with that?
Actually just before the summer I submitted something not quite similar:
I moved all regulator handling *out* of the MMC core because I didn't
trust the way reference counting was being handled.
There is something very disturbing about this code from
regulator/core/core.c mmc_regulator_set_ocr():
enabled = regulator_is_enabled(supply);
if (enabled < 0)
return enabled;
if (...) {
(...)
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 (result == 0 && !enabled)
result = regulator_enable(supply);
} else if (enabled) {
result = regulator_disable(supply);
}
I didn't realize until today where the problem is: if you have
two hosts on the same regulator this does not handle
concurrency correctly. There is no lock taken between reading
the enabled status and acting on it later in the code.
So it looks to me like it is possible for one slot to enable the
regulator and race with another slot which disables it at the
same time and end up with the regulator disabled :-(
My solution would still be to move the enable/disable out
of the core, so I need to follow up on that.
This old patch however, goes in the opposite direction,
moving it all into the core. If you do this, please fix the
concurrency issue also...
Yours,
Linus Walleij
On Sat, Aug 28, 2010 at 04:48:58PM +0200, Linus Walleij wrote:
> 2010/8/27 Chris Ball <[email protected]>:
> > Looks like this patch got dropped because of the missing modifications
> > to arch/arm/mach-omap2/mmc-twl4030.c. ?Are we still interested in the
> > patch otherwise, and can anyone help with that?
> Actually just before the summer I submitted something not quite similar:
> I moved all regulator handling *out* of the MMC core because I didn't
> trust the way reference counting was being handled.
This seems like the wrong approach; if there's a problem it'd seem much
better to fix the core code that everything is sharing rather than
factor it out - the location of the code is orthogonal to its
helpfulness.
> There is something very disturbing about this code from
> regulator/core/core.c mmc_regulator_set_ocr():
The MMC code in the core was last time I looked explicitly reliant on
the regulators not being shared - it wants the regulators to be grebbed
with regulator_get_exclusive() which guarantees that. When the code was
added there was a strong insistance that shared supplies could not be
used with MMC so this was fine.
> So it looks to me like it is possible for one slot to enable the
> regulator and race with another slot which disables it at the
> same time and end up with the regulator disabled :-(
It's not really a race, it's just a simple inability to cope with
something else sharing the same regulator - at the minute it'll do
things like turn off the regulator when one port is done even if the
other port still needs it.
> My solution would still be to move the enable/disable out
> of the core, so I need to follow up on that.
The code needs to be changed so that the port remembers internally if
it itself needs the regulator enabled or disabled rather than inspecting
the current hardware state since that may differ as a result of being
forced by the system or the activity of other ports.
2010/8/29 Mark Brown <[email protected]>:
> On Sat, Aug 28, 2010 at 04:48:58PM +0200, Linus Walleij wrote:
>> 2010/8/27 Chris Ball <[email protected]>:
>
>> > Looks like this patch got dropped because of the missing modifications
>> > to arch/arm/mach-omap2/mmc-twl4030.c. ?Are we still interested in the
>> > patch otherwise, and can anyone help with that?
>
>> Actually just before the summer I submitted something not quite similar:
>> I moved all regulator handling *out* of the MMC core because I didn't
>> trust the way reference counting was being handled.
>
> This seems like the wrong approach; if there's a problem it'd seem much
> better to fix the core code that everything is sharing rather than
> factor it out - the location of the code is orthogonal to its
> helpfulness.
I actually did not move the essential regulator bits out just enable/disable,
so that these were in the sites where the regulators were actually
enabled/disabled in respective driver. That makes the internal
regulator reference count do the trick.
I can get that patch into shape easily, if for nothing else it's a nice
discussion item.
Yours,
Linus Walleij
On Sun, Aug 29, 2010 at 05:30:48PM +0200, Linus Walleij wrote:
> 2010/8/29 Mark Brown <[email protected]>:
> > This seems like the wrong approach; if there's a problem it'd seem much
> > better to fix the core code that everything is sharing rather than
> > factor it out - the location of the code is orthogonal to its
> > helpfulness.
> I actually did not move the essential regulator bits out just enable/disable,
> so that these were in the sites where the regulators were actually
> enabled/disabled in respective driver. That makes the internal
> regulator reference count do the trick.
I'm not sure what "the sites where the regulators were actually
enabled/disabled in respective driver" are but my understanding was that
there's a bit of an issue here in that the MMC core does not guarantee
balanced enable/disable calls.
2010/8/31 Mark Brown <[email protected]>:
> On Sun, Aug 29, 2010 at 05:30:48PM +0200, Linus Walleij wrote:
>> 2010/8/29 Mark Brown <[email protected]>:
>
>> > This seems like the wrong approach; if there's a problem it'd seem much
>> > better to fix the core code that everything is sharing rather than
>> > factor it out - the location of the code is orthogonal to its
>> > helpfulness.
>
>> I actually did not move the essential regulator bits out just enable/disable,
>> so that these were in the sites where the regulators were actually
>> enabled/disabled in respective driver. That makes the internal
>> regulator reference count do the trick.
>
> I'm not sure what "the sites where the regulators were actually
> enabled/disabled in respective driver" are but my understanding was that
> there's a bit of an issue here in that the MMC core does not guarantee
> balanced enable/disable calls.
Yeah I figured out what you wanted and sent an updated patch, check
it out. I'll just weave in some comments from Adrian and it'll do things
the way you want it I believe.
Yours,
Linus Walleij