[PATCH 0/4] mmc_spi: Add support for regulator framework

[PATCH 0/4] mmc_spi: Add support for regulator framework

[Antonio Ospite]

Hi,

this patchset has the purpose of adding support for the regulator framework to 
the mmc_spi driver. The first three patches are preparatory cleanups to make 
the forth one more straightforward.

Maybe the fourth patch can be improved, I am open to any suggestions about it.

These changes take strong inspiration from the pxamci driver; they have been 
tested on a Motorola A910, which uses a regulator to powerup the MMC card 
connected to the SPI bus, a test from a current user of the mmc_spi driver 
would not hurt just to be sure no regressions have been introduced.

Thanks,
   Antonio

Antonio Ospite (4):
  mmc_spi.c: factor out the check for power capability
  mmc_spi.c: factor out the SD card shutdown sequence
  mmc_spi.c: factor out a mmc_spi_setpower() function
  mmc_spi.c: add support for the regulator framework

 drivers/mmc/host/mmc_spi.c |  194 +++++++++++++++++++++++++++++---------------
 1 files changed, 129 insertions(+), 65 deletions(-)

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[PATCH 1/4] mmc_spi.c: factor out the check for power capability

[Antonio Ospite]

Factor out the 'canpower' condition into a dedicated function in order
to avoid repetition and to make changing the condition easier.

Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>
---
 drivers/mmc/host/mmc_spi.c |   18 ++++++++++--------
 1 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
index fd877f6..1db18ce 100644
--- a/drivers/mmc/host/mmc_spi.c
+++ b/drivers/mmc/host/mmc_spi.c
@@ -152,6 +152,10 @@ struct mmc_spi_host {
 	dma_addr_t		ones_dma;
 };
 
+static inline int mmc_spi_canpower(struct mmc_spi_host *host)
+{
+	return host->pdata && host->pdata->setpower;
+}
 
 /****************************************************************************/
 
@@ -1187,19 +1191,16 @@ static void mmc_spi_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	struct mmc_spi_host *host = mmc_priv(mmc);
 
 	if (host->power_mode != ios->power_mode) {
-		int		canpower;
-
-		canpower = host->pdata && host->pdata->setpower;
 
 		dev_dbg(&host->spi->dev, "mmc_spi: power %s (%d)%s\n",
 				mmc_powerstring(ios->power_mode),
 				ios->vdd,
-				canpower ? ", can switch" : "");
+				mmc_spi_canpower(host) ? ", can switch" : "");
 
 		/* switch power on/off if possible, accounting for
 		 * max 250msec powerup time if needed.
 		 */
