Re: FW: Regulator API ignored return values

Re: FW: Regulator API ignored return values

[Kevin Liu]

> -----Original Message-----
> From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-owner@vger.kernel.org] On Behalf Of Chris Ball
> Sent: Tuesday, March 12, 2013 5:56 AM
> To: Arnd Bergmann
> Cc: linux-kernel@vger.kernel.org; Stephen Warren; linux-arm-kernel@lists.infradead.org; Mark Brown; Linus Walleij; Axel Lin; Jingoo Han; Felipe Balbi; Dmitry Torokhov; linux-mmc@vger.kernel.org
> Subject: Re: Regulator API ignored return values
>
> Hi,
>
> On Mon, Mar 11 2013, Arnd Bergmann wrote:
>> ==> build/dove_defconfig/faillog <==
>> /git/arm-soc/drivers/mmc/host/sdhci.c: In function 'sdhci_add_host':
>> /git/arm-soc/drivers/mmc/host/sdhci.c:2910:19: warning: ignoring
>> return value of 'regulator_enable', declared with attribute
>> warn_unused_result [-Wunused-result]
>
> Thanks, this looks like the right fix to me:
>
>
> Subject: [PATCH] mmc: sdhci: Don't ignore regulator_enable() return value
>
> Fixes:
> /git/arm-soc/drivers/mmc/host/sdhci.c: In function 'sdhci_add_host':
> /git/arm-soc/drivers/mmc/host/sdhci.c:2910:19: warning: ignoring
> return value of 'regulator_enable', declared with attribute
> warn_unused_result [-Wunused-result]
>
> Reported-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Chris Ball <cjb@laptop.org>
> ---
>  drivers/mmc/host/sdhci.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 51bbba4..fbf0b93 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2907,12 +2907,17 @@ int sdhci_add_host(struct sdhci_host *host)
>                         host->vqmmc = NULL;
>                 }
>         } else {
> -               regulator_enable(host->vqmmc);
> +               ret = regulator_enable(host->vqmmc);
>                 if (!regulator_is_supported_voltage(host->vqmmc, 1700000,
>                         1950000))
>                         caps[1] &= ~(SDHCI_SUPPORT_SDR104 |
>                                         SDHCI_SUPPORT_SDR50 |
>                                         SDHCI_SUPPORT_DDR50);
> +               if (ret) {
> +                       pr_warn("%s: Failed to enable vqmmc regulator: %d\n",
> +                               mmc_hostname(mmc), ret);

Need add regulator_put here since regulator_get has succeed?

> +                       host->vqmmc = NULL;
> +               }
>         }
>
>         if (host->quirks2 & SDHCI_QUIRK2_NO_1_8_V)
> --
> Chris Ball   <cjb@laptop.org>   <http://printf.net/>
> One Laptop Per Child
> --
> 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

Re: FW: Regulator API ignored return values

[Arnd Bergmann]

On Tuesday 12 March 2013, Kevin Liu wrote:
> > -               regulator_enable(host->vqmmc);
> > +               ret = regulator_enable(host->vqmmc);
> >                 if (!regulator_is_supported_voltage(host->vqmmc, 1700000,
> >                         1950000))
> >                         caps[1] &= ~(SDHCI_SUPPORT_SDR104 |
> >                                         SDHCI_SUPPORT_SDR50 |
> >                                         SDHCI_SUPPORT_DDR50);
> > +               if (ret) {
> > +                       pr_warn("%s: Failed to enable vqmmc regulator: %d\n",
> > +                               mmc_hostname(mmc), ret);
> 
> Need add regulator_put here since regulator_get has succeed?

Hmm, we still don't actually bail out if the error is encountered, so
the reference count is balanced with the current patch, but I maybe
a failed regulator_enable() should actually be a fatal error?

If we do that, using devm_regulator_get() would be a nice way to
track the reference counts.

	Arnd

Re: FW: Regulator API ignored return values

[Chris Ball]

Hi,

On Tue, Mar 12 2013, Arnd Bergmann wrote:
>> Need add regulator_put here since regulator_get has succeed?
>
> Hmm, we still don't actually bail out if the error is encountered, so
> the reference count is balanced with the current patch, but I maybe
> a failed regulator_enable() should actually be a fatal error?

The reason I didn't make it a fatal error is that this is just vqmmc
(responsible for moving from 3.3V to 1.8V for UHS modes), not the
main vmmc regulator.  We can just disable those UHS modes from the
capabilities on the host if vqmmc is missing, or failed to enable,
or doesn't support those voltages, and that's what the code does now.

Thanks,

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

Re: FW: Regulator API ignored return values

[Chris Ball]

Hi,

On Tue, Mar 12 2013, Chris Ball wrote:
> On Tue, Mar 12 2013, Arnd Bergmann wrote:
>>> Need add regulator_put here since regulator_get has succeed?
>>
>> Hmm, we still don't actually bail out if the error is encountered, so
>> the reference count is balanced with the current patch, but I maybe
>> a failed regulator_enable() should actually be a fatal error?
>
> The reason I didn't make it a fatal error is that this is just vqmmc
> (responsible for moving from 3.3V to 1.8V for UHS modes), not the
> main vmmc regulator.  We can just disable those UHS modes from the
> capabilities on the host if vqmmc is missing, or failed to enable,
> or doesn't support those voltages, and that's what the code does now.

I've pushed this patch to mmc-next for 3.10 now, let me know if you
disagree.

Thanks,

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

Re: FW: Regulator API ignored return values

[Arnd Bergmann]

On Friday 22 March 2013, Chris Ball wrote:
> >
> > The reason I didn't make it a fatal error is that this is just vqmmc
> > (responsible for moving from 3.3V to 1.8V for UHS modes), not the
> > main vmmc regulator.  We can just disable those UHS modes from the
> > capabilities on the host if vqmmc is missing, or failed to enable,
> > or doesn't support those voltages, and that's what the code does now.
> 
> I've pushed this patch to mmc-next for 3.10 now, let me know if you
> disagree.

No, it's fine. Sorry for not replying earlier.

	Arnd