iio: adc: ad7768-1: disable IRQ autoenable

[PATCH] iio: adc: ad7768-1: disable IRQ autoenable

Posted by Jonathan Santos 2 months, 2 weeks ago

The device continuously converts data while powered up, generating
interrupts in the background. Configure the IRQ to be enabled and
disabled manually as needed to avoid unnecessary CPU load.

Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
---
 drivers/iio/adc/ad7768-1.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
index a2e061f0cb08..3eea03c004c3 100644
--- a/drivers/iio/adc/ad7768-1.c
+++ b/drivers/iio/adc/ad7768-1.c
@@ -395,8 +395,10 @@ static int ad7768_scan_direct(struct iio_dev *indio_dev)
 	if (ret < 0)
 		return ret;
 
+	enable_irq(st->spi->irq);
 	ret = wait_for_completion_timeout(&st->completion,
 					  msecs_to_jiffies(1000));
+	disable_irq(st->spi->irq);
 	if (!ret)
 		return -ETIMEDOUT;
 
@@ -1130,7 +1132,21 @@ static const struct iio_buffer_setup_ops ad7768_buffer_ops = {
 	.predisable = &ad7768_buffer_predisable,
 };
 
+static int ad7768_set_trigger_state(struct iio_trigger *trig, bool enable)
+{
+	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+	struct ad7768_state *st = iio_priv(indio_dev);
+
+	if (enable)
+		enable_irq(st->spi->irq);
+	else
+		disable_irq(st->spi->irq);
+
+	return 0;
+}
+
 static const struct iio_trigger_ops ad7768_trigger_ops = {
+	.set_trigger_state = ad7768_set_trigger_state,
 	.validate_device = iio_trigger_validate_own_device,
 };
 
@@ -1417,7 +1433,7 @@ static int ad7768_probe(struct spi_device *spi)
 
 	ret = devm_request_irq(&spi->dev, spi->irq,
 			       &ad7768_interrupt,
-			       IRQF_TRIGGER_RISING | IRQF_ONESHOT,
+			       IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN,
 			       indio_dev->name, indio_dev);
 	if (ret)
 		return ret;

base-commit: 0a686b9c4f847dc21346df8e56d5b119918fefef
-- 
2.34.1

Re: [PATCH] iio: adc: ad7768-1: disable IRQ autoenable

Posted by Nuno Sá 2 months, 2 weeks ago

On Thu, Jul 17, 2025 at 10:33:07PM -0300, Jonathan Santos wrote:
> The device continuously converts data while powered up, generating
> interrupts in the background. Configure the IRQ to be enabled and
> disabled manually as needed to avoid unnecessary CPU load.
> 
> Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> ---

LGTM,

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

>  drivers/iio/adc/ad7768-1.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> index a2e061f0cb08..3eea03c004c3 100644
> --- a/drivers/iio/adc/ad7768-1.c
> +++ b/drivers/iio/adc/ad7768-1.c
> @@ -395,8 +395,10 @@ static int ad7768_scan_direct(struct iio_dev *indio_dev)
>  	if (ret < 0)
>  		return ret;
>  
> +	enable_irq(st->spi->irq);
>  	ret = wait_for_completion_timeout(&st->completion,
>  					  msecs_to_jiffies(1000));
> +	disable_irq(st->spi->irq);
>  	if (!ret)
>  		return -ETIMEDOUT;
>  
> @@ -1130,7 +1132,21 @@ static const struct iio_buffer_setup_ops ad7768_buffer_ops = {
>  	.predisable = &ad7768_buffer_predisable,
>  };
>  
> +static int ad7768_set_trigger_state(struct iio_trigger *trig, bool enable)
> +{
> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> +	struct ad7768_state *st = iio_priv(indio_dev);
> +
> +	if (enable)
> +		enable_irq(st->spi->irq);
> +	else
> +		disable_irq(st->spi->irq);
> +
> +	return 0;
> +}
> +
>  static const struct iio_trigger_ops ad7768_trigger_ops = {
> +	.set_trigger_state = ad7768_set_trigger_state,
>  	.validate_device = iio_trigger_validate_own_device,
>  };
>  
> @@ -1417,7 +1433,7 @@ static int ad7768_probe(struct spi_device *spi)
>  
>  	ret = devm_request_irq(&spi->dev, spi->irq,
>  			       &ad7768_interrupt,
> -			       IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> +			       IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN,
>  			       indio_dev->name, indio_dev);
>  	if (ret)
>  		return ret;
> 
> base-commit: 0a686b9c4f847dc21346df8e56d5b119918fefef
> -- 
> 2.34.1
>

