[PATCH 0/7] iio: Use IRQF_NO_AUTOEN

[PATCH 0/7] iio: Use IRQF_NO_AUTOEN

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

This series is dependant on
cbe16f35bee68 ("genirq: Add IRQF_NO_AUTOEN for request_irq/nmi()")
which is available in an immutable tag in the tip tree.
https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/tag/?h=irq-no-autoen-2021-03-25
which I'll merge in to the IIO tree if we need it before it's available
upstream.

That patch introduces a new IRQF_NO_AUTOEN flag for irq requests to avoid
the current dance where we either mark an irq as not to be autoenabled before
we know if we can actually request it succesfully, or (as IIO drivers seem to
have gone with) we disable the interrupt just after requesting it.
In short the flag stops the interrupt being autoenabled in the first place.

So this series applies this magic to IIO :)

Note these are all just compile tested and some of them aren't entirely
trivial because of other aspects of the irq flag handling.

Jonathan Cameron (7):
  iio:adc:ad7766: Use new IRQF_NO_AUTOEN to reduce boilerplate
  iio:adc:exynos-adc: Use new IRQF_NO_AUTOEN flag rather than separate
    irq_disable()
  iio:adc:nau7802: Use IRQF_NO_AUTOEN instead of request then disable
  iio:adc:sun4i-gpadc: Use new IRQF_NO_AUTOEN flag instead of request
    then disable
  iio:chemical:scd30: Use IRQF_NO_AUTOEN to avoid irq request then
    disable
  iio:imu:adis: Use IRQF_NO_AUTOEN instead of irq request then disable
  iio:adc:ad_sigma_delta: Use IRQF_NO_AUTOEN rather than request and
    disable

 drivers/iio/adc/ad7766.c          | 15 +++++++--------
 drivers/iio/adc/ad_sigma_delta.c  |  7 ++-----
 drivers/iio/adc/exynos_adc.c      |  4 ++--
 drivers/iio/adc/nau7802.c         |  6 +++---
 drivers/iio/adc/sun4i-gpadc-iio.c |  4 ++--
 drivers/iio/chemical/scd30_core.c | 16 ++++++++--------
 drivers/iio/imu/adis16460.c       |  4 ++--
 drivers/iio/imu/adis16475.c       |  5 +++--
 drivers/iio/imu/adis_trigger.c    | 11 ++++++-----
 9 files changed, 35 insertions(+), 37 deletions(-)

-- 
2.31.1

[PATCH 1/7] iio:adc:ad7766: Use new IRQF_NO_AUTOEN to reduce boilerplate

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

As iio_poll_trigger() is safe against spurious interrupts when the
trigger is not enabled, this is not a fix despite looking like
a race.  It is nice to simplify the code however so the interrupt
is never enabled in the first place.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/iio/adc/ad7766.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/adc/ad7766.c b/drivers/iio/adc/ad7766.c
index 829a3426f235..1e41759f3ee5 100644
--- a/drivers/iio/adc/ad7766.c
+++ b/drivers/iio/adc/ad7766.c
@@ -255,18 +255,17 @@ static int ad7766_probe(struct spi_device *spi)
 		ad7766->trig->ops = &ad7766_trigger_ops;
 		iio_trigger_set_drvdata(ad7766->trig, ad7766);
 
-		ret = devm_request_irq(&spi->dev, spi->irq, ad7766_irq,
-			IRQF_TRIGGER_FALLING, dev_name(&spi->dev),
-			ad7766->trig);
-		if (ret < 0)
-			return ret;
-
 		/*
 		 * The device generates interrupts as long as it is powered up.
 		 * Some platforms might not allow the option to power it down so
-		 * disable the interrupt to avoid extra load on the system
+		 * don't enable the interrupt to avoid extra load on the system
 		 */
-		disable_irq(spi->irq);
+		ret = devm_request_irq(&spi->dev, spi->irq, ad7766_irq,
+				       IRQF_TRIGGER_FALLING | IRQF_NO_AUTOEN,
+				       dev_name(&spi->dev),
+				       ad7766->trig);
+		if (ret < 0)
+			return ret;
 
 		ret = devm_iio_trigger_register(&spi->dev, ad7766->trig);
 		if (ret)
-- 
2.31.1

RE: [PATCH 1/7] iio:adc:ad7766: Use new IRQF_NO_AUTOEN to reduce boilerplate

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Jonathan Cameron [mailto:jic23@kernel.org]
> Sent: Saturday, April 3, 2021 7:46 AM
> To: linux-iio@vger.kernel.org
> Cc: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>; Lars-Peter Clausen <lars@metafoo.de>
> Subject: [PATCH 1/7] iio:adc:ad7766: Use new IRQF_NO_AUTOEN to reduce
> boilerplate
> 
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> As iio_poll_trigger() is safe against spurious interrupts when the
> trigger is not enabled, this is not a fix despite looking like
> a race.  It is nice to simplify the code however so the interrupt
> is never enabled in the first place.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Lars-Peter Clausen <lars@metafoo.de>
> ---

Reviewed-by: Barry Song <song.bao.hua@hisilicon.com>

BTW, +Cc Tiantao as Tao might be moving drivers to
use IRQF_NO_AUTOEN.


>  drivers/iio/adc/ad7766.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7766.c b/drivers/iio/adc/ad7766.c
> index 829a3426f235..1e41759f3ee5 100644
> --- a/drivers/iio/adc/ad7766.c
> +++ b/drivers/iio/adc/ad7766.c
> @@ -255,18 +255,17 @@ static int ad7766_probe(struct spi_device *spi)
>  		ad7766->trig->ops = &ad7766_trigger_ops;
>  		iio_trigger_set_drvdata(ad7766->trig, ad7766);
> 
> -		ret = devm_request_irq(&spi->dev, spi->irq, ad7766_irq,
> -			IRQF_TRIGGER_FALLING, dev_name(&spi->dev),
> -			ad7766->trig);
> -		if (ret < 0)
> -			return ret;
> -
>  		/*
>  		 * The device generates interrupts as long as it is powered up.
>  		 * Some platforms might not allow the option to power it down so
> -		 * disable the interrupt to avoid extra load on the system
> +		 * don't enable the interrupt to avoid extra load on the system
>  		 */
> -		disable_irq(spi->irq);
> +		ret = devm_request_irq(&spi->dev, spi->irq, ad7766_irq,
> +				       IRQF_TRIGGER_FALLING | IRQF_NO_AUTOEN,
> +				       dev_name(&spi->dev),
> +				       ad7766->trig);
> +		if (ret < 0)
> +			return ret;
> 
>  		ret = devm_iio_trigger_register(&spi->dev, ad7766->trig);
>  		if (ret)
> --
> 2.31.1

Re: [PATCH 1/7] iio:adc:ad7766: Use new IRQF_NO_AUTOEN to reduce boilerplate

[Jonathan Cameron]

On Fri, 2 Apr 2021 20:10:38 +0000
"Song Bao Hua (Barry Song)" <song.bao.hua@hisilicon.com> wrote:

> > -----Original Message-----
> > From: Jonathan Cameron [mailto:jic23@kernel.org]
> > Sent: Saturday, April 3, 2021 7:46 AM
> > To: linux-iio@vger.kernel.org
> > Cc: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; Jonathan Cameron
> > <jonathan.cameron@huawei.com>; Lars-Peter Clausen <lars@metafoo.de>
> > Subject: [PATCH 1/7] iio:adc:ad7766: Use new IRQF_NO_AUTOEN to reduce
> > boilerplate
> > 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > As iio_poll_trigger() is safe against spurious interrupts when the
> > trigger is not enabled, this is not a fix despite looking like
> > a race.  It is nice to simplify the code however so the interrupt
> > is never enabled in the first place.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Lars-Peter Clausen <lars@metafoo.de>
> > ---  
> 
> Reviewed-by: Barry Song <song.bao.hua@hisilicon.com>
Applied to the togreg branch of iio.git and pushed out as testing to
let the autobuilders see if they can find anything we missed.

Thanks,

Jonathan

> 
> BTW, +Cc Tiantao as Tao might be moving drivers to
> use IRQF_NO_AUTOEN.
> 
> 
> >  drivers/iio/adc/ad7766.c | 15 +++++++--------
> >  1 file changed, 7 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad7766.c b/drivers/iio/adc/ad7766.c
> > index 829a3426f235..1e41759f3ee5 100644
> > --- a/drivers/iio/adc/ad7766.c
> > +++ b/drivers/iio/adc/ad7766.c
> > @@ -255,18 +255,17 @@ static int ad7766_probe(struct spi_device *spi)
> >  		ad7766->trig->ops = &ad7766_trigger_ops;
> >  		iio_trigger_set_drvdata(ad7766->trig, ad7766);
> > 
> > -		ret = devm_request_irq(&spi->dev, spi->irq, ad7766_irq,
> > -			IRQF_TRIGGER_FALLING, dev_name(&spi->dev),
> > -			ad7766->trig);
> > -		if (ret < 0)
> > -			return ret;
> > -
> >  		/*
> >  		 * The device generates interrupts as long as it is powered up.
> >  		 * Some platforms might not allow the option to power it down so
> > -		 * disable the interrupt to avoid extra load on the system
> > +		 * don't enable the interrupt to avoid extra load on the system
> >  		 */
> > -		disable_irq(spi->irq);
> > +		ret = devm_request_irq(&spi->dev, spi->irq, ad7766_irq,
> > +				       IRQF_TRIGGER_FALLING | IRQF_NO_AUTOEN,
> > +				       dev_name(&spi->dev),
> > +				       ad7766->trig);
> > +		if (ret < 0)
> > +			return ret;
> > 
> >  		ret = devm_iio_trigger_register(&spi->dev, ad7766->trig);
> >  		if (ret)
> > --
> > 2.31.1  
> 