-		if (canpower) {
+		if (mmc_spi_canpower(host)) {
 			switch (ios->power_mode) {
 			case MMC_POWER_OFF:
 			case MMC_POWER_UP:
@@ -1223,7 +1224,8 @@ static void mmc_spi_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 		 *   - MOSI low comes from writing zero
 		 *   - Chipselect is usually active low...
 		 */
-		if (canpower && ios->power_mode == MMC_POWER_OFF) {
+		if (mmc_spi_canpower(host) &&
+		    ios->power_mode == MMC_POWER_OFF) {
 			int mres;
 			u8 nullbyte = 0;
 
@@ -1399,7 +1401,7 @@ static int mmc_spi_probe(struct spi_device *spi)
 		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 (host->pdata && host->pdata->setpower) {
+	if (mmc_spi_canpower(host)) {
 		host->powerup_msecs = host->pdata->powerup_msecs;
 		if (!host->powerup_msecs || host->powerup_msecs > 250)
 			host->powerup_msecs = 250;
@@ -1459,7 +1461,7 @@ static int mmc_spi_probe(struct spi_device *spi)
 			host->dma_dev ? "" : ", no DMA",
 			(host->pdata && host->pdata->get_ro)
 				? "" : ", no WP",
-			(host->pdata && host->pdata->setpower)
+			mmc_spi_canpower(host)
 				? "" : ", no poweroff",
 			(mmc->caps & MMC_CAP_NEEDS_POLL)
 				? ", cd polling" : "");
-- 
1.7.4.1

[PATCH 2/4] mmc_spi.c: factor out the SD card shutdown sequence

[Antonio Ospite]

Factor out the SD card shutdown sequence to a dedicated
mmc_spi_shutdownsequence() function in order to make mmc_spi_set_ios()
more readable, and also for symmetry with mmc_spi_initsequence() which
is already a dedicated function.

Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>
---
 drivers/mmc/host/mmc_spi.c |   90 +++++++++++++++++++++++--------------------
 1 files changed, 48 insertions(+), 42 deletions(-)

diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
index 1db18ce..fe0fdc4 100644
--- a/drivers/mmc/host/mmc_spi.c
+++ b/drivers/mmc/host/mmc_spi.c
@@ -1176,6 +1176,51 @@ static void mmc_spi_initsequence(struct mmc_spi_host *host)
 	}
 }
 
+/* See Section 6.4.2, in SD "Simplified Physical Layer Specification 2.0"
+ *
+ * If powering down, ground all card inputs to avoid power delivery from data
+ * lines!  On a shared SPI bus, this will probably be temporary; 6.4.2 of
+ * the simplified SD spec says this must last at least 1msec.
+ *
+ *   - Clock low means CPOL 0, e.g. mode 0
+ *   - MOSI low comes from writing zero
+ *   - Chipselect is usually active low...
+ */
+static void mmc_spi_shutdownsequence(struct mmc_spi_host *host)
+{
+	int mres;
+	u8 nullbyte = 0;
+
+	host->spi->mode &= ~(SPI_CPOL|SPI_CPHA);
+	mres = spi_setup(host->spi);
+	if (mres < 0)
+		dev_dbg(&host->spi->dev,
+			"switch to SPI mode 0 failed\n");
+
+	if (spi_write(host->spi, &nullbyte, 1) < 0)
+		dev_dbg(&host->spi->dev,
+			"put spi signals to low failed\n");
+
+	/*
+	 * Now clock should be low due to spi mode 0;
+	 * MOSI should be low because of written 0x00;
+	 * chipselect should be low (it is active low)
+	 * power supply is off, so now MMC is off too!
+	 *
+	 * FIXME no, chipselect can be high since the
+	 * device is inactive and SPI_CS_HIGH is clear...
+	 */
+	msleep(10);
+	if (mres == 0) {
+		host->spi->mode |= (SPI_CPOL|SPI_CPHA);
+		mres = spi_setup(host->spi);
+		if (mres < 0)
+			dev_dbg(&host->spi->dev,
+				"switch back to SPI mode 3"
+				" failed\n");
+	}
+}
+
 static char *mmc_powerstring(u8 power_mode)
 {
 	switch (power_mode) {
@@ -1215,49 +1260,10 @@ static void mmc_spi_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 		if (ios->power_mode == MMC_POWER_ON)
 			mmc_spi_initsequence(host);
 
-		/* If powering down, ground all card inputs to avoid power
-		 * delivery from data lines!  On a shared SPI bus, this
-		 * will probably be temporary; 6.4.2 of the simplified SD
-		 * spec says this must last at least 1msec.
-		 *
-		 *   - Clock low means CPOL 0, e.g. mode 0
-		 *   - MOSI low comes from writing zero
-		 *   - Chipselect is usually active low...
-		 */
+		/* See 6.4.2 in the simplified SD card physical spec 2.0 */
 		if (mmc_spi_canpower(host) &&
-		    ios->power_mode == MMC_POWER_OFF) {
-			int mres;
-			u8 nullbyte = 0;
-
-			host->spi->mode &= ~(SPI_CPOL|SPI_CPHA);
-			mres = spi_setup(host->spi);
-			if (mres < 0)
-				dev_dbg(&host->spi->dev,
-					"switch to SPI mode 0 failed\n");
-
-			if (spi_write(host->spi, &nullbyte, 1) < 0)
-				dev_dbg(&host->spi->dev,
-					"put spi signals to low failed\n");
-
-			/*
-			 * Now clock should be low due to spi mode 0;
-			 * MOSI should be low because of written 0x00;
-			 * chipselect should be low (it is active low)
-			 * power supply is off, so now MMC is off too!
-			 *
-			 * FIXME no, chipselect can be high since the
-			 * device is inactive and SPI_CS_HIGH is clear...
-			 */
-			msleep(10);
-			if (mres == 0) {
-				host->spi->mode |= (SPI_CPOL|SPI_CPHA);
-				mres = spi_setup(host->spi);
-				if (mres < 0)
-					dev_dbg(&host->spi->dev,
-						"switch back to SPI mode 3"
-						" failed\n");
-			}
-		}
+		    ios->power_mode == MMC_POWER_OFF)
+			mmc_spi_shutdownsequence(host);
 
 		host->power_mode = ios->power_mode;
 	}
-- 
1.7.4.1

[PATCH 3/4] mmc_spi.c: factor out a mmc_spi_setpower() function

[Antonio Ospite]

Factor out a mmc_spi_setpower() function so to make changing it more
elegant without adding too much stuff to mmc_spi_set_ios().

Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>
---
 drivers/mmc/host/mmc_spi.c |   43 +++++++++++++++++++++++++++++++------------
 1 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
index fe0fdc4..5e4b2c7 100644
--- a/drivers/mmc/host/mmc_spi.c
+++ b/drivers/mmc/host/mmc_spi.c
@@ -1221,6 +1221,26 @@ static void mmc_spi_shutdownsequence(struct mmc_spi_host *host)
 	}
 }
 
+static inline int mmc_spi_setpower(struct mmc_spi_host *host,
+					unsigned char power_mode,
+					unsigned int vdd)
+{
+	/* 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:
+			host->pdata->setpower(&host->spi->dev, vdd);
+			if (power_mode == MMC_POWER_UP)
+				msleep(host->powerup_msecs);
+		}
+	}
+
+	return 0;
+}
+
 static char *mmc_powerstring(u8 power_mode)
 {
 	switch (power_mode) {
@@ -1236,24 +1256,23 @@ static void mmc_spi_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	struct mmc_spi_host *host = mmc_priv(mmc);
 
 	if (host->power_mode != ios->power_mode) {
+		int ret;
 
 		dev_dbg(&host->spi->dev, "mmc_spi: power %s (%d)%s\n",
 				mmc_powerstring(ios->power_mode),
 				ios->vdd,
 				mmc_spi_canpower(host) ? ", can switch" : "");
 
-		/* switch power on/off if possible, accounting for
-		 * max 250msec powerup time if needed.
-		 */
-		if (mmc_spi_canpower(host)) {
-			switch (ios->power_mode) {
-			case MMC_POWER_OFF:
-			case MMC_POWER_UP:
-				host->pdata->setpower(&host->spi->dev,
-						ios->vdd);
-				if (ios->power_mode == MMC_POWER_UP)
-					msleep(host->powerup_msecs);
-			}
+		ret = mmc_spi_setpower(host, ios->power_mode, ios->vdd);
+		if (ret) {
+			dev_err(mmc_dev(mmc), "unable to set power\n");
+			/*
+			 * The .set_ios() function in the mmc_host_ops
+			 * struct return void, and failing to set the
+			 * power should be rare so we print an error and
+			 * return here.
+			 */
+			return;
 		}
 
 		/* See 6.4.1 in the simplified SD card physical spec 2.0 */
-- 
1.7.4.1

[PATCH 4/4] mmc_spi.c: add support for the regulator framework

[Antonio Ospite]

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>
---
 drivers/mmc/host/mmc_spi.c |   61 +++++++++++++++++++++++++++++++++++--------
 1 files changed, 49 insertions(+), 12 deletions(-)

diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
index 5e4b2c7..d5fe841 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,28 @@ 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;
+#ifdef CONFIG_REGULATOR
+	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");
 	}
+#endif
+	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)
-- 
1.7.4.1

Re: [PATCH 0/4] mmc_spi: Add support for regulator framework

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 1449 bytes --]

On Mon, 21 Mar 2011 19:46:38 +0100
Antonio Ospite <ospite@studenti.unina.it> wrote:

> Hi,
> 
> this patchset has the purpose of adding support for the regulator framework to 
> the mmc_spi driver. The first three patches are preparatory cleanups to make 
> the fourth one more straightforward.
> 
> Maybe the fourth patch can be improved, I am open to any suggestions about it.
>

Ping. I forgot to Cc spi-devel-general on this series, should I resend
it?

> These changes take strong inspiration from the pxamci driver; they have been 
> tested on a Motorola A910, which uses a regulator to powerup the MMC card 
> connected to the SPI bus, a test from a current user of the mmc_spi driver 
> would not hurt just to be sure no regressions have been introduced.
> 
> Thanks,
>    Antonio
> 
> Antonio Ospite (4):
>   mmc_spi.c: factor out the check for power capability
>   mmc_spi.c: factor out the SD card shutdown sequence
>   mmc_spi.c: factor out a mmc_spi_setpower() function
>   mmc_spi.c: add support for the regulator framework
> 
>  drivers/mmc/host/mmc_spi.c |  194 +++++++++++++++++++++++++++++---------------
>  1 files changed, 129 insertions(+), 65 deletions(-)
> 

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 0/4] mmc_spi: Add support for regulator framework

