[PATCH 0/3] Fix fixed regulators support

[PATCH 0/3] Fix fixed regulators support

[Marek Szyprowski]

Hello,

I've noticed that the voltage check in regulator_is_supported_voltage()
function was inverted, what resulted in strange side effects. One of
such side effects appeared in sdhci driver, which has some build-in
support for special case of fixed regulators. 

Now, after fixing regulator_is_supported_voltage(), the sdhci driver
stopped working with fixed regulators. The provided patch series fixes
regulator_is_supported_voltage() function and updates sdhci driver to
correctly operate with fixed voltage regulators. The second patch
unifies handling of fixed regulators and regulators with disabled
voltage change due to their constraints. This restores support for such
regulators in sdhci driver (such case is present on Samsung GONI board).

If possible, I would recomment to push those patches to v3.7-rc series.

Best regards
Marek Szyprowski
Samsung Poland R&D Center


Patch summary:

Marek Szyprowski (3):
  regulator: fix voltage check in regulator_is_supported_voltage()
  regulator: threat regulators with constant volatage as fixed
  mmc: sdhci: apply voltage range check only for non-fixed regulators

 drivers/mmc/host/sdhci.c |    2 +-
 drivers/regulator/core.c |    7 +++++--
 2 files changed, 6 insertions(+), 3 deletions(-)

-- 
1.7.9.5

[PATCH 1/3] regulator: fix voltage check in regulator_is_supported_voltage()

[Marek Szyprowski]