[PATCH 2/7] iio:adc:exynos-adc: Use new IRQF_NO_AUTOEN flag rather than separate irq_disable()

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Disabling an irq before the driver has actually atempted to request it
may work, but is definitely not as clean as just requesting it as
normal but with the auto enable disabled.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/iio/adc/exynos_adc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
index 784c10deeb1a..8c98d8c9ab1f 100644
--- a/drivers/iio/adc/exynos_adc.c
+++ b/drivers/iio/adc/exynos_adc.c
@@ -778,9 +778,9 @@ static int exynos_adc_ts_init(struct exynos_adc *info)
 		return ret;
 	}
 
-	disable_irq(info->tsirq);
 	ret = request_threaded_irq(info->tsirq, NULL, exynos_ts_isr,
-				   IRQF_ONESHOT, "touchscreen", info);
+				   IRQF_ONESHOT | IRQF_NO_AUTOEN,
+				   "touchscreen", info);
 	if (ret)
 		input_unregister_device(info->input);
 
-- 
2.31.1

RE: [PATCH 2/7] iio:adc:exynos-adc: Use new IRQF_NO_AUTOEN flag rather than separate irq_disable()

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Jonathan Cameron [mailto:jic23@kernel.org]
> Sent: Saturday, April 3, 2021 7:46 AM
> To: linux-iio@vger.kernel.org
> Cc: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>; Krzysztof Kozlowski <krzk@kernel.org>
> Subject: [PATCH 2/7] iio:adc:exynos-adc: Use new IRQF_NO_AUTOEN flag rather
> than separate irq_disable()
> 
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Disabling an irq before the driver has actually atempted to request it
> may work, but is definitely not as clean as just requesting it as
> normal but with the auto enable disabled.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Krzysztof Kozlowski <krzk@kernel.org>
> ---
>  drivers/iio/adc/exynos_adc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 

Reviewed-by: Barry Song <song.bao.hua@hisilicon.com>

> diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
> index 784c10deeb1a..8c98d8c9ab1f 100644
> --- a/drivers/iio/adc/exynos_adc.c
> +++ b/drivers/iio/adc/exynos_adc.c
> @@ -778,9 +778,9 @@ static int exynos_adc_ts_init(struct exynos_adc *info)
>  		return ret;
>  	}
> 
> -	disable_irq(info->tsirq);
>  	ret = request_threaded_irq(info->tsirq, NULL, exynos_ts_isr,
> -				   IRQF_ONESHOT, "touchscreen", info);
> +				   IRQF_ONESHOT | IRQF_NO_AUTOEN,
> +				   "touchscreen", info);
>  	if (ret)
>  		input_unregister_device(info->input);
> 
> --
> 2.31.1

Re: [PATCH 2/7] iio:adc:exynos-adc: Use new IRQF_NO_AUTOEN flag rather than separate irq_disable()

[Krzysztof Kozlowski]

On 02/04/2021 20:45, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Disabling an irq before the driver has actually atempted to request it
> may work, but is definitely not as clean as just requesting it as
> normal but with the auto enable disabled.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Krzysztof Kozlowski <krzk@kernel.org>



Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>

Best regards,
Krzysztof

Re: [PATCH 2/7] iio:adc:exynos-adc: Use new IRQF_NO_AUTOEN flag rather than separate irq_disable()

[Jonathan Cameron]

On Sun, 4 Apr 2021 21:20:21 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote:

> On 02/04/2021 20:45, Jonathan Cameron wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > Disabling an irq before the driver has actually atempted to request it
> > may work, but is definitely not as clean as just requesting it as
> > normal but with the auto enable disabled.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Krzysztof Kozlowski <krzk@kernel.org>  
> 
> 
> 
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> 
> Best regards,
> Krzysztof

Applied to the togreg branch of iio.git and pushed out as testing
to let the autobuilders poke at it.

Thanks,

Jonathan