Re: [PATCH] iio: adc: ad7768-1: disable IRQ autoenable

Posted by Jonathan Cameron 2 months, 2 weeks ago

On Fri, 18 Jul 2025 10:18:56 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Thu, Jul 17, 2025 at 10:33:07PM -0300, Jonathan Santos wrote:
> > The device continuously converts data while powered up, generating
> > interrupts in the background. Configure the IRQ to be enabled and
> > disabled manually as needed to avoid unnecessary CPU load.

This generates interrupts continuously even when in oneshot mode?

> > 
> > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> > ---  
> 
> LGTM,
> 
> Reviewed-by: Nuno Sá <nuno.sa@analog.com>
> 
> >  drivers/iio/adc/ad7768-1.c | 18 +++++++++++++++++-
> >  1 file changed, 17 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> > index a2e061f0cb08..3eea03c004c3 100644
> > --- a/drivers/iio/adc/ad7768-1.c
> > +++ b/drivers/iio/adc/ad7768-1.c
> > @@ -395,8 +395,10 @@ static int ad7768_scan_direct(struct iio_dev *indio_dev)
> >  	if (ret < 0)
> >  		return ret;
> >  
> > +	enable_irq(st->spi->irq);

Looks racey to me in a number of ways.

Before this patch:
In continuous mode, reinit_completion called then interrupt before we enter
 oneshot mode. What was captured? 

After this patch
Oneshot mode starts - hardware interrupt happens but enable_irq() is not set
so we miss it - or do we get another pulse later?

I'm not sure how to solve this as a device generating a stream of garbage
interrupts is near impossible to deal with.

I'm not following the datasheet description of this features vs the code 
though. It refers to oneshot mode requiring a pulse on sync in which I can't
find.

> >  	ret = wait_for_completion_timeout(&st->completion,
> >  					  msecs_to_jiffies(1000));
> > +	disable_irq(st->spi->irq);
> >  	if (!ret)
> >  		return -ETIMEDOUT;
> >  
> > @@ -1130,7 +1132,21 @@ static const struct iio_buffer_setup_ops ad7768_buffer_ops = {
> >  	.predisable = &ad7768_buffer_predisable,
> >  };
> >  
> > +static int ad7768_set_trigger_state(struct iio_trigger *trig, bool enable)
> > +{
> > +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> > +	struct ad7768_state *st = iio_priv(indio_dev);
> > +
> > +	if (enable)
> > +		enable_irq(st->spi->irq);
> > +	else
> > +		disable_irq(st->spi->irq);
> > +
> > +	return 0;
> > +}
> > +
> >  static const struct iio_trigger_ops ad7768_trigger_ops = {
> > +	.set_trigger_state = ad7768_set_trigger_state,
> >  	.validate_device = iio_trigger_validate_own_device,
> >  };
> >  
> > @@ -1417,7 +1433,7 @@ static int ad7768_probe(struct spi_device *spi)
> >  
> >  	ret = devm_request_irq(&spi->dev, spi->irq,
> >  			       &ad7768_interrupt,
> > -			       IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> > +			       IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN,

Why drop oneshot?

> >  			       indio_dev->name, indio_dev);
> >  	if (ret)
> >  		return ret;
> > 
> > base-commit: 0a686b9c4f847dc21346df8e56d5b119918fefef
> > -- 
> > 2.34.1
> >   
>

Re: [PATCH] iio: adc: ad7768-1: disable IRQ autoenable

Posted by Jonathan Santos 2 months, 1 week ago

On 07/19, Jonathan Cameron wrote:
> On Fri, 18 Jul 2025 10:18:56 +0100
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> > On Thu, Jul 17, 2025 at 10:33:07PM -0300, Jonathan Santos wrote:
> > > The device continuously converts data while powered up, generating
> > > interrupts in the background. Configure the IRQ to be enabled and
> > > disabled manually as needed to avoid unnecessary CPU load.
> 
> This generates interrupts continuously even when in oneshot mode?
> 

No, but oneshot mode is only enabled for a brief moment when reading the raw data. The rest of the time it stays in continuous conversion mode because datasheet says it is necessary to make configuration changes.

> > > 
> > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> > > ---  
> > 
> > LGTM,
> > 
> > Reviewed-by: Nuno Sá <nuno.sa@analog.com>
> > 
> > >  drivers/iio/adc/ad7768-1.c | 18 +++++++++++++++++-
> > >  1 file changed, 17 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> > > index a2e061f0cb08..3eea03c004c3 100644
> > > --- a/drivers/iio/adc/ad7768-1.c
> > > +++ b/drivers/iio/adc/ad7768-1.c
> > > @@ -395,8 +395,10 @@ static int ad7768_scan_direct(struct iio_dev *indio_dev)
> > >  	if (ret < 0)
> > >  		return ret;
> > >  
> > > +	enable_irq(st->spi->irq);
> 
> Looks racey to me in a number of ways.
> 
> Before this patch:
> In continuous mode, reinit_completion called then interrupt before we enter
>  oneshot mode. What was captured? 
> 
> After this patch
> Oneshot mode starts - hardware interrupt happens but enable_irq() is not set
> so we miss it - or do we get another pulse later?
> 

