Message-ID: <20250727163806.1e76c14f@jic23-huawei>
Date: Sun, 27 Jul 2025 16:38:06 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Jonathan Santos <jonath4nns@...il.com>
Cc: 20250719130844.7559e322@...23-huawei.smtp.subspace.kernel.org, Nuno
 Sá <noname.nuno@...il.com>, Jonathan Santos
 <Jonathan.Santos@...log.com>, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org, lars@...afoo.de,
 Michael.Hennerich@...log.com, dlechner@...libre.com, nuno.sa@...log.com,
 andy@...nel.org, "Uwe Kleine-König"
 <u.kleine-koenig@...libre.com>
Subject: Re: [PATCH] iio: adc: ad7768-1: disable IRQ autoenable

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

> On 07/19, Jonathan Cameron wrote:
> > On Fri, 18 Jul 2025 10:18:56 +0100
> > Nuno Sá <noname.nuno@...il.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@...log.com>
> > > > ---    
> > > 
> > > LGTM,
> > > 
> > > Reviewed-by: Nuno Sá <nuno.sa@...log.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
> > > >     
> > >   
> >   
> 

Powered by blists - more mailing lists