[PATCH] mmc: pxamci: Refactor regulator support

[PATCH] mmc: pxamci: Refactor regulator support

[Andrea Adami]

From: Marko Katic <dromede@gmail.com>

Here's an interesting scenario. The spitz machine has an
Intersil 6271A voltage regulator and an ADS7846 touchscreen
controller.

The ADS7846 driver _requires_ the use of a voltage regulator
or if not present, CONFIG_REGULATOR_DUMMY should be used for proper operation.
This was made mandatory by the following commit:

========================================
91143379b01b2020d8878d627ebe9dfb9d6eb4c8
Input: ads7846 - add regulator support

The ADS7846/TSC2046 touchscreen controllers can (and usually are)
connected to various regulators for power, so add regulator support.

Valid regulator will now be required, so boards without complete
regulator setup should either disable regulator framework or enable
CONFIG_REGULATOR_DUMMY.
========================================

The ADS7846 in spitz machines is not connected to
any power regulator so it needs CONFIG_REGULATOR_DUMMY enabled.
So to support both the Intersil 6271A regulator and
the ADS7846 controller, CONFIG_REGULATOR and CONFIG_REGULATOR_DUMMY have
to be defined.

However, enabling CONFIG_REGULATOR and CONFIG_REGULATOR_DUMMY
will break pxamci driver and cause the following error output:

pxa2xx-mci.0 supply vmmc not found, using dummy regulator
pxa2xx-mci pxa2xx-mci.0: ocr_mask/setpower will not be used
pxa2xx-mci pxa2xx-mci.0: could not set regulator OCR (-22)
pxa2xx-mci pxa2xx-mci.0: unable to set power
pxa2xx-mci pxa2xx-mci.0: could not set regulator OCR (-22)
pxa2xx-mci pxa2xx-mci.0: unable to set power

Above failures occur in two functions;
pxamci_init_ocr() and pxamci_set_power().

Regulator support in pxamci_init_ocr() is not
written with the existence of the dummy regulator driver in
mind. It does not check the return value of mmc_regulator_get_ocrmask()
and it will only fall back to platform data if no regulator was found.

pxamci_set_power() fails because it does not even try to fall back
to platform data if mmc_regulator_set_ocr() fails. It
expects a proper regulator or no regulator at all. It will
only fall back to platform data if no regulator was found. It does
not properly handle the possible existence of the dummy regulator.

This patch refactors pxamci_init_ocr() and  pxamci_set_power() regulator
support to be more modular, to do more checks and to always fall back
to platform data.

Signed-off-by: Marko Katic <dromede@gmail.com>
[andrea.adami@gmail.com: checkpatch.pl dislikes lines over 80 chars]
Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
---
 drivers/mmc/host/pxamci.c | 40 +++++++++++++++++++++++++++-------------
 1 file changed, 27 insertions(+), 13 deletions(-)

diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
index 2b2f65a..341d5dc 100644
--- a/drivers/mmc/host/pxamci.c
+++ b/drivers/mmc/host/pxamci.c
@@ -83,18 +83,26 @@ struct pxamci_host {
 static inline void pxamci_init_ocr(struct pxamci_host *host)
 {
 #ifdef CONFIG_REGULATOR
+	int ocr_mask;
 	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)
+		ocr_mask = mmc_regulator_get_ocrmask(host->vcc);
+		if (ocr_mask <= 0)
+			host->mmc->ocr_avail = 0;
+		else
+			host->mmc->ocr_avail = ocr_mask;
+
+		if (host->pdata &&
+			host->pdata->ocr_mask &&
+			host->mmc->ocr_avail)
 			dev_warn(mmc_dev(host->mmc),
 				"ocr_mask/setpower will not be used\n");
 	}
 #endif
