[RESEND] mmc: core: Improve support for deferred regulators

It is important that callers of mmc_regulator_get_supply know if either
vmmc or vqmmc are present but not yet available.  To support this need,
modify this function to return zero except when either of the regulator
get operations fail with -EPROBE_DEFER.  Current callers don't check the
return value and may continue to use IS_ERR on the vmmc/vqmmc values to
determine if those regulators are available.  Furthermore, since all of
these callers are capable of dealing with absent regulators, switch over
to use the non-optional variety of the regulator get call.

Signed-off-by: Tim Kryger <tim.kryger@linaro.org>
---
 drivers/mmc/core/core.c |   27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

Hi Tim,

> It is important that callers of mmc_regulator_get_supply know if either
> vmmc or vqmmc are present but not yet available.  To support this need,
> modify this function to return zero except when either of the regulator
> get operations fail with -EPROBE_DEFER.  Current callers don't check the
> return value and may continue to use IS_ERR on the vmmc/vqmmc values to
> determine if those regulators are available.  Furthermore, since all of
> these callers are capable of dealing with absent regulators, switch over
> to use the non-optional variety of the regulator get call.
>
> Signed-off-by: Tim Kryger <tim.kryger@linaro.org>
> ---
>  drivers/mmc/core/core.c |   27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index acbc3f2..095eef0 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1313,21 +1313,28 @@ EXPORT_SYMBOL_GPL(mmc_regulator_set_ocr);
>  int mmc_regulator_get_supply(struct mmc_host *mmc)
>  {
>         struct device *dev = mmc_dev(mmc);
> -       struct regulator *supply;
>         int ret;
>
> -       supply = devm_regulator_get(dev, "vmmc");
> -       mmc->supply.vmmc = supply;
> +       mmc->supply.vmmc = devm_regulator_get_optional(dev, "vmmc");
>         mmc->supply.vqmmc = devm_regulator_get_optional(dev, "vqmmc");
>
> -       if (IS_ERR(supply))
> -               return PTR_ERR(supply);
> +       if (IS_ERR(mmc->supply.vmmc)) {
> +               if (PTR_ERR(mmc->supply.vmmc) == -EPROBE_DEFER)
> +                       return -EPROBE_DEFER;
> +               dev_info(dev, "No vmmc regulator found\n");
> +       } else {
> +               ret = mmc_regulator_get_ocrmask(mmc->supply.vmmc);
> +               if (ret > 0)
> +                       mmc->ocr_avail = ret;
> +               else
> +                       dev_warn(dev, "Failed getting OCR mask: %d\n", ret);
> +       }

I think we also need to handle the case where supply is NULL, i.e.
!CONFIG_REGULATOR.  The SDHCI driver, for example, calls
regulator_is_supported_voltage() which will always return false on
NULL regulators.  Perhaps we should set the supplies to
ERR_PTR(-ENODEV) in that case?

Thanks,
Andrew

>
> -       ret = mmc_regulator_get_ocrmask(supply);
> -       if (ret > 0)
> -               mmc->ocr_avail = ret;
> -       else
> -               dev_warn(mmc_dev(mmc), "Failed getting OCR mask: %d\n", ret);
> +       if (IS_ERR(mmc->supply.vqmmc)) {
> +               if (PTR_ERR(mmc->supply.vqmmc) == -EPROBE_DEFER)
> +                       return -EPROBE_DEFER;
> +               dev_info(dev, "No vqmmc regulator found\n");
> +       }
>
>         return 0;
>  }
> --
> 1.7.9.5
>
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Apr 25, 2014 at 2:33 PM, Andrew Bresticker
<abrestic@chromium.org> wrote:
>>
>> -       if (IS_ERR(supply))
>> -               return PTR_ERR(supply);
>> +       if (IS_ERR(mmc->supply.vmmc)) {
>> +               if (PTR_ERR(mmc->supply.vmmc) == -EPROBE_DEFER)
>> +                       return -EPROBE_DEFER;
>> +               dev_info(dev, "No vmmc regulator found\n");
>> +       } else {
>> +               ret = mmc_regulator_get_ocrmask(mmc->supply.vmmc);
>> +               if (ret > 0)
>> +                       mmc->ocr_avail = ret;
>> +               else
>> +                       dev_warn(dev, "Failed getting OCR mask: %d\n", ret);
>> +       }
>
> I think we also need to handle the case where supply is NULL, i.e.
> !CONFIG_REGULATOR.  The SDHCI driver, for example, calls
> regulator_is_supported_voltage() which will always return false on
> NULL regulators.  Perhaps we should set the supplies to
> ERR_PTR(-ENODEV) in that case?

As of v3.15-rc3, this function would do the right thing because
df7926f modified the regulator get optional stubs to return -ENODEV in
the !CONFIG_REGULATOR case.  However, this implementation of
mmc_regulator_get_supply doesn't actually get built for that case.  A
stub implementation is used instead.  Presently that stub
mmc_regulator_get_supply does nothing other than return zero but
perhaps it should initialize the supplies as you suggest.
Alternatively, the stub could be eliminated such that this
implementation is used in all cases.  Do you have a preference?

-Tim
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Apr 29, 2014 at 10:00 AM, Tim Kryger <tim.kryger@linaro.org> wrote:
> On Fri, Apr 25, 2014 at 2:33 PM, Andrew Bresticker
> <abrestic@chromium.org> wrote:
>>>
>>> -       if (IS_ERR(supply))
>>> -               return PTR_ERR(supply);
>>> +       if (IS_ERR(mmc->supply.vmmc)) {
>>> +               if (PTR_ERR(mmc->supply.vmmc) == -EPROBE_DEFER)
>>> +                       return -EPROBE_DEFER;
>>> +               dev_info(dev, "No vmmc regulator found\n");
>>> +       } else {
>>> +               ret = mmc_regulator_get_ocrmask(mmc->supply.vmmc);
>>> +               if (ret > 0)
>>> +                       mmc->ocr_avail = ret;
>>> +               else
>>> +                       dev_warn(dev, "Failed getting OCR mask: %d\n", ret);
>>> +       }
>>
>> I think we also need to handle the case where supply is NULL, i.e.
>> !CONFIG_REGULATOR.  The SDHCI driver, for example, calls
>> regulator_is_supported_voltage() which will always return false on
>> NULL regulators.  Perhaps we should set the supplies to
>> ERR_PTR(-ENODEV) in that case?
>
> As of v3.15-rc3, this function would do the right thing because
> df7926f modified the regulator get optional stubs to return -ENODEV in
> the !CONFIG_REGULATOR case.  However, this implementation of
> mmc_regulator_get_supply doesn't actually get built for that case.  A
> stub implementation is used instead.  Presently that stub
> mmc_regulator_get_supply does nothing other than return zero but
> perhaps it should initialize the supplies as you suggest.
> Alternatively, the stub could be eliminated such that this
> implementation is used in all cases.  Do you have a preference?

Since this now behaves correctly regardless of CONFIG_REGULATOR, then
perhaps it's best to eliminate the stub.

Thanks,
Andrew

>
> -Tim
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html