[PATCH] mmc: move regulator handling to core
Adrian Hunter
adrian.hunter at nokia.com
Thu Dec 3 09:27:39 EST 2009
gDaniel Mack wrote:
> At the moment, regulator operations are done from individual mmc host
> drivers. This is a problem because the regulators are not claimed
> exclusively but the mmc core enables and disables them according to the
> return value of regulator_is_enabled(). That can lead to a number of
> problems and warnings when regulators are shared among multiple
> consumers or if regulators are marked as 'always_on'.
>
> Fix this by moving the some logic to the core, and put the regulator
> reference to the mmc_host struct and let it do its own supply state
> tracking so that the reference counting in the regulator won't get
> confused.
>
> [Note that I could only compile-test the mmci.c change]
>
> Signed-off-by: Daniel Mack <daniel at caiaq.de>
> Cc: Mark Brown <broonie at opensource.wolfsonmicro.com>
> Cc: Liam Girdwood <lrg at slimlogic.co.uk>
> Cc: Pierre Ossman <pierre at ossman.eu>
> Cc: Andrew Morton <akpm at linux-foundation.org>
> Cc: Matt Fleming <matt at console-pimps.org>
> Cc: Adrian Hunter <adrian.hunter at nokia.com>
> Cc: David Brownell <dbrownell at users.sourceforge.net>
> Cc: Russell King <rmk+kernel at arm.linux.org.uk>
> Cc: Linus Walleij <linus.walleij at stericsson.com>
> Cc: Eric Miao <eric.y.miao at gmail.com>
> Cc: Robert Jarzmik <robert.jarzmik at free.fr>
> Cc: Cliff Brake <cbrake at bec-systems.com>
> Cc: Jarkko Lavinen <jarkko.lavinen at nokia.com>
> Cc: linux-mmc at vger.kernel.org
> Cc: linux-arm-kernel at lists.infradead.org
> ---
> 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);
More information about the linux-arm-kernel
mailing list