Staging: iio: meter: Use devm functions

PATCH In review Shraddha Barke 3 participants 5 messages Oct 4, 2015

Subsystems (4)

Staging IIO Analog Devices IIO drivers Staging - industrial io

lore

Share & tools

lore

Shraddha Barke

author

2015-10-04 04:34 UTC #0 lore raw

PATCH 1/1 Staging: iio: meter: Use devm functions

Introduce use of managed resource function devm_iio_trigger_alloc instead of iio_trigger_alloc and devm_request_irq instead of request_irq Remove corresponding calls to iio_trigger_free and free_irq in the probe and remove functions.
The now unnecessary labels error_free_trig and err_free_irq are dropped.

Signed-off-by Shraddha Barke <[email protected]>

---

 drivers/staging/iio/meter/ade7758_trigger.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

drivers/staging/iio/meter/ade7758_trigger.c +10 ~~-16~~

index 5b35a7f..e313b37 100644
@@ -63,21 +63,21 @@ int ade7758_probe_trigger(struct iio_dev *indio_dev)
 	struct ade7758_state *st = iio_priv(indio_dev);
 	int ret;
 
-	st->trig = iio_trigger_alloc("%s-dev%d",
-					spi_get_device_id(st->us)->name,
-					indio_dev->id);
+	st->trig = devm_iio_trigger_alloc(&indio_dev->id, "%s-dev%d",
+					  spi_get_device_id(st->us)->name,
+					  indio_dev->id);
 	if (!st->trig) {
 		ret = -ENOMEM;
 		goto error_ret;
 	}
 
-	ret = request_irq(st->us->irq,
-			  ade7758_data_rdy_trig_poll,
-			  IRQF_TRIGGER_LOW,
-			  spi_get_device_id(st->us)->name,
-			  st->trig);
+	ret = devm_request_irq(&indio_dev->dev, st->us->irq,
+			       ade7758_data_rdy_trig_poll,
+			       IRQF_TRIGGER_LOW,
+			       spi_get_device_id(st->us)->name,
+			       st->trig);
 	if (ret)
-		goto error_free_trig;
+		goto error_ret
 
 	st->trig->dev.parent = &st->us->dev;
 	st->trig->ops = &ade7758_trigger_ops;

@@ -87,14 +87,10 @@ int ade7758_probe_trigger(struct iio_dev *indio_dev)
 	/* select default trigger */
 	indio_dev->trig = iio_trigger_get(st->trig);
 	if (ret)
-		goto error_free_irq;
+		goto error_ret;
 
 	return 0;
 
-error_free_irq:
-	free_irq(st->us->irq, st->trig);
-error_free_trig:
-	iio_trigger_free(st->trig);
 error_ret:
 	return ret;
 }

@@ -104,6 +100,4 @@ void ade7758_remove_trigger(struct iio_dev *indio_dev)
 	struct ade7758_state *st = iio_priv(indio_dev);
 
 	iio_trigger_unregister(st->trig);
-	free_irq(st->us->irq, st->trig);
-	iio_trigger_free(st->trig);
 }

Signature · 1 line

2.1.4

Lars-Peter Clausen

2015-10-04 12:04 UTC #1 lore raw

Quoted 1 line · Shraddha Barke

[...]

Hi,

Thanks for the patch.

drivers/staging/iio/meter/ade7758_trigger.c int ade7758_probe_trigger(struct iio_dev *indio_dev) · line 66

Quoted 4 lines

drivers/staging/iio/meter/ade7758_trigger.c +1 -3

-	st->trig = iio_trigger_alloc("%s-dev%d",
-					spi_get_device_id(st->us)->name,
-					indio_dev->id);
+	st->trig = devm_iio_trigger_alloc(&indio_dev->id, "%s-dev%d",

But I don't think this compiles. The first parameter needs to be a struct device.

drivers/staging/iio/meter/ade7758_trigger.c int ade7758_probe_trigger(struct iio_dev *indio_dev) · line 74

Quoted 13 lines

drivers/staging/iio/meter/ade7758_trigger.c +3 -5

+					  spi_get_device_id(st->us)->name,
+					  indio_dev->id);
 	if (!st->trig) {
 		ret = -ENOMEM;
 		goto error_ret;
 	}
 
-	ret = request_irq(st->us->irq,
-			  ade7758_data_rdy_trig_poll,
-			  IRQF_TRIGGER_LOW,
-			  spi_get_device_id(st->us)->name,
-			  st->trig);
+	ret = devm_request_irq(&indio_dev->dev, st->us->irq,

And the device also needs to be the device from whose probe function this is called. Please add a parameter to ade7758_probe_trigger that supplies this device.

Quoted 4 lines

drivers/staging/iio/meter/ade7758_trigger.c +4 -0

+			       ade7758_data_rdy_trig_poll,
+			       IRQF_TRIGGER_LOW,
+			       spi_get_device_id(st->us)->name,
+			       st->trig);

[...]

Jonathan Cameron

maintainer

2015-10-04 12:06 UTC #2 lore raw

In reply to Shraddha Barke