[Grant Likely]

On Mon, Apr 04, 2011 at 11:56:31AM +0200, Antonio Ospite wrote:
> On Mon, 21 Mar 2011 19:46:38 +0100
> Antonio Ospite <ospite@studenti.unina.it> wrote:
> 
> > Hi,
> > 
> > this patchset has the purpose of adding support for the regulator framework to 
> > the mmc_spi driver. The first three patches are preparatory cleanups to make 
> > the fourth one more straightforward.
> > 
> > Maybe the fourth patch can be improved, I am open to any suggestions about it.
> >
> 
> Ping. I forgot to Cc spi-devel-general on this series, should I resend
> it?

Not a bad idea.  It doesn't go via my tree since it is an mmc patch,
not an spi one, but I don't mind taking a look at the spi bits.

g.

> 
> > These changes take strong inspiration from the pxamci driver; they have been 
> > tested on a Motorola A910, which uses a regulator to powerup the MMC card 
> > connected to the SPI bus, a test from a current user of the mmc_spi driver 
> > would not hurt just to be sure no regressions have been introduced.
> > 
> > Thanks,
> >    Antonio
> > 
> > Antonio Ospite (4):
> >   mmc_spi.c: factor out the check for power capability
> >   mmc_spi.c: factor out the SD card shutdown sequence
> >   mmc_spi.c: factor out a mmc_spi_setpower() function
> >   mmc_spi.c: add support for the regulator framework
> > 
> >  drivers/mmc/host/mmc_spi.c |  194 +++++++++++++++++++++++++++++---------------
> >  1 files changed, 129 insertions(+), 65 deletions(-)
> > 
> 
> -- 
> Antonio Ospite
> http://ao2.it
> 
> PGP public key ID: 0x4553B001
> 
> A: Because it messes up the order in which people normally read text.
>    See http://en.wikipedia.org/wiki/Posting_style
> Q: Why is top-posting such a bad thing?

Re: [PATCH 0/4] mmc_spi: Add support for regulator framework

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 1246 bytes --]

On Mon, 4 Apr 2011 21:05:43 -0600
Grant Likely <grant.likely@secretlab.ca> wrote:

> On Mon, Apr 04, 2011 at 11:56:31AM +0200, Antonio Ospite wrote:
> > On Mon, 21 Mar 2011 19:46:38 +0100
> > Antonio Ospite <ospite@studenti.unina.it> wrote:
> > 
> > > Hi,
> > > 
> > > this patchset has the purpose of adding support for the regulator framework to 
> > > the mmc_spi driver. The first three patches are preparatory cleanups to make 
> > > the fourth one more straightforward.
> > > 
> > > Maybe the fourth patch can be improved, I am open to any suggestions about it.
> > >
> > 
> > Ping. I forgot to Cc spi-devel-general on this series, should I resend
> > it?
> 
> Not a bad idea.  It doesn't go via my tree since it is an mmc patch,
> not an spi one, but I don't mind taking a look at the spi bits.
> 

Grant you were on Cc from the start so you should have the patches
somewhere; please tell me if you don't.
I'd avoid resending if not strictly necessary.

Regards,
   Antonio

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 0/4] mmc_spi: Add support for regulator framework

[Grant Likely]

