| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4C7C1A2B.1010400@nokia.com>
Date: Mon, 30 Aug 2010 23:52:59 +0300
From: Adrian Hunter <adrian.hunter@...ia.com>
To: Linus Walleij <linus.walleij@...ricsson.com>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Daniel Mack <daniel@...aq.de>,
Pierre Ossman <pierre@...man.eu>,
Matt Fleming <matt@...sole-pimps.org>,
David Brownell <dbrownell@...rs.sourceforge.net>,
Russell King <rmk+kernel@....linux.org.uk>,
Eric Miao <eric.y.miao@...il.com>,
Cliff Brake <cbrake@...-systems.com>,
"Lavinen Jarkko (Nokia-MS/Helsinki)" <jarkko.lavinen@...ia.com>,
Mark Brown <broonie@...nsource.wolfsonmicro.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Liam Girdwood <lrg@...mlogic.co.uk>,
Tony Lindgren <tony@...mide.com>,
Robert Jarzmik <robert.jarzmik@...e.fr>,
Sundar Iyer <sundar.iyer@...ricsson.com>,
Bengt Jonsson <bengt.jonsson@...ricsson.com>,
"linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH] MMC: move regulator handling closer to core
Linus Walleij 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.
>
> 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: Daniel Mack <daniel@...aq.de>
> Cc: Pierre Ossman <pierre@...man.eu>
> Cc: Matt Fleming <matt@...sole-pimps.org>
> Cc: David Brownell <dbrownell@...rs.sourceforge.net>
> Cc: Russell King <rmk+kernel@....linux.org.uk>
> Cc: Eric Miao <eric.y.miao@...il.com>
> Cc: Cliff Brake <cbrake@...-systems.com>
> Cc: Jarkko Lavinen <jarkko.lavinen@...ia.com>
> Cc: Mark Brown <broonie@...nsource.wolfsonmicro.com>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Cc: Liam Girdwood <lrg@...mlogic.co.uk>
> Cc: Mark Brown <broonie@...nsource.wolfsonmicro.com>
> Cc: Tony Lindgren <tony@...mide.com>
> Cc: Adrian Hunter <adrian.hunter@...ia.com>
> Cc: Robert Jarzmik <robert.jarzmik@...e.fr>
> Cc: Sundar Iyer <sundar.iyer@...ricsson.com>
> Cc: Bengt Jonsson <bengt.jonsson@...ricsson.com>
> Cc: linux-mmc@...r.kernel.org
> Cc: linux-arm-kernel@...ts.infradead.org
> Signed-off-by: Linus Walleij <linus.walleij@...ricsson.com>
> ---
> This is not the final movement of regulator code into the
> MMC framework by a long shot, but it's atleast a starter.
> If you like it, ACK it.
>
> It's not easy for me to test this code since both the OMAP2 and
> PXA3XX defconfigs have (unrelated) build failures on the current
> -next tree, however the U300 builds fine and seems to work nicely,
> I'll stress-test it a bit more though.
> ---
> drivers/mmc/core/core.c | 38 +++++++++++++++++++++++++++++---------
> drivers/mmc/host/mmci.c | 21 +++++++++++++--------
> drivers/mmc/host/omap_hsmmc.c | 32 ++++++++++++++++++++++----------
> drivers/mmc/host/pxamci.c | 24 ++++++++++++++++++------
> include/linux/mmc/host.h | 8 +++++++-
> 5 files changed, 89 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 5db49b1..1e8a70e 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,16 +817,38 @@ 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;
> }
>
> return result;
> }
> EXPORT_SYMBOL(mmc_regulator_set_ocr);
> +#else
> +/*
> + * For drivers with optional regulator support we offer to
> + * just compile this thing out with functions that always
> + * succeed, just like the default stuff from the regulator
> + * core.
> + */
Shouldn't these be in the header? The comment is not needed.
> +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
>
> /*
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index 840b301..0734ccb 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -507,19 +507,24 @@ 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);
> + if (ret)
> + dev_err(mmc_dev(mmc),
> + "could not disable regulator\n");
What about putting the dev_err inside mmc_regulator_set_ocr()?
> + }
> 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
> + if (host->vcc) {
> + ret = mmc_regulator_set_ocr(mmc, host->vcc, ios->vdd);
> + if (ret)
> + dev_err(mmc_dev(mmc),
> + "could not set regulator OCR\n");
> + }
> if (host->plat->vdd_handler)
> pwr |= host->plat->vdd_handler(mmc_dev(mmc), ios->vdd,
> ios->power_mode);
> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> index 4a8776f..9debfe2 100644
> --- a/drivers/mmc/host/omap_hsmmc.c
> +++ b/drivers/mmc/host/omap_hsmmc.c
> @@ -249,10 +249,15 @@ static int omap_hsmmc_1_set_power(struct device *dev, int slot, int power_on,
> if (mmc_slot(host).before_set_reg)
> mmc_slot(host).before_set_reg(dev, slot, power_on, vdd);
>
> - if (power_on)
> - ret = mmc_regulator_set_ocr(host->vcc, vdd);
> - else
> - ret = mmc_regulator_set_ocr(host->vcc, 0);
> + if (power_on) {
> + ret = mmc_regulator_set_ocr(host->mmc, host->vcc, vdd);
> + if (ret)
> + dev_err(dev, "could not set regulator OCR\n");
> + } else {
> + ret = mmc_regulator_set_ocr(host->mmc, host->vcc, 0);
> + if (ret)
> + dev_err(dev, "could not disable regulator\n");
> + }
>
> if (mmc_slot(host).after_set_reg)
> mmc_slot(host).after_set_reg(dev, slot, power_on, vdd);
> @@ -291,18 +296,25 @@ 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);
> + if (ret)
> + dev_err(dev, "could not set regulator OCR\n");
> /* 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 +355,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..47dae9b 100644
> --- a/drivers/mmc/host/pxamci.c
> +++ b/drivers/mmc/host/pxamci.c
> @@ -99,14 +99,26 @@ 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);
> + if (ret)
> + dev_err(mmc_dev(host->mmc),
> + "could not set regulator OCR\n");
> + } else if (power_mode == MMC_POWER_OFF)
> + ret = mmc_regulator_set_ocr(host->mmc, host->vcc, 0);
> + if (ret)
> + dev_err(mmc_dev(host->mmc),
> + "could not disable regulator\n");
> + }
mmc_power_off() does set ios->vdd to 0 so the original code was fine
wrt to ignoring power_mode.
> if (!host->vcc && host->pdata &&
> gpio_is_valid(host->pdata->gpio_power)) {
> on = ((1 << vdd) & host->pdata->ocr_mask);
> @@ -492,7 +504,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..8f5d765 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;
> @@ -251,7 +255,9 @@ static inline void mmc_signal_sdio_irq(struct mmc_host *host)
> 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_set_ocr(struct mmc_host *mmc,
> + struct regulator *supply,
> + unsigned short vdd_bit);
>
> int mmc_card_awake(struct mmc_host *host);
> int mmc_card_sleep(struct mmc_host *host);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists