iio: adc: vf610: Propagate the real error when platform_get_irq() fails

PATCH Applied Fabio Estevam 2 participants 13 messages Oct 3, 2014 – Oct 10, 2014

Subsystems (2)

IIO NXP i.MX 7D/6SX/6UL/93 and VF610 ADC driver

lore

Share & tools

lore

Fabio Estevam

author

2014-10-03 01:26 UTC #0 lore raw

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

From: Fabio Estevam <[email protected]>

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 <[email protected]>

---
 drivers/iio/adc/vf610_adc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/iio/adc/vf610_adc.c +2 -2

index 4a10ae9..c259901 100644
@@ -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,

Signature · 1 line

1.9.1

Fabio Estevam

author

2014-10-03 01:26 UTC #1 lore raw

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

From: Fabio Estevam <[email protected]>

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

Just return the error directly.

Signed-off-by Fabio Estevam <[email protected]>

---
 drivers/iio/adc/vf610_adc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/iio/adc/vf610_adc.c +1 -2

index c259901..a9c41ec 100644
@@ -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");

Signature · 1 line

1.9.1

Jonathan Cameron

maintainer Applied

2014-10-04 11:28 UTC #2 lore raw

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

Quoted 7 lines · Fabio Estevam

From: Fabio Estevam <[email protected]>
There is no need to pass the error clock code to the variable 'ret'.
Just return the error directly.
Signed-off-by Fabio Estevam <[email protected]>

Applied to the togreg branch of iio.git.

Thanks,

Quoted 19 lines

---
 drivers/iio/adc/vf610_adc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/iio/adc/vf610_adc.c +1 -2

index c259901..a9c41ec 100644
@@ -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");

Fabio Estevam

author

2014-10-03 01:26 UTC #3 lore raw

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

In reply to Fabio Estevam

From: Fabio Estevam <[email protected]>

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

Signed-off-by Fabio Estevam <[email protected]>

---
 drivers/iio/adc/vf610_adc.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

drivers/iio/adc/vf610_adc.c +5 -1

index a9c41ec..ae56753 100644
@@ -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

Signature · 1 line

1.9.1

Jonathan Cameron

maintainer Applied

2014-10-04 11:30 UTC #4 lore raw

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

Quoted 6 lines · Fabio Estevam

From: Fabio Estevam <[email protected]>
If clk_prepare_enable() fails we should disable the regulator that was previously enabled.
Signed-off-by Fabio Estevam <[email protected]>

Applied to the togreg branch of iio.git

Thanks,

Quoted 26 lines

---
 drivers/iio/adc/vf610_adc.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

drivers/iio/adc/vf610_adc.c +5 -1

index a9c41ec..ae56753 100644
@@ -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

Fabio Estevam

author

2014-10-03 01:26 UTC #5 lore raw

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

In reply to Fabio Estevam

From: Fabio Estevam <[email protected]>

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 <[email protected]>

---
 drivers/iio/adc/vf610_adc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

drivers/iio/adc/vf610_adc.c +1 -3

index ae56753..6164558 100644
@@ -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,

Signature · 1 line

1.9.1

Jonathan Cameron

maintainer Applied

2014-10-04 11:31 UTC #6 lore raw

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

Quoted 7 lines · Fabio Estevam

From: Fabio Estevam <[email protected]>
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 <[email protected]>

Applied

Quoted 20 lines

---
 drivers/iio/adc/vf610_adc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

drivers/iio/adc/vf610_adc.c +1 -3

index ae56753..6164558 100644
@@ -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,

Fabio Estevam

author

2014-10-03 01:26 UTC #7 lore raw

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

In reply to Fabio Estevam

From: Fabio Estevam <[email protected]>

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 <[email protected]>

---
 drivers/iio/adc/vf610_adc.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

drivers/iio/adc/vf610_adc.c +2 -4

index 6164558..bae9638 100644
@@ -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);

Signature · 1 line

1.9.1

Jonathan Cameron

maintainer

2014-10-04 11:34 UTC #8 lore raw

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

Quoted 6 lines · Fabio Estevam

From: Fabio Estevam <[email protected]>
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 <[email protected]>

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!

Quoted 36 lines

---
 drivers/iio/adc/vf610_adc.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

drivers/iio/adc/vf610_adc.c +2 -4

index 6164558..bae9638 100644
@@ -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);

Jonathan Cameron

maintainer

2014-10-04 11:27 UTC #9 lore raw

In reply to Fabio Estevam

Quoted 8 lines · Fabio Estevam

From: Fabio Estevam <[email protected]>
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!

Quoted 23 lines

Signed-off-by Fabio Estevam <[email protected]>

---
 drivers/iio/adc/vf610_adc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/iio/adc/vf610_adc.c +2 -2

index 4a10ae9..c259901 100644
@@ -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,

Fabio Estevam

author

2014-10-04 18:46 UTC #10 lore raw

Quoted 5 lines · Jonathan Cameron

Quoted 4 lines · Fabio Estevam
From: Fabio Estevam <[email protected]>
There is no need to pass a 'fake' return value when platform_get_irq() fails.

Quoted 7 lines

Quoted 4 lines
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.

Jonathan Cameron

maintainer

2014-10-10 23:46 UTC #11 lore raw

Quoted 21 lines · Fabio Estevam

Quoted 5 lines · Jonathan Cameron
Quoted 4 lines · Fabio Estevam
From: Fabio Estevam <[email protected]>
There is no need to pass a 'fake' return value when platform_get_irq() fails.
Quoted 7 lines
Quoted 4 lines
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.

Quoted 7 lines

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.

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Jonathan Cameron

maintainer

2014-10-10 23:46 UTC #12 lore raw

In reply to Fabio Estevam

Quoted 21 lines · Fabio Estevam

Quoted 5 lines · Jonathan Cameron
Quoted 4 lines · Fabio Estevam
From: Fabio Estevam <[email protected]>
There is no need to pass a 'fake' return value when platform_get_irq() fails.
Quoted 7 lines
Quoted 4 lines
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.

Quoted 7 lines

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.

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html