On Tue, Apr 05, 2011 at 10:43:47AM +0200, Antonio Ospite wrote:
> On Mon, 4 Apr 2011 21:05:43 -0600
> Grant Likely <grant.likely@secretlab.ca> wrote:
> 
> > On Mon, Apr 04, 2011 at 11:56:31AM +0200, Antonio Ospite wrote:
> > > On Mon, 21 Mar 2011 19:46:38 +0100
> > > Antonio Ospite <ospite@studenti.unina.it> wrote:
> > > 
> > > > Hi,
> > > > 
> > > > this patchset has the purpose of adding support for the regulator framework to 
> > > > the mmc_spi driver. The first three patches are preparatory cleanups to make 
> > > > the fourth one more straightforward.
> > > > 
> > > > Maybe the fourth patch can be improved, I am open to any suggestions about it.
> > > >
> > > 
> > > Ping. I forgot to Cc spi-devel-general on this series, should I resend
> > > it?
> > 
> > Not a bad idea.  It doesn't go via my tree since it is an mmc patch,
> > not an spi one, but I don't mind taking a look at the spi bits.
> > 
> 
> Grant you were on Cc from the start so you should have the patches
> somewhere; please tell me if you don't.
> I'd avoid resending if not strictly necessary.

Ah, then I probably scanned it briefly and decided I didn't need to
respond to it.  Don't worry about reposting.

g.

[RESEND PATCH 0/4] mmc_spi: Add support for regulator framework

[Antonio Ospite]

Hi,

resending the patchset as I missed some recipients on the first round.

This patchset has the purpose of adding support for the regulator framework to 
the mmc_spi driver. The first three patches are preparatory cleanups to make 
the forth one more straightforward.

Maybe the fourth patch can be improved, I am open to any suggestions about it.

These changes take strong inspiration from the pxamci driver; they have been 
tested on a Motorola A910, which uses a regulator to powerup the MMC card 
connected to the SPI bus, a test from a current user of the mmc_spi driver 
would not hurt just to be sure no regressions have been introduced.

Thanks,
   Antonio

Antonio Ospite (4):
  mmc_spi.c: factor out the check for power capability
  mmc_spi.c: factor out the SD card shutdown sequence
  mmc_spi.c: factor out a mmc_spi_setpower() function
  mmc_spi.c: add support for the regulator framework

 drivers/mmc/host/mmc_spi.c |  194 +++++++++++++++++++++++++++++---------------
 1 files changed, 129 insertions(+), 65 deletions(-)

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[RESEND PATCH 1/4] mmc_spi.c: factor out the check for power capability

[Antonio Ospite]

Factor out the 'canpower' condition into a dedicated function in order
to avoid repetition and to make changing the condition easier.

Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>
---
 drivers/mmc/host/mmc_spi.c |   18 ++++++++++--------
 1 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
index 7c1e16a..49f5725 100644
--- a/drivers/mmc/host/mmc_spi.c
+++ b/drivers/mmc/host/mmc_spi.c
@@ -152,6 +152,10 @@ struct mmc_spi_host {
 	dma_addr_t		ones_dma;
 };
 
+static inline int mmc_spi_canpower(struct mmc_spi_host *host)
+{
+	return host->pdata && host->pdata->setpower;
+}
 
 /****************************************************************************/
 
@@ -1187,19 +1191,16 @@ static void mmc_spi_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	struct mmc_spi_host *host = mmc_priv(mmc);
 
 	if (host->power_mode != ios->power_mode) {
-		int		canpower;
-
-		canpower = host->pdata && host->pdata->setpower;
 
 		dev_dbg(&host->spi->dev, "mmc_spi: power %s (%d)%s\n",
 				mmc_powerstring(ios->power_mode),
 				ios->vdd,
-				canpower ? ", can switch" : "");
+				mmc_spi_canpower(host) ? ", can switch" : "");
 
 		/* switch power on/off if possible, accounting for
 		 * max 250msec powerup time if needed.
 		 */
