[PATCH 0/4] Fixing boot warnings in -next

[PATCH 0/4] Fixing boot warnings in -next

[Lee Jones]

When booting -next on the u8500 based Snowball development board these
new warnings appeared. If applicable these should probably be filtered
though your -fixes branches.

[PATCH 1/4] ARM: ux500: Rid ignored return value of regulator_enable() compiler warning

[Lee Jones]

arch/arm/mach-ux500/board-mop500.c: In function ?mop500_prox_activate?:
arch/arm/mach-ux500/board-mop500.c:406:18: warning: ignoring return value of
        ?regulator_enable?, declared with attribute warn_unused_result
        [-Wunused-result]

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/board-mop500.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
index a15dd6b..9bc762a 100644
--- a/arch/arm/mach-ux500/board-mop500.c
+++ b/arch/arm/mach-ux500/board-mop500.c
@@ -403,8 +403,8 @@ static int mop500_prox_activate(struct device *dev)
 			"no regulator\n");
 		return PTR_ERR(prox_regulator);
 	}
-	regulator_enable(prox_regulator);
-	return 0;
+	
+	return regulator_enable(prox_regulator);
 }
 
 static void mop500_prox_deactivate(struct device *dev)
-- 
1.7.10.4

[PATCH 1/4] ARM: ux500: Rid ignored return value of regulator_enable() compiler warning

[Linus Walleij]

On Thu, May 2, 2013 at 5:48 PM, Lee Jones <lee.jones@linaro.org> wrote:

> arch/arm/mach-ux500/board-mop500.c: In function ?mop500_prox_activate?:
> arch/arm/mach-ux500/board-mop500.c:406:18: warning: ignoring return value of
>         ?regulator_enable?, declared with attribute warn_unused_result
>         [-Wunused-result]
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Applied!

Thanks,
Linus Walleij

[PATCH 2/4] mfd: ab8500-gpadc: Suppress 'ignoring regulator_enable() return value' warning

[Lee Jones]

drivers/mfd/ab8500-gpadc.c: In function ?ab8500_gpadc_resume?:
drivers/mfd/ab8500-gpadc.c:911:18: warning: ignoring return value of
        ?regulator_enable?, declared with attribute warn_unused_result
        [-Wunused-result]

