LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Miquel Raynal <miquel.raynal@bootlin.com>
To: "Sa, Nuno" <Nuno.Sa@analog.com>
Cc: Jonathan Cameron <jic23@kernel.org>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	"linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 14/16] iio: adc: max1027: Consolidate the end of conversion helper
Date: Thu, 2 Sep 2021 17:12:32 +0200	[thread overview]
Message-ID: <20210902171232.450a6d62@xps13> (raw)
In-Reply-To: <MW4PR03MB6363F50DC6A4B5D682BA16E299CB9@MW4PR03MB6363.namprd03.prod.outlook.com>

Hi Nuno,

"Sa, Nuno" <Nuno.Sa@analog.com> wrote on Mon, 30 Aug 2021 12:44:48
+0000:

> > -----Original Message-----
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Monday, August 30, 2021 12:37 PM
> > To: Miquel Raynal <miquel.raynal@bootlin.com>
> > Cc: Lars-Peter Clausen <lars@metafoo.de>; Thomas Petazzoni
> > <thomas.petazzoni@bootlin.com>; linux-iio@vger.kernel.org; linux-
> > kernel@vger.kernel.org
> > Subject: Re: [PATCH 14/16] iio: adc: max1027: Consolidate the end of
> > conversion helper
> > 
> > On Wed, 18 Aug 2021 13:11:37 +0200
> > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >   
> > > Now that we have a dedicated handler for End Of Conversion  
> > interrupts,  
> > > let's create a second path:
> > > - Situation 1: we are using the external hardware trigger, a  
> > conversion  
> > >   has been triggered and the ADC pushed the data to its FIFO, we  
> > need to  
> > >   retrieve the data and push it to the IIO buffers.
> > > - Situation 2: we are not using the external hardware trigger, hence  
> > we  
> > >   are likely waiting in a blocked thread waiting for this interrupt to
> > >   happen: in this case we just wake up the waiting thread.
> > >
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  drivers/iio/adc/max1027.c | 20 +++++++++++++++++---
> > >  1 file changed, 17 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> > > index 8d86e77fb5db..8c5995ae59f2 100644
> > > --- a/drivers/iio/adc/max1027.c
> > > +++ b/drivers/iio/adc/max1027.c
> > > @@ -235,6 +235,7 @@ struct max1027_state {
> > >  	struct iio_trigger		*trig;
> > >  	__be16				*buffer;
> > >  	struct mutex			lock;
> > > +	bool				data_rdy;
> > >  	bool				cnvst_trigger;
> > >  	u8				reg ____cacheline_aligned;
> > >  };
> > > @@ -243,12 +244,22 @@ static  
> > DECLARE_WAIT_QUEUE_HEAD(max1027_queue);  
> > >
> > >  static int max1027_wait_eoc(struct iio_dev *indio_dev)
> > >  {
> > > +	struct max1027_state *st = iio_priv(indio_dev);
> > >  	unsigned int conversion_time =  
> > MAX1027_CONVERSION_UDELAY;  
> > > +	int ret;
> > >
> > > -	if (indio_dev->active_scan_mask)
> > > -		conversion_time *= hweight32(*indio_dev-
> > >active_scan_mask);
> > > +	if (st->spi->irq) {
> > > +		ret =  
> > wait_event_interruptible_timeout(max1027_queue,  
> > > +						       st->data_rdy, HZ /  
> > 1000);  
> > > +		st->data_rdy = false;
> > > +		if (ret == -ERESTARTSYS)
> > > +			return ret;
> > > +	} else {
> > > +		if (indio_dev->active_scan_mask)
> > > +			conversion_time *= hweight32(*indio_dev-
> > >active_scan_mask);
> > >
> > > -	usleep_range(conversion_time, conversion_time * 2);
> > > +		usleep_range(conversion_time, conversion_time * 2);
> > > +	}
> > >
> > >  	return 0;
> > >  }
> > > @@ -481,6 +492,9 @@ static irqreturn_t  
> > max1027_eoc_irq_handler(int irq, void *private)  
> > >  	if (st->cnvst_trigger) {
> > >  		ret = max1027_read_scan(indio_dev);
> > >  		iio_trigger_notify_done(indio_dev->trig);
> > > +	} else {
> > > +		st->data_rdy = true;
> > > +		wake_up(&max1027_queue);  
> > 
> > I can't see why a queue is appropriate for this.  Use a completion and
> > have
> > one per instance of the device.  No need for the flag etc in that case as
> > complete() means we have had an interrupt.
> >   
> 
> In the case that 'st-> cnvst_trigger' is not set but the spi IRQ
> is present, we will wait until we get 'wake_up()' called from here. I wonder if
> that is a good idea as the device own trigger is not being used. FWIW, I think this
> sync logic is a bit confusing... I would still use the normal trigger infrastructure
> ('iio_trigger_generic_data_rdy_poll()') and use the 'cnvst_trigger' flag in the
> trigger handler to manually start conversions + wait till eoc. But I might be missing
> something though.

I implemented it your way, but I think I found a situation that was not
fully handled (the 3rd), which makes the handler very complicated
as we need to handle all the following cases:
1/ no trigger, irq enabled -> single read EOC interrupt
2/ external trigger, no irq -> handle the whole conversion process
3/ external trigger, irq enabled -> handle the whole conversion process
   but also have a dedicated condition to handle the EOC interrupt
   properly (fortunately this is a threaded handler that can be
   preempted): we need to wait from the handler itself that the
   handler gets called again: the first time it is executed as
   "pollfunc", the second time as "EOC interrupt". In the second
   instance, call complete() in order to deliver the first running
   instance of the handler and continue until the reading part.
4/ cnvst trigger, irq enabled -> only reads the data.
5/ cnvst trigger, irq disabled -> not possible.

I added a lot of comments to make it clearer.

> Regarding this handler, I just realized that this is the hard IRQ handler which
> might end up calling 'max1027_read_scan()' which in turn calls 'spi_read()'. Am I
> missing something here?

I renamed it to make it clear, but this is already a threaded handler.

Thanks,
Miquèl

  parent reply	other threads:[~2021-09-02 15:12 UTC|newest]

Thread overview: 71+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-18 11:11 [PATCH 00/16] Bring software triggers support to MAX1027-like ADCs Miquel Raynal
2021-08-18 11:11 ` [PATCH 01/16] iio: adc: max1027: Fix wrong shift with 12-bit devices Miquel Raynal
2021-08-20  7:02   ` Sa, Nuno
2021-08-30  9:57     ` Jonathan Cameron
2021-08-18 11:11 ` [PATCH 02/16] iio: adc: max1027: Fix the number of max1X31 channels Miquel Raynal
2021-08-20  7:03   ` Sa, Nuno
2021-08-30 10:00     ` Jonathan Cameron
2021-08-18 11:11 ` [PATCH 03/16] iio: adc: max1027: Push only the requested samples Miquel Raynal
2021-08-20  7:10   ` Sa, Nuno
2021-08-30 10:07     ` Jonathan Cameron
2021-08-30 10:49       ` Sa, Nuno
2021-08-30 14:29         ` Jonathan Cameron
2021-08-30 15:02           ` Sa, Nuno
2021-09-01  8:12             ` Miquel Raynal
2021-09-04 14:06               ` Jonathan Cameron
2021-09-06  8:59               ` Sa, Nuno
2021-09-06 16:56                 ` Jonathan Cameron
2021-09-06 17:34                   ` Miquel Raynal
2021-08-30 10:06   ` Jonathan Cameron
2021-08-18 11:11 ` [PATCH 04/16] iio: adc: max1027: Lower conversion time Miquel Raynal
2021-08-20  7:12   ` Sa, Nuno
2021-08-30 10:10   ` Jonathan Cameron
2021-08-18 11:11 ` [PATCH 05/16] iio: adc: max1027: Drop extra warning message Miquel Raynal
2021-08-20  7:12   ` Sa, Nuno
2021-08-30 10:12   ` Jonathan Cameron
2021-08-18 11:11 ` [PATCH 06/16] iio: adc: max1027: Rename a helper Miquel Raynal
2021-08-20  7:13   ` Sa, Nuno
2021-08-18 11:11 ` [PATCH 07/16] iio: adc: max1027: Create a helper to configure the trigger Miquel Raynal
2021-08-20  7:16   ` Sa, Nuno
2021-08-30 10:16   ` Jonathan Cameron
2021-08-18 11:11 ` [PATCH 08/16] iio: adc: max1027: Explain better how the trigger state gets changed Miquel Raynal
2021-08-20  7:17   ` Sa, Nuno
2021-08-18 11:11 ` [PATCH 09/16] iio: adc: max1027: Create a helper to configure the channels to scan Miquel Raynal
2021-08-20  7:18   ` Sa, Nuno
2021-08-18 11:11 ` [PATCH 10/16] iio: adc: max1027: Prevent single channel accesses during buffer reads Miquel Raynal
2021-08-20  7:20   ` Sa, Nuno
2021-08-20  7:30     ` Sa, Nuno
2021-08-30 10:20       ` Jonathan Cameron
2021-09-02  8:56         ` Miquel Raynal
2021-08-18 11:11 ` [PATCH 11/16] iio: adc: max1027: Separate the IRQ handler from the read logic Miquel Raynal
2021-08-20  7:23   ` Sa, Nuno
2021-09-02  8:55     ` Miquel Raynal
2021-09-04 14:08       ` Jonathan Cameron
2021-08-18 11:11 ` [PATCH 12/16] iio: adc: max1027: Introduce an end of conversion helper Miquel Raynal
2021-08-20  7:28   ` Sa, Nuno
2021-09-02  9:26     ` Miquel Raynal
2021-08-30 10:34   ` Jonathan Cameron
2021-08-18 11:11 ` [PATCH 13/16] iio: adc: max1027: Prepare re-using the EOC interrupt Miquel Raynal
2021-08-20  7:31   ` Sa, Nuno
2021-08-30 10:30   ` Jonathan Cameron
2021-08-30 10:47   ` Jonathan Cameron
2021-08-18 11:11 ` [PATCH 14/16] iio: adc: max1027: Consolidate the end of conversion helper Miquel Raynal
2021-08-20  7:45   ` Sa, Nuno
2021-08-30 10:37   ` Jonathan Cameron
2021-08-30 12:44     ` Sa, Nuno
2021-08-30 14:32       ` Jonathan Cameron
2021-09-02 15:12       ` Miquel Raynal [this message]
2021-09-03 14:28         ` Sa, Nuno
2021-09-03 14:46           ` Miquel Raynal
2021-09-05  9:41             ` Jonathan Cameron
2021-09-06  9:12               ` Sa, Nuno
2021-09-06  9:30                 ` Sa, Nuno
2021-08-18 11:11 ` [PATCH 15/16] iio: adc: max1027: Support software triggers Miquel Raynal
2021-08-20  7:58   ` Sa, Nuno
2021-09-02 12:25     ` Miquel Raynal
2021-09-03 14:20       ` Sa, Nuno
2021-08-30 10:50   ` Jonathan Cameron
2021-09-02 15:21     ` Miquel Raynal
2021-09-05  9:43       ` Jonathan Cameron
2021-08-18 11:11 ` [PATCH 16/16] iio: adc: max1027: Enable software triggers to be used without IRQ Miquel Raynal
2021-08-30 10:54   ` Jonathan Cameron

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210902171232.450a6d62@xps13 \
    --to=miquel.raynal@bootlin.com \
    --cc=Nuno.Sa@analog.com \
    --cc=jic23@kernel.org \
    --cc=lars@metafoo.de \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=thomas.petazzoni@bootlin.com \
    --subject='Re: [PATCH 14/16] iio: adc: max1027: Consolidate the end of conversion helper' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).