-	if (host->vcc == NULL) {
+	if (host->vcc == NULL || host->mmc->ocr_avail == 0) {
 		/* fall-back to platform data */
 		host->mmc->ocr_avail = host->pdata ?
 			host->pdata->ocr_mask :
@@ -108,26 +116,32 @@ static inline int pxamci_set_power(struct pxamci_host *host,
 {
 	int on;
 
+#ifdef CONFIG_REGULATOR
 	if (host->vcc) {
-		int ret;
+		int ret = 0;
 
-		if (power_mode == MMC_POWER_UP) {
+		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) {
+
+		if (power_mode == MMC_POWER_OFF)
 			ret = mmc_regulator_set_ocr(host->mmc, host->vcc, 0);
-			if (ret)
-				return ret;
-		}
+
+		if (ret == 0)
+			return 0;
+		else
+			dev_warn(mmc_dev(host->mmc),
+			 "mmc_regulator_set_ocr failed, "
+			 "falling back to platform data\n");
 	}
-	if (!host->vcc && host->pdata &&
+#endif
+
+	if (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)
+	if (host->pdata && host->pdata->setpower)
 		host->pdata->setpower(mmc_dev(host->mmc), vdd);
 
 	return 0;
-- 
1.8.1.5

Re: [PATCH] mmc: pxamci: Refactor regulator support

[Mark Brown]

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

On Sun, Jul 21, 2013 at 01:07:48AM +0200, Andrea Adami wrote:

> The ADS7846 driver _requires_ the use of a voltage regulator
> or if not present, CONFIG_REGULATOR_DUMMY should be used for proper operation.
> This was made mandatory by the following commit:

No, CONFIG_REGULATOR_DUMMY should *never* be used in production.

> The ADS7846 in spitz machines is not connected to
> any power regulator so it needs CONFIG_REGULATOR_DUMMY enabled.

I don't think that's the case, it would be a very unusual piece of
silicon that was able to operate without power.  It may be that it's not
using a software controllable regulator but there will be one or more
power supplies.  Judging by your problem here it seems like there is
actually a software controllable regulator though...

> However, enabling CONFIG_REGULATOR and CONFIG_REGULATOR_DUMMY
> will break pxamci driver and cause the following error output:

This is why you should never use CONFIG_REGULATOR_DUMMY in production,
it's just a crutch to help things boot during development.

> Regulator support in pxamci_init_ocr() is not
> written with the existence of the dummy regulator driver in
> mind. It does not check the return value of mmc_regulator_get_ocrmask()
> and it will only fall back to platform data if no regulator was found.

No regulator driver should be written with CONFIG_REGULATOR_DUMMY in
mind.  My guess here is that your custom callbacks for this board should
be being replaced by a regulator, for example the fixed voltage
regulator if there's only one voltage supported.

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

Re: [PATCH] mmc: pxamci: Refactor regulator support

[Andrea Adami]

On Mon, Jul 22, 2013 at 1:03 PM, Mark Brown <broonie@kernel.org> wrote:
> On Sun, Jul 21, 2013 at 01:07:48AM +0200, Andrea Adami wrote:
>
>> The ADS7846 driver _requires_ the use of a voltage regulator
>> or if not present, CONFIG_REGULATOR_DUMMY should be used for proper operation.
>> This was made mandatory by the following commit:
>
> No, CONFIG_REGULATOR_DUMMY should *never* be used in production.
>
>> The ADS7846 in spitz machines is not connected to
>> any power regulator so it needs CONFIG_REGULATOR_DUMMY enabled.
>
> I don't think that's the case, it would be a very unusual piece of
> silicon that was able to operate without power.  It may be that it's not
> using a software controllable regulator but there will be one or more
> power supplies.  Judging by your problem here it seems like there is
> actually a software controllable regulator though...
>
>> However, enabling CONFIG_REGULATOR and CONFIG_REGULATOR_DUMMY
>> will break pxamci driver and cause the following error output:
>
> This is why you should never use CONFIG_REGULATOR_DUMMY in production,
> it's just a crutch to help things boot during development.
>
>> Regulator support in pxamci_init_ocr() is not
>> written with the existence of the dummy regulator driver in
>> mind. It does not check the return value of mmc_regulator_get_ocrmask()
>> and it will only fall back to platform data if no regulator was found.
>
> No regulator driver should be written with CONFIG_REGULATOR_DUMMY in
> mind.  My guess here is that your custom callbacks for this board should
> be being replaced by a regulator, for example the fixed voltage
> regulator if there's only one voltage supported.


Thanks Mark,

patch is withdrawn.

In fact adding regulators has been self-inflicted pain...
Poodle and spitz boot normally with # CONFIG_REGULATOR is not set.

Sorry for the noise

Andrea Adami