Cc: Samuel Ortiz <sameo@linux.intel.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mfd/ab8500-gpadc.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/ab8500-gpadc.c b/drivers/mfd/ab8500-gpadc.c
index 5e65b28..7278e53 100644
--- a/drivers/mfd/ab8500-gpadc.c
+++ b/drivers/mfd/ab8500-gpadc.c
@@ -907,8 +907,11 @@ static int ab8500_gpadc_suspend(struct device *dev)
 static int ab8500_gpadc_resume(struct device *dev)
 {
 	struct ab8500_gpadc *gpadc = dev_get_drvdata(dev);
+	int ret;
 
-	regulator_enable(gpadc->regu);
+	ret = regulator_enable(gpadc->regu);
+	if (ret < 0)
+		return ret;
 
 	pm_runtime_mark_last_busy(gpadc->dev);
 	pm_runtime_put_autosuspend(gpadc->dev);
-- 
1.7.10.4

[PATCH 2/4] mfd: ab8500-gpadc: Suppress 'ignoring regulator_enable() return value' warning

[Linus Walleij]

On Thu, May 2, 2013 at 5:48 PM, Lee Jones <lee.jones@linaro.org> wrote:
> drivers/mfd/ab8500-gpadc.c: In function ?ab8500_gpadc_resume?:
> drivers/mfd/ab8500-gpadc.c:911:18: warning: ignoring return value of
>         ?regulator_enable?, declared with attribute warn_unused_result
>         [-Wunused-result]
>
> Cc: Samuel Ortiz <sameo@linux.intel.com>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/mfd/ab8500-gpadc.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mfd/ab8500-gpadc.c b/drivers/mfd/ab8500-gpadc.c
> index 5e65b28..7278e53 100644
> --- a/drivers/mfd/ab8500-gpadc.c
> +++ b/drivers/mfd/ab8500-gpadc.c
> @@ -907,8 +907,11 @@ static int ab8500_gpadc_suspend(struct device *dev)
>  static int ab8500_gpadc_resume(struct device *dev)
>  {
>         struct ab8500_gpadc *gpadc = dev_get_drvdata(dev);
> +       int ret;
>
> -       regulator_enable(gpadc->regu);
> +       ret = regulator_enable(gpadc->regu);
> +       if (ret < 0)
> +               return ret;
>
>         pm_runtime_mark_last_busy(gpadc->dev);
>         pm_runtime_put_autosuspend(gpadc->dev);
> --
> 1.7.10.4

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 2/4] mfd: ab8500-gpadc: Suppress 'ignoring regulator_enable() return value' warning

[Lee Jones]

On Fri, 03 May 2013, Linus Walleij wrote:

> On Thu, May 2, 2013 at 5:48 PM, Lee Jones <lee.jones@linaro.org> wrote:
> > drivers/mfd/ab8500-gpadc.c: In function ?ab8500_gpadc_resume?:
> > drivers/mfd/ab8500-gpadc.c:911:18: warning: ignoring return value of
> >         ?regulator_enable?, declared with attribute warn_unused_result
> >         [-Wunused-result]
> >
> > Cc: Samuel Ortiz <sameo@linux.intel.com>
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  drivers/mfd/ab8500-gpadc.c |    5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mfd/ab8500-gpadc.c b/drivers/mfd/ab8500-gpadc.c
> > index 5e65b28..7278e53 100644
> > --- a/drivers/mfd/ab8500-gpadc.c
> > +++ b/drivers/mfd/ab8500-gpadc.c
> > @@ -907,8 +907,11 @@ static int ab8500_gpadc_suspend(struct device *dev)
> >  static int ab8500_gpadc_resume(struct device *dev)
> >  {
> >         struct ab8500_gpadc *gpadc = dev_get_drvdata(dev);
> > +       int ret;
> >
> > -       regulator_enable(gpadc->regu);
> > +       ret = regulator_enable(gpadc->regu);
> > +       if (ret < 0)
> > +               return ret;
> >
> >         pm_runtime_mark_last_busy(gpadc->dev);
> >         pm_runtime_put_autosuspend(gpadc->dev);
> 
> Acked-by: Linus Walleij <linus.walleij@linaro.org>

As it has your Ack, I'm going to add an error print as per Srinidhi's
suggestion, test it again and apply this to my MFD tree.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 2/4] mfd: ab8500-gpadc: Suppress 'ignoring regulator_enable() return value' warning

[Lee Jones]

On Fri, 03 May 2013, Lee Jones wrote:

> On Fri, 03 May 2013, Linus Walleij wrote:
> 
> > On Thu, May 2, 2013 at 5:48 PM, Lee Jones <lee.jones@linaro.org> wrote:
> > > drivers/mfd/ab8500-gpadc.c: In function ?ab8500_gpadc_resume?:
> > > drivers/mfd/ab8500-gpadc.c:911:18: warning: ignoring return value of
> > >         ?regulator_enable?, declared with attribute warn_unused_result
> > >         [-Wunused-result]
> > >
> > > Cc: Samuel Ortiz <sameo@linux.intel.com>
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > >  drivers/mfd/ab8500-gpadc.c |    5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/mfd/ab8500-gpadc.c b/drivers/mfd/ab8500-gpadc.c
> > > index 5e65b28..7278e53 100644
> > > --- a/drivers/mfd/ab8500-gpadc.c
> > > +++ b/drivers/mfd/ab8500-gpadc.c
> > > @@ -907,8 +907,11 @@ static int ab8500_gpadc_suspend(struct device *dev)
> > >  static int ab8500_gpadc_resume(struct device *dev)
> > >  {
> > >         struct ab8500_gpadc *gpadc = dev_get_drvdata(dev);
> > > +       int ret;
> > >
> > > -       regulator_enable(gpadc->regu);
> > > +       ret = regulator_enable(gpadc->regu);
> > > +       if (ret < 0)
> > > +               return ret;
> > >
> > >         pm_runtime_mark_last_busy(gpadc->dev);
> > >         pm_runtime_put_autosuspend(gpadc->dev);
> > 
> > Acked-by: Linus Walleij <linus.walleij@linaro.org>
> 
> As it has your Ack, I'm going to add an error print as per Srinidhi's
> suggestion, test it again and apply this to my MFD tree.

Slight change of plan, as I've changed the semantics.

Would you be kind enough to see v2?

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 2/4 v2] mfd: ab8500-gpadc: Suppress 'ignoring regulator_enable() return value' warning

[Lee Jones]

drivers/mfd/ab8500-gpadc.c: In function ?ab8500_gpadc_resume?:
drivers/mfd/ab8500-gpadc.c:911:18: warning: ignoring return value of
        ?regulator_enable?, declared with attribute warn_unused_result
        [-Wunused-result]

Cc: Samuel Ortiz <sameo@linux.intel.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>

diff --git a/drivers/mfd/ab8500-gpadc.c b/drivers/mfd/ab8500-gpadc.c
index 5e65b28..13f7866 100644
--- a/drivers/mfd/ab8500-gpadc.c
+++ b/drivers/mfd/ab8500-gpadc.c
@@ -907,14 +907,17 @@ static int ab8500_gpadc_suspend(struct device *dev)
 static int ab8500_gpadc_resume(struct device *dev)
 {
 	struct ab8500_gpadc *gpadc = dev_get_drvdata(dev);
+	int ret;
 
-	regulator_enable(gpadc->regu);
+	ret = regulator_enable(gpadc->regu);
+	if (ret)
+		dev_err(dev, "Failed to enable vtvout LDO: %d\n", ret);
 
 	pm_runtime_mark_last_busy(gpadc->dev);
 	pm_runtime_put_autosuspend(gpadc->dev);
 
 	mutex_unlock(&gpadc->ab8500_gpadc_lock);
-	return 0;
+	return ret;
 }
 
 static int ab8500_gpadc_probe(struct platform_device *pdev)

[PATCH 2/4 v2] mfd: ab8500-gpadc: Suppress 'ignoring regulator_enable() return value' warning

[Linus Walleij]

On Fri, May 3, 2013 at 1:15 PM, Lee Jones <lee.jones@linaro.org> wrote:

> drivers/mfd/ab8500-gpadc.c: In function ?ab8500_gpadc_resume?:
> drivers/mfd/ab8500-gpadc.c:911:18: warning: ignoring return value of
>         ?regulator_enable?, declared with attribute warn_unused_result
>         [-Wunused-result]
>
> Cc: Samuel Ortiz <sameo@linux.intel.com>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 2/4 v2] mfd: ab8500-gpadc: Suppress 'ignoring regulator_enable() return value' warning

