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

2011-05-23 Thread Antonio Ospite 
On Wed, 18 May 2011 12:42:12 -0700
Mark Brown broo...@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?


pgpYUFBkRQs9W.pgp
Description: PGP signature

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

2011-05-18 Thread 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

2011-05-11 Thread 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 osp...@studenti.unina.it

Reviwed-by: Mark Brown broo...@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.