[PATCH] sdhci: Forward EPROBE_DEFER on vmmc and vqmmc regulators

[PATCH] sdhci: Forward EPROBE_DEFER on vmmc and vqmmc regulators

[Mike Looijmans]

If vmmc or vqmmc regulators are controlled by an I2C device, the
request for the regulator is likely to fail because the I2C bus has
not been probed yet. The sdhci then incorrectly assumes that the user
never wanted to use a regulator anyway and continues without ever
enabling or configuring the required regulator.

To solve this, when a required voltage regulator returns
EPROBE_DEFER, signalling that the regulator exists but is not
available yet, forward this error to the probe method instead
of simply assuming that the user never wanted to use a regulator
anyway.

Tested on a custom board that has an I2C regulator for one of the sdhcis
and no regulators at all for the other. This patch enables such a system
to work correctly.
---
 drivers/mmc/host/sdhci.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 34aef81..43b90c1 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2972,6 +2972,8 @@ int sdhci_add_host(struct sdhci_host *host)
 	host->vqmmc = regulator_get_optional(mmc_dev(mmc), "vqmmc");
 	if (IS_ERR_OR_NULL(host->vqmmc)) {
 		if (PTR_ERR(host->vqmmc) < 0) {
+			if (PTR_ERR(host->vqmmc) == -EPROBE_DEFER)
+				return -EPROBE_DEFER;
 			pr_info("%s: no vqmmc regulator found\n",
 				mmc_hostname(mmc));
 			host->vqmmc = NULL;
@@ -3048,8 +3050,10 @@ int sdhci_add_host(struct sdhci_host *host)
 	host->vmmc = regulator_get_optional(mmc_dev(mmc), "vmmc");
 	if (IS_ERR_OR_NULL(host->vmmc)) {
 		if (PTR_ERR(host->vmmc) < 0) {
-			pr_info("%s: no vmmc regulator found\n",
-				mmc_hostname(mmc));
+			if (PTR_ERR(host->vmmc) == -EPROBE_DEFER)
+				return -EPROBE_DEFER;
+			pr_info("%s: no vmmc regulator found (%d)\n",
+				mmc_hostname(mmc), PTR_ERR(host->vmmc));
 			host->vmmc = NULL;
 		}
 	}
-- 
1.7.9.5

Re: [PATCH] sdhci: Forward EPROBE_DEFER on vmmc and vqmmc regulators

[Georgi Djakov]

On 03/07/2014 04:18 PM, Mike Looijmans wrote:
> If vmmc or vqmmc regulators are controlled by an I2C device, the
> request for the regulator is likely to fail because the I2C bus has
> not been probed yet. The sdhci then incorrectly assumes that the user
> never wanted to use a regulator anyway and continues without ever
> enabling or configuring the required regulator.
>
> To solve this, when a required voltage regulator returns
> EPROBE_DEFER, signalling that the regulator exists but is not
> available yet, forward this error to the probe method instead
> of simply assuming that the user never wanted to use a regulator
> anyway.
>
> Tested on a custom board that has an I2C regulator for one of the sdhcis
> and no regulators at all for the other. This patch enables such a system
> to work correctly.
> ---
>   drivers/mmc/host/sdhci.c |    8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 34aef81..43b90c1 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2972,6 +2972,8 @@ int sdhci_add_host(struct sdhci_host *host)
>   	host->vqmmc = regulator_get_optional(mmc_dev(mmc), "vqmmc");
>   	if (IS_ERR_OR_NULL(host->vqmmc)) {
>   		if (PTR_ERR(host->vqmmc) < 0) {
> +			if (PTR_ERR(host->vqmmc) == -EPROBE_DEFER)
> +				return -EPROBE_DEFER;
>   			pr_info("%s: no vqmmc regulator found\n",
>   				mmc_hostname(mmc));
>   			host->vqmmc = NULL;
> @@ -3048,8 +3050,10 @@ int sdhci_add_host(struct sdhci_host *host)
>   	host->vmmc = regulator_get_optional(mmc_dev(mmc), "vmmc");
>   	if (IS_ERR_OR_NULL(host->vmmc)) {
>   		if (PTR_ERR(host->vmmc) < 0) {
> -			pr_info("%s: no vmmc regulator found\n",
> -				mmc_hostname(mmc));
> +			if (PTR_ERR(host->vmmc) == -EPROBE_DEFER)
> +				return -EPROBE_DEFER;
> +			pr_info("%s: no vmmc regulator found (%d)\n",

I suggest using %ld here. The rest looks fine to me! Thanks!

> +				mmc_hostname(mmc), PTR_ERR(host->vmmc));
>   			host->vmmc = NULL;
>   		}
>   	}
>

BR,
Georgi

Re: [PATCH] sdhci: Forward EPROBE_DEFER on vmmc and vqmmc regulators

[Mike Looijmans]

On 26-3-2014 16:09, Georgi Djakov wrote:
> On 03/07/2014 04:18 PM, Mike Looijmans wrote:
>> If vmmc or vqmmc regulators are controlled by an I2C device, the
>> request for the regulator is likely to fail because the I2C bus has
>> not been probed yet. The sdhci then incorrectly assumes that the user
>> never wanted to use a regulator anyway and continues without ever
>> enabling or configuring the required regulator.
>>
>> To solve this, when a required voltage regulator returns
>> EPROBE_DEFER, signalling that the regulator exists but is not
>> available yet, forward this error to the probe method instead
>> of simply assuming that the user never wanted to use a regulator
>> anyway.
>>
>> Tested on a custom board that has an I2C regulator for one of the sdhcis
>> and no regulators at all for the other. This patch enables such a system
>> to work correctly.
>> ---
>>   drivers/mmc/host/sdhci.c |    8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 34aef81..43b90c1 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -2972,6 +2972,8 @@ int sdhci_add_host(struct sdhci_host *host)
>>       host->vqmmc = regulator_get_optional(mmc_dev(mmc), "vqmmc");
>>       if (IS_ERR_OR_NULL(host->vqmmc)) {
>>           if (PTR_ERR(host->vqmmc) < 0) {
>> +            if (PTR_ERR(host->vqmmc) == -EPROBE_DEFER)
>> +                return -EPROBE_DEFER;
>>               pr_info("%s: no vqmmc regulator found\n",
>>                   mmc_hostname(mmc));
>>               host->vqmmc = NULL;
>> @@ -3048,8 +3050,10 @@ int sdhci_add_host(struct sdhci_host *host)
>>       host->vmmc = regulator_get_optional(mmc_dev(mmc), "vmmc");
>>       if (IS_ERR_OR_NULL(host->vmmc)) {
>>           if (PTR_ERR(host->vmmc) < 0) {
>> -            pr_info("%s: no vmmc regulator found\n",
>> -                mmc_hostname(mmc));
>> +            if (PTR_ERR(host->vmmc) == -EPROBE_DEFER)
>> +                return -EPROBE_DEFER;
>> +            pr_info("%s: no vmmc regulator found (%d)\n",
>
> I suggest using %ld here. The rest looks fine to me! Thanks!

Does this mean you're waiting for a patch v2 that I should submit?

>
>> +                mmc_hostname(mmc), PTR_ERR(host->vmmc));
>>               host->vmmc = NULL;
>>           }
>>       }
>>
>
> BR,
> Georgi
>


-- 
Mike Looijmans

Re: [PATCH] sdhci: Forward EPROBE_DEFER on vmmc and vqmmc regulators

[Georgi Djakov]

On 27.03.14, 19:47, Mike Looijmans wrote:
> On 26-3-2014 16:09, Georgi Djakov wrote:
>> On 03/07/2014 04:18 PM, Mike Looijmans wrote:
>>> If vmmc or vqmmc regulators are controlled by an I2C device, the
>>> request for the regulator is likely to fail because the I2C bus has
>>> not been probed yet. The sdhci then incorrectly assumes that the user
>>> never wanted to use a regulator anyway and continues without ever
>>> enabling or configuring the required regulator.
>>>
>>> To solve this, when a required voltage regulator returns
>>> EPROBE_DEFER, signalling that the regulator exists but is not
>>> available yet, forward this error to the probe method instead
>>> of simply assuming that the user never wanted to use a regulator
>>> anyway.
>>>
>>> Tested on a custom board that has an I2C regulator for one of the sdhcis
>>> and no regulators at all for the other. This patch enables such a system
>>> to work correctly.
>>> ---
>>>   drivers/mmc/host/sdhci.c |    8 ++++++--
>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index 34aef81..43b90c1 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -2972,6 +2972,8 @@ int sdhci_add_host(struct sdhci_host *host)
>>>       host->vqmmc = regulator_get_optional(mmc_dev(mmc), "vqmmc");
>>>       if (IS_ERR_OR_NULL(host->vqmmc)) {
>>>           if (PTR_ERR(host->vqmmc) < 0) {
>>> +            if (PTR_ERR(host->vqmmc) == -EPROBE_DEFER)
>>> +                return -EPROBE_DEFER;
>>>               pr_info("%s: no vqmmc regulator found\n",
>>>                   mmc_hostname(mmc));
>>>               host->vqmmc = NULL;
>>> @@ -3048,8 +3050,10 @@ int sdhci_add_host(struct sdhci_host *host)
>>>       host->vmmc = regulator_get_optional(mmc_dev(mmc), "vmmc");
>>>       if (IS_ERR_OR_NULL(host->vmmc)) {
>>>           if (PTR_ERR(host->vmmc) < 0) {
>>> -            pr_info("%s: no vmmc regulator found\n",
>>> -                mmc_hostname(mmc));
>>> +            if (PTR_ERR(host->vmmc) == -EPROBE_DEFER)
>>> +                return -EPROBE_DEFER;
>>> +            pr_info("%s: no vmmc regulator found (%d)\n",
>>
>> I suggest using %ld here. The rest looks fine to me! Thanks!
> 
> Does this mean you're waiting for a patch v2 that I should submit?
> 

Yes, please submit v2 and hope to get some more feedback from the community.

Thanks,
Georgi

>>
>>> +                mmc_hostname(mmc), PTR_ERR(host->vmmc));
>>>               host->vmmc = NULL;
>>>           }
>>>       }
>>>
>>
>> BR,
>> Georgi
>>

[PATCH] sdhci: Forward EPROBE_DEFER on vmmc and vqmmc regulators

[Mike Looijmans]

If vmmc or vqmmc regulators are controlled by an I2C device, the
request for the regulator is likely to fail because the I2C bus has
not been probed yet. The sdhci then incorrectly assumes that the user
never wanted to use a regulator anyway and continues without ever
enabling or configuring the required regulator.

To solve this, when a required voltage regulator returns
EPROBE_DEFER, signalling that the regulator exists but is not
available yet, forward this error to the probe method instead
of simply assuming that the user never wanted to use a regulator
anyway.

Tested on a custom board that has an I2C regulator for one of the sdhcis
and no regulators at all for the other. This patch enables such a system
to work correctly.

v2: Changed "%d" into "%ld" for PTR_ERR value.
---
 drivers/mmc/host/sdhci.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 34aef81..de601f5 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2972,6 +2972,8 @@ int sdhci_add_host(struct sdhci_host *host)
 	host->vqmmc = regulator_get_optional(mmc_dev(mmc), "vqmmc");
 	if (IS_ERR_OR_NULL(host->vqmmc)) {
 		if (PTR_ERR(host->vqmmc) < 0) {
+			if (PTR_ERR(host->vqmmc) == -EPROBE_DEFER)
+				return -EPROBE_DEFER;
 			pr_info("%s: no vqmmc regulator found\n",
 				mmc_hostname(mmc));
 			host->vqmmc = NULL;
@@ -3048,8 +3050,10 @@ int sdhci_add_host(struct sdhci_host *host)
 	host->vmmc = regulator_get_optional(mmc_dev(mmc), "vmmc");
 	if (IS_ERR_OR_NULL(host->vmmc)) {
 		if (PTR_ERR(host->vmmc) < 0) {
-			pr_info("%s: no vmmc regulator found\n",
-				mmc_hostname(mmc));
+			if (PTR_ERR(host->vmmc) == -EPROBE_DEFER)
+				return -EPROBE_DEFER;
+			pr_info("%s: no vmmc regulator found (%ld)\n",
+				mmc_hostname(mmc), PTR_ERR(host->vmmc));
 			host->vmmc = NULL;
 		}
 	}
-- 
1.7.9.5

Re: [PATCH] sdhci: Forward EPROBE_DEFER on vmmc and vqmmc regulators

[Ulf Hansson]

On 28 March 2014 08:30, Mike Looijmans <mike.looijmans@topic.nl> wrote:
> If vmmc or vqmmc regulators are controlled by an I2C device, the
> request for the regulator is likely to fail because the I2C bus has
> not been probed yet. The sdhci then incorrectly assumes that the user
> never wanted to use a regulator anyway and continues without ever
> enabling or configuring the required regulator.
>
> To solve this, when a required voltage regulator returns
> EPROBE_DEFER, signalling that the regulator exists but is not
> available yet, forward this error to the probe method instead
> of simply assuming that the user never wanted to use a regulator
> anyway.
>
> Tested on a custom board that has an I2C regulator for one of the sdhcis
> and no regulators at all for the other. This patch enables such a system
> to work correctly.

Missing Signed-off-by, could you resend?

Otherwise looks good, you may have my ack.

Kind regards
Ulf Hansson

>
> v2: Changed "%d" into "%ld" for PTR_ERR value.
> ---
>  drivers/mmc/host/sdhci.c |    8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 34aef81..de601f5 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2972,6 +2972,8 @@ int sdhci_add_host(struct sdhci_host *host)
>         host->vqmmc = regulator_get_optional(mmc_dev(mmc), "vqmmc");
>         if (IS_ERR_OR_NULL(host->vqmmc)) {
>                 if (PTR_ERR(host->vqmmc) < 0) {
> +                       if (PTR_ERR(host->vqmmc) == -EPROBE_DEFER)
> +                               return -EPROBE_DEFER;
>                         pr_info("%s: no vqmmc regulator found\n",
>                                 mmc_hostname(mmc));
>                         host->vqmmc = NULL;
> @@ -3048,8 +3050,10 @@ int sdhci_add_host(struct sdhci_host *host)
>         host->vmmc = regulator_get_optional(mmc_dev(mmc), "vmmc");
>         if (IS_ERR_OR_NULL(host->vmmc)) {
>                 if (PTR_ERR(host->vmmc) < 0) {
> -                       pr_info("%s: no vmmc regulator found\n",
> -                               mmc_hostname(mmc));
> +                       if (PTR_ERR(host->vmmc) == -EPROBE_DEFER)
> +                               return -EPROBE_DEFER;
> +                       pr_info("%s: no vmmc regulator found (%ld)\n",
> +                               mmc_hostname(mmc), PTR_ERR(host->vmmc));
>                         host->vmmc = NULL;
>                 }
>         }
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] sdhci: Forward EPROBE_DEFER on vmmc and vqmmc regulators

[Mike Looijmans]

If vmmc or vqmmc regulators are controlled by an I2C device, the
request for the regulator is likely to fail because the I2C bus has
not been probed yet. The sdhci then incorrectly assumes that the user
never wanted to use a regulator anyway and continues without ever
enabling or configuring the required regulator.

To solve this, when a required voltage regulator returns
EPROBE_DEFER, signalling that the regulator exists but is not
available yet, forward this error to the probe method instead
of simply assuming that the user never wanted to use a regulator
anyway.

Tested on a custom board that has an I2C regulator for one of the sdhcis
and no regulators at all for the other. This patch enables such a system
to work correctly.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
---
 drivers/mmc/host/sdhci.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 34aef81..43b90c1 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2972,6 +2972,8 @@ int sdhci_add_host(struct sdhci_host *host)
 	host->vqmmc = regulator_get_optional(mmc_dev(mmc), "vqmmc");
 	if (IS_ERR_OR_NULL(host->vqmmc)) {
 		if (PTR_ERR(host->vqmmc) < 0) {
+			if (PTR_ERR(host->vqmmc) == -EPROBE_DEFER)
+				return -EPROBE_DEFER;
 			pr_info("%s: no vqmmc regulator found\n",
 				mmc_hostname(mmc));
 			host->vqmmc = NULL;
@@ -3048,8 +3050,10 @@ int sdhci_add_host(struct sdhci_host *host)
 	host->vmmc = regulator_get_optional(mmc_dev(mmc), "vmmc");
 	if (IS_ERR_OR_NULL(host->vmmc)) {
 		if (PTR_ERR(host->vmmc) < 0) {
-			pr_info("%s: no vmmc regulator found\n",
-				mmc_hostname(mmc));
+			if (PTR_ERR(host->vmmc) == -EPROBE_DEFER)
+				return -EPROBE_DEFER;
+			pr_info("%s: no vmmc regulator found (%d)\n",
+				mmc_hostname(mmc), PTR_ERR(host->vmmc));
 			host->vmmc = NULL;
 		}
 	}
-- 
1.7.9.5

[PATCH] sdhci: Forward EPROBE_DEFER on vmmc and vqmmc regulators

[Mike Looijmans]

If vmmc or vqmmc regulators are controlled by an I2C device, the
request for the regulator is likely to fail because the I2C bus has
not been probed yet. The sdhci then incorrectly assumes that the user
never wanted to use a regulator anyway and continues without ever
enabling or configuring the required regulator.

To solve this, when a required voltage regulator returns
EPROBE_DEFER, signalling that the regulator exists but is not
available yet, forward this error to the probe method instead
of simply assuming that the user never wanted to use a regulator
anyway.

Tested on a custom board that has an I2C regulator for one of the sdhcis
and no regulators at all for the other. This patch enables such a system
to work correctly.

v2: Do not change logging output

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
---
 drivers/mmc/host/sdhci.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 34aef81..c92a5a1 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2972,6 +2972,8 @@ int sdhci_add_host(struct sdhci_host *host)
 	host->vqmmc = regulator_get_optional(mmc_dev(mmc), "vqmmc");
 	if (IS_ERR_OR_NULL(host->vqmmc)) {
 		if (PTR_ERR(host->vqmmc) < 0) {
+			if (PTR_ERR(host->vqmmc) == -EPROBE_DEFER)
+				return -EPROBE_DEFER;
 			pr_info("%s: no vqmmc regulator found\n",
 				mmc_hostname(mmc));
 			host->vqmmc = NULL;
@@ -3048,6 +3050,8 @@ int sdhci_add_host(struct sdhci_host *host)
 	host->vmmc = regulator_get_optional(mmc_dev(mmc), "vmmc");
 	if (IS_ERR_OR_NULL(host->vmmc)) {
 		if (PTR_ERR(host->vmmc) < 0) {
+			if (PTR_ERR(host->vmmc) == -EPROBE_DEFER)
+				return -EPROBE_DEFER;
 			pr_info("%s: no vmmc regulator found\n",
 				mmc_hostname(mmc));
 			host->vmmc = NULL;
-- 
1.7.9.5

Re: [PATCH] sdhci: Forward EPROBE_DEFER on vmmc and vqmmc regulators

[Arnd Bergmann]

On Monday 07 April 2014 08:38:28 Mike Looijmans wrote:
> index 34aef81..43b90c1 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2972,6 +2972,8 @@ int sdhci_add_host(struct sdhci_host *host)
>         host->vqmmc = regulator_get_optional(mmc_dev(mmc), "vqmmc");
>         if (IS_ERR_OR_NULL(host->vqmmc)) {
>                 if (PTR_ERR(host->vqmmc) < 0) {
> +                       if (PTR_ERR(host->vqmmc) == -EPROBE_DEFER)
> +                               return -EPROBE_DEFER;
>                         pr_info("%s: no vqmmc regulator found\n",
>                                 mmc_hostname(mmc));
>                         host->vqmmc = NULL;
> @@ -3048,8 +3050,10 @@ int sdhci_add_host(struct sdhci_host *host)
>         host->vmmc = regulator_get_optional(mmc_dev(mmc), "vmmc");
>         if (IS_ERR_OR_NULL(host->vmmc)) {
>                 if (PTR_ERR(host->vmmc) < 0) {
> -                       pr_info("%s: no vmmc regulator found\n",
> -                               mmc_hostname(mmc));
> +                       if (PTR_ERR(host->vmmc) == -EPROBE_DEFER)
> +                               return -EPROBE_DEFER;
> +                       pr_info("%s: no vmmc regulator found (%d)\n",
> +                               mmc_hostname(mmc), PTR_ERR(host->vmmc));
>                         host->vmmc = NULL;
>                 }

Please change the code to not use IS_ERR_OR_NULL() instead, getting
a NULL return value from regulator_get_optional() should not be
considered a bug, while getting an error return should always
cause the probe function to fail.

	Arnd

Re: [PATCH] sdhci: Forward EPROBE_DEFER on vmmc and vqmmc regulators

[Mike Looijmans]

On 04/07/2014 10:11 AM, Arnd Bergmann wrote:
> On Monday 07 April 2014 08:38:28 Mike Looijmans wrote:
>> index 34aef81..43b90c1 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -2972,6 +2972,8 @@ int sdhci_add_host(struct sdhci_host *host)
>>          host->vqmmc = regulator_get_optional(mmc_dev(mmc), "vqmmc");
>>          if (IS_ERR_OR_NULL(host->vqmmc)) {
>>                  if (PTR_ERR(host->vqmmc) < 0) {
>> +                       if (PTR_ERR(host->vqmmc) == -EPROBE_DEFER)
>> +                               return -EPROBE_DEFER;
>>                          pr_info("%s: no vqmmc regulator found\n",
>>                                  mmc_hostname(mmc));
>>                          host->vqmmc = NULL;
>> @@ -3048,8 +3050,10 @@ int sdhci_add_host(struct sdhci_host *host)
>>          host->vmmc = regulator_get_optional(mmc_dev(mmc), "vmmc");
>>          if (IS_ERR_OR_NULL(host->vmmc)) {
>>                  if (PTR_ERR(host->vmmc) < 0) {
>> -                       pr_info("%s: no vmmc regulator found\n",
>> -                               mmc_hostname(mmc));
>> +                       if (PTR_ERR(host->vmmc) == -EPROBE_DEFER)
>> +                               return -EPROBE_DEFER;
>> +                       pr_info("%s: no vmmc regulator found (%d)\n",
>> +                               mmc_hostname(mmc), PTR_ERR(host->vmmc));
>>                          host->vmmc = NULL;
>>                  }
>
> Please change the code to not use IS_ERR_OR_NULL() instead, getting
> a NULL return value from regulator_get_optional() should not be
> considered a bug, while getting an error return should always
> cause the probe function to fail.

Please make that a separate patch, because doing so will break most (if not 
all) boards using this controller.

Mike.


Met vriendelijke groet / kind regards,

Mike Looijmans

TOPIC Embedded Systems
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: (+31) (0) 499 33 69 79
Telefax:  (+31) (0) 499 33 69 70
E-mail: mike.looijmans@topic.nl
Website: www.topic.nl

Please consider the environment before printing this e-mail

Visit us at the Hannover Messe 7 - 11 April 2014 - Hall 002/D10 (Dutch Pavillion)
http://www.hannovermesse.de/exhibitor/topic-embedded-products/V229623

Re: [PATCH] sdhci: Forward EPROBE_DEFER on vmmc and vqmmc regulators

[Ben Dooks]

On 07/04/14 13:09, Mike Looijmans wrote:
> On 04/07/2014 10:11 AM, Arnd Bergmann wrote:
>> On Monday 07 April 2014 08:38:28 Mike Looijmans wrote:
>>> index 34aef81..43b90c1 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -2972,6 +2972,8 @@ int sdhci_add_host(struct sdhci_host *host)
>>>          host->vqmmc = regulator_get_optional(mmc_dev(mmc), "vqmmc");
>>>          if (IS_ERR_OR_NULL(host->vqmmc)) {
>>>                  if (PTR_ERR(host->vqmmc) < 0) {
>>> +                       if (PTR_ERR(host->vqmmc) == -EPROBE_DEFER)
>>> +                               return -EPROBE_DEFER;
>>>                          pr_info("%s: no vqmmc regulator found\n",
>>>                                  mmc_hostname(mmc));
>>>                          host->vqmmc = NULL;
>>> @@ -3048,8 +3050,10 @@ int sdhci_add_host(struct sdhci_host *host)
>>>          host->vmmc = regulator_get_optional(mmc_dev(mmc), "vmmc");
>>>          if (IS_ERR_OR_NULL(host->vmmc)) {
>>>                  if (PTR_ERR(host->vmmc) < 0) {
>>> -                       pr_info("%s: no vmmc regulator found\n",
>>> -                               mmc_hostname(mmc));
>>> +                       if (PTR_ERR(host->vmmc) == -EPROBE_DEFER)
>>> +                               return -EPROBE_DEFER;
>>> +                       pr_info("%s: no vmmc regulator found (%d)\n",
>>> +                               mmc_hostname(mmc), PTR_ERR(host->vmmc));
>>>                          host->vmmc = NULL;
>>>                  }
>>
>> Please change the code to not use IS_ERR_OR_NULL() instead, getting
>> a NULL return value from regulator_get_optional() should not be
>> considered a bug, while getting an error return should always
>> cause the probe function to fail.

Surely it needs to be changed to IS_ERR(), nor IS_ERR_OR_NULL()?


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH] sdhci: Forward EPROBE_DEFER on vmmc and vqmmc regulators

[Ben Dooks]

On 07/04/14 13:16, Ben Dooks wrote:
> On 07/04/14 13:09, Mike Looijmans wrote:
>> On 04/07/2014 10:11 AM, Arnd Bergmann wrote:
>>> On Monday 07 April 2014 08:38:28 Mike Looijmans wrote:
>>>> index 34aef81..43b90c1 100644
>>>> --- a/drivers/mmc/host/sdhci.c
>>>> +++ b/drivers/mmc/host/sdhci.c
>>>> @@ -2972,6 +2972,8 @@ int sdhci_add_host(struct sdhci_host *host)
>>>>          host->vqmmc = regulator_get_optional(mmc_dev(mmc), "vqmmc");
>>>>          if (IS_ERR_OR_NULL(host->vqmmc)) {
>>>>                  if (PTR_ERR(host->vqmmc) < 0) {
>>>> +                       if (PTR_ERR(host->vqmmc) == -EPROBE_DEFER)
>>>> +                               return -EPROBE_DEFER;
>>>>                          pr_info("%s: no vqmmc regulator found\n",
>>>>                                  mmc_hostname(mmc));
>>>>                          host->vqmmc = NULL;
>>>> @@ -3048,8 +3050,10 @@ int sdhci_add_host(struct sdhci_host *host)
>>>>          host->vmmc = regulator_get_optional(mmc_dev(mmc), "vmmc");
>>>>          if (IS_ERR_OR_NULL(host->vmmc)) {
>>>>                  if (PTR_ERR(host->vmmc) < 0) {
>>>> -                       pr_info("%s: no vmmc regulator found\n",
>>>> -                               mmc_hostname(mmc));
>>>> +                       if (PTR_ERR(host->vmmc) == -EPROBE_DEFER)
>>>> +                               return -EPROBE_DEFER;
>>>> +                       pr_info("%s: no vmmc regulator found (%d)\n",
>>>> +                               mmc_hostname(mmc),
>>>> PTR_ERR(host->vmmc));
>>>>                          host->vmmc = NULL;
>>>>                  }
>>>
>>> Please change the code to not use IS_ERR_OR_NULL() instead, getting
>>> a NULL return value from regulator_get_optional() should not be
>>> considered a bug, while getting an error return should always
>>> cause the probe function to fail.
>
> Surely it needs to be changed to IS_ERR(), nor IS_ERR_OR_NULL()?


And by that, I mean the use of "PTR_ERR(host->vmmc) < 0" to be
changed to IS_ERR(), which I think Arnd was originally complaining
about.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH] sdhci: Forward EPROBE_DEFER on vmmc and vqmmc regulators

[Arnd Bergmann]

On Monday 07 April 2014 13:18:54 Ben Dooks wrote:
> On 07/04/14 13:16, Ben Dooks wrote:
> > On 07/04/14 13:09, Mike Looijmans wrote:
> >> On 04/07/2014 10:11 AM, Arnd Bergmann wrote:
> >>> On Monday 07 April 2014 08:38:28 Mike Looijmans wrote:
> >>>> index 34aef81..43b90c1 100644
> >>>> --- a/drivers/mmc/host/sdhci.c
> >>>> +++ b/drivers/mmc/host/sdhci.c
> >>>> @@ -2972,6 +2972,8 @@ int sdhci_add_host(struct sdhci_host *host)
> >>>>          host->vqmmc = regulator_get_optional(mmc_dev(mmc), "vqmmc");
> >>>>          if (IS_ERR_OR_NULL(host->vqmmc)) {
> >>>>                  if (PTR_ERR(host->vqmmc) < 0) {
> >>>> +                       if (PTR_ERR(host->vqmmc) == -EPROBE_DEFER)
> >>>> +                               return -EPROBE_DEFER;
> >>>>                          pr_info("%s: no vqmmc regulator found\n",
> >>>>                                  mmc_hostname(mmc));
> >>>>                          host->vqmmc = NULL;
> >>>> @@ -3048,8 +3050,10 @@ int sdhci_add_host(struct sdhci_host *host)
> >>>>          host->vmmc = regulator_get_optional(mmc_dev(mmc), "vmmc");
> >>>>          if (IS_ERR_OR_NULL(host->vmmc)) {
> >>>>                  if (PTR_ERR(host->vmmc) < 0) {
> >>>> -                       pr_info("%s: no vmmc regulator found\n",
> >>>> -                               mmc_hostname(mmc));
> >>>> +                       if (PTR_ERR(host->vmmc) == -EPROBE_DEFER)
> >>>> +                               return -EPROBE_DEFER;
> >>>> +                       pr_info("%s: no vmmc regulator found (%d)\n",
> >>>> +                               mmc_hostname(mmc),
> >>>> PTR_ERR(host->vmmc));
> >>>>                          host->vmmc = NULL;
> >>>>                  }
> >>>
> >>> Please change the code to not use IS_ERR_OR_NULL() instead, getting
> >>> a NULL return value from regulator_get_optional() should not be
> >>> considered a bug, while getting an error return should always
> >>> cause the probe function to fail.
> >
> > Surely it needs to be changed to IS_ERR(), nor IS_ERR_OR_NULL()?
> 
> 
> And by that, I mean the use of "PTR_ERR(host->vmmc) < 0" to be
> changed to IS_ERR(), which I think Arnd was originally complaining
> about.

I mean something like

	host->vqmmc = regulator_get_optional(mmc_dev(mmc), "vqmmc");

	if (IS_ERR(host->vqmmc)) {
		if (PTR_ERR(host->vqmmc) != -EPROBE_DEFER)
			complain(); /* only print a message if we won't retry */
		return ERR_PTR(host->vqmmc); /* never ignore an error */
	}
	/* silently continue if no regulator is defined */

regulator_get_optional() means we can continue if it's not there, but
we should not continue if there is a regulator that we fail to use
for some reason.

As has been found a number of times, any use of IS_ERR_OR_NULL()
means that the person responsible for the code is confused about
what the API is supposed to be.

	Arnd

Re: [PATCH] sdhci: Forward EPROBE_DEFER on vmmc and vqmmc regulators

[Mike Looijmans]

On 04/07/2014 02:25 PM, Arnd Bergmann wrote:
> On Monday 07 April 2014 13:18:54 Ben Dooks wrote:
>> On 07/04/14 13:16, Ben Dooks wrote:
>>> On 07/04/14 13:09, Mike Looijmans wrote:
>>>> On 04/07/2014 10:11 AM, Arnd Bergmann wrote:
>>>>> On Monday 07 April 2014 08:38:28 Mike Looijmans wrote:
>>>>>> index 34aef81..43b90c1 100644
>>>>>> --- a/drivers/mmc/host/sdhci.c
>>>>>> +++ b/drivers/mmc/host/sdhci.c
>>>>>> @@ -2972,6 +2972,8 @@ int sdhci_add_host(struct sdhci_host *host)
>>>>>>           host->vqmmc = regulator_get_optional(mmc_dev(mmc), "vqmmc");
>>>>>>           if (IS_ERR_OR_NULL(host->vqmmc)) {
>>>>>>                   if (PTR_ERR(host->vqmmc) < 0) {
>>>>>> +                       if (PTR_ERR(host->vqmmc) == -EPROBE_DEFER)
>>>>>> +                               return -EPROBE_DEFER;
>>>>>>                           pr_info("%s: no vqmmc regulator found\n",
>>>>>>                                   mmc_hostname(mmc));
>>>>>>                           host->vqmmc = NULL;
>>>>>> @@ -3048,8 +3050,10 @@ int sdhci_add_host(struct sdhci_host *host)
>>>>>>           host->vmmc = regulator_get_optional(mmc_dev(mmc), "vmmc");
>>>>>>           if (IS_ERR_OR_NULL(host->vmmc)) {
>>>>>>                   if (PTR_ERR(host->vmmc) < 0) {
>>>>>> -                       pr_info("%s: no vmmc regulator found\n",
>>>>>> -                               mmc_hostname(mmc));
>>>>>> +                       if (PTR_ERR(host->vmmc) == -EPROBE_DEFER)
>>>>>> +                               return -EPROBE_DEFER;
>>>>>> +                       pr_info("%s: no vmmc regulator found (%d)\n",
>>>>>> +                               mmc_hostname(mmc),
>>>>>> PTR_ERR(host->vmmc));
>>>>>>                           host->vmmc = NULL;
>>>>>>                   }
>>>>>
>>>>> Please change the code to not use IS_ERR_OR_NULL() instead, getting
>>>>> a NULL return value from regulator_get_optional() should not be
>>>>> considered a bug, while getting an error return should always
>>>>> cause the probe function to fail.
>>>
>>> Surely it needs to be changed to IS_ERR(), nor IS_ERR_OR_NULL()?
>>
>>
>> And by that, I mean the use of "PTR_ERR(host->vmmc) < 0" to be
>> changed to IS_ERR(), which I think Arnd was originally complaining
>> about.
>
> I mean something like
>
> 	host->vqmmc = regulator_get_optional(mmc_dev(mmc), "vqmmc");
>
> 	if (IS_ERR(host->vqmmc)) {
> 		if (PTR_ERR(host->vqmmc) != -EPROBE_DEFER)
> 			complain(); /* only print a message if we won't retry */
> 		return ERR_PTR(host->vqmmc); /* never ignore an error */
> 	}
> 	/* silently continue if no regulator is defined */
>
> regulator_get_optional() means we can continue if it's not there, but
> we should not continue if there is a regulator that we fail to use
> for some reason.
>
> As has been found a number of times, any use of IS_ERR_OR_NULL()
> means that the person responsible for the code is confused about
> what the API is supposed to be.

Judging from the kernel output, regulator_get_optional returns -ENODEV if the 
supply wasn't found.

Maybe the API is confusing (or wrong?) here.

If you change the code as per your suggestion, the SD will not work unless you 
explicitly assign supplies. And judging from what I've seen so far, I am the 
first to have ever attached a power supply to this controller...

Mike.



Met vriendelijke groet / kind regards,

Mike Looijmans

TOPIC Embedded Systems
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: (+31) (0) 499 33 69 79
Telefax:  (+31) (0) 499 33 69 70
E-mail: mike.looijmans@topic.nl
Website: www.topic.nl

Please consider the environment before printing this e-mail

Visit us at the Hannover Messe 7 - 11 April 2014 - Hall 002/D10 (Dutch Pavillion)
http://www.hannovermesse.de/exhibitor/topic-embedded-products/V229623

Re: [PATCH] sdhci: Forward EPROBE_DEFER on vmmc and vqmmc regulators

[Arnd Bergmann]

On Monday 07 April 2014 14:32:20 Mike Looijmans wrote:
> On 04/07/2014 02:25 PM, Arnd Bergmann wrote:
> 
> Judging from the kernel output, regulator_get_optional returns -ENODEV if the 
> supply wasn't found.
> 
> Maybe the API is confusing (or wrong?) here.
> 
> If you change the code as per your suggestion, the SD will not work unless you 
> explicitly assign supplies. And judging from what I've seen so far, I am the 
> first to have ever attached a power supply to this controller...

It's certainly not very "optional" if it returns an error here.

You could try to special-case the "-ENODEV" return here, but I think it
would be better to change the regulator interface to be less confusing.

	Arnd

Re: [PATCH] sdhci: Forward EPROBE_DEFER on vmmc and vqmmc regulators

[Mike Looijmans]

On 04/07/2014 02:51 PM, Arnd Bergmann wrote:
> On Monday 07 April 2014 14:32:20 Mike Looijmans wrote:
>> On 04/07/2014 02:25 PM, Arnd Bergmann wrote:
>>
>> Judging from the kernel output, regulator_get_optional returns -ENODEV if the
>> supply wasn't found.
>>
>> Maybe the API is confusing (or wrong?) here.
>>
>> If you change the code as per your suggestion, the SD will not work unless you
>> explicitly assign supplies. And judging from what I've seen so far, I am the
>> first to have ever attached a power supply to this controller...
>
> It's certainly not very "optional" if it returns an error here.
>
> You could try to special-case the "-ENODEV" return here, but I think it
> would be better to change the regulator interface to be less confusing.
>

Judging from the code:
http://lxr.free-electrons.com/source/drivers/regulator/core.c#L1476

The behaviour is supposed to be something like "regulator_get_optional" will 
return an error if the supply wasn't found, but "regulator_get" will create a 
dummy supply for you instead of returning an error code.

So "regulator_get_optional" means: "I handle my own problems, just gimme the 
bad news". But "regulator_get" appears to imply: "I will shoot the messenger, 
so do whatever you can to get me something to say 'enable' to".

In this case, the IS_NULL part is wrong indeed, it will never return NULL.

I still think it's unrelated to my patch and should be submitted separately.

Mike.


Met vriendelijke groet / kind regards,

Mike Looijmans

TOPIC Embedded Systems
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: (+31) (0) 499 33 69 79
Telefax:  (+31) (0) 499 33 69 70
E-mail: mike.looijmans@topic.nl
Website: www.topic.nl

Please consider the environment before printing this e-mail

Visit us at the Hannover Messe 7 - 11 April 2014 - Hall 002/D10 (Dutch Pavillion)
http://www.hannovermesse.de/exhibitor/topic-embedded-products/V229623

Re: [PATCH] sdhci: Forward EPROBE_DEFER on vmmc and vqmmc regulators

[Andrew Bresticker]

Hi Mike,

On Mon, Apr 7, 2014 at 6:11 AM, Mike Looijmans <mike.looijmans@topic.nl> wrote:
> On 04/07/2014 02:51 PM, Arnd Bergmann wrote:
>>
>> On Monday 07 April 2014 14:32:20 Mike Looijmans wrote:
>>>
>>> On 04/07/2014 02:25 PM, Arnd Bergmann wrote:
>>>
>>> Judging from the kernel output, regulator_get_optional returns -ENODEV if
>>> the
>>> supply wasn't found.
>>>
>>> Maybe the API is confusing (or wrong?) here.
>>>
>>> If you change the code as per your suggestion, the SD will not work
>>> unless you
>>> explicitly assign supplies. And judging from what I've seen so far, I am
>>> the
>>> first to have ever attached a power supply to this controller...
>>
>>
>> It's certainly not very "optional" if it returns an error here.
>>
>> You could try to special-case the "-ENODEV" return here, but I think it
>> would be better to change the regulator interface to be less confusing.
>>
>
> Judging from the code:
> http://lxr.free-electrons.com/source/drivers/regulator/core.c#L1476
>
> The behaviour is supposed to be something like "regulator_get_optional" will
> return an error if the supply wasn't found, but "regulator_get" will create
> a dummy supply for you instead of returning an error code.
>
> So "regulator_get_optional" means: "I handle my own problems, just gimme the
> bad news". But "regulator_get" appears to imply: "I will shoot the
> messenger, so do whatever you can to get me something to say 'enable' to".
>
> In this case, the IS_NULL part is wrong indeed, it will never return NULL.

Not true.  If !CONFIG_REGULATOR, regulator_get() and co. will return
NULL.  Normally it is ok to ignore this case, but the sdhci driver
will call regulator_is_supported_voltage() on vqmmc to determine
whether 1.8V signalling is supported.  With !CONFIG_REGULATOR,
regulator_is_supported_voltage() will always return false, causing the
sdhci driver to disable all UHS modes.  So we still need to handle the
NULL (!CONFIG_REGULATOR) case.  One possibility is to guard the
voltage check with #ifdef CONFIG_REGULATOR, as is done later with
vmmc.

BTW, I was working on a similar patch at:
https://patchwork.kernel.org/patch/3988271/.

Thanks,
Andrew

>
> I still think it's unrelated to my patch and should be submitted separately.
>
>
> Mike.
>
>
> Met vriendelijke groet / kind regards,
>
> Mike Looijmans
>
> TOPIC Embedded Systems
> Eindhovenseweg 32-C, NL-5683 KH Best
> Postbus 440, NL-5680 AK Best
> Telefoon: (+31) (0) 499 33 69 79
> Telefax:  (+31) (0) 499 33 69 70
> E-mail: mike.looijmans@topic.nl
> Website: www.topic.nl
>
> Please consider the environment before printing this e-mail
>
> Visit us at the Hannover Messe 7 - 11 April 2014 - Hall 002/D10 (Dutch
> Pavillion)
> http://www.hannovermesse.de/exhibitor/topic-embedded-products/V229623
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] sdhci: Forward EPROBE_DEFER on vmmc and vqmmc regulators

[Mark Brown]

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

On Wed, Apr 16, 2014 at 01:15:53PM -0700, Andrew Bresticker wrote:

> Not true.  If !CONFIG_REGULATOR, regulator_get() and co. will return
> NULL.  Normally it is ok to ignore this case, but the sdhci driver
> will call regulator_is_supported_voltage() on vqmmc to determine
> whether 1.8V signalling is supported.  With !CONFIG_REGULATOR,
> regulator_is_supported_voltage() will always return false, causing the
> sdhci driver to disable all UHS modes.  So we still need to handle the
> NULL (!CONFIG_REGULATOR) case.  One possibility is to guard the
> voltage check with #ifdef CONFIG_REGULATOR, as is done later with
> vmmc.

It seems better to just fix regulator_get_optional() to return an error
if the API is disabled, that case didn't really get considered since
it's a bit surprising that something actively using regulators (rather
than simply ensuring they're enabled) would be using that function.
Though if you already have ifdefs the ifdef is probably fine.

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