[Lee Jones]

On Fri, 03 May 2013, Linus Walleij wrote:

> On Fri, May 3, 2013 at 1:15 PM, Lee Jones <lee.jones@linaro.org> wrote:
> 
> > drivers/mfd/ab8500-gpadc.c: In function ?ab8500_gpadc_resume?:
> > drivers/mfd/ab8500-gpadc.c:911:18: warning: ignoring return value of
> >         ?regulator_enable?, declared with attribute warn_unused_result
> >         [-Wunused-result]
> >
> > Cc: Samuel Ortiz <sameo@linux.intel.com>
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> 
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Yours,
> Linus Walleij

Nice, thanks. I'm applying this now.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 3/4] mmc: mmci: Ensure return value of regulator_enable() is checked

[Lee Jones]

This patch suppresses the warning below:

drivers/mmc/host/mmci.c: In function ?mmci_set_ios?:
drivers/mmc/host/mmci.c:1165:20: warning: ignoring return value of
        ?regulator_enable?, declared with attribute warn_unused_result
        [-Wunused-result]

Cc: Russell King <linux@arm.linux.org.uk>
Cc: Chris Ball <cjb@laptop.org>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mmc/host/mmci.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 375c109..f4f3038 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -1130,6 +1130,7 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	struct variant_data *variant = host->variant;
 	u32 pwr = 0;
 	unsigned long flags;
+	int ret;
 
 	pm_runtime_get_sync(mmc_dev(mmc));
 
@@ -1161,8 +1162,12 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 		break;
 	case MMC_POWER_ON:
 		if (!IS_ERR(mmc->supply.vqmmc) &&
-		    !regulator_is_enabled(mmc->supply.vqmmc))
-			regulator_enable(mmc->supply.vqmmc);
+		    !regulator_is_enabled(mmc->supply.vqmmc)) {
+			ret = regulator_enable(mmc->supply.vqmmc);
+			if (ret < 0)
+				dev_err(mmc_dev(mmc),
+					"failed to enable vqmmc regulator\n");
+		}
 
 		pwr |= MCI_PWR_ON;
 		break;
-- 
1.7.10.4

[PATCH 3/4] mmc: mmci: Ensure return value of regulator_enable() is checked

[Ulf Hansson]