-		if (canpower) {
+		if (mmc_spi_canpower(host)) {
 			switch (ios->power_mode) {
 			case MMC_POWER_OFF:
 			case MMC_POWER_UP:
@@ -1223,7 +1224,8 @@ static void mmc_spi_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 		 *   - MOSI low comes from writing zero
 		 *   - Chipselect is usually active low...
 		 */
-		if (canpower && ios->power_mode == MMC_POWER_OFF) {
+		if (mmc_spi_canpower(host) &&
+		    ios->power_mode == MMC_POWER_OFF) {
 			int mres;
 			u8 nullbyte = 0;
 
@@ -1399,7 +1401,7 @@ static int mmc_spi_probe(struct spi_device *spi)
 		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 (host->pdata && host->pdata->setpower) {
+	if (mmc_spi_canpower(host)) {
 		host->powerup_msecs = host->pdata->powerup_msecs;
 		if (!host->powerup_msecs || host->powerup_msecs > 250)
 			host->powerup_msecs = 250;
@@ -1459,7 +1461,7 @@ static int mmc_spi_probe(struct spi_device *spi)
 			host->dma_dev ? "" : ", no DMA",
 			(host->pdata && host->pdata->get_ro)
 				? "" : ", no WP",
-			(host->pdata && host->pdata->setpower)
+			mmc_spi_canpower(host)
 				? "" : ", no poweroff",
 			(mmc->caps & MMC_CAP_NEEDS_POLL)
 				? ", cd polling" : "");
-- 
1.7.4.4

[RESEND PATCH 2/4] mmc_spi.c: factor out the SD card shutdown sequence

[Antonio Ospite]

Factor out the SD card shutdown sequence to a dedicated
mmc_spi_shutdownsequence() function in order to make mmc_spi_set_ios()
more readable, and also for symmetry with mmc_spi_initsequence() which
is already a dedicated function.

Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>
---
 drivers/mmc/host/mmc_spi.c |   90 +++++++++++++++++++++++--------------------
 1 files changed, 48 insertions(+), 42 deletions(-)

diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
index 49f5725..9eae23c 100644
--- a/drivers/mmc/host/mmc_spi.c
+++ b/drivers/mmc/host/mmc_spi.c
@@ -1176,6 +1176,51 @@ static void mmc_spi_initsequence(struct mmc_spi_host *host)
 	}
 }
 
+/* See Section 6.4.2, in SD "Simplified Physical Layer Specification 2.0"
+ *
+ * If powering down, ground all card inputs to avoid power delivery from data
+ * lines!  On a shared SPI bus, this will probably be temporary; 6.4.2 of
+ * the simplified SD spec says this must last at least 1msec.
+ *
+ *   - Clock low means CPOL 0, e.g. mode 0
+ *   - MOSI low comes from writing zero
+ *   - Chipselect is usually active low...
+ */
+static void mmc_spi_shutdownsequence(struct mmc_spi_host *host)
+{
+	int mres;
+	u8 nullbyte = 0;
+
+	host->spi->mode &= ~(SPI_CPOL|SPI_CPHA);
+	mres = spi_setup(host->spi);
+	if (mres < 0)
+		dev_dbg(&host->spi->dev,
+			"switch to SPI mode 0 failed\n");
+
+	if (spi_write(host->spi, &nullbyte, 1) < 0)
+		dev_dbg(&host->spi->dev,
+			"put spi signals to low failed\n");
+
+	/*
+	 * Now clock should be low due to spi mode 0;
+	 * MOSI should be low because of written 0x00;
+	 * chipselect should be low (it is active low)
+	 * power supply is off, so now MMC is off too!
+	 *
+	 * FIXME no, chipselect can be high since the
+	 * device is inactive and SPI_CS_HIGH is clear...
+	 */
+	msleep(10);
+	if (mres == 0) {
+		host->spi->mode |= (SPI_CPOL|SPI_CPHA);
+		mres = spi_setup(host->spi);
+		if (mres < 0)
+			dev_dbg(&host->spi->dev,
+				"switch back to SPI mode 3"
+				" failed\n");
+	}
+}
+
 static char *mmc_powerstring(u8 power_mode)
 {
 	switch (power_mode) {
@@ -1215,49 +1260,10 @@ static void mmc_spi_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 		if (ios->power_mode == MMC_POWER_ON)
 			mmc_spi_initsequence(host);
 
-		/* If powering down, ground all card inputs to avoid power
-		 * delivery from data lines!  On a shared SPI bus, this
-		 * will probably be temporary; 6.4.2 of the simplified SD
-		 * spec says this must last at least 1msec.
-		 *
-		 *   - Clock low means CPOL 0, e.g. mode 0
-		 *   - MOSI low comes from writing zero
-		 *   - Chipselect is usually active low...
-		 */
+		/* See 6.4.2 in the simplified SD card physical spec 2.0 */
 		if (mmc_spi_canpower(host) &&
-		    ios->power_mode == MMC_POWER_OFF) {
-			int mres;
-			u8 nullbyte = 0;
-
-			host->spi->mode &= ~(SPI_CPOL|SPI_CPHA);
-			mres = spi_setup(host->spi);
-			if (mres < 0)
-				dev_dbg(&host->spi->dev,
-					"switch to SPI mode 0 failed\n");
-
-			if (spi_write(host->spi, &nullbyte, 1) < 0)
-				dev_dbg(&host->spi->dev,
-					"put spi signals to low failed\n");
-
-			/*
-			 * Now clock should be low due to spi mode 0;
-			 * MOSI should be low because of written 0x00;
-			 * chipselect should be low (it is active low)
-			 * power supply is off, so now MMC is off too!
-			 *
-			 * FIXME no, chipselect can be high since the
-			 * device is inactive and SPI_CS_HIGH is clear...
-			 */
-			msleep(10);
-			if (mres == 0) {
-				host->spi->mode |= (SPI_CPOL|SPI_CPHA);
-				mres = spi_setup(host->spi);
-				if (mres < 0)
-					dev_dbg(&host->spi->dev,
-						"switch back to SPI mode 3"
-						" failed\n");
-			}
-		}
+		    ios->power_mode == MMC_POWER_OFF)
+			mmc_spi_shutdownsequence(host);
 
 		host->power_mode = ios->power_mode;
 	}
-- 
1.7.4.4

[RESEND PATCH 3/4] mmc_spi.c: factor out a mmc_spi_setpower() function

[Antonio Ospite]

Factor out a mmc_spi_setpower() function so to make changing it more
elegant without adding too much stuff to mmc_spi_set_ios().

Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>
---
 drivers/mmc/host/mmc_spi.c |   43 +++++++++++++++++++++++++++++++------------
 1 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
index 9eae23c..0f3672d 100644
--- a/drivers/mmc/host/mmc_spi.c
+++ b/drivers/mmc/host/mmc_spi.c
@@ -1221,6 +1221,26 @@ static void mmc_spi_shutdownsequence(struct mmc_spi_host *host)
 	}
 }
 
+static inline int mmc_spi_setpower(struct mmc_spi_host *host,
+					unsigned char power_mode,
+					unsigned int vdd)
+{
+	/* 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:
+			host->pdata->setpower(&host->spi->dev, vdd);
+			if (power_mode == MMC_POWER_UP)
+				msleep(host->powerup_msecs);
+		}
+	}
+
+	return 0;
+}
+
 static char *mmc_powerstring(u8 power_mode)
 {
 	switch (power_mode) {
@@ -1236,24 +1256,23 @@ static void mmc_spi_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	struct mmc_spi_host *host = mmc_priv(mmc);
 
 	if (host->power_mode != ios->power_mode) {
+		int ret;
 
 		dev_dbg(&host->spi->dev, "mmc_spi: power %s (%d)%s\n",
 				mmc_powerstring(ios->power_mode),
 				ios->vdd,
 				mmc_spi_canpower(host) ? ", can switch" : "");
 
-		/* switch power on/off if possible, accounting for
-		 * max 250msec powerup time if needed.
-		 */
-		if (mmc_spi_canpower(host)) {
-			switch (ios->power_mode) {
-			case MMC_POWER_OFF:
-			case MMC_POWER_UP:
-				host->pdata->setpower(&host->spi->dev,
-						ios->vdd);
-				if (ios->power_mode == MMC_POWER_UP)
-					msleep(host->powerup_msecs);
-			}
+		ret = mmc_spi_setpower(host, ios->power_mode, ios->vdd);
+		if (ret) {
+			dev_err(mmc_dev(mmc), "unable to set power\n");
+			/*
+			 * The .set_ios() function in the mmc_host_ops
+			 * struct return void, and failing to set the
+			 * power should be rare so we print an error and
+			 * return here.
+			 */
+			return;
 		}
 
 		/* See 6.4.1 in the simplified SD card physical spec 2.0 */
-- 
1.7.4.4

[RESEND PATCH 4/4] mmc_spi.c: add support for the regulator framework

[Antonio Ospite]

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>
---
 drivers/mmc/host/mmc_spi.c |   61 +++++++++++++++++++++++++++++++++++--------
 1 files changed, 49 insertions(+), 12 deletions(-)

diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
index 0f3672d..acc2ec8 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,28 @@ 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;
+#ifdef CONFIG_REGULATOR
+	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");
 	}
+#endif
+	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)
-- 
1.7.4.4

Re: [RESEND PATCH 4/4] mmc_spi.c: add support for the regulator framework

[Mark Brown]

On Thu, Apr 21, 2011 at 02:27:53PM +0200, Antonio Ospite wrote:

> +#ifdef CONFIG_REGULATOR
> +	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");
>  	}
> +#endif

Why is this code conditional?  The regulator API will stub itself out
(by returning a null pointer, which plays well with your use of null) if
it's disabled.  I'm also not seeing any corresponding code to release
the regulator.

Re: [RESEND PATCH 4/4] mmc_spi.c: add support for the regulator framework

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 1458 bytes --]

