[PATCH]staging:iio:ad799x use devm_request_threaded_irq

[PATCH]staging:iio:ad799x use devm_request_threaded_irq

[Hartmut Knaack]

Move to devm_request_threaded_irq to make device-removal easier, and also fix
error_free_irq, which did not check if an irq was requested at all.

Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
---
diff --git a/drivers/staging/iio/adc/ad799x_core.c b/drivers/staging/iio/adc/ad799x_core.c
index e32a555..bc886f6 100644
--- a/drivers/staging/iio/adc/ad799x_core.c
+++ b/drivers/staging/iio/adc/ad799x_core.c
@@ -573,24 +573,23 @@ static int ad799x_probe(struct i2c_client *client,
 		goto error_disable_reg;
 
 	if (client->irq > 0) {
-		ret = request_threaded_irq(client->irq,
-					   NULL,
-					   ad799x_event_handler,
-					   IRQF_TRIGGER_FALLING |
-					   IRQF_ONESHOT,
-					   client->name,
-					   indio_dev);
+		ret = devm_request_threaded_irq(&client->dev,
+						client->irq,
+						NULL,
+						ad799x_event_handler,
+						IRQF_TRIGGER_FALLING |
+						IRQF_ONESHOT,
+						client->name,
+						indio_dev);
 		if (ret)
 			goto error_cleanup_ring;
 	}
 	ret = iio_device_register(indio_dev);
 	if (ret)
-		goto error_free_irq;
+		goto error_cleanup_ring;
 
 	return 0;
 
-error_free_irq:
-	free_irq(client->irq, indio_dev);
 error_cleanup_ring:
 	ad799x_ring_cleanup(indio_dev);
 error_disable_reg:
@@ -606,8 +605,6 @@ static int ad799x_remove(struct i2c_client *client)
 	struct ad799x_state *st = iio_priv(indio_dev);
 
 	iio_device_unregister(indio_dev);
-	if (client->irq > 0)
-		free_irq(client->irq, indio_dev);
 
 	ad799x_ring_cleanup(indio_dev);
 	if (!IS_ERR(st->reg))

Re: [PATCH]staging:iio:ad799x use devm_request_threaded_irq

[Jonathan Cameron]

On 29/12/13 11:51, Hartmut Knaack wrote:
> Move to devm_request_threaded_irq to make device-removal easier, and also fix
> error_free_irq, which did not check if an irq was requested at all.
>
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
Hi Hartmut,

Please break this into two patches - the first fixing the irq check 
(whilst it will go away in the second patch - we will probably want to 
push the fix to stable, and can't do that with the other change in here).

On the devm_request_threaded_irq usage, this is often considered 
dangerous as it changes the ordering of resource freeing so that it
doesn't match that of allocation.  Here I think you are fine, but
haven't looked into that closely yet.

Lars, do you think this is a good idea?
(any chance you are planning to get this driver out of staging any time 
soon? :)
> ---
> diff --git a/drivers/staging/iio/adc/ad799x_core.c b/drivers/staging/iio/adc/ad799x_core.c
> index e32a555..bc886f6 100644
> --- a/drivers/staging/iio/adc/ad799x_core.c
> +++ b/drivers/staging/iio/adc/ad799x_core.c
> @@ -573,24 +573,23 @@ static int ad799x_probe(struct i2c_client *client,
>   		goto error_disable_reg;
>
>   	if (client->irq > 0) {
> -		ret = request_threaded_irq(client->irq,
> -					   NULL,
> -					   ad799x_event_handler,
> -					   IRQF_TRIGGER_FALLING |
> -					   IRQF_ONESHOT,
> -					   client->name,
> -					   indio_dev);
> +		ret = devm_request_threaded_irq(&client->dev,
> +						client->irq,
> +						NULL,
> +						ad799x_event_handler,
> +						IRQF_TRIGGER_FALLING |
> +						IRQF_ONESHOT,
> +						client->name,
> +						indio_dev);
>   		if (ret)
>   			goto error_cleanup_ring;
>   	}
>   	ret = iio_device_register(indio_dev);
>   	if (ret)
> -		goto error_free_irq;
> +		goto error_cleanup_ring;
>
>   	return 0;
>
> -error_free_irq:
> -	free_irq(client->irq, indio_dev);
>   error_cleanup_ring:
>   	ad799x_ring_cleanup(indio_dev);
>   error_disable_reg:
> @@ -606,8 +605,6 @@ static int ad799x_remove(struct i2c_client *client)
>   	struct ad799x_state *st = iio_priv(indio_dev);
>
>   	iio_device_unregister(indio_dev);
> -	if (client->irq > 0)
> -		free_irq(client->irq, indio_dev);
>
>   	ad799x_ring_cleanup(indio_dev);
>   	if (!IS_ERR(st->reg))
> --
> 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
>