On 2 May 2013 17:48, Lee Jones <lee.jones@linaro.org> wrote:
> This patch suppresses the warning below:
>
> drivers/mmc/host/mmci.c: In function ?mmci_set_ios?:
> drivers/mmc/host/mmci.c:1165:20: warning: ignoring return value of
>         ?regulator_enable?, declared with attribute warn_unused_result
>         [-Wunused-result]
>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Chris Ball <cjb@laptop.org>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/mmc/host/mmci.c |    9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index 375c109..f4f3038 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -1130,6 +1130,7 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>         struct variant_data *variant = host->variant;
>         u32 pwr = 0;
>         unsigned long flags;
> +       int ret;
>
>         pm_runtime_get_sync(mmc_dev(mmc));
>
> @@ -1161,8 +1162,12 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>                 break;
>         case MMC_POWER_ON:
>                 if (!IS_ERR(mmc->supply.vqmmc) &&
> -                   !regulator_is_enabled(mmc->supply.vqmmc))

I realize that we actually have a bug here (and in the MMC_POWER_OFF
mode as well).

We shall not use regulator_is_enabled API as a trigger to enable/fetch
a reference to the regulator, since it will only tell us if the
regulator is enabled - hw wise.
Instead we need a local variable in the mmci host struct keeping track
if we have enabled the regulator. Do you mind fix this up in this
patch as well since it is tightly coupled to the regulator handling!?

Otherwise it looks good to me.

Kind regards
Ulf Hansson

> -                       regulator_enable(mmc->supply.vqmmc);
> +                   !regulator_is_enabled(mmc->supply.vqmmc)) {
> +                       ret = regulator_enable(mmc->supply.vqmmc);
> +                       if (ret < 0)
> +                               dev_err(mmc_dev(mmc),
> +                                       "failed to enable vqmmc regulator\n");
> +               }
>
>                 pwr |= MCI_PWR_ON;
>                 break;
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at 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 3/4] mmc: mmci: Ensure return value of regulator_enable() is checked

[Lee Jones]

On Fri, 03 May 2013, Ulf Hansson wrote:

> On 2 May 2013 17:48, Lee Jones <lee.jones@linaro.org> wrote:
> > This patch suppresses the warning below:
> >
> > drivers/mmc/host/mmci.c: In function ?mmci_set_ios?:
> > drivers/mmc/host/mmci.c:1165:20: warning: ignoring return value of
> >         ?regulator_enable?, declared with attribute warn_unused_result
> >         [-Wunused-result]
> >
> > Cc: Russell King <linux@arm.linux.org.uk>
> > Cc: Chris Ball <cjb@laptop.org>
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  drivers/mmc/host/mmci.c |    9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> > index 375c109..f4f3038 100644
> > --- a/drivers/mmc/host/mmci.c
> > +++ b/drivers/mmc/host/mmci.c
> > @@ -1130,6 +1130,7 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> >         struct variant_data *variant = host->variant;
> >         u32 pwr = 0;
> >         unsigned long flags;
> > +       int ret;
> >
> >         pm_runtime_get_sync(mmc_dev(mmc));
> >
> > @@ -1161,8 +1162,12 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> >                 break;
> >         case MMC_POWER_ON:
> >                 if (!IS_ERR(mmc->supply.vqmmc) &&
> > -                   !regulator_is_enabled(mmc->supply.vqmmc))
> 
> I realize that we actually have a bug here (and in the MMC_POWER_OFF
> mode as well).
> 
> We shall not use regulator_is_enabled API as a trigger to enable/fetch
> a reference to the regulator, since it will only tell us if the
> regulator is enabled - hw wise.
> Instead we need a local variable in the mmci host struct keeping track
> if we have enabled the regulator. Do you mind fix this up in this
> patch as well since it is tightly coupled to the regulator handling!?

IMHO I think that should be taken care of in a separate patch. This
patch only touches the line containing regulator_is_enabled() to
encompass a multi-line comparison result.

Care to write that patch? I have so much on my TODO already. :|

> Otherwise it looks good to me.
> 
> Kind regards
> Ulf Hansson
> 
> > -                       regulator_enable(mmc->supply.vqmmc);
> > +                   !regulator_is_enabled(mmc->supply.vqmmc)) {
> > +                       ret = regulator_enable(mmc->supply.vqmmc);
> > +                       if (ret < 0)
> > +                               dev_err(mmc_dev(mmc),
> > +                                       "failed to enable vqmmc regulator\n");
> > +               }
> >
> >                 pwr |= MCI_PWR_ON;
> >                 break;
> >

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 3/4] mmc: mmci: Ensure return value of regulator_enable() is checked

[Ulf Hansson]