regulator_is_supported_voltage() should return true only if the voltage
of fixed/constant regulator is between min_uV and max_uV.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/regulator/core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 1a35251..042c1ff 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1974,7 +1974,7 @@ int regulator_is_supported_voltage(struct regulator *regulator,
 	if (!(rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE)) {
 		ret = regulator_get_voltage(regulator);
 		if (ret >= 0)
-			return (min_uV >= ret && ret <= max_uV);
+			return (min_uV <= ret && ret <= max_uV);
 		else
 			return ret;
 	}
-- 
1.7.9.5

[PATCH 2/3] regulator: threat regulators with constant volatage as fixed

[Marek Szyprowski]

Some drivers has additional logic for fixed regulators. Let regulator core
to threat regulators which cannot change their voltage due to applied
constraints as fixed.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/regulator/core.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 042c1ff..271182e 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1872,7 +1872,10 @@ int regulator_count_voltages(struct regulator *regulator)
 {
 	struct regulator_dev	*rdev = regulator->rdev;
 
-	return rdev->desc->n_voltages ? : -EINVAL;
+	if (rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE)
+		return rdev->desc->n_voltages ? : -EINVAL;
+	else
+		return 1;
 }
 EXPORT_SYMBOL_GPL(regulator_count_voltages);
 
-- 
1.7.9.5

[PATCH 3/3] mmc: sdhci: apply voltage range check only for non-fixed regulators

[Marek Szyprowski]

Fixed regulators cannot change their voltage, so disable all voltage range
checking for them, otherwise the driver fails to operate with fixed
regulators.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/mmc/host/sdhci.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index c7851c0..6f6534e 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2923,7 +2923,7 @@ int sdhci_add_host(struct sdhci_host *host)
 		regulator_enable(host->vmmc);
 
 #ifdef CONFIG_REGULATOR
-	if (host->vmmc) {
+	if (host->vmmc && regulator_count_voltages(host->vmmc) > 1) {
 		ret = regulator_is_supported_voltage(host->vmmc, 3300000,
 			3300000);
 		if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))
-- 
1.7.9.5

Re: [PATCH 2/3] regulator: threat regulators with constant volatage as fixed

[Mark Brown]

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

On Tue, Nov 13, 2012 at 09:48:52AM +0100, Marek Szyprowski wrote:

> Some drivers has additional logic for fixed regulators. Let regulator core
> to threat regulators which cannot change their voltage due to applied

YM "treat".

> +	if (rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE)
> +		return rdev->desc->n_voltages ? : -EINVAL;

Please don't perpetuate the use of ? : as it's not a triumph of
legibility (even worse than the regular ternery operator).  I realise
that the original code did this but there's no need to carry on doing
the same thing.

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

Re: [PATCH 1/3] regulator: fix voltage check in regulator_is_supported_voltage()

[Mark Brown]

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

On Tue, Nov 13, 2012 at 09:48:51AM +0100, Marek Szyprowski wrote:
> regulator_is_supported_voltage() should return true only if the voltage
> of fixed/constant regulator is between min_uV and max_uV.

Applied, thanks.

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

[PATCH v2] regulator: treat regulators with constant volatage as fixed

[Marek Szyprowski]

Some drivers has additional logic for fixed regulators. Let regulator core
to treat regulators which cannot change their voltage due to applied
constraints as fixed.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/regulator/core.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 042c1ff..78b34b7 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1872,7 +1872,11 @@ int regulator_count_voltages(struct regulator *regulator)
 {
 	struct regulator_dev	*rdev = regulator->rdev;
 
-	return rdev->desc->n_voltages ? : -EINVAL;
+	if (rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE)
+		return rdev->desc->n_voltages ? rdev->desc->n_voltages :
+		       -EINVAL;
+	else
+		return 1;
 }
 EXPORT_SYMBOL_GPL(regulator_count_voltages);
 
-- 
1.7.9.5

Re: [PATCH v2] regulator: treat regulators with constant volatage as fixed

[Mark Brown]

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

On Tue, Nov 13, 2012 at 10:35:34AM +0100, Marek Szyprowski wrote:

> +	if (rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE)
> +		return rdev->desc->n_voltages ? rdev->desc->n_voltages :
> +		       -EINVAL;

The idea here was to avoid the ternery operator completely.

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

[PATCH v3] regulator: treat regulators with constant volatage as fixed

[Marek Szyprowski]

Some drivers has additional logic for fixed regulators. Let regulator core
to treat regulators which cannot change their voltage due to applied
constraints as fixed.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/regulator/core.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 042c1ff..d07c240 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1872,7 +1872,14 @@ int regulator_count_voltages(struct regulator *regulator)
 {
 	struct regulator_dev	*rdev = regulator->rdev;
 
-	return rdev->desc->n_voltages ? : -EINVAL;
+	if (rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE) {
+		if (rdev->desc->n_voltages)
+			return rdev->desc->n_voltages;
+		else
+			return -EINVAL;
+	} else {
+		return 1;
+	}
 }
 EXPORT_SYMBOL_GPL(regulator_count_voltages);
 
-- 
1.7.9.5

Re: [PATCH 3/3] mmc: sdhci: apply voltage range check only for non-fixed regulators

[Chris Ball]

Hi Marek,

On Tue, Nov 13 2012, Marek Szyprowski wrote:
> Fixed regulators cannot change their voltage, so disable all voltage range
> checking for them, otherwise the driver fails to operate with fixed
> regulators.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

Perhaps it's a good idea to mention that the regulator API is changing
(being fixed) at the same time, and that's why this patch is necessary.

> ---
>  drivers/mmc/host/sdhci.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index c7851c0..6f6534e 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2923,7 +2923,7 @@ int sdhci_add_host(struct sdhci_host *host)
>  		regulator_enable(host->vmmc);
>  
>  #ifdef CONFIG_REGULATOR
> -	if (host->vmmc) {
> +	if (host->vmmc && regulator_count_voltages(host->vmmc) > 1) {
>  		ret = regulator_is_supported_voltage(host->vmmc, 3300000,
>  			3300000);
>  		if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))

What about other occasions where is_supported_voltage() is used, like
this one -- is it necessary here too?

        else if (regulator_is_supported_voltage(host->vqmmc, 1800000, 1800000))
                regulator_enable(host->vqmmc);

Thanks,                

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

[PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators

[Marek Szyprowski]

Fixed regulators cannot change their voltage, so disable all voltage
range checking for them, otherwise the driver fails to operate with
fixed regulators. Up to now it worked only by luck, because
regulator_is_supported_voltage() function returned incorrect values.
Commit "regulator: fix voltage check in regulator_is_supported_voltage()"
fixed that function and now additional check is needed for fixed
regulators.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/mmc/host/sdhci.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index c7851c0..6f6534e 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2923,7 +2923,7 @@ int sdhci_add_host(struct sdhci_host *host)
 		regulator_enable(host->vmmc);
 
 #ifdef CONFIG_REGULATOR
-	if (host->vmmc) {
+	if (host->vmmc && regulator_count_voltages(host->vmmc) > 1) {
 		ret = regulator_is_supported_voltage(host->vmmc, 3300000,
 			3300000);
 		if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))
-- 
1.7.9.5

Re: [PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators

[Chris Ball]

Hi Marek,

On Tue, Nov 13 2012, Marek Szyprowski wrote:
> Fixed regulators cannot change their voltage, so disable all voltage
> range checking for them, otherwise the driver fails to operate with
> fixed regulators. Up to now it worked only by luck, because
> regulator_is_supported_voltage() function returned incorrect values.
> Commit "regulator: fix voltage check in regulator_is_supported_voltage()"
> fixed that function and now additional check is needed for fixed
> regulators.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/mmc/host/sdhci.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index c7851c0..6f6534e 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2923,7 +2923,7 @@ int sdhci_add_host(struct sdhci_host *host)
>  		regulator_enable(host->vmmc);
>  
>  #ifdef CONFIG_REGULATOR
> -	if (host->vmmc) {
> +	if (host->vmmc && regulator_count_voltages(host->vmmc) > 1) {
>  		ret = regulator_is_supported_voltage(host->vmmc, 3300000,
>  			3300000);
>  		if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))

Thanks for the longer explanation.  I'm still missing something, though;
what's wrong with running the check as it was with the new regulator code?
(I haven't tried it yet.)

#ifdef CONFIG_REGULATOR
        if (host->vmmc) {
                ret = regulator_is_supported_voltage(host->vmmc, 3300000,
                        3300000);
                if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))
                        caps[0] &= ~SDHCI_CAN_VDD_330;
                ret = regulator_is_supported_voltage(host->vmmc, 3000000,
                        3000000);
                if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_300)))
                        caps[0] &= ~SDHCI_CAN_VDD_300;
                ret = regulator_is_supported_voltage(host->vmmc, 1800000,
                        1800000);
                if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_180)))
                        caps[0] &= ~SDHCI_CAN_VDD_180;
        }