On Thu, 21 Apr 2011 13:40:41 +0100
Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:

> On Thu, Apr 21, 2011 at 02:27:53PM +0200, Antonio Ospite wrote:
> 
> > +#ifdef CONFIG_REGULATOR
> > +	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");
> >  	}
> > +#endif
> 
> Why is this code conditional?  The regulator API will stub itself out
> (by returning a null pointer, which plays well with your use of null) if
> it's disabled.  I'm also not seeing any corresponding code to release
> the regulator.
>

Actually I saw this done conditionally in pxamci.c and thought it was
really needed. About the call to regulator_put() if was there but got
lost when squashing the commits into the final patch and I was still
reading that part in my mind even if it was not there anymore.
The importance of reviewers with fresh eyes.

I'll resubmit this forth patch with the needed changes.

Thanks,
   Antonio

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[PATCH v2 4/4] mmc_spi.c: add support for the regulator framework

[Antonio Ospite]

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(-)

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

Re: [RESEND PATCH 4/4] mmc_spi.c: add support for the regulator framework

[Daniel Ribeiro]

[-- Attachment #1: Type: text/plain, Size: 1711 bytes --]

Em Qui, 2011-04-21 às 14:49 +0200, Antonio Ospite escreveu:
> On Thu, 21 Apr 2011 13:40:41 +0100
> Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
> 
> > On Thu, Apr 21, 2011 at 02:27:53PM +0200, Antonio Ospite wrote:
> > 
> > > +#ifdef CONFIG_REGULATOR
> > > +	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");
> > >  	}
> > > +#endif
> > 
> > Why is this code conditional?  The regulator API will stub itself out
> > (by returning a null pointer, which plays well with your use of null) if
> > it's disabled.  I'm also not seeing any corresponding code to release
> > the regulator.
> >
> 
> Actually I saw this done conditionally in pxamci.c and thought it was
> really needed. About the call to regulator_put() if was there but got
> lost when squashing the commits into the final patch and I was still
> reading that part in my mind even if it was not there anymore.
> The importance of reviewers with fresh eyes.
> 
> I'll resubmit this forth patch with the needed changes.

Hi,

The #ifdefs on pxamci were needed because at the time it was written
IS_ERR(regulator_get()) would return _false_ if CONFIG_REGULATOR is not
defined, breaking compatibility with the older ocr_mask/setpower.

I don't know if the regulator api changed it's old behaviour, see
http://www.mail-archive.com/openezx-devel@lists.openezx.org/msg01954.html


-- 
Daniel Ribeiro

[-- Attachment #2: Esta é uma parte de mensagem assinada digitalmente --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v2 4/4] mmc_spi.c: add support for the regulator framework

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 4371 bytes --]

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
> 
> 


-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[PATCH v3] mmc_spi.c: add support for the regulator framework

[Antonio Ospite]

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 v2:
  - Use IS_ERR_OR_NULL() to correctly handle the stub regulator_get()

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()