On 3 May 2013 10:16, Lee Jones <lee.jones@linaro.org> wrote:
> On Fri, 03 May 2013, Ulf Hansson wrote:
>
>> On 2 May 2013 17:48, Lee Jones <lee.jones@linaro.org> wrote:
>> > This patch suppresses the warning below:
>> >
>> > drivers/mmc/host/mmci.c: In function ?mmci_set_ios?:
>> > drivers/mmc/host/mmci.c:1165:20: warning: ignoring return value of
>> >         ?regulator_enable?, declared with attribute warn_unused_result
>> >         [-Wunused-result]
>> >
>> > Cc: Russell King <linux@arm.linux.org.uk>
>> > Cc: Chris Ball <cjb@laptop.org>
>> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
>> > ---
>> >  drivers/mmc/host/mmci.c |    9 +++++++--
>> >  1 file changed, 7 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
>> > index 375c109..f4f3038 100644
>> > --- a/drivers/mmc/host/mmci.c
>> > +++ b/drivers/mmc/host/mmci.c
>> > @@ -1130,6 +1130,7 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>> >         struct variant_data *variant = host->variant;
>> >         u32 pwr = 0;
>> >         unsigned long flags;
>> > +       int ret;
>> >
>> >         pm_runtime_get_sync(mmc_dev(mmc));
>> >
>> > @@ -1161,8 +1162,12 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>> >                 break;
>> >         case MMC_POWER_ON:
>> >                 if (!IS_ERR(mmc->supply.vqmmc) &&
>> > -                   !regulator_is_enabled(mmc->supply.vqmmc))
>>
>> I realize that we actually have a bug here (and in the MMC_POWER_OFF
>> mode as well).
>>
>> We shall not use regulator_is_enabled API as a trigger to enable/fetch
>> a reference to the regulator, since it will only tell us if the
>> regulator is enabled - hw wise.
>> Instead we need a local variable in the mmci host struct keeping track
>> if we have enabled the regulator. Do you mind fix this up in this
>> patch as well since it is tightly coupled to the regulator handling!?
>
> IMHO I think that should be taken care of in a separate patch. This
> patch only touches the line containing regulator_is_enabled() to
> encompass a multi-line comparison result.
>
> Care to write that patch? I have so much on my TODO already. :|

Sure, I can fix it.

I guess this patch will be going through Russell's patch tracker?

>
>> Otherwise it looks good to me.
>>
>> Kind regards
>> Ulf Hansson
>>
>> > -                       regulator_enable(mmc->supply.vqmmc);
>> > +                   !regulator_is_enabled(mmc->supply.vqmmc)) {
>> > +                       ret = regulator_enable(mmc->supply.vqmmc);
>> > +                       if (ret < 0)
>> > +                               dev_err(mmc_dev(mmc),
>> > +                                       "failed to enable vqmmc regulator\n");
>> > +               }
>> >
>> >                 pwr |= MCI_PWR_ON;
>> >                 break;
>> >
>
> --
> Lee Jones
> Linaro ST-Ericsson Landing Team Lead
> Linaro.org ? Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

[PATCH 3/4] mmc: mmci: Ensure return value of regulator_enable() is checked

[Lee Jones]

On Fri, 03 May 2013, Ulf Hansson wrote:

> On 3 May 2013 10:16, Lee Jones <lee.jones@linaro.org> wrote:
> > On Fri, 03 May 2013, Ulf Hansson wrote:
> >
> >> On 2 May 2013 17:48, Lee Jones <lee.jones@linaro.org> wrote:
> >> > This patch suppresses the warning below:
> >> >
> >> > drivers/mmc/host/mmci.c: In function ?mmci_set_ios?:
> >> > drivers/mmc/host/mmci.c:1165:20: warning: ignoring return value of
> >> >         ?regulator_enable?, declared with attribute warn_unused_result
> >> >         [-Wunused-result]
> >> >
> >> > Cc: Russell King <linux@arm.linux.org.uk>
> >> > Cc: Chris Ball <cjb@laptop.org>
> >> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> >> > ---
> >> >  drivers/mmc/host/mmci.c |    9 +++++++--
> >> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> >> > index 375c109..f4f3038 100644
> >> > --- a/drivers/mmc/host/mmci.c
> >> > +++ b/drivers/mmc/host/mmci.c
> >> > @@ -1130,6 +1130,7 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> >> >         struct variant_data *variant = host->variant;
> >> >         u32 pwr = 0;
> >> >         unsigned long flags;
> >> > +       int ret;
> >> >
> >> >         pm_runtime_get_sync(mmc_dev(mmc));
> >> >
> >> > @@ -1161,8 +1162,12 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> >> >                 break;
> >> >         case MMC_POWER_ON:
> >> >                 if (!IS_ERR(mmc->supply.vqmmc) &&
> >> > -                   !regulator_is_enabled(mmc->supply.vqmmc))
> >>
> >> I realize that we actually have a bug here (and in the MMC_POWER_OFF
> >> mode as well).
> >>
> >> We shall not use regulator_is_enabled API as a trigger to enable/fetch
> >> a reference to the regulator, since it will only tell us if the
> >> regulator is enabled - hw wise.
> >> Instead we need a local variable in the mmci host struct keeping track
> >> if we have enabled the regulator. Do you mind fix this up in this
> >> patch as well since it is tightly coupled to the regulator handling!?
> >
> > IMHO I think that should be taken care of in a separate patch. This
> > patch only touches the line containing regulator_is_enabled() to
> > encompass a multi-line comparison result.
> >
> > Care to write that patch? I have so much on my TODO already. :|
> 
> Sure, I can fix it.
> 
> I guess this patch will be going through Russell's patch tracker?