#endif /* CONFIG_REGULATOR */

The point is to remove unsupported voltages, so if someone sets up a
fixed regulator at 3300000, all of the other caps are disabled.  Why
wouldn't that work without this change, and how are we supposed to
remove those caps on a fixed regulator after your patchset?

Thanks, sorry if I'm missing something obvious,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators

[Marek Szyprowski]

Hello,

On 11/13/2012 2:45 PM, Chris Ball wrote:
> Hi Marek,
>
> On Tue, Nov 13 2012, Marek Szyprowski wrote:
> > Fixed regulators cannot change their voltage, so disable all voltage
> > range checking for them, otherwise the driver fails to operate with
> > fixed regulators. Up to now it worked only by luck, because
> > regulator_is_supported_voltage() function returned incorrect values.
> > Commit "regulator: fix voltage check in regulator_is_supported_voltage()"
> > fixed that function and now additional check is needed for fixed
> > regulators.
> >
> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > ---
> >  drivers/mmc/host/sdhci.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > index c7851c0..6f6534e 100644
> > --- a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> > @@ -2923,7 +2923,7 @@ int sdhci_add_host(struct sdhci_host *host)
> >  		regulator_enable(host->vmmc);
> >
> >  #ifdef CONFIG_REGULATOR
> > -	if (host->vmmc) {
> > +	if (host->vmmc && regulator_count_voltages(host->vmmc) > 1) {
> >  		ret = regulator_is_supported_voltage(host->vmmc, 3300000,
> >  			3300000);
> >  		if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))
>
> Thanks for the longer explanation.  I'm still missing something, though;
> what's wrong with running the check as it was with the new regulator code?
> (I haven't tried it yet.)
>
> #ifdef CONFIG_REGULATOR
>          if (host->vmmc) {
>                  ret = regulator_is_supported_voltage(host->vmmc, 3300000,
>                          3300000);
>                  if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))
>                          caps[0] &= ~SDHCI_CAN_VDD_330;
>                  ret = regulator_is_supported_voltage(host->vmmc, 3000000,
>                          3000000);
>                  if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_300)))
>                          caps[0] &= ~SDHCI_CAN_VDD_300;
>                  ret = regulator_is_supported_voltage(host->vmmc, 1800000,
>                          1800000);
>                  if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_180)))
>                          caps[0] &= ~SDHCI_CAN_VDD_180;
>          }
> #endif /* CONFIG_REGULATOR */
>
> The point is to remove unsupported voltages, so if someone sets up a
> fixed regulator at 3300000, all of the other caps are disabled.  Why
> wouldn't that work without this change, and how are we supposed to
> remove those caps on a fixed regulator after your patchset?
>
> Thanks, sorry if I'm missing something obvious,