Chris if this one turns to be OK you could pick up patches 1-3 from previous 
messages and patch 4 v3 from this one. Let me know if you want me to resend the 
whole series instead.

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..70dd603 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_OR_NULL(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.5.1

Re: [PATCH v3] mmc_spi.c: add support for the regulator framework

[Mark Brown]

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-aNJ+ML1ZbiP93QAQaVx+gl6hYfS7NtTn@public.gmane.org>

Reviwed-by: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>

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.

------------------------------------------------------------------------------
Achieve unprecedented app performance and reliability
What every C/C++ and Fortran developer should know.
Learn how Intel has extended the reach of its next-generation tools
to help boost performance applications - inlcuding clusters.
http://p.sf.net/sfu/intel-dev2devmay

Re: [PATCH v3] mmc_spi.c: add support for the regulator framework

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 1526 bytes --]

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

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v3] mmc_spi.c: add support for the regulator framework

[Mark Brown]

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.

Re: [PATCH v3] mmc_spi.c: add support for the regulator framework

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 2224 bytes --]

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

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v3] mmc_spi.c: add support for the regulator framework

[Mark Brown]

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.

Re: [PATCH v3] mmc_spi.c: add support for the regulator framework

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 3975 bytes --]

On Wed, 18 May 2011 12:42:12 -0700
Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:

> On Wed, May 18, 2011 at 07:23:20PM +0200, Antonio Ospite wrote:
> 
[...]
> >   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.

A WIP patch, I don't have a lot of time to spend on this, I tried to
handle the driver specific 'setpower' callback with a macro, let me
know if you have a better idea:

diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index bcb793e..dbbe535 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -298,6 +298,32 @@ static inline int mmc_regulator_set_ocr(struct mmc_host *mmc,
 }
 #endif
 
+#define STRUCT_FIELD(s, f) ((s) && (s)->f ? (s)->f : NULL )
+
+static inline int mmc_regulator_set_power(struct mmc_host *mmc,
+				 unsigned char	power_mode,
+				 struct regulator *supply,
+				 unsigned short vdd_bit,
+				 struct device *device,
+				 void (*setpower_cb)(struct device *, unsigned int))
+{
+	if (supply) {
+		int ret;
+
+		if (power_mode == MMC_POWER_OFF)
+			vdd_bit = 0;
+
+		ret = mmc_regulator_set_ocr(mmc, supply, vdd_bit);
+		if (ret)
+			return ret;
+	} else {
+		if (setpower_cb)
+			setpower_cb(device, vdd_bit);
+	}
+
+	return 0;
+}
+
 int mmc_card_awake(struct mmc_host *host);
 int mmc_card_sleep(struct mmc_host *host);
 int mmc_card_can_sleep(struct mmc_host *host);
diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
index 2e8db20..c9a229f 100644
--- a/drivers/mmc/host/mmc_spi.c
+++ b/drivers/mmc/host/mmc_spi.c
@@ -1231,21 +1231,18 @@ static inline int mmc_spi_setpower(struct mmc_spi_host *host,
 					unsigned char power_mode,
 					unsigned int vdd)
 {
+	int ret;
+
 	if (!mmc_spi_canpower(host))
 		return -ENOSYS;
 
-	if (host->vcc) {
-		int ret;
 
-		if (power_mode == MMC_POWER_OFF)
-			vdd = 0;
+	ret = mmc_regulator_set_power(host->mmc, power_mode, host->vcc, vdd,
+				      &host->spi->dev,
+				      STRUCT_FIELD(host->pdata, setpower));
+	if (ret)
+		return ret;
 
-		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);
diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
index fbdb21e..f5d9529 100644
--- a/drivers/mmc/host/pxamci.c
+++ b/drivers/mmc/host/pxamci.c
@@ -103,29 +103,20 @@ static inline int pxamci_set_power(struct pxamci_host *host,
 				    unsigned int vdd)
 {
 	int on;
+	int ret;
 
-	if (host->vcc) {
-		int ret;
+	ret = mmc_regulator_set_power(host->mmc, power_mode, host->vcc, vdd,
+				      mmc_dev(host->mmc),
+				      STRUCT_FIELD(host->pdata, setpower));
+	if (ret)
+		return ret;
 
-		if (power_mode == MMC_POWER_UP) {
-			ret = mmc_regulator_set_ocr(host->mmc, host->vcc, vdd);
-			if (ret)
-				return ret;
-		} else if (power_mode == MMC_POWER_OFF) {
-			ret = mmc_regulator_set_ocr(host->mmc, host->vcc, 0);
-			if (ret)
-				return ret;
-		}
-	}
 	if (!host->vcc && host->pdata &&
 	    gpio_is_valid(host->pdata->gpio_power)) {
 		on = ((1 << vdd) & host->pdata->ocr_mask);
 		gpio_set_value(host->pdata->gpio_power,
 			       !!on ^ host->pdata->gpio_power_invert);
 	}
-	if (!host->vcc && host->pdata && host->pdata->setpower)
-		host->pdata->setpower(mmc_dev(host->mmc), vdd);
-
 	return 0;
 }
 


-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v3] mmc_spi.c: add support for the regulator framework

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 2858 bytes --]

On Mon, 23 May 2011 17:10:23 +0200
Antonio Ospite <ospite@studenti.unina.it> wrote:

