[PATCH 1/5] iio: adc: vf610: Propagate the real error when platform_get_irq() fails

[PATCH 1/5] iio: adc: vf610: Propagate the real error when platform_get_irq() fails

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@freescale.com>

There is no need to pass a 'fake' return value when platform_get_irq() fails.

Propagate the real error instead. 

While at it, only consider negative numbers returned by platform_get_irq()
as error.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 drivers/iio/adc/vf610_adc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c
index 4a10ae9..c259901 100644
--- a/drivers/iio/adc/vf610_adc.c
+++ b/drivers/iio/adc/vf610_adc.c
@@ -569,9 +569,9 @@ static int vf610_adc_probe(struct platform_device *pdev)
 		return PTR_ERR(info->regs);
 
 	irq = platform_get_irq(pdev, 0);
-	if (irq <= 0) {
+	if (irq < 0) {
 		dev_err(&pdev->dev, "no irq resource?\n");
-		return -EINVAL;
+		return irq;
 	}
 
 	ret = devm_request_irq(info->dev, irq,
-- 
1.9.1

[PATCH 2/5] iio: adc: vf610: Return the error code directly

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@freescale.com>

There is no need to pass the error clock code to the variable 'ret'.

Just return the error directly.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 drivers/iio/adc/vf610_adc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c
index c259901..a9c41ec 100644
--- a/drivers/iio/adc/vf610_adc.c
+++ b/drivers/iio/adc/vf610_adc.c
@@ -586,8 +586,7 @@ static int vf610_adc_probe(struct platform_device *pdev)
 	if (IS_ERR(info->clk)) {
 		dev_err(&pdev->dev, "failed getting clock, err = %ld\n",
 						PTR_ERR(info->clk));
-		ret = PTR_ERR(info->clk);
-		return ret;
+		return PTR_ERR(info->clk);
 	}
 
 	info->vref = devm_regulator_get(&pdev->dev, "vref");
-- 
1.9.1

Re: [PATCH 2/5] iio: adc: vf610: Return the error code directly

[Jonathan Cameron]

On 03/10/14 02:25, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> There is no need to pass the error clock code to the variable 'ret'.
> 
> Just return the error directly.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
Applied to the togreg branch of iio.git.

Thanks,
> ---
>  drivers/iio/adc/vf610_adc.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c
> index c259901..a9c41ec 100644
> --- a/drivers/iio/adc/vf610_adc.c
> +++ b/drivers/iio/adc/vf610_adc.c
> @@ -586,8 +586,7 @@ static int vf610_adc_probe(struct platform_device *pdev)
>  	if (IS_ERR(info->clk)) {
>  		dev_err(&pdev->dev, "failed getting clock, err = %ld\n",
>  						PTR_ERR(info->clk));
> -		ret = PTR_ERR(info->clk);
> -		return ret;
> +		return PTR_ERR(info->clk);
>  	}
>  
>  	info->vref = devm_regulator_get(&pdev->dev, "vref");
> 

[PATCH 3/5] iio: adc: vf610: Disable the regulator on error

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@freescale.com>

If clk_prepare_enable() fails we should disable the regulator that was 
previously enabled.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 drivers/iio/adc/vf610_adc.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c
index a9c41ec..ae56753 100644
--- a/drivers/iio/adc/vf610_adc.c
+++ b/drivers/iio/adc/vf610_adc.c
@@ -680,11 +680,15 @@ static int vf610_adc_resume(struct device *dev)
 
 	ret = clk_prepare_enable(info->clk);
 	if (ret)
-		return ret;
+		goto disable_reg;
 
 	vf610_adc_hw_init(info);
 
 	return 0;
+
+disable_reg:
+	regulator_disable(info->vref);
+	return ret;
 }
 #endif
 
-- 
1.9.1

Re: [PATCH 3/5] iio: adc: vf610: Disable the regulator on error

[Jonathan Cameron]

On 03/10/14 02:25, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> If clk_prepare_enable() fails we should disable the regulator that was 
> previously enabled.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
Applied to the togreg branch of iio.git

Thanks,
> ---
>  drivers/iio/adc/vf610_adc.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c
> index a9c41ec..ae56753 100644
> --- a/drivers/iio/adc/vf610_adc.c
> +++ b/drivers/iio/adc/vf610_adc.c
> @@ -680,11 +680,15 @@ static int vf610_adc_resume(struct device *dev)
>  
>  	ret = clk_prepare_enable(info->clk);
>  	if (ret)
> -		return ret;
> +		goto disable_reg;
>  
>  	vf610_adc_hw_init(info);
>  
>  	return 0;
> +
> +disable_reg:
> +	regulator_disable(info->vref);
> +	return ret;
>  }
>  #endif
>  
> 

[PATCH 4/5] iio: adc: vf610: SIMPLE_DEV_PM_OPS can fit on a single line

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@freescale.com>

No need to call the SIMPLE_DEV_PM_OPS() macro in several lines.