On our boards eMMC is connected to fixed 2.8V regulator, what results in
clearing all available voltages and fail. The same situation is when one
enable dummy regulator and try to use sdhci with it. My patch fixes this
and restores sdhci to working state as it was before (before fixing
regulator regulator_is_supported_voltage() function and earlier when
MMC_BROKEN_VOLATGE capability was used).

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center

Re: [PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators

[Chris Ball]

Hi,

On Tue, Nov 13 2012, Marek Szyprowski wrote:
>> On Tue, Nov 13 2012, Marek Szyprowski wrote:
>> > Fixed regulators cannot change their voltage, so disable all voltage
>> > range checking for them, otherwise the driver fails to operate with
>> > fixed regulators. Up to now it worked only by luck, because
>> > regulator_is_supported_voltage() function returned incorrect values.
>> > Commit "regulator: fix voltage check in regulator_is_supported_voltage()"
>> > fixed that function and now additional check is needed for fixed
>> > regulators.
>> >
>> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> > ---
>> >  drivers/mmc/host/sdhci.c |    2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> > index c7851c0..6f6534e 100644
>> > --- a/drivers/mmc/host/sdhci.c
>> > +++ b/drivers/mmc/host/sdhci.c
>> > @@ -2923,7 +2923,7 @@ int sdhci_add_host(struct sdhci_host *host)
>> >  		regulator_enable(host->vmmc);
>> >
>> >  #ifdef CONFIG_REGULATOR
>> > -	if (host->vmmc) {
>> > +	if (host->vmmc && regulator_count_voltages(host->vmmc) > 1) {
>> >  		ret = regulator_is_supported_voltage(host->vmmc, 3300000,
>> >  			3300000);
>> >  		if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))
>>
>> Thanks for the longer explanation.  I'm still missing something, though;
>> what's wrong with running the check as it was with the new regulator code?
>> (I haven't tried it yet.)
>>
>> #ifdef CONFIG_REGULATOR
>>          if (host->vmmc) {
>>                  ret = regulator_is_supported_voltage(host->vmmc, 3300000,
>>                          3300000);
>>                  if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))
>>                          caps[0] &= ~SDHCI_CAN_VDD_330;
>>                  ret = regulator_is_supported_voltage(host->vmmc, 3000000,
>>                          3000000);
>>                  if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_300)))
>>                          caps[0] &= ~SDHCI_CAN_VDD_300;
>>                  ret = regulator_is_supported_voltage(host->vmmc, 1800000,
>>                          1800000);
>>                  if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_180)))
>>                          caps[0] &= ~SDHCI_CAN_VDD_180;
>>          }
>> #endif /* CONFIG_REGULATOR */
>>
>> The point is to remove unsupported voltages, so if someone sets up a
>> fixed regulator at 3300000, all of the other caps are disabled.  Why
>> wouldn't that work without this change, and how are we supposed to
>> remove those caps on a fixed regulator after your patchset?
>>
>> Thanks, sorry if I'm missing something obvious,
>
> On our boards eMMC is connected to fixed 2.8V regulator, what results in
> clearing all available voltages and fail. The same situation is when one
> enable dummy regulator and try to use sdhci with it. My patch fixes this
> and restores sdhci to working state as it was before (before fixing
> regulator regulator_is_supported_voltage() function and earlier when
> MMC_BROKEN_VOLATGE capability was used).