> On Wed, 18 May 2011 12:42:12 -0700
> Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
> 
> > On Wed, May 18, 2011 at 07:23:20PM +0200, Antonio Ospite wrote:
> > 
> [...]
> > >   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.
> 
> A WIP patch, I don't have a lot of time to spend on this, I tried to
> handle the driver specific 'setpower' callback with a macro, let me
> know if you have a better idea:
> 
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index bcb793e..dbbe535 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -298,6 +298,32 @@ static inline int mmc_regulator_set_ocr(struct mmc_host *mmc,
>  }
>  #endif
>  
> +#define STRUCT_FIELD(s, f) ((s) && (s)->f ? (s)->f : NULL )
> +

Any opinion on this macro? See its use below. It is meant to deal with
driver specific struct fields, which can have arbitrary names, I though
that using some syntactic sugar to deal with those as arguments when
calling the function was not that horrible.

If that looks acceptable to you too I will submit the
mmc_regulator_set_power () patch, otherwise I would ask to consider the
simple patch to mmc_spi.c for now.

[...]
> diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
> index 2e8db20..c9a229f 100644
> --- a/drivers/mmc/host/mmc_spi.c
> +++ b/drivers/mmc/host/mmc_spi.c
> @@ -1231,21 +1231,18 @@ static inline int mmc_spi_setpower(struct mmc_spi_host *host,
>  					unsigned char power_mode,
>  					unsigned int vdd)
>  {
> +	int ret;
> +
>  	if (!mmc_spi_canpower(host))
>  		return -ENOSYS;
>  
> -	if (host->vcc) {
> -		int ret;
>  
> -		if (power_mode == MMC_POWER_OFF)
> -			vdd = 0;
> +	ret = mmc_regulator_set_power(host->mmc, power_mode, host->vcc, vdd,
> +				      &host->spi->dev,
> +				      STRUCT_FIELD(host->pdata, setpower));
                                      ^^^^^^^^^^^^^
> +	if (ret)
> +		return ret;
>  
> -		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

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v3] mmc_spi.c: add support for the regulator framework

[Mark Brown]

On Mon, May 30, 2011 at 11:07:49AM +0200, Antonio Ospite wrote:
> On Mon, 23 May 2011 17:10:23 +0200

> > +#define STRUCT_FIELD(s, f) ((s) && (s)->f ? (s)->f : NULL )

> Any opinion on this macro? See its use below. It is meant to deal with
> driver specific struct fields, which can have arbitrary names, I though
> that using some syntactic sugar to deal with those as arguments when
> calling the function was not that horrible.

> If that looks acceptable to you too I will submit the
> mmc_regulator_set_power () patch, otherwise I would ask to consider the
> simple patch to mmc_spi.c for now.

Would it not be simpler just to provide a standard generic struct that
people can embed into their pdata?

Re: [PATCH v3] mmc_spi.c: add support for the regulator framework

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 2281 bytes --]

On Mon, 30 May 2011 18:14:48 +0800
Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:

> On Mon, May 30, 2011 at 11:07:49AM +0200, Antonio Ospite wrote:
> > On Mon, 23 May 2011 17:10:23 +0200
> 
> > > +#define STRUCT_FIELD(s, f) ((s) && (s)->f ? (s)->f : NULL )
> 
> > Any opinion on this macro? See its use below. It is meant to deal with
> > driver specific struct fields, which can have arbitrary names, I though
> > that using some syntactic sugar to deal with those as arguments when
> > calling the function was not that horrible.
> 
> > If that looks acceptable to you too I will submit the
> > mmc_regulator_set_power () patch, otherwise I would ask to consider the
> > simple patch to mmc_spi.c for now.
> 
> Would it not be simpler just to provide a standard generic struct that
> people can embed into their pdata?
> 

I thought to something like:

struct mmc_driver_ops {
	int (*setpower)(struct device *, unsigned int);
};

struct pxamci_platform_data {
	[...]
	struct mmc_driver_ops *ops;
};

static inline int mmc_regulator_set_power(struct mmc_host *mmc,
				 unsigned char	power_mode,
				 struct regulator *supply,
				 unsigned short vdd_bit,
				 struct device *device,
				 struct mmc_driver_ops ops)
{
	[...]
		if (ops->setpower)
			ops->setpower(device, vdd_bit);
	[...]
}

static inline int pxamci_set_power(struct pxamci_host *host,
				    unsigned char power_mode,
				    unsigned int vdd)
{
	[...]
	ret = mmc_regulator_set_power(host->mmc, power_mode, host->vcc,
				      vdd, mmc_dev(host->mmc),
				      host->pdata->ops);
	[...]
}

It is cleaner and more uniform indeed, but it does not solve the problem
I am seeing, the issue is still there when pdata is NULL, we are at the
same point as before (the current code checks for (pdata &&
pdata->field)), and we cannot pass pdata directly as a function argument
(can we?) as it is driver specific as well.

I would be glad to discover than I am still missing something :)

Thanks,
   Antonio

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v3] mmc_spi.c: add support for the regulator framework

[Mark Brown]

On Mon, May 30, 2011 at 12:57:57PM +0200, Antonio Ospite wrote:

> It is cleaner and more uniform indeed, but it does not solve the problem
> I am seeing, the issue is still there when pdata is NULL, we are at the
> same point as before (the current code checks for (pdata &&
> pdata->field)), and we cannot pass pdata directly as a function argument
> (can we?) as it is driver specific as well.

You can do a nasty type punning trick if the generic pdata is the first
member of the platform data, otherwise the easiest thing is usually to
provide a defualt pdata if the pdata is NULL.

I'd expect that in a lot of cases the standard platform data would be
the only platform data so for many drivers they wouldn't need to worry
about extra data but perhaps MMC isn't like that, I've never really
looked at the code.