> -----Original Message-----
> From: [email protected] [mailto:[email protected]] On Behalf Of Chris Ball
> Sent: Tuesday, March 12, 2013 5:56 AM
> To: Arnd Bergmann
> Cc: [email protected]; Stephen Warren; [email protected]; Mark Brown; Linus Walleij; Axel Lin; Jingoo Han; Felipe Balbi; Dmitry Torokhov; [email protected]
> 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 <[email protected]>
> Signed-off-by: Chris Ball <[email protected]>
> ---
> 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 <[email protected]> <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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 <[email protected]> <http://printf.net/>
One Laptop Per Child
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 <[email protected]> <http://printf.net/>
One Laptop Per Child
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