I see.  Sounds like a separate bug -- Philip (or anyone else), any
idea how we should be treating eMMCs with a fixed voltage here?

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators

[Philip Rakity]

Hi Marek,

Is the regulator dedicated ?  or is it shared ?   Is it used for eMMC ?

If it cannot be turned off -- then just don't list it in the regulators list for vmmc.  

If it CAN be turned off then need to get back to you.

Philip 

On Nov 13, 2012, at 2:14 PM, Chris Ball <cjb@laptop.org> wrote:

> Hi,
> 
> On Tue, Nov 13 2012, Marek Szyprowski wrote:
>>> On Tue, Nov 13 2012, Marek Szyprowski wrote:
>>>> Fixed regulators cannot change their voltage, so disable all voltage
>>>> range checking for them, otherwise the driver fails to operate with
>>>> fixed regulators. Up to now it worked only by luck, because
>>>> regulator_is_supported_voltage() function returned incorrect values.
>>>> Commit "regulator: fix voltage check in regulator_is_supported_voltage()"
>>>> fixed that function and now additional check is needed for fixed
>>>> regulators.
>>>> 
>>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>>> ---
>>>> drivers/mmc/host/sdhci.c |    2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>> 
>>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>>> index c7851c0..6f6534e 100644
>>>> --- a/drivers/mmc/host/sdhci.c
>>>> +++ b/drivers/mmc/host/sdhci.c
>>>> @@ -2923,7 +2923,7 @@ int sdhci_add_host(struct sdhci_host *host)
>>>> 		regulator_enable(host->vmmc);
>>>> 
>>>> #ifdef CONFIG_REGULATOR
>>>> -	if (host->vmmc) {
>>>> +	if (host->vmmc && regulator_count_voltages(host->vmmc) > 1) {
>>>> 		ret = regulator_is_supported_voltage(host->vmmc, 3300000,
>>>> 			3300000);
>>>> 		if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))
>>> 
>>> Thanks for the longer explanation.  I'm still missing something, though;
>>> what's wrong with running the check as it was with the new regulator code?
>>> (I haven't tried it yet.)
>>> 
>>> #ifdef CONFIG_REGULATOR
>>>         if (host->vmmc) {
>>>                 ret = regulator_is_supported_voltage(host->vmmc, 3300000,
>>>                         3300000);
>>>                 if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330)))
>>>                         caps[0] &= ~SDHCI_CAN_VDD_330;
>>>                 ret = regulator_is_supported_voltage(host->vmmc, 3000000,
>>>                         3000000);
>>>                 if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_300)))
>>>                         caps[0] &= ~SDHCI_CAN_VDD_300;
>>>                 ret = regulator_is_supported_voltage(host->vmmc, 1800000,
>>>                         1800000);
>>>                 if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_180)))
>>>                         caps[0] &= ~SDHCI_CAN_VDD_180;
>>>         }
>>> #endif /* CONFIG_REGULATOR */
>>> 
>>> The point is to remove unsupported voltages, so if someone sets up a
>>> fixed regulator at 3300000, all of the other caps are disabled.  Why
>>> wouldn't that work without this change, and how are we supposed to
>>> remove those caps on a fixed regulator after your patchset?
>>> 
>>> Thanks, sorry if I'm missing something obvious,
>> 
>> On our boards eMMC is connected to fixed 2.8V regulator, what results in
>> clearing all available voltages and fail. The same situation is when one
>> enable dummy regulator and try to use sdhci with it. My patch fixes this
>> and restores sdhci to working state as it was before (before fixing
>> regulator regulator_is_supported_voltage() function and earlier when
>> MMC_BROKEN_VOLATGE capability was used).
> 
> I see.  Sounds like a separate bug -- Philip (or anyone else), any
> idea how we should be treating eMMCs with a fixed voltage here?
> 
> Thanks,
> 
> - Chris.
> -- 
> Chris Ball   <cjb@laptop.org>   <http://printf.net/>
> One Laptop Per Child