Yeah, I'll queue this and another one I have in a bit.

> >> Otherwise it looks good to me.
> >>
> >> Kind regards
> >> Ulf Hansson
> >>
> >> > -                       regulator_enable(mmc->supply.vqmmc);
> >> > +                   !regulator_is_enabled(mmc->supply.vqmmc)) {
> >> > +                       ret = regulator_enable(mmc->supply.vqmmc);
> >> > +                       if (ret < 0)
> >> > +                               dev_err(mmc_dev(mmc),
> >> > +                                       "failed to enable vqmmc regulator\n");
> >> > +               }
> >> >
> >> >                 pwr |= MCI_PWR_ON;
> >> >                 break;
> >> >
> >

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 4/4] staging: ste_rmi4: Suppress 'ignoring return value of ‘regulator_enable()' warning

[Lee Jones]

drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c:
        In function ?synaptics_rmi4_resume?:
drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c:1090:18:
        warning: ignoring return value of ?regulator_enable?, declared
        with attribute warn_unused_result [-Wunused-result

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: devel at driverdev.osuosl.org
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c b/drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c
index fe667dd..c4d013d 100644
--- a/drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c
+++ b/drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c
@@ -1087,7 +1087,9 @@ static int synaptics_rmi4_resume(struct device *dev)
 	unsigned char intr_status;
 	struct synaptics_rmi4_data *rmi4_data = dev_get_drvdata(dev);
 
-	regulator_enable(rmi4_data->regulator);
+	retval = regulator_enable(rmi4_data->regulator);
+	if (retval < 0)
+		return retval;
 
 	enable_irq(rmi4_data->i2c_client->irq);
 	rmi4_data->touch_stopped = false;
-- 
1.7.10.4

[PATCH 4/4] staging: ste_rmi4: Suppress 'ignoring return value of ‘regulator_enable()' warning

[Srinidhi Kasagar]

On Thu, May 02, 2013 at 17:48:10 +0200, Lee Jones wrote:
> drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c:
>         In function ?synaptics_rmi4_resume?:
> drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c:1090:18:
>         warning: ignoring return value of ?regulator_enable?, declared
>         with attribute warn_unused_result [-Wunused-result
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: devel at driverdev.osuosl.org
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c b/drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c
> index fe667dd..c4d013d 100644
> --- a/drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c
> +++ b/drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c
> @@ -1087,7 +1087,9 @@ static int synaptics_rmi4_resume(struct device *dev)
>  	unsigned char intr_status;
>  	struct synaptics_rmi4_data *rmi4_data = dev_get_drvdata(dev);
>  
> -	regulator_enable(rmi4_data->regulator);
> +	retval = regulator_enable(rmi4_data->regulator);
> +	if (retval < 0)
> +		return retval;
Does it make sense to add a dev_err?


Otherwise, you can add my
Acked-by:srinidhi kasagar <srinidhi.kasagar@stericsson.com> for this series.

>  
>  	enable_irq(rmi4_data->i2c_client->irq);
>  	rmi4_data->touch_stopped = false;
> -- 
> 1.7.10.4
> 

[PATCH 4/4] staging: ste_rmi4: Suppress 'ignoring return value of ‘regulator_enable()' warning

[Lee Jones]

On Fri, 03 May 2013, Srinidhi Kasagar wrote:

> On Thu, May 02, 2013 at 17:48:10 +0200, Lee Jones wrote:
> > drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c:
> >         In function ?synaptics_rmi4_resume?:
> > drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c:1090:18:
> >         warning: ignoring return value of ?regulator_enable?, declared
> >         with attribute warn_unused_result [-Wunused-result
> > 
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: devel at driverdev.osuosl.org
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c |    4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c b/drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c
> > index fe667dd..c4d013d 100644
> > --- a/drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c
> > +++ b/drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c
> > @@ -1087,7 +1087,9 @@ static int synaptics_rmi4_resume(struct device *dev)
> >  	unsigned char intr_status;
> >  	struct synaptics_rmi4_data *rmi4_data = dev_get_drvdata(dev);
> >  
> > -	regulator_enable(rmi4_data->regulator);
> > +	retval = regulator_enable(rmi4_data->regulator);
> > +	if (retval < 0)
> > +		return retval;
> Does it make sense to add a dev_err?

Yes, I can do that.

> Otherwise, you can add my
> Acked-by:srinidhi kasagar <srinidhi.kasagar@stericsson.com> for this series.

Thanks.

> >  
> >  	enable_irq(rmi4_data->i2c_client->irq);
> >  	rmi4_data->touch_stopped = false;

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 4/4] staging: ste_rmi4: Suppress 'ignoring return value of ‘regulator_enable()' warning

[Dan Carpenter]

On Fri, May 03, 2013 at 12:37:14PM +0530, Srinidhi Kasagar wrote:
> On Thu, May 02, 2013 at 17:48:10 +0200, Lee Jones wrote:
> > drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c:
> >         In function ?synaptics_rmi4_resume?:
> > drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c:1090:18:
> >         warning: ignoring return value of ?regulator_enable?, declared
> >         with attribute warn_unused_result [-Wunused-result
> > 
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: devel at driverdev.osuosl.org
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c |    4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c b/drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c
> > index fe667dd..c4d013d 100644
> > --- a/drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c
> > +++ b/drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c
> > @@ -1087,7 +1087,9 @@ static int synaptics_rmi4_resume(struct device *dev)
> >  	unsigned char intr_status;
> >  	struct synaptics_rmi4_data *rmi4_data = dev_get_drvdata(dev);
> >  
> > -	regulator_enable(rmi4_data->regulator);
> > +	retval = regulator_enable(rmi4_data->regulator);
> > +	if (retval < 0)
> > +		return retval;
> Does it make sense to add a dev_err?
> 

Is that a question?

regulator_enable() already prints some warnings.  Probably it's not
going to fail and adding code that is duplicative or will never be
run is pointless.

regards,
dan carpenter

[PATCH 4/4] staging: ste_rmi4: Suppress 'ignoring return value of ‘regulator_enable()' warning

[Srinidhi Kasagar]

On Sun, May 05, 2013 at 16:18:55 +0200, Dan Carpenter wrote:
> On Fri, May 03, 2013 at 12:37:14PM +0530, Srinidhi Kasagar wrote:
> > On Thu, May 02, 2013 at 17:48:10 +0200, Lee Jones wrote:
> > > drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c:
> > >         In function ?synaptics_rmi4_resume?:
> > > drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c:1090:18:
> > >         warning: ignoring return value of ?regulator_enable?, declared
> > >         with attribute warn_unused_result [-Wunused-result
> > > 
> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Cc: devel at driverdev.osuosl.org
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > >  drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c |    4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c b/drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c
> > > index fe667dd..c4d013d 100644
> > > --- a/drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c
> > > +++ b/drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c
> > > @@ -1087,7 +1087,9 @@ static int synaptics_rmi4_resume(struct device *dev)
> > >  	unsigned char intr_status;
> > >  	struct synaptics_rmi4_data *rmi4_data = dev_get_drvdata(dev);
> > >  
> > > -	regulator_enable(rmi4_data->regulator);
> > > +	retval = regulator_enable(rmi4_data->regulator);
> > > +	if (retval < 0)
> > > +		return retval;
> > Does it make sense to add a dev_err?
> > 
> 
> Is that a question?
> 
> regulator_enable() already prints some warnings.  Probably it's not
> going to fail and adding code that is duplicative or will never be
> run is pointless.

It has become a habit checking the return value and spit some errors.
And BTW, there are many drivers which does this way..Anyway if your
intention is to avoid them for the new drivers..I dont mind skipping
this..

regards/srinidhi


> 
> regards,
> dan carpenter
> 

[PATCH 4/4] staging: ste_rmi4: Suppress 'ignoring return value of ‘regulator_enable()' warning

[Dan Carpenter]

On Mon, May 06, 2013 at 12:21:39PM +0530, Srinidhi Kasagar wrote:
> > > > -	regulator_enable(rmi4_data->regulator);
> > > > +	retval = regulator_enable(rmi4_data->regulator);
> > > > +	if (retval < 0)
> > > > +		return retval;
> > > Does it make sense to add a dev_err?
> > > 
> > 
> > Is that a question?
> > 
> > regulator_enable() already prints some warnings.  Probably it's not
> > going to fail and adding code that is duplicative or will never be
> > run is pointless.
> 
> It has become a habit checking the return value and spit some errors.
> And BTW, there are many drivers which does this way..Anyway if your
> intention is to avoid them for the new drivers..I dont mind skipping
> this..

It's not like I'm trying to ban error messages.  There isn't any
official policy on error messages.

regards,
dan carpenter

[PATCH 4/4 v2] staging: ste_rmi4: Suppress 'ignoring return value of ‘regulator_enable()' warning

[Lee Jones]

drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c:
        In function ?synaptics_rmi4_resume?:
drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c:1090:18:
        warning: ignoring return value of ?regulator_enable?, declared
        with attribute warn_unused_result [-Wunused-result

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: devel at driverdev.osuosl.org
Acked-by: Srinidhi Kasagar <srinidhi.kasagar@stericsson.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>

diff --git a/drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c b/drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c
index fe667dd..386362c 100644
--- a/drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c
+++ b/drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c
@@ -1087,7 +1087,11 @@ static int synaptics_rmi4_resume(struct device *dev)
 	unsigned char intr_status;
 	struct synaptics_rmi4_data *rmi4_data = dev_get_drvdata(dev);
 
-	regulator_enable(rmi4_data->regulator);
+	retval = regulator_enable(rmi4_data->regulator);
+	if (retval) {
+		dev_err(dev, "Regulator enable failed (%d)\n", retval);
+		return retval;
+	}
 
 	enable_irq(rmi4_data->i2c_client->irq);
 	rmi4_data->touch_stopped = false;

[PATCH 4/4] staging: ste_rmi4: Suppress 'ignoring return value of ‘regulator_enable()' warning

[Lee Jones]

Hi Greg,

> drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c:
>         In function ?synaptics_rmi4_resume?:
> drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c:1090:18:
>         warning: ignoring return value of ?regulator_enable?, declared
>         with attribute warn_unused_result [-Wunused-result
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: devel at driverdev.osuosl.org
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c b/drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c
> index fe667dd..c4d013d 100644
> --- a/drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c
> +++ b/drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c
> @@ -1087,7 +1087,9 @@ static int synaptics_rmi4_resume(struct device *dev)
>  	unsigned char intr_status;
>  	struct synaptics_rmi4_data *rmi4_data = dev_get_drvdata(dev);
>  
> -	regulator_enable(rmi4_data->regulator);
> +	retval = regulator_enable(rmi4_data->regulator);
> +	if (retval < 0)
> +		return retval;
>  
>  	enable_irq(rmi4_data->i2c_client->irq);
>  	rmi4_data->touch_stopped = false;

Are you planning to take this into the v3.10 -rcs?

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 4/4] staging: ste_rmi4: Suppress 'ignoring return value of ‘regulator_enable()' warning

[Greg Kroah-Hartman]

On Tue, May 14, 2013 at 11:46:31AM +0100, Lee Jones wrote:
> Hi Greg,
> 
> > drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c:
> >         In function ?synaptics_rmi4_resume?:
> > drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c:1090:18:
> >         warning: ignoring return value of ?regulator_enable?, declared
> >         with attribute warn_unused_result [-Wunused-result
> > 
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: devel at driverdev.osuosl.org
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c |    4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c b/drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c
> > index fe667dd..c4d013d 100644
> > --- a/drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c
> > +++ b/drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c
> > @@ -1087,7 +1087,9 @@ static int synaptics_rmi4_resume(struct device *dev)
> >  	unsigned char intr_status;
> >  	struct synaptics_rmi4_data *rmi4_data = dev_get_drvdata(dev);
> >  
> > -	regulator_enable(rmi4_data->regulator);
> > +	retval = regulator_enable(rmi4_data->regulator);
> > +	if (retval < 0)
> > +		return retval;
> >  
> >  	enable_irq(rmi4_data->i2c_client->irq);
> >  	rmi4_data->touch_stopped = false;
> 
> Are you planning to take this into the v3.10 -rcs?

Yes, I was, give me some time to catch up on my pending patch queue (644
patches and counting).  Don't worry, it's not lost.

thanks,

greg k-h