[PATCH 3/7] iio:adc:nau7802: Use IRQF_NO_AUTOEN instead of request then disable

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Whilst a race during interrupt enabling is probably not a problem,
it is better to not enable the interrupt at all.  The new
IRQF_NO_AUTOEN flag allows us to do that.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 drivers/iio/adc/nau7802.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/nau7802.c b/drivers/iio/adc/nau7802.c
index 07c85434b568..bb70b51d25b1 100644
--- a/drivers/iio/adc/nau7802.c
+++ b/drivers/iio/adc/nau7802.c
@@ -498,7 +498,8 @@ static int nau7802_probe(struct i2c_client *client,
 		ret = request_threaded_irq(client->irq,
 				NULL,
 				nau7802_eoc_trigger,
-				IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
+				IRQF_TRIGGER_HIGH | IRQF_ONESHOT |
+				IRQF_NO_AUTOEN,
 				client->dev.driver->name,
 				indio_dev);
 		if (ret) {
@@ -513,8 +514,7 @@ static int nau7802_probe(struct i2c_client *client,
 			dev_info(&client->dev,
 				"Failed to allocate IRQ, using polling mode\n");
 			client->irq = 0;
-		} else
-			disable_irq(client->irq);
+		}
 	}
 
 	if (!client->irq) {
-- 
2.31.1

RE: [PATCH 3/7] iio:adc:nau7802: Use IRQF_NO_AUTOEN instead of request then disable

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Jonathan Cameron [mailto:jic23@kernel.org]
> Sent: Saturday, April 3, 2021 7:46 AM
> To: linux-iio@vger.kernel.org
> Cc: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>; Alexandre Belloni
> <alexandre.belloni@bootlin.com>
> Subject: [PATCH 3/7] iio:adc:nau7802: Use IRQF_NO_AUTOEN instead of request
> then disable
> 
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Whilst a race during interrupt enabling is probably not a problem,
> it is better to not enable the interrupt at all.  The new
> IRQF_NO_AUTOEN flag allows us to do that.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> ---

Reviewed-by: Barry Song <song.bao.hua@hisilicon.com>

>  drivers/iio/adc/nau7802.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/nau7802.c b/drivers/iio/adc/nau7802.c
> index 07c85434b568..bb70b51d25b1 100644
> --- a/drivers/iio/adc/nau7802.c
> +++ b/drivers/iio/adc/nau7802.c
> @@ -498,7 +498,8 @@ static int nau7802_probe(struct i2c_client *client,
>  		ret = request_threaded_irq(client->irq,
>  				NULL,
>  				nau7802_eoc_trigger,
> -				IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> +				IRQF_TRIGGER_HIGH | IRQF_ONESHOT |
> +				IRQF_NO_AUTOEN,
>  				client->dev.driver->name,
>  				indio_dev);
>  		if (ret) {
> @@ -513,8 +514,7 @@ static int nau7802_probe(struct i2c_client *client,
>  			dev_info(&client->dev,
>  				"Failed to allocate IRQ, using polling mode\n");
>  			client->irq = 0;
> -		} else
> -			disable_irq(client->irq);
> +		}
>  	}
> 
>  	if (!client->irq) {
> --
> 2.31.1

Re: [PATCH 3/7] iio:adc:nau7802: Use IRQF_NO_AUTOEN instead of request then disable

[Jonathan Cameron]

On Fri, 2 Apr 2021 20:12:13 +0000
"Song Bao Hua (Barry Song)" <song.bao.hua@hisilicon.com> wrote:

> > -----Original Message-----
> > From: Jonathan Cameron [mailto:jic23@kernel.org]
> > Sent: Saturday, April 3, 2021 7:46 AM
> > To: linux-iio@vger.kernel.org
> > Cc: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; Jonathan Cameron
> > <jonathan.cameron@huawei.com>; Alexandre Belloni
> > <alexandre.belloni@bootlin.com>
> > Subject: [PATCH 3/7] iio:adc:nau7802: Use IRQF_NO_AUTOEN instead of request
> > then disable
> > 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > Whilst a race during interrupt enabling is probably not a problem,
> > it is better to not enable the interrupt at all.  The new
> > IRQF_NO_AUTOEN flag allows us to do that.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > ---  
> 
> Reviewed-by: Barry Song <song.bao.hua@hisilicon.com>
Applied and pushed out as testing to let the autobuilders test it a bit.
Still time for others to respond though as I can rebase for at least the
next few days as I expect I'll do a pull request mid week.

Thanks,

Jonathan

> 
> >  drivers/iio/adc/nau7802.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/nau7802.c b/drivers/iio/adc/nau7802.c
> > index 07c85434b568..bb70b51d25b1 100644
> > --- a/drivers/iio/adc/nau7802.c
> > +++ b/drivers/iio/adc/nau7802.c
> > @@ -498,7 +498,8 @@ static int nau7802_probe(struct i2c_client *client,
> >  		ret = request_threaded_irq(client->irq,
> >  				NULL,
> >  				nau7802_eoc_trigger,
> > -				IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> > +				IRQF_TRIGGER_HIGH | IRQF_ONESHOT |
> > +				IRQF_NO_AUTOEN,
> >  				client->dev.driver->name,
> >  				indio_dev);
> >  		if (ret) {
> > @@ -513,8 +514,7 @@ static int nau7802_probe(struct i2c_client *client,
> >  			dev_info(&client->dev,
> >  				"Failed to allocate IRQ, using polling mode\n");
> >  			client->irq = 0;
> > -		} else
> > -			disable_irq(client->irq);
> > +		}
> >  	}
> > 
> >  	if (!client->irq) {
> > --
> > 2.31.1  
> 

[PATCH 4/7] iio:adc:sun4i-gpadc: Use new IRQF_NO_AUTOEN flag instead of request then disable

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

This new flag ensures a requested irq is not autoenabled, thus removing
the need for the disable_irq() that follows and closing off any chance
of spurious interrupts.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/iio/adc/sun4i-gpadc-iio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c
index 99b43f28e879..2d393a4dfff6 100644
--- a/drivers/iio/adc/sun4i-gpadc-iio.c
+++ b/drivers/iio/adc/sun4i-gpadc-iio.c
@@ -470,7 +470,8 @@ static int sun4i_irq_init(struct platform_device *pdev, const char *name,
 	}
 
 	*irq = ret;
-	ret = devm_request_any_context_irq(&pdev->dev, *irq, handler, 0,
+	ret = devm_request_any_context_irq(&pdev->dev, *irq, handler,
+					   IRQF_NO_AUTOEN,
 					   devname, info);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "could not request %s interrupt: %d\n",
@@ -478,7 +479,6 @@ static int sun4i_irq_init(struct platform_device *pdev, const char *name,
 		return ret;
 	}
 
-	disable_irq(*irq);
 	atomic_set(atomic, 0);
 
 	return 0;
-- 
2.31.1

RE: [PATCH 4/7] iio:adc:sun4i-gpadc: Use new IRQF_NO_AUTOEN flag instead of request then disable

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Jonathan Cameron [mailto:jic23@kernel.org]
> Sent: Saturday, April 3, 2021 7:46 AM
> To: linux-iio@vger.kernel.org
> Cc: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>; Maxime Ripard <maxime.ripard@bootlin.com>
> Subject: [PATCH 4/7] iio:adc:sun4i-gpadc: Use new IRQF_NO_AUTOEN flag instead
> of request then disable
> 
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> This new flag ensures a requested irq is not autoenabled, thus removing
> the need for the disable_irq() that follows and closing off any chance
> of spurious interrupts.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> ---

Reviewed-by: Barry Song <song.bao.hua@hisilicon.com>

>  drivers/iio/adc/sun4i-gpadc-iio.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c
> b/drivers/iio/adc/sun4i-gpadc-iio.c
> index 99b43f28e879..2d393a4dfff6 100644
> --- a/drivers/iio/adc/sun4i-gpadc-iio.c
> +++ b/drivers/iio/adc/sun4i-gpadc-iio.c
> @@ -470,7 +470,8 @@ static int sun4i_irq_init(struct platform_device *pdev,
> const char *name,
>  	}
> 
>  	*irq = ret;
> -	ret = devm_request_any_context_irq(&pdev->dev, *irq, handler, 0,
> +	ret = devm_request_any_context_irq(&pdev->dev, *irq, handler,
> +					   IRQF_NO_AUTOEN,
>  					   devname, info);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "could not request %s interrupt: %d\n",
> @@ -478,7 +479,6 @@ static int sun4i_irq_init(struct platform_device *pdev,
> const char *name,
>  		return ret;
>  	}
> 
> -	disable_irq(*irq);
>  	atomic_set(atomic, 0);
> 
>  	return 0;
> --
> 2.31.1

Re: [PATCH 4/7] iio:adc:sun4i-gpadc: Use new IRQF_NO_AUTOEN flag instead of request then disable

[Jonathan Cameron]

On Fri, 2 Apr 2021 20:13:00 +0000
"Song Bao Hua (Barry Song)" <song.bao.hua@hisilicon.com> wrote:

> > -----Original Message-----
> > From: Jonathan Cameron [mailto:jic23@kernel.org]
> > Sent: Saturday, April 3, 2021 7:46 AM
> > To: linux-iio@vger.kernel.org
> > Cc: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; Jonathan Cameron
> > <jonathan.cameron@huawei.com>; Maxime Ripard <maxime.ripard@bootlin.com>
> > Subject: [PATCH 4/7] iio:adc:sun4i-gpadc: Use new IRQF_NO_AUTOEN flag instead
> > of request then disable
> > 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > This new flag ensures a requested irq is not autoenabled, thus removing
> > the need for the disable_irq() that follows and closing off any chance
> > of spurious interrupts.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> > ---  
> 
> Reviewed-by: Barry Song <song.bao.hua@hisilicon.com>

Applied to the togreg branch of iio.git and pushed out as testing
to let 0-day poke at it.   Other feedback whilst it's doing that
most welcome.

Thanks,

Jonathan

> 
> >  drivers/iio/adc/sun4i-gpadc-iio.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c
> > b/drivers/iio/adc/sun4i-gpadc-iio.c
> > index 99b43f28e879..2d393a4dfff6 100644
> > --- a/drivers/iio/adc/sun4i-gpadc-iio.c
> > +++ b/drivers/iio/adc/sun4i-gpadc-iio.c
> > @@ -470,7 +470,8 @@ static int sun4i_irq_init(struct platform_device *pdev,
> > const char *name,
> >  	}
> > 
> >  	*irq = ret;
> > -	ret = devm_request_any_context_irq(&pdev->dev, *irq, handler, 0,
> > +	ret = devm_request_any_context_irq(&pdev->dev, *irq, handler,
> > +					   IRQF_NO_AUTOEN,
> >  					   devname, info);
> >  	if (ret < 0) {
> >  		dev_err(&pdev->dev, "could not request %s interrupt: %d\n",
> > @@ -478,7 +479,6 @@ static int sun4i_irq_init(struct platform_device *pdev,
> > const char *name,
> >  		return ret;
> >  	}
> > 
> > -	disable_irq(*irq);
> >  	atomic_set(atomic, 0);
> > 
> >  	return 0;
> > --
> > 2.31.1  
> 

[PATCH 5/7] iio:chemical:scd30: Use IRQF_NO_AUTOEN to avoid irq request then disable

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

This new flag cleanly avoids the need for a dance where we request the
interrupt only to immediately disabling it by ensuring it is not
auto-enabled in the first place.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Tomasz Duszynski <tomasz.duszynski@octakon.com>
---
 drivers/iio/chemical/scd30_core.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/chemical/scd30_core.c b/drivers/iio/chemical/scd30_core.c
index 261c277ac4a5..d89f117dd0ef 100644
--- a/drivers/iio/chemical/scd30_core.c
+++ b/drivers/iio/chemical/scd30_core.c
@@ -655,19 +655,19 @@ static int scd30_setup_trigger(struct iio_dev *indio_dev)
 
 	indio_dev->trig = iio_trigger_get(trig);
 
+	/*
+	 * Interrupt is enabled just before taking a fresh measurement
+	 * and disabled afterwards. This means we need to ensure it is not
+	 * enabled here to keep calls to enable/disable balanced.
+	 */
 	ret = devm_request_threaded_irq(dev, state->irq, scd30_irq_handler,
-					scd30_irq_thread_handler, IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
+					scd30_irq_thread_handler,
+					IRQF_TRIGGER_HIGH | IRQF_ONESHOT |
+					IRQF_NO_AUTOEN,
 					indio_dev->name, indio_dev);
 	if (ret)
 		dev_err(dev, "failed to request irq\n");
 
-	/*
-	 * Interrupt is enabled just before taking a fresh measurement
-	 * and disabled afterwards. This means we need to disable it here
-	 * to keep calls to enable/disable balanced.
-	 */
-	disable_irq(state->irq);
-
 	return ret;
 }
 
-- 
2.31.1

RE: [PATCH 5/7] iio:chemical:scd30: Use IRQF_NO_AUTOEN to avoid irq request then disable

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Jonathan Cameron [mailto:jic23@kernel.org]
> Sent: Saturday, April 3, 2021 7:46 AM
> To: linux-iio@vger.kernel.org
> Cc: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>; Tomasz Duszynski
> <tomasz.duszynski@octakon.com>
> Subject: [PATCH 5/7] iio:chemical:scd30: Use IRQF_NO_AUTOEN to avoid irq request
> then disable
> 
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> This new flag cleanly avoids the need for a dance where we request the
> interrupt only to immediately disabling it by ensuring it is not
> auto-enabled in the first place.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Tomasz Duszynski <tomasz.duszynski@octakon.com>
> ---

Reviewed-by: Barry Song <song.bao.hua@hisilicon.com>

>  drivers/iio/chemical/scd30_core.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/chemical/scd30_core.c
> b/drivers/iio/chemical/scd30_core.c
> index 261c277ac4a5..d89f117dd0ef 100644
> --- a/drivers/iio/chemical/scd30_core.c
> +++ b/drivers/iio/chemical/scd30_core.c
> @@ -655,19 +655,19 @@ static int scd30_setup_trigger(struct iio_dev *indio_dev)
> 
>  	indio_dev->trig = iio_trigger_get(trig);
> 
> +	/*
> +	 * Interrupt is enabled just before taking a fresh measurement
> +	 * and disabled afterwards. This means we need to ensure it is not
> +	 * enabled here to keep calls to enable/disable balanced.
> +	 */
>  	ret = devm_request_threaded_irq(dev, state->irq, scd30_irq_handler,
> -					scd30_irq_thread_handler, IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> +					scd30_irq_thread_handler,
> +					IRQF_TRIGGER_HIGH | IRQF_ONESHOT |
> +					IRQF_NO_AUTOEN,
>  					indio_dev->name, indio_dev);
>  	if (ret)
>  		dev_err(dev, "failed to request irq\n");
> 
> -	/*
> -	 * Interrupt is enabled just before taking a fresh measurement
> -	 * and disabled afterwards. This means we need to disable it here
> -	 * to keep calls to enable/disable balanced.
> -	 */
> -	disable_irq(state->irq);
> -
>  	return ret;
>  }
> 
> --
> 2.31.1

Re: [PATCH 5/7] iio:chemical:scd30: Use IRQF_NO_AUTOEN to avoid irq request then disable

[Jonathan Cameron]

On Fri, 2 Apr 2021 20:14:04 +0000
"Song Bao Hua (Barry Song)" <song.bao.hua@hisilicon.com> wrote:

> > -----Original Message-----
> > From: Jonathan Cameron [mailto:jic23@kernel.org]
> > Sent: Saturday, April 3, 2021 7:46 AM
> > To: linux-iio@vger.kernel.org
> > Cc: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; Jonathan Cameron
> > <jonathan.cameron@huawei.com>; Tomasz Duszynski
> > <tomasz.duszynski@octakon.com>
> > Subject: [PATCH 5/7] iio:chemical:scd30: Use IRQF_NO_AUTOEN to avoid irq request
> > then disable
> > 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > This new flag cleanly avoids the need for a dance where we request the
> > interrupt only to immediately disabling it by ensuring it is not
> > auto-enabled in the first place.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Tomasz Duszynski <tomasz.duszynski@octakon.com>
> > ---  
> 
> Reviewed-by: Barry Song <song.bao.hua@hisilicon.com>

Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to poke at it.  Whilst they are doing that, any
additional feedback welcomed.

Thanks,

Jonathan

> 
> >  drivers/iio/chemical/scd30_core.c | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/iio/chemical/scd30_core.c
> > b/drivers/iio/chemical/scd30_core.c
> > index 261c277ac4a5..d89f117dd0ef 100644
> > --- a/drivers/iio/chemical/scd30_core.c
> > +++ b/drivers/iio/chemical/scd30_core.c
> > @@ -655,19 +655,19 @@ static int scd30_setup_trigger(struct iio_dev *indio_dev)
> > 
> >  	indio_dev->trig = iio_trigger_get(trig);
> > 
> > +	/*
> > +	 * Interrupt is enabled just before taking a fresh measurement
> > +	 * and disabled afterwards. This means we need to ensure it is not
> > +	 * enabled here to keep calls to enable/disable balanced.
> > +	 */
> >  	ret = devm_request_threaded_irq(dev, state->irq, scd30_irq_handler,
> > -					scd30_irq_thread_handler, IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> > +					scd30_irq_thread_handler,
> > +					IRQF_TRIGGER_HIGH | IRQF_ONESHOT |
> > +					IRQF_NO_AUTOEN,
> >  					indio_dev->name, indio_dev);
> >  	if (ret)
> >  		dev_err(dev, "failed to request irq\n");
> > 
> > -	/*
> > -	 * Interrupt is enabled just before taking a fresh measurement
> > -	 * and disabled afterwards. This means we need to disable it here
> > -	 * to keep calls to enable/disable balanced.
> > -	 */
> > -	disable_irq(state->irq);
> > -
> >  	return ret;
> >  }
> > 
> > --
> > 2.31.1  
> 

[PATCH 6/7] iio:imu:adis: Use IRQF_NO_AUTOEN instead of irq request then disable

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

This is a bit involved as the adis library code already has some
sanity checking of the flags of the requested irq that we need
to ensure is happy to pass through the IRQF_NO_AUTOEN flag untouched.

Using this flag avoids us autoenabling the irq in the adis16460 and
adis16475 drivers which cover parts that don't have any means of
masking the interrupt on the device end.

Note, compile tested only!

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Alexandru Ardelean <ardeleanalex@gmail.com>
Cc: Nuno Sa <Nuno.Sa@analog.com>
---
 drivers/iio/imu/adis16460.c    |  4 ++--
 drivers/iio/imu/adis16475.c    |  5 +++--
 drivers/iio/imu/adis_trigger.c | 11 ++++++-----
 3 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/imu/adis16460.c b/drivers/iio/imu/adis16460.c
index 74a161e39733..73bf45e859b8 100644
--- a/drivers/iio/imu/adis16460.c
+++ b/drivers/iio/imu/adis16460.c
@@ -403,12 +403,12 @@ static int adis16460_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
+	/* We cannot mask the interrupt, so ensure it isn't auto enabled */
+	st->adis.irq_flag |= IRQF_NO_AUTOEN;
 	ret = devm_adis_setup_buffer_and_trigger(&st->adis, indio_dev, NULL);
 	if (ret)
 		return ret;
 
-	adis16460_enable_irq(&st->adis, 0);
-
 	ret = __adis_initial_startup(&st->adis);
 	if (ret)
 		return ret;
diff --git a/drivers/iio/imu/adis16475.c b/drivers/iio/imu/adis16475.c
index 8f6bea4b6608..1de62fc79e0f 100644
--- a/drivers/iio/imu/adis16475.c
+++ b/drivers/iio/imu/adis16475.c
@@ -1258,6 +1258,9 @@ static int adis16475_config_irq_pin(struct adis16475 *st)
 		return -EINVAL;
 	}
 
+	/* We cannot mask the interrupt so ensure it's not enabled at request */
+	st->adis.irq_flag |= IRQF_NO_AUTOEN;
+
 	val = ADIS16475_MSG_CTRL_DR_POL(polarity);
 	ret = __adis_update_bits(&st->adis, ADIS16475_REG_MSG_CTRL,
 				 ADIS16475_MSG_CTRL_DR_POL_MASK, val);
@@ -1362,8 +1365,6 @@ static int adis16475_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
-	adis16475_enable_irq(&st->adis, false);
-
 	ret = devm_iio_device_register(&spi->dev, indio_dev);
 	if (ret)
 		return ret;
diff --git a/drivers/iio/imu/adis_trigger.c b/drivers/iio/imu/adis_trigger.c
index 0f29e56200af..39b50d6fdb6a 100644
--- a/drivers/iio/imu/adis_trigger.c
+++ b/drivers/iio/imu/adis_trigger.c
@@ -29,18 +29,19 @@ static const struct iio_trigger_ops adis_trigger_ops = {
 
 static int adis_validate_irq_flag(struct adis *adis)
 {
+	unsigned long direction = adis->irq_flag & IRQF_TRIGGER_MASK;
 	/*
 	 * Typically this devices have data ready either on the rising edge or
 	 * on the falling edge of the data ready pin. This checks enforces that
 	 * one of those is set in the drivers... It defaults to
-	 * IRQF_TRIGGER_RISING for backward compatibility wiht devices that
+	 * IRQF_TRIGGER_RISING for backward compatibility with devices that
 	 * don't support changing the pin polarity.
 	 */
-	if (!adis->irq_flag) {
-		adis->irq_flag = IRQF_TRIGGER_RISING;
+	if (!direction) {
+		adis->irq_flag |= IRQF_TRIGGER_RISING;
 		return 0;
-	} else if (adis->irq_flag != IRQF_TRIGGER_RISING &&
-		   adis->irq_flag != IRQF_TRIGGER_FALLING) {
+	} else if (direction != IRQF_TRIGGER_RISING &&
+		   direction != IRQF_TRIGGER_FALLING) {
 		dev_err(&adis->spi->dev, "Invalid IRQ mask: %08lx\n",
 			adis->irq_flag);
 		return -EINVAL;
-- 
2.31.1

RE: [PATCH 6/7] iio:imu:adis: Use IRQF_NO_AUTOEN instead of irq request then disable

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Jonathan Cameron [mailto:jic23@kernel.org]
> Sent: Saturday, April 3, 2021 7:46 AM
> To: linux-iio@vger.kernel.org
> Cc: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>; Alexandru Ardelean <ardeleanalex@gmail.com>;
> Nuno Sa <Nuno.Sa@analog.com>
> Subject: [PATCH 6/7] iio:imu:adis: Use IRQF_NO_AUTOEN instead of irq request
> then disable
> 
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> This is a bit involved as the adis library code already has some
> sanity checking of the flags of the requested irq that we need
> to ensure is happy to pass through the IRQF_NO_AUTOEN flag untouched.
> 
> Using this flag avoids us autoenabling the irq in the adis16460 and
> adis16475 drivers which cover parts that don't have any means of
> masking the interrupt on the device end.
> 
> Note, compile tested only!
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Alexandru Ardelean <ardeleanalex@gmail.com>
> Cc: Nuno Sa <Nuno.Sa@analog.com>
> ---
>  drivers/iio/imu/adis16460.c    |  4 ++--
>  drivers/iio/imu/adis16475.c    |  5 +++--
>  drivers/iio/imu/adis_trigger.c | 11 ++++++-----
>  3 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis16460.c b/drivers/iio/imu/adis16460.c
> index 74a161e39733..73bf45e859b8 100644
> --- a/drivers/iio/imu/adis16460.c
> +++ b/drivers/iio/imu/adis16460.c
> @@ -403,12 +403,12 @@ static int adis16460_probe(struct spi_device *spi)
>  	if (ret)
>  		return ret;
> 
> +	/* We cannot mask the interrupt, so ensure it isn't auto enabled */
> +	st->adis.irq_flag |= IRQF_NO_AUTOEN;
>  	ret = devm_adis_setup_buffer_and_trigger(&st->adis, indio_dev, NULL);
>  	if (ret)
>  		return ret;
> 
> -	adis16460_enable_irq(&st->adis, 0);
> -
>  	ret = __adis_initial_startup(&st->adis);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/iio/imu/adis16475.c b/drivers/iio/imu/adis16475.c
> index 8f6bea4b6608..1de62fc79e0f 100644
> --- a/drivers/iio/imu/adis16475.c
> +++ b/drivers/iio/imu/adis16475.c
> @@ -1258,6 +1258,9 @@ static int adis16475_config_irq_pin(struct adis16475 *st)
>  		return -EINVAL;
>  	}
> 
> +	/* We cannot mask the interrupt so ensure it's not enabled at request */
> +	st->adis.irq_flag |= IRQF_NO_AUTOEN;
> +
>  	val = ADIS16475_MSG_CTRL_DR_POL(polarity);
>  	ret = __adis_update_bits(&st->adis, ADIS16475_REG_MSG_CTRL,
>  				 ADIS16475_MSG_CTRL_DR_POL_MASK, val);
> @@ -1362,8 +1365,6 @@ static int adis16475_probe(struct spi_device *spi)
>  	if (ret)
>  		return ret;
> 
> -	adis16475_enable_irq(&st->adis, false);
> -
>  	ret = devm_iio_device_register(&spi->dev, indio_dev);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/iio/imu/adis_trigger.c b/drivers/iio/imu/adis_trigger.c
> index 0f29e56200af..39b50d6fdb6a 100644
> --- a/drivers/iio/imu/adis_trigger.c
> +++ b/drivers/iio/imu/adis_trigger.c
> @@ -29,18 +29,19 @@ static const struct iio_trigger_ops adis_trigger_ops = {
> 
>  static int adis_validate_irq_flag(struct adis *adis)
>  {
> +	unsigned long direction = adis->irq_flag & IRQF_TRIGGER_MASK;
>  	/*
>  	 * Typically this devices have data ready either on the rising edge or
>  	 * on the falling edge of the data ready pin. This checks enforces that
>  	 * one of those is set in the drivers... It defaults to
> -	 * IRQF_TRIGGER_RISING for backward compatibility wiht devices that
> +	 * IRQF_TRIGGER_RISING for backward compatibility with devices that
>  	 * don't support changing the pin polarity.
>  	 */
> -	if (!adis->irq_flag) {
> -		adis->irq_flag = IRQF_TRIGGER_RISING;
> +	if (!direction) {

Reviewed-by: Barry Song <song.bao.hua@hisilicon.com>

Though a better way might be:

if (direction == IRQF_TRIGGER_NONE)

> +		adis->irq_flag |= IRQF_TRIGGER_RISING;
>  		return 0;
> -	} else if (adis->irq_flag != IRQF_TRIGGER_RISING &&
> -		   adis->irq_flag != IRQF_TRIGGER_FALLING) {
> +	} else if (direction != IRQF_TRIGGER_RISING &&
> +		   direction != IRQF_TRIGGER_FALLING) {
>  		dev_err(&adis->spi->dev, "Invalid IRQ mask: %08lx\n",
>  			adis->irq_flag);
>  		return -EINVAL;
> --
> 2.31.1

Thanks
Barry

Re: [PATCH 6/7] iio:imu:adis: Use IRQF_NO_AUTOEN instead of irq request then disable

[Jonathan Cameron]

On Fri, 2 Apr 2021 20:37:37 +0000
"Song Bao Hua (Barry Song)" <song.bao.hua@hisilicon.com> wrote:

> > -----Original Message-----
> > From: Jonathan Cameron [mailto:jic23@kernel.org]
> > Sent: Saturday, April 3, 2021 7:46 AM
> > To: linux-iio@vger.kernel.org
> > Cc: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; Jonathan Cameron
> > <jonathan.cameron@huawei.com>; Alexandru Ardelean <ardeleanalex@gmail.com>;
> > Nuno Sa <Nuno.Sa@analog.com>
> > Subject: [PATCH 6/7] iio:imu:adis: Use IRQF_NO_AUTOEN instead of irq request
> > then disable
> > 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > This is a bit involved as the adis library code already has some
> > sanity checking of the flags of the requested irq that we need
> > to ensure is happy to pass through the IRQF_NO_AUTOEN flag untouched.
> > 
> > Using this flag avoids us autoenabling the irq in the adis16460 and
> > adis16475 drivers which cover parts that don't have any means of
> > masking the interrupt on the device end.
> > 
> > Note, compile tested only!
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Alexandru Ardelean <ardeleanalex@gmail.com>
> > Cc: Nuno Sa <Nuno.Sa@analog.com>
> > ---
> >  drivers/iio/imu/adis16460.c    |  4 ++--
> >  drivers/iio/imu/adis16475.c    |  5 +++--
> >  drivers/iio/imu/adis_trigger.c | 11 ++++++-----
> >  3 files changed, 11 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/iio/imu/adis16460.c b/drivers/iio/imu/adis16460.c
> > index 74a161e39733..73bf45e859b8 100644
> > --- a/drivers/iio/imu/adis16460.c
> > +++ b/drivers/iio/imu/adis16460.c
> > @@ -403,12 +403,12 @@ static int adis16460_probe(struct spi_device *spi)
> >  	if (ret)
> >  		return ret;
> > 
> > +	/* We cannot mask the interrupt, so ensure it isn't auto enabled */
> > +	st->adis.irq_flag |= IRQF_NO_AUTOEN;
> >  	ret = devm_adis_setup_buffer_and_trigger(&st->adis, indio_dev, NULL);
> >  	if (ret)
> >  		return ret;
> > 
> > -	adis16460_enable_irq(&st->adis, 0);
> > -
> >  	ret = __adis_initial_startup(&st->adis);
> >  	if (ret)
> >  		return ret;
> > diff --git a/drivers/iio/imu/adis16475.c b/drivers/iio/imu/adis16475.c
> > index 8f6bea4b6608..1de62fc79e0f 100644
> > --- a/drivers/iio/imu/adis16475.c
> > +++ b/drivers/iio/imu/adis16475.c
> > @@ -1258,6 +1258,9 @@ static int adis16475_config_irq_pin(struct adis16475 *st)
> >  		return -EINVAL;
> >  	}
> > 
> > +	/* We cannot mask the interrupt so ensure it's not enabled at request */
> > +	st->adis.irq_flag |= IRQF_NO_AUTOEN;
> > +
> >  	val = ADIS16475_MSG_CTRL_DR_POL(polarity);
> >  	ret = __adis_update_bits(&st->adis, ADIS16475_REG_MSG_CTRL,
> >  				 ADIS16475_MSG_CTRL_DR_POL_MASK, val);
> > @@ -1362,8 +1365,6 @@ static int adis16475_probe(struct spi_device *spi)
> >  	if (ret)
> >  		return ret;
> > 
> > -	adis16475_enable_irq(&st->adis, false);
> > -
> >  	ret = devm_iio_device_register(&spi->dev, indio_dev);
> >  	if (ret)
> >  		return ret;
> > diff --git a/drivers/iio/imu/adis_trigger.c b/drivers/iio/imu/adis_trigger.c
> > index 0f29e56200af..39b50d6fdb6a 100644
> > --- a/drivers/iio/imu/adis_trigger.c
> > +++ b/drivers/iio/imu/adis_trigger.c
> > @@ -29,18 +29,19 @@ static const struct iio_trigger_ops adis_trigger_ops = {
> > 
> >  static int adis_validate_irq_flag(struct adis *adis)
> >  {
> > +	unsigned long direction = adis->irq_flag & IRQF_TRIGGER_MASK;
> >  	/*
> >  	 * Typically this devices have data ready either on the rising edge or
> >  	 * on the falling edge of the data ready pin. This checks enforces that
> >  	 * one of those is set in the drivers... It defaults to
> > -	 * IRQF_TRIGGER_RISING for backward compatibility wiht devices that
> > +	 * IRQF_TRIGGER_RISING for backward compatibility with devices that
> >  	 * don't support changing the pin polarity.
> >  	 */
> > -	if (!adis->irq_flag) {
> > -		adis->irq_flag = IRQF_TRIGGER_RISING;
> > +	if (!direction) {  
> 
> Reviewed-by: Barry Song <song.bao.hua@hisilicon.com>
> 
> Though a better way might be:
> 
> if (direction == IRQF_TRIGGER_NONE)
Agreed.  That makes it a tiny bit more obvious what is going on.

Applied with that change (seems unlikely to affect other's tags)
to the togreg branch of iio.git and pushed out as testing for the
autobuilders to see if they can find anything we missed.

As normal, whilst 0-day and friends are working their magic, there
is time for any additional feedback anyone would like to share.

Thanks,

Jonathan

> 
> > +		adis->irq_flag |= IRQF_TRIGGER_RISING;
> >  		return 0;
> > -	} else if (adis->irq_flag != IRQF_TRIGGER_RISING &&
> > -		   adis->irq_flag != IRQF_TRIGGER_FALLING) {
> > +	} else if (direction != IRQF_TRIGGER_RISING &&
> > +		   direction != IRQF_TRIGGER_FALLING) {
> >  		dev_err(&adis->spi->dev, "Invalid IRQ mask: %08lx\n",
> >  			adis->irq_flag);
> >  		return -EINVAL;
> > --
> > 2.31.1  
> 
> Thanks
> Barry
> 

RE: [PATCH 6/7] iio:imu:adis: Use IRQF_NO_AUTOEN instead of irq request then disable

[Sa, Nuno]

> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Monday, April 5, 2021 4:41 PM
> To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>
> Cc: linux-iio@vger.kernel.org; Jonathan Cameron
> <jonathan.cameron@huawei.com>; Alexandru Ardelean
> <ardeleanalex@gmail.com>; Sa, Nuno <Nuno.Sa@analog.com>;
> tiantao (H) <tiantao6@hisilicon.com>
> Subject: Re: [PATCH 6/7] iio:imu:adis: Use IRQF_NO_AUTOEN instead
> of irq request then disable
> 
> On Fri, 2 Apr 2021 20:37:37 +0000
> "Song Bao Hua (Barry Song)" <song.bao.hua@hisilicon.com> wrote:
> 
> > > -----Original Message-----
> > > From: Jonathan Cameron [mailto:jic23@kernel.org]
> > > Sent: Saturday, April 3, 2021 7:46 AM
> > > To: linux-iio@vger.kernel.org
> > > Cc: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>;
> Jonathan Cameron
> > > <jonathan.cameron@huawei.com>; Alexandru Ardelean
> <ardeleanalex@gmail.com>;
> > > Nuno Sa <Nuno.Sa@analog.com>
> > > Subject: [PATCH 6/7] iio:imu:adis: Use IRQF_NO_AUTOEN instead
> of irq request
> > > then disable
> > >
> > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > >
> > > This is a bit involved as the adis library code already has some
> > > sanity checking of the flags of the requested irq that we need
> > > to ensure is happy to pass through the IRQF_NO_AUTOEN flag
> untouched.
> > >
> > > Using this flag avoids us autoenabling the irq in the adis16460 and
> > > adis16475 drivers which cover parts that don't have any means of
> > > masking the interrupt on the device end.
> > >
> > > Note, compile tested only!
> > >
> > > Signed-off-by: Jonathan Cameron
> <Jonathan.Cameron@huawei.com>
> > > Cc: Alexandru Ardelean <ardeleanalex@gmail.com>
> > > Cc: Nuno Sa <Nuno.Sa@analog.com>
> > > ---
> > >  drivers/iio/imu/adis16460.c    |  4 ++--
> > >  drivers/iio/imu/adis16475.c    |  5 +++--
> > >  drivers/iio/imu/adis_trigger.c | 11 ++++++-----
> > >  3 files changed, 11 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/iio/imu/adis16460.c
> b/drivers/iio/imu/adis16460.c
> > > index 74a161e39733..73bf45e859b8 100644
> > > --- a/drivers/iio/imu/adis16460.c
> > > +++ b/drivers/iio/imu/adis16460.c
> > > @@ -403,12 +403,12 @@ static int adis16460_probe(struct
> spi_device *spi)
> > >  	if (ret)
> > >  		return ret;
> > >
> > > +	/* We cannot mask the interrupt, so ensure it isn't auto
> enabled */
> > > +	st->adis.irq_flag |= IRQF_NO_AUTOEN;
> > >  	ret = devm_adis_setup_buffer_and_trigger(&st->adis,
> indio_dev, NULL);
> > >  	if (ret)
> > >  		return ret;
> > >
> > > -	adis16460_enable_irq(&st->adis, 0);
> > > -
> > >  	ret = __adis_initial_startup(&st->adis);
> > >  	if (ret)
> > >  		return ret;
> > > diff --git a/drivers/iio/imu/adis16475.c
> b/drivers/iio/imu/adis16475.c
> > > index 8f6bea4b6608..1de62fc79e0f 100644
> > > --- a/drivers/iio/imu/adis16475.c
> > > +++ b/drivers/iio/imu/adis16475.c
> > > @@ -1258,6 +1258,9 @@ static int adis16475_config_irq_pin(struct
> adis16475 *st)
> > >  		return -EINVAL;
> > >  	}
> > >
> > > +	/* We cannot mask the interrupt so ensure it's not enabled at
> request */
> > > +	st->adis.irq_flag |= IRQF_NO_AUTOEN;
> > > +
> > >  	val = ADIS16475_MSG_CTRL_DR_POL(polarity);
> > >  	ret = __adis_update_bits(&st->adis,
> ADIS16475_REG_MSG_CTRL,
> > >
> ADIS16475_MSG_CTRL_DR_POL_MASK, val);
> > > @@ -1362,8 +1365,6 @@ static int adis16475_probe(struct
> spi_device *spi)
> > >  	if (ret)
> > >  		return ret;
> > >
> > > -	adis16475_enable_irq(&st->adis, false);
> > > -
> > >  	ret = devm_iio_device_register(&spi->dev, indio_dev);
> > >  	if (ret)
> > >  		return ret;
> > > diff --git a/drivers/iio/imu/adis_trigger.c
> b/drivers/iio/imu/adis_trigger.c
> > > index 0f29e56200af..39b50d6fdb6a 100644
> > > --- a/drivers/iio/imu/adis_trigger.c
> > > +++ b/drivers/iio/imu/adis_trigger.c
> > > @@ -29,18 +29,19 @@ static const struct iio_trigger_ops
> adis_trigger_ops = {
> > >
> > >  static int adis_validate_irq_flag(struct adis *adis)
> > >  {
> > > +	unsigned long direction = adis->irq_flag &
> IRQF_TRIGGER_MASK;
> > >  	/*
> > >  	 * Typically this devices have data ready either on the rising
> edge or
> > >  	 * on the falling edge of the data ready pin. This checks
> enforces that
> > >  	 * one of those is set in the drivers... It defaults to
> > > -	 * IRQF_TRIGGER_RISING for backward compatibility wiht
> devices that
> > > +	 * IRQF_TRIGGER_RISING for backward compatibility with
> devices that
> > >  	 * don't support changing the pin polarity.
> > >  	 */
> > > -	if (!adis->irq_flag) {
> > > -		adis->irq_flag = IRQF_TRIGGER_RISING;
> > > +	if (!direction) {
> >
> > Reviewed-by: Barry Song <song.bao.hua@hisilicon.com>
> >
> > Though a better way might be:
> >
> > if (direction == IRQF_TRIGGER_NONE)
> Agreed.  That makes it a tiny bit more obvious what is going on.
> 
> Applied with that change (seems unlikely to affect other's tags)
> to the togreg branch of iio.git and pushed out as testing for the
> autobuilders to see if they can find anything we missed.
> 
> As normal, whilst 0-day and friends are working their magic, there
> is time for any additional feedback anyone would like to share.

If not too late :)...

Reviewed-by: Nuno Sa <nuno.sa@analog.com>

[PATCH 7/7] iio:adc:ad_sigma_delta: Use IRQF_NO_AUTOEN rather than request and disable

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

These devices are not able to mask the signal used as a data ready
interrupt.  As such they previously requested the irq then immediately
disabled it.  Now we can avoid the potential of a spurious interrupt
by avoiding the irq being auto enabled in the first place.

I'm not sure how this code could have been called with the irq already
disabled, so I believe the conditional would always have been true and
have removed it.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: Alexandru Ardelean <ardeleanalex@gmail.com>
---
 drivers/iio/adc/ad_sigma_delta.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index 9289812c0a94..e777ec718973 100644
--- a/drivers/iio/adc/ad_sigma_delta.c
+++ b/drivers/iio/adc/ad_sigma_delta.c
@@ -485,18 +485,15 @@ static int ad_sd_probe_trigger(struct iio_dev *indio_dev)
 	sigma_delta->trig->ops = &ad_sd_trigger_ops;
 	init_completion(&sigma_delta->completion);
 
+	sigma_delta->irq_dis = true;
 	ret = request_irq(sigma_delta->spi->irq,
 			  ad_sd_data_rdy_trig_poll,
-			  sigma_delta->info->irq_flags,
+			  sigma_delta->info->irq_flags | IRQF_NO_AUTOEN,
 			  indio_dev->name,
 			  sigma_delta);
 	if (ret)
 		goto error_free_trig;
 
-	if (!sigma_delta->irq_dis) {
-		sigma_delta->irq_dis = true;
-		disable_irq_nosync(sigma_delta->spi->irq);
-	}
 	iio_trigger_set_drvdata(sigma_delta->trig, sigma_delta);
 
 	ret = iio_trigger_register(sigma_delta->trig);
-- 
2.31.1

RE: [PATCH 7/7] iio:adc:ad_sigma_delta: Use IRQF_NO_AUTOEN rather than request and disable

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Jonathan Cameron [mailto:jic23@kernel.org]
> Sent: Saturday, April 3, 2021 7:46 AM
> To: linux-iio@vger.kernel.org
> Cc: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>; Lars-Peter Clausen <lars@metafoo.de>;
> Alexandru Ardelean <ardeleanalex@gmail.com>
> Subject: [PATCH 7/7] iio:adc:ad_sigma_delta: Use IRQF_NO_AUTOEN rather than
> request and disable
> 
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> These devices are not able to mask the signal used as a data ready
> interrupt.  As such they previously requested the irq then immediately
> disabled it.  Now we can avoid the potential of a spurious interrupt
> by avoiding the irq being auto enabled in the first place.
> 
> I'm not sure how this code could have been called with the irq already
> disabled, so I believe the conditional would always have been true and
> have removed it.
> 

If irq_dis is true, it seems the original code assumed interrupt was
open. Now the code is moving to disable-irq for true irq_dis. For
false irq_dis, this patch has no side effect so looks correct.

A safer way might be as below?

if(!sigma_delta->irq_dis)
	irq_flags = sigma_delta->info->irq_flags | IRQF_NO_AUTOEN;

request_irq(irq_flags);

sigma_delta->irq_dis =  true;

> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Alexandru Ardelean <ardeleanalex@gmail.com>
> ---
>  drivers/iio/adc/ad_sigma_delta.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad_sigma_delta.c
> b/drivers/iio/adc/ad_sigma_delta.c
> index 9289812c0a94..e777ec718973 100644
> --- a/drivers/iio/adc/ad_sigma_delta.c
> +++ b/drivers/iio/adc/ad_sigma_delta.c
> @@ -485,18 +485,15 @@ static int ad_sd_probe_trigger(struct iio_dev *indio_dev)
>  	sigma_delta->trig->ops = &ad_sd_trigger_ops;
>  	init_completion(&sigma_delta->completion);
> 
> +	sigma_delta->irq_dis = true;
>  	ret = request_irq(sigma_delta->spi->irq,
>  			  ad_sd_data_rdy_trig_poll,
> -			  sigma_delta->info->irq_flags,
> +			  sigma_delta->info->irq_flags | IRQF_NO_AUTOEN,
>  			  indio_dev->name,
>  			  sigma_delta);
>  	if (ret)
>  		goto error_free_trig;
> 
> -	if (!sigma_delta->irq_dis) {
> -		sigma_delta->irq_dis = true;
> -		disable_irq_nosync(sigma_delta->spi->irq);
> -	}
>  	iio_trigger_set_drvdata(sigma_delta->trig, sigma_delta);
> 
>  	ret = iio_trigger_register(sigma_delta->trig);
> --
> 2.31.1

Thanks
Barry

Re: [PATCH 7/7] iio:adc:ad_sigma_delta: Use IRQF_NO_AUTOEN rather than request and disable

[Jonathan Cameron]

On Fri, 2 Apr 2021 20:30:17 +0000
"Song Bao Hua (Barry Song)" <song.bao.hua@hisilicon.com> wrote:

> > -----Original Message-----
> > From: Jonathan Cameron [mailto:jic23@kernel.org]
> > Sent: Saturday, April 3, 2021 7:46 AM
> > To: linux-iio@vger.kernel.org
> > Cc: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; Jonathan Cameron
> > <jonathan.cameron@huawei.com>; Lars-Peter Clausen <lars@metafoo.de>;
> > Alexandru Ardelean <ardeleanalex@gmail.com>
> > Subject: [PATCH 7/7] iio:adc:ad_sigma_delta: Use IRQF_NO_AUTOEN rather than
> > request and disable
> > 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > These devices are not able to mask the signal used as a data ready
> > interrupt.  As such they previously requested the irq then immediately
> > disabled it.  Now we can avoid the potential of a spurious interrupt
> > by avoiding the irq being auto enabled in the first place.
> > 
> > I'm not sure how this code could have been called with the irq already
> > disabled, so I believe the conditional would always have been true and
> > have removed it.
> >   
> 
> If irq_dis is true, it seems the original code assumed interrupt was
> open. Now the code is moving to disable-irq for true irq_dis. For
> false irq_dis, this patch has no side effect so looks correct.
Hi Barry,

As far as I can tell (and I looked closely :), at the point where
ad_sd_probe_trigger() can be called, irq_dis is always false,
so the original check is misleading by implying
otherwise.

Taken in isolation of what is visible in this patch I'd agree
the change you suggest makes sense, but I'd also like to clean
up the wrong impression the original check was giving.  It is
also worth noting that ad_sigma_delta.h lists irq_dis as a
private member of the structure that should only be used in the
library (reducing chances of any drivers in future doing something
odd with it).

The other checks on irq_dis look suspicious as well, but removing
those would rather spread the scope of this patch.

As Lars has given an Reviewed-by and this is his code, I'm going
to press ahead with this as it stands.

Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to see if we missed anything.

Thanks,

Jonathan

> 
> A safer way might be as below?
> 
> if(!sigma_delta->irq_dis)
> 	irq_flags = sigma_delta->info->irq_flags | IRQF_NO_AUTOEN;
> 
> request_irq(irq_flags);
> 
> sigma_delta->irq_dis =  true;
> 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Lars-Peter Clausen <lars@metafoo.de>
> > Cc: Alexandru Ardelean <ardeleanalex@gmail.com>
> > ---
> >  drivers/iio/adc/ad_sigma_delta.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad_sigma_delta.c
> > b/drivers/iio/adc/ad_sigma_delta.c
> > index 9289812c0a94..e777ec718973 100644
> > --- a/drivers/iio/adc/ad_sigma_delta.c
> > +++ b/drivers/iio/adc/ad_sigma_delta.c
> > @@ -485,18 +485,15 @@ static int ad_sd_probe_trigger(struct iio_dev *indio_dev)
> >  	sigma_delta->trig->ops = &ad_sd_trigger_ops;
> >  	init_completion(&sigma_delta->completion);
> > 
> > +	sigma_delta->irq_dis = true;
> >  	ret = request_irq(sigma_delta->spi->irq,
> >  			  ad_sd_data_rdy_trig_poll,
> > -			  sigma_delta->info->irq_flags,
> > +			  sigma_delta->info->irq_flags | IRQF_NO_AUTOEN,
> >  			  indio_dev->name,
> >  			  sigma_delta);
> >  	if (ret)
> >  		goto error_free_trig;
> > 
> > -	if (!sigma_delta->irq_dis) {
> > -		sigma_delta->irq_dis = true;
> > -		disable_irq_nosync(sigma_delta->spi->irq);
> > -	}
> >  	iio_trigger_set_drvdata(sigma_delta->trig, sigma_delta);
> > 
> >  	ret = iio_trigger_register(sigma_delta->trig);
> > --
> > 2.31.1  
> 
> Thanks
> Barry
> 

RE: [PATCH 7/7] iio:adc:ad_sigma_delta: Use IRQF_NO_AUTOEN rather than request and disable

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Jonathan Cameron [mailto:jic23@kernel.org]
> Sent: Tuesday, April 6, 2021 2:52 AM
> To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>
> Cc: linux-iio@vger.kernel.org; Jonathan Cameron <jonathan.cameron@huawei.com>;
> Lars-Peter Clausen <lars@metafoo.de>; Alexandru Ardelean
> <ardeleanalex@gmail.com>; tiantao (H) <tiantao6@hisilicon.com>
> Subject: Re: [PATCH 7/7] iio:adc:ad_sigma_delta: Use IRQF_NO_AUTOEN rather than
> request and disable
> 
> On Fri, 2 Apr 2021 20:30:17 +0000
> "Song Bao Hua (Barry Song)" <song.bao.hua@hisilicon.com> wrote:
> 
> > > -----Original Message-----
> > > From: Jonathan Cameron [mailto:jic23@kernel.org]
> > > Sent: Saturday, April 3, 2021 7:46 AM
> > > To: linux-iio@vger.kernel.org
> > > Cc: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; Jonathan Cameron
> > > <jonathan.cameron@huawei.com>; Lars-Peter Clausen <lars@metafoo.de>;
> > > Alexandru Ardelean <ardeleanalex@gmail.com>
> > > Subject: [PATCH 7/7] iio:adc:ad_sigma_delta: Use IRQF_NO_AUTOEN rather than
> > > request and disable
> > >
> > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > >
> > > These devices are not able to mask the signal used as a data ready
> > > interrupt.  As such they previously requested the irq then immediately
> > > disabled it.  Now we can avoid the potential of a spurious interrupt
> > > by avoiding the irq being auto enabled in the first place.
> > >
> > > I'm not sure how this code could have been called with the irq already
> > > disabled, so I believe the conditional would always have been true and
> > > have removed it.
> > >
> >
> > If irq_dis is true, it seems the original code assumed interrupt was
> > open. Now the code is moving to disable-irq for true irq_dis. For
> > false irq_dis, this patch has no side effect so looks correct.
> Hi Barry,
> 
> As far as I can tell (and I looked closely :), at the point where
> ad_sd_probe_trigger() can be called, irq_dis is always false,
> so the original check is misleading by implying
> otherwise.

I didn't realize that. Thanks for clarification.

> 
> Taken in isolation of what is visible in this patch I'd agree
> the change you suggest makes sense, but I'd also like to clean
> up the wrong impression the original check was giving.  It is
> also worth noting that ad_sigma_delta.h lists irq_dis as a
> private member of the structure that should only be used in the
> library (reducing chances of any drivers in future doing something
> odd with it).
> 

Yes. It makes sense.

> The other checks on irq_dis look suspicious as well, but removing
> those would rather spread the scope of this patch.
> 
> As Lars has given an Reviewed-by and this is his code, I'm going
> to press ahead with this as it stands.
> 
> Applied to the togreg branch of iio.git and pushed out as testing
> for the autobuilders to see if we missed anything.
> 
> Thanks,
> 
> Jonathan
> 
> >
> > A safer way might be as below?
> >
> > if(!sigma_delta->irq_dis)
> > 	irq_flags = sigma_delta->info->irq_flags | IRQF_NO_AUTOEN;
> >
> > request_irq(irq_flags);
> >
> > sigma_delta->irq_dis =  true;
> >
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > Cc: Lars-Peter Clausen <lars@metafoo.de>
> > > Cc: Alexandru Ardelean <ardeleanalex@gmail.com>
> > > ---
> > >  drivers/iio/adc/ad_sigma_delta.c | 7 ++-----
> > >  1 file changed, 2 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/iio/adc/ad_sigma_delta.c
> > > b/drivers/iio/adc/ad_sigma_delta.c
> > > index 9289812c0a94..e777ec718973 100644
> > > --- a/drivers/iio/adc/ad_sigma_delta.c
> > > +++ b/drivers/iio/adc/ad_sigma_delta.c
> > > @@ -485,18 +485,15 @@ static int ad_sd_probe_trigger(struct iio_dev
> *indio_dev)
> > >  	sigma_delta->trig->ops = &ad_sd_trigger_ops;
> > >  	init_completion(&sigma_delta->completion);
> > >
> > > +	sigma_delta->irq_dis = true;
> > >  	ret = request_irq(sigma_delta->spi->irq,
> > >  			  ad_sd_data_rdy_trig_poll,
> > > -			  sigma_delta->info->irq_flags,
> > > +			  sigma_delta->info->irq_flags | IRQF_NO_AUTOEN,
> > >  			  indio_dev->name,
> > >  			  sigma_delta);
> > >  	if (ret)
> > >  		goto error_free_trig;
> > >
> > > -	if (!sigma_delta->irq_dis) {
> > > -		sigma_delta->irq_dis = true;
> > > -		disable_irq_nosync(sigma_delta->spi->irq);
> > > -	}
> > >  	iio_trigger_set_drvdata(sigma_delta->trig, sigma_delta);
> > >
> > >  	ret = iio_trigger_register(sigma_delta->trig);
> > > --
> > > 2.31.1
> >

Thanks
Barry

Re: [PATCH 0/7] iio: Use IRQF_NO_AUTOEN

[Andy Shevchenko]

On Fri, Apr 2, 2021 at 9:48 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> This series is dependant on
> cbe16f35bee68 ("genirq: Add IRQF_NO_AUTOEN for request_irq/nmi()")
> which is available in an immutable tag in the tip tree.
> https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/tag/?h=irq-no-autoen-2021-03-25
> which I'll merge in to the IIO tree if we need it before it's available
> upstream.
>
> That patch introduces a new IRQF_NO_AUTOEN flag for irq requests to avoid
> the current dance where we either mark an irq as not to be autoenabled before
> we know if we can actually request it succesfully, or (as IIO drivers seem to

successfully

> have gone with) we disable the interrupt just after requesting it.
> In short the flag stops the interrupt being autoenabled in the first place.
>
> So this series applies this magic to IIO :)
>
> Note these are all just compile tested and some of them aren't entirely
> trivial because of other aspects of the irq flag handling.

I have looked at them and indeed in all cases it's clearer now what is going on.
FWIW,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Jonathan Cameron (7):
>   iio:adc:ad7766: Use new IRQF_NO_AUTOEN to reduce boilerplate
>   iio:adc:exynos-adc: Use new IRQF_NO_AUTOEN flag rather than separate
>     irq_disable()
>   iio:adc:nau7802: Use IRQF_NO_AUTOEN instead of request then disable
>   iio:adc:sun4i-gpadc: Use new IRQF_NO_AUTOEN flag instead of request
>     then disable
>   iio:chemical:scd30: Use IRQF_NO_AUTOEN to avoid irq request then
>     disable
>   iio:imu:adis: Use IRQF_NO_AUTOEN instead of irq request then disable
>   iio:adc:ad_sigma_delta: Use IRQF_NO_AUTOEN rather than request and
>     disable
>
>  drivers/iio/adc/ad7766.c          | 15 +++++++--------
>  drivers/iio/adc/ad_sigma_delta.c  |  7 ++-----
>  drivers/iio/adc/exynos_adc.c      |  4 ++--
>  drivers/iio/adc/nau7802.c         |  6 +++---
>  drivers/iio/adc/sun4i-gpadc-iio.c |  4 ++--
>  drivers/iio/chemical/scd30_core.c | 16 ++++++++--------
>  drivers/iio/imu/adis16460.c       |  4 ++--
>  drivers/iio/imu/adis16475.c       |  5 +++--
>  drivers/iio/imu/adis_trigger.c    | 11 ++++++-----
>  9 files changed, 35 insertions(+), 37 deletions(-)
>
> --
> 2.31.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/7] iio: Use IRQF_NO_AUTOEN

[Lars-Peter Clausen]

On 4/2/21 8:45 PM, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> This series is dependant on
> cbe16f35bee68 ("genirq: Add IRQF_NO_AUTOEN for request_irq/nmi()")
> which is available in an immutable tag in the tip tree.
> https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/tag/?h=irq-no-autoen-2021-03-25
> which I'll merge in to the IIO tree if we need it before it's available
> upstream.
>
> That patch introduces a new IRQF_NO_AUTOEN flag for irq requests to avoid
> the current dance where we either mark an irq as not to be autoenabled before
> we know if we can actually request it succesfully, or (as IIO drivers seem to
> have gone with) we disable the interrupt just after requesting it.
> In short the flag stops the interrupt being autoenabled in the first place.
>
> So this series applies this magic to IIO :)
>
> Note these are all just compile tested and some of them aren't entirely
> trivial because of other aspects of the irq flag handling.

Something like IRQF_NO_AUTOEN has been on my wish list for a long time. 
Thanks Barry!

Series looks good.

Reviewed-by: Lars-Peter Clausen <lars@metafoo.de>