Re: [PATCH 3/3] mmc: sdhci: apply voltage range check only for non-fixed regulators

[Mark Brown]

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

On Tue, Nov 13, 2012 at 07:45:07AM -0500, Chris Ball wrote:
> On Tue, Nov 13 2012, Marek Szyprowski wrote:
> > Fixed regulators cannot change their voltage, so disable all voltage range
> > checking for them, otherwise the driver fails to operate with fixed
> > regulators.

> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

> Perhaps it's a good idea to mention that the regulator API is changing
> (being fixed) at the same time, and that's why this patch is necessary.

This should be totally unrelated to any externally visible change in the
regulator API, if anything I would expect fixes in the API to *improve*
the ability of clients to work with fixed voltage regulators.  What's
missing here is some explanation as to what the problem is and how it's
being fixed.

As I understand it this should really be a workaround for hardware which
runs cards at out of spec voltages; we have a fixed voltage regulator
which claims to not support any of the voltages that are in spec (due to
this being what the hardware does) so if we try to use the voltage
management stuff in the MMC API it gets upset.  As a workaround if we
can't change the voltage we just ignore the voltage aspects of the API.

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

Re: [PATCH v3] regulator: treat regulators with constant volatage as fixed

[Mark Brown]

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

On Tue, Nov 13, 2012 at 10:49:37AM +0100, Marek Szyprowski wrote:

> +	if (rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE) {
> +		if (rdev->desc->n_voltages)
> +			return rdev->desc->n_voltages;
> +		else
> +			return -EINVAL;
> +	} else {
> +		return 1;
> +	}

Hrm, now I can read the logic I'm not convinced this is a good idea.
This will report that we have an available voltage for devices which
don't know their voltage (things like battery supplies often do this as
the voltage is unregulated) and it will mean that we are doing something 
different for the case where there's only one voltage (reporting the
restricted count instead of the physically supported count).

I think we want a regulator_can_change_voltage() or possibly a count
function (though I can't see any use cases except this) which answers
the question directly instead of layering on top of this function.

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

Re: [PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators

[Marek Szyprowski]

On 11/13/2012 10:23 PM, Philip Rakity wrote:
> Hi Marek,
>
> Is the regulator dedicated ?  or is it shared ?   Is it used for eMMC ?
>
> If it cannot be turned off -- then just don't list it in the regulators list for vmmc.
>
> If it CAN be turned off then need to get back to you.

It is dedicated to eMMC device and can be turned off. Patch "mmc: sdhci: 
apply
voltage range check only for non-fixed regulators" restored sdhci to 
working state
after merging the regulator fix. However there are lots of error 
messages from sdhci
driver:
s3c-sdhci s3c-sdhci.0: could not set regulator OCR (-1)

The only remaining problem is sdhci driver operation with dummy 
regulator. Right now
it simply fails to initialize if dummy regulator is enabled. Do you have 
any idea how
to fix it properly?

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center

Re: [PATCH v3] regulator: treat regulators with constant volatage as fixed

[Marek Szyprowski]

Hello,

On 11/14/2012 3:01 AM, Mark Brown wrote:
> On Tue, Nov 13, 2012 at 10:49:37AM +0100, Marek Szyprowski wrote:
>
> > +	if (rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE) {
> > +		if (rdev->desc->n_voltages)
> > +			return rdev->desc->n_voltages;
> > +		else
> > +			return -EINVAL;
> > +	} else {
> > +		return 1;
> > +	}
>
> Hrm, now I can read the logic I'm not convinced this is a good idea.
> This will report that we have an available voltage for devices which
> don't know their voltage (things like battery supplies often do this as
> the voltage is unregulated) and it will mean that we are doing something
> different for the case where there's only one voltage (reporting the
> restricted count instead of the physically supported count).
>
> I think we want a regulator_can_change_voltage() or possibly a count
> function (though I can't see any use cases except this) which answers
> the question directly instead of layering on top of this function.

Right, regulator_can_change_voltage() sounds much better than my hacky
approach. The first client would be probably sdhci/mmc driver, as
'can_change_voltage' check sounds much more appropriate than counting
available voltage values.

I will prepare patches soon.

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center