Quoted 52 lines · Shraddha Barke

Signed-off-by Shraddha Barke <[email protected]>

---

 drivers/staging/iio/meter/ade7758_trigger.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

drivers/staging/iio/meter/ade7758_trigger.c +9 ~~-10~~

index 5b35a7f..e313b37 100644
@@ -63,21 +63,21 @@ int ade7758_probe_trigger(struct iio_dev *indio_dev)
 	struct ade7758_state *st = iio_priv(indio_dev);
 	int ret;
 
-	st->trig = iio_trigger_alloc("%s-dev%d",
-					spi_get_device_id(st->us)->name,
-					indio_dev->id);
+	st->trig = devm_iio_trigger_alloc(&indio_dev->id, "%s-dev%d",
+					  spi_get_device_id(st->us)->name,
+					  indio_dev->id);
 	if (!st->trig) {
 		ret = -ENOMEM;
 		goto error_ret;
 	}
 
-	ret = request_irq(st->us->irq,
-			  ade7758_data_rdy_trig_poll,
-			  IRQF_TRIGGER_LOW,
-			  spi_get_device_id(st->us)->name,
-			  st->trig);
+	ret = devm_request_irq(&indio_dev->dev, st->us->irq,
+			       ade7758_data_rdy_trig_poll,
+			       IRQF_TRIGGER_LOW,
+			       spi_get_device_id(st->us)->name,
+			       st->trig);
 	if (ret)
-		goto error_free_trig;
+		goto error_ret
 
 	st->trig->dev.parent = &st->us->dev;
 	st->trig->ops = &ade7758_trigger_ops;

@@ -87,14 +87,10 @@ int ade7758_probe_trigger(struct iio_dev *indio_dev)
 	/* select default trigger */
 	indio_dev->trig = iio_trigger_get(st->trig);
 	if (ret)
-		goto error_free_irq;

drivers/staging/iio/meter/ade7758_trigger.c int ade7758_probe_trigger(struct iio_dev *indio_dev) · line 95

Quoted 10 lines

drivers/staging/iio/meter/ade7758_trigger.c +1 -4

+		goto error_ret;
 
 	return 0;
 
-error_free_irq:
-	free_irq(st->us->irq, st->trig);
-error_free_trig:
-	iio_trigger_free(st->trig);
 error_ret:
 	return ret;

Whilst you are here, direct returns are preferred when there is no unwinding to do so if you could just return the error from anywhere it does the goto error_ret that would be great!

Thanks,

Jonathan

Quoted 9 lines

drivers/staging/iio/meter/ade7758_trigger.c +0 -2

@@ -104,6 +100,4 @@ void ade7758_remove_trigger(struct iio_dev *indio_dev)
 	struct ade7758_state *st = iio_priv(indio_dev);
 
 	iio_trigger_unregister(st->trig);
-	free_irq(st->us->irq, st->trig);
-	iio_trigger_free(st->trig);
 }

Shraddha Barke

author

2015-10-04 12:44 UTC #3 lore raw

Quoted 57 lines · Jonathan Cameron

Quoted 56 lines · Shraddha Barke

Signed-off-by Shraddha Barke <[email protected]>

---

 drivers/staging/iio/meter/ade7758_trigger.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

drivers/staging/iio/meter/ade7758_trigger.c +10 ~~-10~~

index 5b35a7f..e313b37 100644
@@ -63,21 +63,21 @@ int ade7758_probe_trigger(struct iio_dev *indio_dev)
 	struct ade7758_state *st = iio_priv(indio_dev);
 	int ret;
 
-	st->trig = iio_trigger_alloc("%s-dev%d",
-					spi_get_device_id(st->us)->name,
-					indio_dev->id);
+	st->trig = devm_iio_trigger_alloc(&indio_dev->id, "%s-dev%d",
+					  spi_get_device_id(st->us)->name,
+					  indio_dev->id);
 	if (!st->trig) {
 		ret = -ENOMEM;
 		goto error_ret;
 	}
 
-	ret = request_irq(st->us->irq,
-			  ade7758_data_rdy_trig_poll,
-			  IRQF_TRIGGER_LOW,
-			  spi_get_device_id(st->us)->name,
-			  st->trig);
+	ret = devm_request_irq(&indio_dev->dev, st->us->irq,
+			       ade7758_data_rdy_trig_poll,
+			       IRQF_TRIGGER_LOW,
+			       spi_get_device_id(st->us)->name,
+			       st->trig);
 	if (ret)
-		goto error_free_trig;
+		goto error_ret
 
 	st->trig->dev.parent = &st->us->dev;
 	st->trig->ops = &ade7758_trigger_ops;

@@ -87,14 +87,10 @@ int ade7758_probe_trigger(struct iio_dev *indio_dev)
 	/* select default trigger */
 	indio_dev->trig = iio_trigger_get(st->trig);
 	if (ret)
-		goto error_free_irq;
+		goto error_ret;
 
 	return 0;

Quoted 9 lines