After some debugging, i think the device gets the last interrupt before
getting into oneshot mode because I don't see any DRDY later. it should have a sync_in pulse after to
update the data value, as you said.

> I'm not sure how to solve this as a device generating a stream of garbage
> interrupts is near impossible to deal with.
> 
> I'm not following the datasheet description of this features vs the code 
> though. It refers to oneshot mode requiring a pulse on sync in which I can't
> find.
> 

If we made something like this wouldn't sufice?

...
	reinit_completion(&st->completion);

	ret = ad7768_set_mode(st, AD7768_ONE_SHOT);
	if (ret < 0)
		return ret;

	enable_irq(st->spi->irq);
	ad7768_send_sync_pulse(st);

	ret = wait_for_completion_timeout(&st->completion,
					  msecs_to_jiffies(1000));
	disable_irq(st->spi->irq);
	if (!ret)
		return -ETIMEDOUT;
...


> > >  	ret = wait_for_completion_timeout(&st->completion,
> > >  					  msecs_to_jiffies(1000));
> > > +	disable_irq(st->spi->irq);
> > >  	if (!ret)
> > >  		return -ETIMEDOUT;
> > >  
> > > @@ -1130,7 +1132,21 @@ static const struct iio_buffer_setup_ops ad7768_buffer_ops = {
> > >  	.predisable = &ad7768_buffer_predisable,
> > >  };
> > >  
> > > +static int ad7768_set_trigger_state(struct iio_trigger *trig, bool enable)
> > > +{
> > > +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> > > +	struct ad7768_state *st = iio_priv(indio_dev);
> > > +
> > > +	if (enable)
> > > +		enable_irq(st->spi->irq);
> > > +	else
> > > +		disable_irq(st->spi->irq);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static const struct iio_trigger_ops ad7768_trigger_ops = {
> > > +	.set_trigger_state = ad7768_set_trigger_state,
> > >  	.validate_device = iio_trigger_validate_own_device,
> > >  };
> > >  
> > > @@ -1417,7 +1433,7 @@ static int ad7768_probe(struct spi_device *spi)
> > >  
> > >  	ret = devm_request_irq(&spi->dev, spi->irq,
> > >  			       &ad7768_interrupt,
> > > -			       IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> > > +			       IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN,
> 
> Why drop oneshot?
> 
> > >  			       indio_dev->name, indio_dev);
> > >  	if (ret)
> > >  		return ret;
> > > 
> > > base-commit: 0a686b9c4f847dc21346df8e56d5b119918fefef
> > > -- 
> > > 2.34.1
> > >   
> > 
>

Re: [PATCH] iio: adc: ad7768-1: disable IRQ autoenable

Posted by Jonathan Cameron 2 months, 1 week ago

On Fri, 25 Jul 2025 17:01:24 -0300
Jonathan Santos <jonath4nns@gmail.com> wrote:

> On 07/19, Jonathan Cameron wrote:
> > On Fri, 18 Jul 2025 10:18:56 +0100
> > Nuno Sá <noname.nuno@gmail.com> wrote:
> >   
> > > On Thu, Jul 17, 2025 at 10:33:07PM -0300, Jonathan Santos wrote:  
> > > > The device continuously converts data while powered up, generating
> > > > interrupts in the background. Configure the IRQ to be enabled and
> > > > disabled manually as needed to avoid unnecessary CPU load.  
> > 
> > This generates interrupts continuously even when in oneshot mode?
> >   
> 
> No, but oneshot mode is only enabled for a brief moment when reading the raw data. The rest of the time it stays in continuous conversion mode because datasheet says it is necessary to make configuration changes.

Hmm.  So if we want to suppress interrupts we would need to switch out of 
continuous mode.  That might be helpful, though with the sync in pulse
in the mix we might not need it.  There are complications though.

> 
> > > > 
> > > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> > > > ---    
> > > 
> > > LGTM,
> > > 
> > > Reviewed-by: Nuno Sá <nuno.sa@analog.com>
> > >   
> > > >  drivers/iio/adc/ad7768-1.c | 18 +++++++++++++++++-
> > > >  1 file changed, 17 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> > > > index a2e061f0cb08..3eea03c004c3 100644
> > > > --- a/drivers/iio/adc/ad7768-1.c
> > > > +++ b/drivers/iio/adc/ad7768-1.c
> > > > @@ -395,8 +395,10 @@ static int ad7768_scan_direct(struct iio_dev *indio_dev)
> > > >  	if (ret < 0)
> > > >  		return ret;
> > > >  
> > > > +	enable_irq(st->spi->irq);  
> > 
> > Looks racey to me in a number of ways.
> > 
> > Before this patch:
> > In continuous mode, reinit_completion called then interrupt before we enter
> >  oneshot mode. What was captured? 
> > 
> > After this patch
> > Oneshot mode starts - hardware interrupt happens but enable_irq() is not set
> > so we miss it - or do we get another pulse later?
> >   
> 
> After some debugging, i think the device gets the last interrupt before
> getting into oneshot mode because I don't see any DRDY later. it should have a sync_in pulse after to
> update the data value, as you said.
> 
> > I'm not sure how to solve this as a device generating a stream of garbage
> > interrupts is near impossible to deal with.
> > 
> > I'm not following the datasheet description of this features vs the code 
> > though. It refers to oneshot mode requiring a pulse on sync in which I can't
> > find.
> >   
> 
> If we made something like this wouldn't sufice?

Yes. I think that looks fine but it is relying on particular behavior
of the interrupt controller.

> 
> ...
> 	reinit_completion(&st->completion);
> 
> 	ret = ad7768_set_mode(st, AD7768_ONE_SHOT);
> 	if (ret < 0)
> 		return ret;
> 
> 	enable_irq(st->spi->irq);

In some interrupt controllers, IIRC interrupts are annoyingly queued
when disable_irq() has been called, so when you next enable_irq() you
may immediately get one. If it happens, should happen very fast though
I'm not 100% sure it will happen before we return to here which makes
dealing with that race hard.

Do we have a way to check the interrupt was due to the sync pulse?
If not maybe we need a flag that we set here - but that will race
with a slow response to a previously queued interrupt. Maybe that case
doesn't actually exist though - I'm not sure we got that far with
analysizing the guarantees. 

+CC Uwe who I think was dealing with this a while back with the
ad_sigma_delta library and might remember it a clearer than me.

https://lore.kernel.org/all/io53lznz3qp3jd5rohqsjhosnmdzd6d44sdbwu5jcfrs3rz2a2@orquwgflrtyc/
was the main thread in which Thomas Gleixner replied.

Jonathan


> 	ad7768_send_sync_pulse(st);
> 
> 	ret = wait_for_completion_timeout(&st->completion,
> 					  msecs_to_jiffies(1000));
> 	disable_irq(st->spi->irq);
> 	if (!ret)
> 		return -ETIMEDOUT;
> ...
> 
> 
> > > >  	ret = wait_for_completion_timeout(&st->completion,
> > > >  					  msecs_to_jiffies(1000));
> > > > +	disable_irq(st->spi->irq);
> > > >  	if (!ret)
> > > >  		return -ETIMEDOUT;
> > > >  
> > > > @@ -1130,7 +1132,21 @@ static const struct iio_buffer_setup_ops ad7768_buffer_ops = {
> > > >  	.predisable = &ad7768_buffer_predisable,
> > > >  };
> > > >  
> > > > +static int ad7768_set_trigger_state(struct iio_trigger *trig, bool enable)
> > > > +{
> > > > +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> > > > +	struct ad7768_state *st = iio_priv(indio_dev);
> > > > +
> > > > +	if (enable)
> > > > +		enable_irq(st->spi->irq);
> > > > +	else
> > > > +		disable_irq(st->spi->irq);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  static const struct iio_trigger_ops ad7768_trigger_ops = {
> > > > +	.set_trigger_state = ad7768_set_trigger_state,
> > > >  	.validate_device = iio_trigger_validate_own_device,
> > > >  };
> > > >  
> > > > @@ -1417,7 +1433,7 @@ static int ad7768_probe(struct spi_device *spi)
> > > >  
> > > >  	ret = devm_request_irq(&spi->dev, spi->irq,
> > > >  			       &ad7768_interrupt,
> > > > -			       IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> > > > +			       IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN,  
> > 
> > Why drop oneshot?
> >   
> > > >  			       indio_dev->name, indio_dev);
> > > >  	if (ret)
> > > >  		return ret;
> > > > 
> > > > base-commit: 0a686b9c4f847dc21346df8e56d5b119918fefef
> > > > -- 
> > > > 2.34.1
> > > >     
> > >   
> >   
>