It can fit into the 80-column range.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 drivers/iio/adc/vf610_adc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c
index ae56753..6164558 100644
--- a/drivers/iio/adc/vf610_adc.c
+++ b/drivers/iio/adc/vf610_adc.c
@@ -692,9 +692,7 @@ disable_reg:
 }
 #endif
 
-static SIMPLE_DEV_PM_OPS(vf610_adc_pm_ops,
-			vf610_adc_suspend,
-			vf610_adc_resume);
+static SIMPLE_DEV_PM_OPS(vf610_adc_pm_ops, vf610_adc_suspend, vf610_adc_resume);
 
 static struct platform_driver vf610_adc_driver = {
 	.probe          = vf610_adc_probe,
-- 
1.9.1

Re: [PATCH 4/5] iio: adc: vf610: SIMPLE_DEV_PM_OPS can fit on a single line

[Jonathan Cameron]

On 03/10/14 02:25, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> No need to call the SIMPLE_DEV_PM_OPS() macro in several lines.
> 
> It can fit into the 80-column range.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
Applied
> ---
>  drivers/iio/adc/vf610_adc.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c
> index ae56753..6164558 100644
> --- a/drivers/iio/adc/vf610_adc.c
> +++ b/drivers/iio/adc/vf610_adc.c
> @@ -692,9 +692,7 @@ disable_reg:
>  }
>  #endif
>  
> -static SIMPLE_DEV_PM_OPS(vf610_adc_pm_ops,
> -			vf610_adc_suspend,
> -			vf610_adc_resume);
> +static SIMPLE_DEV_PM_OPS(vf610_adc_pm_ops, vf610_adc_suspend, vf610_adc_resume);
>  
>  static struct platform_driver vf610_adc_driver = {
>  	.probe          = vf610_adc_probe,
> 

[PATCH 5/5] iio: adc: vf610: Remove CONFIG_PM_SLEEP ifdef's

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@freescale.com>

Mark the suspend/resume functions as '__maybe_unused' so that we can get rid
of the CONFIG_PM_SLEEP ifdef.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 drivers/iio/adc/vf610_adc.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c
index 6164558..bae9638 100644
--- a/drivers/iio/adc/vf610_adc.c
+++ b/drivers/iio/adc/vf610_adc.c
@@ -650,8 +650,7 @@ static int vf610_adc_remove(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_PM_SLEEP
-static int vf610_adc_suspend(struct device *dev)
+static int __maybe_unused vf610_adc_suspend(struct device *dev)
 {
 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
 	struct vf610_adc *info = iio_priv(indio_dev);
@@ -668,7 +667,7 @@ static int vf610_adc_suspend(struct device *dev)
 	return 0;
 }
 
-static int vf610_adc_resume(struct device *dev)
+static int __maybe_unused vf610_adc_resume(struct device *dev)
 {
 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
 	struct vf610_adc *info = iio_priv(indio_dev);
@@ -690,7 +689,6 @@ disable_reg:
 	regulator_disable(info->vref);
 	return ret;
 }
-#endif
 
 static SIMPLE_DEV_PM_OPS(vf610_adc_pm_ops, vf610_adc_suspend, vf610_adc_resume);
 
-- 
1.9.1

Re: [PATCH 5/5] iio: adc: vf610: Remove CONFIG_PM_SLEEP ifdef's

[Jonathan Cameron]

On 03/10/14 02:25, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> Mark the suspend/resume functions as '__maybe_unused' so that we can get rid
> of the CONFIG_PM_SLEEP ifdef.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
This is always debatable.  Personally my view is that it's actually
slightly clearer what is going on with the ifdef than the maybe_unused
so my gut feeling is to leave it alone.  Now if we get strong sentiments
the other way I might change my mind!


> ---
>  drivers/iio/adc/vf610_adc.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c
> index 6164558..bae9638 100644
> --- a/drivers/iio/adc/vf610_adc.c
> +++ b/drivers/iio/adc/vf610_adc.c
> @@ -650,8 +650,7 @@ static int vf610_adc_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -#ifdef CONFIG_PM_SLEEP
> -static int vf610_adc_suspend(struct device *dev)
> +static int __maybe_unused vf610_adc_suspend(struct device *dev)
>  {
>  	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>  	struct vf610_adc *info = iio_priv(indio_dev);
> @@ -668,7 +667,7 @@ static int vf610_adc_suspend(struct device *dev)
>  	return 0;
>  }
>  
> -static int vf610_adc_resume(struct device *dev)
> +static int __maybe_unused vf610_adc_resume(struct device *dev)
>  {
>  	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>  	struct vf610_adc *info = iio_priv(indio_dev);
> @@ -690,7 +689,6 @@ disable_reg:
>  	regulator_disable(info->vref);
>  	return ret;
>  }
> -#endif
>  
>  static SIMPLE_DEV_PM_OPS(vf610_adc_pm_ops, vf610_adc_suspend, vf610_adc_resume);
>  
> 

Re: [PATCH 1/5] iio: adc: vf610: Propagate the real error when platform_get_irq() fails

[Jonathan Cameron]

On 03/10/14 02:25, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> There is no need to pass a 'fake' return value when platform_get_irq() fails.
> 
> Propagate the real error instead. 
> 
> While at it, only consider negative numbers returned by platform_get_irq()
> as error.
Returning an irq of 0 is still invalid isn't it?
(there was a lot of 'fun' making this true for Arm a few years back).
Doesn't it effectively mean no irq is present?

Naturally I may have missed a change where it is allowed again!
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>  drivers/iio/adc/vf610_adc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c
> index 4a10ae9..c259901 100644
> --- a/drivers/iio/adc/vf610_adc.c
> +++ b/drivers/iio/adc/vf610_adc.c
> @@ -569,9 +569,9 @@ static int vf610_adc_probe(struct platform_device *pdev)
>  		return PTR_ERR(info->regs);
>  
>  	irq = platform_get_irq(pdev, 0);
> -	if (irq <= 0) {
> +	if (irq < 0) {
>  		dev_err(&pdev->dev, "no irq resource?\n");
> -		return -EINVAL;
> +		return irq;
>  	}
>  
>  	ret = devm_request_irq(info->dev, irq,
> 

Re: [PATCH 1/5] iio: adc: vf610: Propagate the real error when platform_get_irq() fails

[Fabio Estevam]

On Sat, Oct 4, 2014 at 8:26 AM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 03/10/14 02:25, Fabio Estevam wrote:
>> From: Fabio Estevam <fabio.estevam@freescale.com>
>>
>> There is no need to pass a 'fake' return value when platform_get_irq() fails.
>>
>> Propagate the real error instead.
>>
>> While at it, only consider negative numbers returned by platform_get_irq()
>> as error.
> Returning an irq of 0 is still invalid isn't it?
> (there was a lot of 'fun' making this true for Arm a few years back).
> Doesn't it effectively mean no irq is present?

In practice I cannot see how platform_get_irq() could return 0. Only
if the dtsi was providing 0 as the irq number for the vf610-adc
driver, which would mean a very broken dtsi.

This driver is used only by vf610 and imx6sx and their dtsi provide
the correct values for the adc irq.

This type of check:

       if (irq < 0)
               return irq;

is commonly used on the imx drivers and I was not aware of any
problems in this regard.

Re: [PATCH 1/5] iio: adc: vf610: Propagate the real error when platform_get_irq() fails

[Jonathan Cameron]

On 04/10/14 19:46, Fabio Estevam wrote:
> On Sat, Oct 4, 2014 at 8:26 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 03/10/14 02:25, Fabio Estevam wrote:
>>> From: Fabio Estevam <fabio.estevam@freescale.com>
>>>
>>> There is no need to pass a 'fake' return value when platform_get_irq() fails.
>>>
>>> Propagate the real error instead.
>>>
>>> While at it, only consider negative numbers returned by platform_get_irq()
>>> as error.
>> Returning an irq of 0 is still invalid isn't it?
>> (there was a lot of 'fun' making this true for Arm a few years back).
>> Doesn't it effectively mean no irq is present?
> 
> In practice I cannot see how platform_get_irq() could return 0. Only
> if the dtsi was providing 0 as the irq number for the vf610-adc
> driver, which would mean a very broken dtsi.
> 
> This driver is used only by vf610 and imx6sx and their dtsi provide
> the correct values for the adc irq.
> 
> This type of check:
> 
>        if (irq < 0)
>                return irq;
> 
> is commonly used on the imx drivers and I was not aware of any
> problems in this regard.
Fair enough.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 1/5] iio: adc: vf610: Propagate the real error when platform_get_irq() fails

[Jonathan Cameron]

On 04/10/14 19:46, Fabio Estevam wrote:
> On Sat, Oct 4, 2014 at 8:26 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 03/10/14 02:25, Fabio Estevam wrote:
>>> From: Fabio Estevam <fabio.estevam@freescale.com>
>>>
>>> There is no need to pass a 'fake' return value when platform_get_irq() fails.
>>>
>>> Propagate the real error instead.
>>>
>>> While at it, only consider negative numbers returned by platform_get_irq()
>>> as error.
>> Returning an irq of 0 is still invalid isn't it?
>> (there was a lot of 'fun' making this true for Arm a few years back).
>> Doesn't it effectively mean no irq is present?
> 
> In practice I cannot see how platform_get_irq() could return 0. Only
> if the dtsi was providing 0 as the irq number for the vf610-adc
> driver, which would mean a very broken dtsi.
> 
> This driver is used only by vf610 and imx6sx and their dtsi provide
> the correct values for the adc irq.
> 
> This type of check:
> 
>        if (irq < 0)
>                return irq;
> 
> is commonly used on the imx drivers and I was not aware of any
> problems in this regard.

Applied to the togreg branch of iio.git.  Will get pushed out as testing
for the autobuilders to play when I'm not hanging out in a clean room
(not that clean as my laptop is in it ;) with no network connection
and a delightful hair net.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>