drivers/staging/iio/meter/ade7758_trigger.c int ade7758_probe_trigger(struct iio_dev *indio_dev) · line 95
Quoted 6 lines
drivers/staging/iio/meter/ade7758_trigger.c +0 -4
-error_free_irq:
-	free_irq(st->us->irq, st->trig);
-error_free_trig:
-	iio_trigger_free(st->trig);
 error_ret:
 	return ret;
Whilst you are here, direct returns are preferred when there is no unwinding to do so if you could just return the error from anywhere it does the goto error_ret that would be great!

I was a little skeptical about the change from request_irq to devm_request_irq because I'm not sure whether the things that are freed by the function are needed by the interrupt handler. Do you think it's correct?

Shraddha

Quoted 15 lines

Thanks,

Jonathan

Quoted 9 lines

drivers/staging/iio/meter/ade7758_trigger.c +0 -2

@@ -104,6 +100,4 @@ void ade7758_remove_trigger(struct iio_dev *indio_dev)
 	struct ade7758_state *st = iio_priv(indio_dev);
 
 	iio_trigger_unregister(st->trig);
-	free_irq(st->us->irq, st->trig);
-	iio_trigger_free(st->trig);
 }

Jonathan Cameron

maintainer

2015-10-04 14:35 UTC #4 lore raw

Quoted 71 lines · Shraddha Barke

Quoted 57 lines · Jonathan Cameron

Quoted 56 lines · Shraddha Barke

Signed-off-by Shraddha Barke <[email protected]>

---

 drivers/staging/iio/meter/ade7758_trigger.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

drivers/staging/iio/meter/ade7758_trigger.c +10 ~~-10~~

index 5b35a7f..e313b37 100644
@@ -63,21 +63,21 @@ int ade7758_probe_trigger(struct iio_dev *indio_dev)
     struct ade7758_state *st = iio_priv(indio_dev);
     int ret;
 
-    st->trig = iio_trigger_alloc("%s-dev%d",
-                    spi_get_device_id(st->us)->name,
-                    indio_dev->id);
+    st->trig = devm_iio_trigger_alloc(&indio_dev->id, "%s-dev%d",
+                      spi_get_device_id(st->us)->name,
+                      indio_dev->id);
     if (!st->trig) {
         ret = -ENOMEM;
         goto error_ret;
     }
 
-    ret = request_irq(st->us->irq,
-              ade7758_data_rdy_trig_poll,
-              IRQF_TRIGGER_LOW,
-              spi_get_device_id(st->us)->name,
-              st->trig);
+    ret = devm_request_irq(&indio_dev->dev, st->us->irq,
+                   ade7758_data_rdy_trig_poll,
+                   IRQF_TRIGGER_LOW,
+                   spi_get_device_id(st->us)->name,
+                   st->trig);
     if (ret)
-        goto error_free_trig;
+        goto error_ret
 
     st->trig->dev.parent = &st->us->dev;
     st->trig->ops = &ade7758_trigger_ops;

@@ -87,14 +87,10 @@ int ade7758_probe_trigger(struct iio_dev *indio_dev)
     /* select default trigger */
     indio_dev->trig = iio_trigger_get(st->trig);
     if (ret)
-        goto error_free_irq;
+        goto error_ret;
 
     return 0;

Quoted 9 lines

drivers/staging/iio/meter/ade7758_trigger.c int ade7758_probe_trigger(struct iio_dev *indio_dev) · line 95
Quoted 6 lines
drivers/staging/iio/meter/ade7758_trigger.c +0 -4
-error_free_irq:
-    free_irq(st->us->irq, st->trig);
-error_free_trig:
-    iio_trigger_free(st->trig);
 error_ret:
     return ret;
Whilst you are here, direct returns are preferred when there is no unwinding to do so if you could just return the error from anywhere it does the goto error_ret that would be great!

Quoted 2 lines

I was a little skeptical about the change from request_irq to devm_request_irq because I'm not sure whether the things that are freed by the function are needed by the interrupt handler. Do you think it's correct?

Good point. It's a dangerous corner as there is a whole load of stuff in the _core.c file remove which will move from after to before this call. I'd actually be a bit dubious about using any devm stuff in here without working on that file a touch first.

The st->tx, st->rx allocations could be trivially made devm as well. (be careful here as there are two lots of rx and tx buffers so that the other lot can be left intact to do the ring reads nice and quickly whatever other reads and writes occur).

The unconfigure_ring does a bit more and is the sticky issue.

I'd leave the whole lot as non devm functions as it meets the 'obviously correct' requirement and the devm equivalent does not...

Still worth cleaning up those goto error_ret bits though!

Jonathan

Quoted 21 lines

Shraddha

Quoted 15 lines

Thanks,

Jonathan

Quoted 9 lines

drivers/staging/iio/meter/ade7758_trigger.c +0 -2

@@ -104,6 +100,4 @@ void ade7758_remove_trigger(struct iio_dev *indio_dev)
     struct ade7758_state *st = iio_priv(indio_dev);
 
     iio_trigger_unregister(st->trig);
-    free_irq(st->us->irq, st->trig);
-    iio_trigger_free(st->trig);
 }

Signature · 3 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