| Message ID | 1303476191-20663-1-git-send-email-ospite@studenti.unina.it (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 22 Apr 2011 14:43:11 +0200 Antonio Ospite <ospite@studenti.unina.it> wrote: > Add support for powering up SD cards driven by regulators. > This makes the mmc_spi driver work also with the Motorola A910 phone. > > Signed-off-by: Antonio Ospite <ospite@studenti.unina.it> > --- > > Changes since v1: > - Remove the ifdef CONFIG_REGULATOR as regulator_get() degenerates to > a stub as well when the regulator framework is disabled. > - Release the regulator in mmc_spi_remove() > Ping. > Thanks, > Antonio > > drivers/mmc/host/mmc_spi.c | 63 +++++++++++++++++++++++++++++++++++-------- > 1 files changed, 51 insertions(+), 12 deletions(-) > > diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c > index 0f3672d..62b3b02 100644 > --- a/drivers/mmc/host/mmc_spi.c > +++ b/drivers/mmc/host/mmc_spi.c > @@ -31,6 +31,7 @@ > #include <linux/dma-mapping.h> > #include <linux/crc7.h> > #include <linux/crc-itu-t.h> > +#include <linux/regulator/consumer.h> > #include <linux/scatterlist.h> > > #include <linux/mmc/host.h> > @@ -150,11 +151,13 @@ struct mmc_spi_host { > */ > void *ones; > dma_addr_t ones_dma; > + > + struct regulator *vcc; > }; > > static inline int mmc_spi_canpower(struct mmc_spi_host *host) > { > - return host->pdata && host->pdata->setpower; > + return (host->pdata && host->pdata->setpower) || host->vcc; > } > > /****************************************************************************/ > @@ -1225,17 +1228,35 @@ static inline int mmc_spi_setpower(struct mmc_spi_host *host, > unsigned char power_mode, > unsigned int vdd) > { > + if (!mmc_spi_canpower(host)) > + return -ENOSYS; > + > /* switch power on/off if possible, accounting for > * max 250msec powerup time if needed. > */ > - if (mmc_spi_canpower(host)) { > - switch (power_mode) { > - case MMC_POWER_OFF: > - case MMC_POWER_UP: > + switch (power_mode) { > + case MMC_POWER_OFF: > + if (host->vcc) { > + int ret = mmc_regulator_set_ocr(host->mmc, > + host->vcc, 0); > + if (ret) > + return ret; > + } else { > + host->pdata->setpower(&host->spi->dev, vdd); > + } > + break; > + > + case MMC_POWER_UP: > + if (host->vcc) { > + int ret = mmc_regulator_set_ocr(host->mmc, > + host->vcc, vdd); > + if (ret) > + return ret; > + } else { > host->pdata->setpower(&host->spi->dev, vdd); > - if (power_mode == MMC_POWER_UP) > - msleep(host->powerup_msecs); > } > + msleep(host->powerup_msecs); > + break; > } > > return 0; > @@ -1420,12 +1441,27 @@ static int mmc_spi_probe(struct spi_device *spi) > * and power switching gpios. > */ > host->pdata = mmc_spi_get_pdata(spi); > - if (host->pdata) > - mmc->ocr_avail = host->pdata->ocr_mask; > - if (!mmc->ocr_avail) { > - dev_warn(&spi->dev, "ASSUMING 3.2-3.4 V slot power\n"); > - mmc->ocr_avail = MMC_VDD_32_33|MMC_VDD_33_34; > + > + host->vcc = regulator_get(mmc_dev(host->mmc), "vmmc"); > + if (IS_ERR(host->vcc)) { > + host->vcc = NULL; > + } else { > + host->mmc->ocr_avail = mmc_regulator_get_ocrmask(host->vcc); > + if (host->pdata && host->pdata->ocr_mask) > + dev_warn(mmc_dev(host->mmc), > + "ocr_mask/setpower will not be used\n"); > } > + > + if (host->vcc == NULL) { > + /* fall-back to platform data */ > + if (host->pdata) > + mmc->ocr_avail = host->pdata->ocr_mask; > + if (!mmc->ocr_avail) { > + dev_warn(&spi->dev, "ASSUMING 3.2-3.4 V slot power\n"); > + mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34; > + } > + } > + > if (mmc_spi_canpower(host)) { > host->powerup_msecs = host->pdata->powerup_msecs; > if (!host->powerup_msecs || host->powerup_msecs > 250) > @@ -1523,6 +1559,9 @@ static int __devexit mmc_spi_remove(struct spi_device *spi) > if (host->pdata && host->pdata->exit) > host->pdata->exit(&spi->dev, mmc); > > + if (host->vcc) > + regulator_put(host->vcc); > + > mmc_remove_host(mmc); > > if (host->dma_dev) { > -- > 1.7.4.4 > >
On Wed, May 11, 2011 at 12:39:39PM +0200, Antonio Ospite wrote: > Add support for powering up SD cards driven by regulators. > This makes the mmc_spi driver work also with the Motorola A910 phone. > > Signed-off-by: Antonio Ospite <ospite@studenti.unina.it> Reviwed-by: Mark Brown <broonie@opensource.wolfsonmicro.com> but > + switch (power_mode) { > + case MMC_POWER_OFF: > + if (host->vcc) { > + int ret = mmc_regulator_set_ocr(host->mmc, > + host->vcc, 0); > + if (ret) > + return ret; > + } else { > + host->pdata->setpower(&host->spi->dev, vdd); > + } > + break; > + > + case MMC_POWER_UP: > + if (host->vcc) { > + int ret = mmc_regulator_set_ocr(host->mmc, > + host->vcc, vdd); > + if (ret) > + return ret; > + } else { > host->pdata->setpower(&host->spi->dev, vdd); > - if (power_mode == MMC_POWER_UP) > - msleep(host->powerup_msecs); > } > + msleep(host->powerup_msecs); > + break; This stuff all looks like it should be factored out. -- 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
On Wed, 11 May 2011 15:08:53 +0200 Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: [...] > > + switch (power_mode) { > > + case MMC_POWER_OFF: > > + if (host->vcc) { > > + int ret = mmc_regulator_set_ocr(host->mmc, > > + host->vcc, 0); > > + if (ret) > > + return ret; > > + } else { > > + host->pdata->setpower(&host->spi->dev, vdd); > > + } > > + break; > > + > > + case MMC_POWER_UP: > > + if (host->vcc) { > > + int ret = mmc_regulator_set_ocr(host->mmc, > > + host->vcc, vdd); > > + if (ret) > > + return ret; > > + } else { > > host->pdata->setpower(&host->spi->dev, vdd); > > - if (power_mode == MMC_POWER_UP) > > - msleep(host->powerup_msecs); > > } > > + msleep(host->powerup_msecs); > > + break; > > This stuff all looks like it should be factored out. > OK, avoiding some duplication will be good, I agree. I am resending a v4 with the equivalent code: if (host->vcc) { int ret; if (power_mode == MMC_POWER_OFF) vdd = 0; ret = mmc_regulator_set_ocr(host->mmc, host->vcc, vdd); if (ret) return ret; } else { host->pdata->setpower(&host->spi->dev, vdd); } if (power_mode == MMC_POWER_UP) msleep(host->powerup_msecs); Thanks, Antonio
On Wed, May 11, 2011 at 10:53:37PM +0200, Antonio Ospite wrote: > OK, avoiding some duplication will be good, I agree. > I am resending a v4 with the equivalent code: > > if (host->vcc) { > int ret; > > if (power_mode == MMC_POWER_OFF) > vdd = 0; > This isn't really what I meant - what I meant was that all this logic looks like it's generic to multiple drivers. We either set a regulator or call a pdata callback to set power, both of which are completely external to the controller. -- 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
On Wed, 11 May 2011 22:57:04 +0200 Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Wed, May 11, 2011 at 10:53:37PM +0200, Antonio Ospite wrote: > > > OK, avoiding some duplication will be good, I agree. > > > I am resending a v4 with the equivalent code: > > > > if (host->vcc) { > > int ret; > > > > if (power_mode == MMC_POWER_OFF) > > vdd = 0; > > > > This isn't really what I meant - what I meant was that all this logic > looks like it's generic to multiple drivers. We either set a regulator > or call a pdata callback to set power, both of which are completely > external to the controller. > So you mean something like the following: mmc_regulator_set_power(...) { if (host->vcc) { ... } if (host->pdata && host->pdata->setpower) host->pdata->setpower(mmc_dev(host->mmc), vdd); } I like the idea, the only concern I have is that the mmc host drivers can call 'vcc' and 'pdata' and 'setpower' with arbitrary names, what is the kernel practice in cases like this where we want to use some common code accessing driver-specific structures? I can think to three alternatives right now: 1. Consider only code currently in mainline (names are consistently vcc, pdata and setpower) and use a _macro_ based on that, assuming than future patches are going to copy the var names anyways. 2. Add a proper function with all the needed parameters to abstract the actual var names, this would be something like: mmc_regulator_set_power(mmc, power_mode, vdd, vcc, pdata) and yet checking for pdata->setpower can be tricky as 'setpower' is an arbitrary name. 3. Move stuff from driver structures to subsystem structures, which I don't think is needed in this case. 4. (The hidden lazy one) make the code equal across all the drivers so new drivers copying it will be consistent, but still leave it duplicate. Regards, Antonio
On Wed, May 18, 2011 at 07:23:20PM +0200, Antonio Ospite wrote: > So you mean something like the following: > mmc_regulator_set_power(...) > { Yes. > 2. Add a proper function with all the needed parameters to abstract > the actual var names, this would be something like: > mmc_regulator_set_power(mmc, power_mode, vdd, vcc, pdata) > and yet checking for pdata->setpower can be tricky as 'setpower' > is an arbitrary name. This would be good. > 3. Move stuff from driver structures to subsystem structures, which I > don't think is needed in this case. Though this option is also common - often you get a mix of subsystem and device specific things with for example a subsystem wide data structure which the device keeps and passes into a function provided by the subsystem at appropriate moments. -- 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
diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c index 0f3672d..62b3b02 100644 --- a/drivers/mmc/host/mmc_spi.c +++ b/drivers/mmc/host/mmc_spi.c @@ -31,6 +31,7 @@ #include <linux/dma-mapping.h> #include <linux/crc7.h> #include <linux/crc-itu-t.h> +#include <linux/regulator/consumer.h> #include <linux/scatterlist.h> #include <linux/mmc/host.h> @@ -150,11 +151,13 @@ struct mmc_spi_host { */ void *ones; dma_addr_t ones_dma; + + struct regulator *vcc; }; static inline int mmc_spi_canpower(struct mmc_spi_host *host) { - return host->pdata && host->pdata->setpower; + return (host->pdata && host->pdata->setpower) || host->vcc; } /****************************************************************************/ @@ -1225,17 +1228,35 @@ static inline int mmc_spi_setpower(struct mmc_spi_host *host, unsigned char power_mode, unsigned int vdd) { + if (!mmc_spi_canpower(host)) + return -ENOSYS; + /* switch power on/off if possible, accounting for * max 250msec powerup time if needed. */ - if (mmc_spi_canpower(host)) { - switch (power_mode) { - case MMC_POWER_OFF: - case MMC_POWER_UP: + switch (power_mode) { + case MMC_POWER_OFF: + if (host->vcc) { + int ret = mmc_regulator_set_ocr(host->mmc, + host->vcc, 0); + if (ret) + return ret; + } else { + host->pdata->setpower(&host->spi->dev, vdd); + } + break; + + case MMC_POWER_UP: + if (host->vcc) { + int ret = mmc_regulator_set_ocr(host->mmc, + host->vcc, vdd); + if (ret) + return ret; + } else { host->pdata->setpower(&host->spi->dev, vdd); - if (power_mode == MMC_POWER_UP) - msleep(host->powerup_msecs); } + msleep(host->powerup_msecs); + break; } return 0; @@ -1420,12 +1441,27 @@ static int mmc_spi_probe(struct spi_device *spi) * and power switching gpios. */ host->pdata = mmc_spi_get_pdata(spi); - if (host->pdata) - mmc->ocr_avail = host->pdata->ocr_mask; - if (!mmc->ocr_avail) { - dev_warn(&spi->dev, "ASSUMING 3.2-3.4 V slot power\n"); - mmc->ocr_avail = MMC_VDD_32_33|MMC_VDD_33_34; + + host->vcc = regulator_get(mmc_dev(host->mmc), "vmmc"); + if (IS_ERR(host->vcc)) { + host->vcc = NULL; + } else { + host->mmc->ocr_avail = mmc_regulator_get_ocrmask(host->vcc); + if (host->pdata && host->pdata->ocr_mask) + dev_warn(mmc_dev(host->mmc), + "ocr_mask/setpower will not be used\n"); } + + if (host->vcc == NULL) { + /* fall-back to platform data */ + if (host->pdata) + mmc->ocr_avail = host->pdata->ocr_mask; + if (!mmc->ocr_avail) { + dev_warn(&spi->dev, "ASSUMING 3.2-3.4 V slot power\n"); + mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34; + } + } + if (mmc_spi_canpower(host)) { host->powerup_msecs = host->pdata->powerup_msecs; if (!host->powerup_msecs || host->powerup_msecs > 250) @@ -1523,6 +1559,9 @@ static int __devexit mmc_spi_remove(struct spi_device *spi) if (host->pdata && host->pdata->exit) host->pdata->exit(&spi->dev, mmc); + if (host->vcc) + regulator_put(host->vcc); + mmc_remove_host(mmc); if (host->dma_dev) {
Add support for powering up SD cards driven by regulators. This makes the mmc_spi driver work also with the Motorola A910 phone. Signed-off-by: Antonio Ospite <ospite@studenti.unina.it> --- Changes since v1: - Remove the ifdef CONFIG_REGULATOR as regulator_get() degenerates to a stub as well when the regulator framework is disabled. - Release the regulator in mmc_spi_remove() Thanks, Antonio drivers/mmc/host/mmc_spi.c | 63 +++++++++++++++++++++++++++++++++++-------- 1 files changed, 51 insertions(+), 12 deletions(-)