staging: iio: ad799x: cleanup probe error handling

PATCH In review Dan Carpenter 4 participants 6 messages Apr 30, 2014 – May 3, 2014

Subsystems (3)

Kernel janitors IIO Analog Devices IIO drivers

lore

Share & tools

lore

Dan Carpenter

author

2014-04-30 10:08 UTC #0 lore raw

PATCH 1/1 staging: iio: ad799x: cleanup probe error handling

My static checker is upset that we check IS_ERR(t->reg) when we know it is not an ERR_PTR(). The code is harmless but I have cleaned it up and removed the condition.

Signed-off-by Dan Carpenter <[email protected]>

drivers/iio/adc/ad799x.c +4 -5

index 16a8b14..5c95cd9 100644
@@ -717,7 +717,7 @@ static int ad799x_probe(struct i2c_client *client,
 	ret = iio_triggered_buffer_setup(indio_dev, NULL,
 		&ad799x_trigger_handler, NULL);
 	if (ret)
-		goto error_disable_reg;
+		goto error_disable_vref;
 
 	if (client->irq > 0) {
 		ret = devm_request_threaded_irq(&client->dev,

@@ -739,11 +739,10 @@ static int ad799x_probe(struct i2c_client *client,
 
 error_cleanup_ring:
 	iio_triggered_buffer_cleanup(indio_dev);
+error_disable_vref:
+	regulator_disable(st->vref);
 error_disable_reg:
-	if (!IS_ERR(st->vref))
-		regulator_disable(st->vref);
-	if (!IS_ERR(st->reg))
-		regulator_disable(st->reg);
+	regulator_disable(st->reg);
 
 	return ret;
 }

Lars-Peter Clausen

Acked-by on v1 1/1

2014-04-30 12:00 UTC #1 lore raw

Quoted 5 lines · Dan Carpenter

My static checker is upset that we check IS_ERR(t->reg) when we know it is not an ERR_PTR(). The code is harmless but I have cleaned it up and removed the condition.
Signed-off-by Dan Carpenter <[email protected]>

Yea, I think that's a left over of another cleanup. Thanks.

Acked-by Lars-Peter Clausen <[email protected]>

Quoted 34 lines

drivers/iio/adc/ad799x.c +4 -6

index 16a8b14..5c95cd9 100644
@@ -717,7 +717,7 @@ static int ad799x_probe(struct i2c_client *client,
  	ret = iio_triggered_buffer_setup(indio_dev, NULL,
  		&ad799x_trigger_handler, NULL);
  	if (ret)
-		goto error_disable_reg;
+		goto error_disable_vref;
 
  	if (client->irq > 0) {
  		ret = devm_request_threaded_irq(&client->dev,

@@ -739,11 +739,10 @@ static int ad799x_probe(struct i2c_client *client,
 
  error_cleanup_ring:
  	iio_triggered_buffer_cleanup(indio_dev);
+error_disable_vref:
+	regulator_disable(st->vref);
  error_disable_reg:
-	if (!IS_ERR(st->vref))
-		regulator_disable(st->vref);
-	if (!IS_ERR(st->reg))
-		regulator_disable(st->reg);
+	regulator_disable(st->reg);
 
  	return ret;
  }
--

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

Hartmut Knaack

2014-04-30 20:37 UTC #2 lore raw

In reply to Dan Carpenter

Quoted 3 lines · Dan Carpenter

My static checker is upset that we check IS_ERR(t->reg) when we know it is not an ERR_PTR(). The code is harmless but I have cleaned it up and removed the condition.

Good spot. But doesn't that also apply to the two checks in ad799x_remove()?

Quoted 35 lines

Signed-off-by Dan Carpenter <[email protected]>

drivers/iio/adc/ad799x.c +4 -6

index 16a8b14..5c95cd9 100644
@@ -717,7 +717,7 @@ static int ad799x_probe(struct i2c_client *client,
 	ret = iio_triggered_buffer_setup(indio_dev, NULL,
 		&ad799x_trigger_handler, NULL);
 	if (ret)
-		goto error_disable_reg;
+		goto error_disable_vref;
 
 	if (client->irq > 0) {
 		ret = devm_request_threaded_irq(&client->dev,

@@ -739,11 +739,10 @@ static int ad799x_probe(struct i2c_client *client,
 
 error_cleanup_ring:
 	iio_triggered_buffer_cleanup(indio_dev);
+error_disable_vref:
+	regulator_disable(st->vref);
 error_disable_reg:
-	if (!IS_ERR(st->vref))
-		regulator_disable(st->vref);
-	if (!IS_ERR(st->reg))
-		regulator_disable(st->reg);
+	regulator_disable(st->reg);
 
 	return ret;
 }
--

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

Dan Carpenter

author

2014-04-30 21:05 UTC #3 lore raw

PATCH v2 1/1 staging: iio: ad799x: remove some unneeded IS_ERR() checks

My static checker is upset that we check IS_ERR(t->reg) when we know it is not an ERR_PTR.

Checking for IS_ERR() twice is often a sign of confusion and buggy code. In this case, if the call to "ret = regulator_enable(st->vref);" fails, then we call "regulator_disable(st->vref);" and that's a mistake because "st->vref" is not enabled.

I fixed these problems and Hartmut Knaack pointed out a couple unneeded IS_ERR() checks in ad799x_remove() so I have removed those as well.

Signed-off-by Dan Carpenter <[email protected]>

---
v2: remove the unneeded checks in ad799x_remove() as well. Updated the
commit message to say that it's actually a small bug fix.

drivers/iio/adc/ad799x.c +6 -9

index 16a8b14..39b4cb4 100644
@@ -717,7 +717,7 @@ static int ad799x_probe(struct i2c_client *client,
 	ret = iio_triggered_buffer_setup(indio_dev, NULL,
 		&ad799x_trigger_handler, NULL);
 	if (ret)
-		goto error_disable_reg;
+		goto error_disable_vref;
 
 	if (client->irq > 0) {
 		ret = devm_request_threaded_irq(&client->dev,

@@ -739,11 +739,10 @@ static int ad799x_probe(struct i2c_client *client,
 
 error_cleanup_ring:
 	iio_triggered_buffer_cleanup(indio_dev);
+error_disable_vref:
+	regulator_disable(st->vref);
 error_disable_reg:
-	if (!IS_ERR(st->vref))
-		regulator_disable(st->vref);
-	if (!IS_ERR(st->reg))
-		regulator_disable(st->reg);
+	regulator_disable(st->reg);
 
 	return ret;
 }

@@ -756,10 +755,8 @@ static int ad799x_remove(struct i2c_client *client)
 	iio_device_unregister(indio_dev);
 
 	iio_triggered_buffer_cleanup(indio_dev);
-	if (!IS_ERR(st->vref))
-		regulator_disable(st->vref);
-	if (!IS_ERR(st->reg))
-		regulator_disable(st->reg);
+	regulator_disable(st->vref);
+	regulator_disable(st->reg);
 	kfree(st->rx_buf);
 
 	return 0;

Hartmut Knaack

Acked-by on v2 1/1

2014-04-30 21:52 UTC #4 lore raw

Re: [patch v2] [patch] staging: iio: ad799x: remove some unneeded IS_ERR() checks

Quoted 8 lines · Dan Carpenter

My static checker is upset that we check IS_ERR(t->reg) when we know it is not an ERR_PTR.
Checking for IS_ERR() twice is often a sign of confusion and buggy code. In this case, if the call to "ret = regulator_enable(st->vref);" fails, then we call "regulator_disable(st->vref);" and that's a mistake because "st->vref" is not enabled.

Quoted 4 lines

I fixed these problems and Hartmut Knaack pointed out a couple unneeded IS_ERR() checks in ad799x_remove() so I have removed those as well.
Signed-off-by Dan Carpenter <[email protected]>

Acked-by Hartmut Knaack <[email protected]>

Quoted 50 lines

---
v2: remove the unneeded checks in ad799x_remove() as well. Updated the
commit message to say that it's actually a small bug fix.

drivers/iio/adc/ad799x.c +6 ~~-10~~

index 16a8b14..39b4cb4 100644
@@ -717,7 +717,7 @@ static int ad799x_probe(struct i2c_client *client,
 	ret = iio_triggered_buffer_setup(indio_dev, NULL,
 		&ad799x_trigger_handler, NULL);
 	if (ret)
-		goto error_disable_reg;
+		goto error_disable_vref;
 
 	if (client->irq > 0) {
 		ret = devm_request_threaded_irq(&client->dev,

@@ -739,11 +739,10 @@ static int ad799x_probe(struct i2c_client *client,
 
 error_cleanup_ring:
 	iio_triggered_buffer_cleanup(indio_dev);
+error_disable_vref:
+	regulator_disable(st->vref);
 error_disable_reg:
-	if (!IS_ERR(st->vref))
-		regulator_disable(st->vref);
-	if (!IS_ERR(st->reg))
-		regulator_disable(st->reg);
+	regulator_disable(st->reg);
 
 	return ret;
 }

@@ -756,10 +755,8 @@ static int ad799x_remove(struct i2c_client *client)
 	iio_device_unregister(indio_dev);
 
 	iio_triggered_buffer_cleanup(indio_dev);
-	if (!IS_ERR(st->vref))
-		regulator_disable(st->vref);
-	if (!IS_ERR(st->reg))
-		regulator_disable(st->reg);
+	regulator_disable(st->vref);
+	regulator_disable(st->reg);
 	kfree(st->rx_buf);
 
 	return 0;
--

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 Applied

2014-05-03 10:15 UTC #5 lore raw

Re: [patch v2] [patch] staging: iio: ad799x: remove some unneeded IS_ERR() checks

Quoted 9 lines · Hartmut Knaack

Quoted 8 lines · Dan Carpenter
My static checker is upset that we check IS_ERR(t->reg) when we know it is not an ERR_PTR.
Checking for IS_ERR() twice is often a sign of confusion and buggy code. In this case, if the call to "ret = regulator_enable(st->vref);" fails, then we call "regulator_disable(st->vref);" and that's a mistake because "st->vref" is not enabled.

Quoted 5 lines

Quoted 4 lines
I fixed these problems and Hartmut Knaack pointed out a couple unneeded IS_ERR() checks in ad799x_remove() so I have removed those as well.
Signed-off-by Dan Carpenter <[email protected]>
Acked-by Hartmut Knaack <[email protected]>

Thanks to both of you for this.

Applied to the togreg branch of iio.git as the cleanup that caused this was most likely part of a series that is only in staging-next at the moment.

Jonathan

Quoted 51 lines

Quoted 50 lines

---
v2: remove the unneeded checks in ad799x_remove() as well. Updated the
commit message to say that it's actually a small bug fix.

drivers/iio/adc/ad799x.c +6 ~~-10~~

index 16a8b14..39b4cb4 100644
@@ -717,7 +717,7 @@ static int ad799x_probe(struct i2c_client *client,
  	ret = iio_triggered_buffer_setup(indio_dev, NULL,
  		&ad799x_trigger_handler, NULL);
  	if (ret)
-		goto error_disable_reg;
+		goto error_disable_vref;
 
  	if (client->irq > 0) {
  		ret = devm_request_threaded_irq(&client->dev,

@@ -739,11 +739,10 @@ static int ad799x_probe(struct i2c_client *client,
 
  error_cleanup_ring:
  	iio_triggered_buffer_cleanup(indio_dev);
+error_disable_vref:
+	regulator_disable(st->vref);
  error_disable_reg:
-	if (!IS_ERR(st->vref))
-		regulator_disable(st->vref);
-	if (!IS_ERR(st->reg))
-		regulator_disable(st->reg);
+	regulator_disable(st->reg);
 
  	return ret;
  }

@@ -756,10 +755,8 @@ static int ad799x_remove(struct i2c_client *client)
  	iio_device_unregister(indio_dev);
 
  	iio_triggered_buffer_cleanup(indio_dev);
-	if (!IS_ERR(st->vref))
-		regulator_disable(st->vref);
-	if (!IS_ERR(st->reg))
-		regulator_disable(st->reg);
+	regulator_disable(st->vref);
+	regulator_disable(st->reg);
  	kfree(st->rx_buf);
 
  	return 0;
--

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