| Message ID | 20210921115408.66711-15-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Bring external triggers support to MAX1027-like ADCs | expand |
On Tue, 21 Sep 2021 13:54:06 +0200 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > So far the End-Of-Conversion interrupt was only used in conjunction with > the internal trigger to process the data. Let's extend the use of this > interrupt handler to support regular single-shot conversions as well. > > Doing so requires writing our own hard IRQ handler. This handler has to > check if buffers are enabled or not: > > *** Buffers disabled condition *** > > This means the user requested a single conversion and the sample is > ready to be retrieved. > > -> This implies adding the relevant completion boilerplate. > > *** Buffers enabled condition *** > > Triggers are used. So far there is only support for the internal > trigger but this trigger might soon be attached to another device as > well so it is the core duty to decide which handler to call in order > to process the data. The core will decide to either: > > * Call the internal trigger handler which will extract the data that > is already present in the ADC FIFOs > > or > > * Call the trigger handler of another driver when using this trigger > with another device, even though this call will be slightly delayed > by the fact that the max1027 IRQ is a data-ready interrupt rather > than a real trigger: > > -> The new handler will manually inform the core about the trigger > having transitioned by directly calling iio_trigger_poll() (which > iio_trigger_generic_data_rdy_poll() initially did). > > In order for the handler to be "source" agnostic, we also need to change > the private pointer and provide the IIO device instead of the trigger > object. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > > Jonathan, > > I hope this fits the IIO model now. In order to be sure I got the big > picture I first refused to look at your code snippets. Just with your > "plain english" explanations I wrote most of these three patches, before > checking back that they were indeed fully aligned with your examples. I > truly hope they do now, but do not hesitate if I missed something. Looks great to me (in fact I just applied it but I'll reply to the cover letter shortly for that). Thanks for persisting with this and I'm looking forward to that blog you mentioned. If you have time / inclination to help improve the documentation in the kernel tree that would also be great. This discussion has made it clear to me that it would be great to have a set of 'patterns' for common types of device + how we map them onto the model of IIO (particularly when they don't quite fit that idealised model). There are similar compromises around when to use multiple buffers for instance. It is always on the list of things to work on but somehow there is always something else more urgent :( Thanks, Jonathan > > Cheers, > Miquèl > > drivers/iio/adc/max1027.c | 43 +++++++++++++++++++++++++++++++++++---- > 1 file changed, 39 insertions(+), 4 deletions(-) > > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c > index 84217e18ef70..0fa7b0fbdba0 100644 > --- a/drivers/iio/adc/max1027.c > +++ b/drivers/iio/adc/max1027.c > @@ -270,15 +270,26 @@ struct max1027_state { > struct iio_trigger *trig; > __be16 *buffer; > struct mutex lock; > + struct completion complete; > > u8 reg ____cacheline_aligned; > }; > > 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; > > - usleep_range(conversion_time, conversion_time * 2); > + if (st->spi->irq) { > + ret = wait_for_completion_timeout(&st->complete, > + msecs_to_jiffies(1000)); > + reinit_completion(&st->complete); > + if (!ret) > + return ret; > + } else { > + usleep_range(conversion_time, conversion_time * 2); > + } > > return 0; > } > @@ -473,6 +484,30 @@ static int max1027_read_scan(struct iio_dev *indio_dev) > return 0; > } > > +static irqreturn_t max1027_handler(int irq, void *private) > +{ > + struct iio_dev *indio_dev = private; > + struct max1027_state *st = iio_priv(indio_dev); > + > + /* > + * If buffers are disabled (raw read), we just need to unlock the > + * waiters which will then handle the data. > + * > + * When using the internal trigger, we must hand-off the choice of the > + * handler to the core which will then lookup through the interrupt tree > + * for the right handler registered with iio_triggered_buffer_setup() > + * to execute, as this trigger might very well be used in conjunction > + * with another device. The core will then call the relevant handler to > + * perform the data processing step. > + */ > + if (!iio_buffer_enabled(indio_dev)) > + complete(&st->complete); > + else > + iio_trigger_poll(indio_dev->trig); > + > + return IRQ_HANDLED; > +} > + > static irqreturn_t max1027_trigger_handler(int irq, void *private) > { > struct iio_poll_func *pf = private; > @@ -517,6 +552,7 @@ static int max1027_probe(struct spi_device *spi) > st->info = &max1027_chip_info_tbl[spi_get_device_id(spi)->driver_data]; > > mutex_init(&st->lock); > + init_completion(&st->complete); > > indio_dev->name = spi_get_device_id(spi)->name; > indio_dev->info = &max1027_info; > @@ -560,10 +596,9 @@ static int max1027_probe(struct spi_device *spi) > return ret; > } > > - ret = devm_request_irq(&spi->dev, spi->irq, > - iio_trigger_generic_data_rdy_poll, > + ret = devm_request_irq(&spi->dev, spi->irq, max1027_handler, > IRQF_TRIGGER_FALLING, > - spi->dev.driver->name, st->trig); > + spi->dev.driver->name, indio_dev); > if (ret < 0) { > dev_err(&indio_dev->dev, "Failed to allocate IRQ.\n"); > return ret;
Hi Jonathan, jic23@kernel.org wrote on Sun, 26 Sep 2021 15:36:59 +0100: > On Tue, 21 Sep 2021 13:54:06 +0200 > Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > So far the End-Of-Conversion interrupt was only used in conjunction with > > the internal trigger to process the data. Let's extend the use of this > > interrupt handler to support regular single-shot conversions as well. > > > > Doing so requires writing our own hard IRQ handler. This handler has to > > check if buffers are enabled or not: > > > > *** Buffers disabled condition *** > > > > This means the user requested a single conversion and the sample is > > ready to be retrieved. > > > > -> This implies adding the relevant completion boilerplate. > > > > *** Buffers enabled condition *** > > > > Triggers are used. So far there is only support for the internal > > trigger but this trigger might soon be attached to another device as > > well so it is the core duty to decide which handler to call in order > > to process the data. The core will decide to either: > > > > * Call the internal trigger handler which will extract the data that > > is already present in the ADC FIFOs > > > > or > > > > * Call the trigger handler of another driver when using this trigger > > with another device, even though this call will be slightly delayed > > by the fact that the max1027 IRQ is a data-ready interrupt rather > > than a real trigger: > > > > -> The new handler will manually inform the core about the trigger > > having transitioned by directly calling iio_trigger_poll() (which > > iio_trigger_generic_data_rdy_poll() initially did). > > > > In order for the handler to be "source" agnostic, we also need to change > > the private pointer and provide the IIO device instead of the trigger > > object. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > > > Jonathan, > > > > I hope this fits the IIO model now. In order to be sure I got the big > > picture I first refused to look at your code snippets. Just with your > > "plain english" explanations I wrote most of these three patches, before > > checking back that they were indeed fully aligned with your examples. I > > truly hope they do now, but do not hesitate if I missed something. > > Looks great to me (in fact I just applied it but I'll reply to the cover letter > shortly for that). Great, thanks! > Thanks for persisting with this and I'm looking forward to that blog you > mentioned. Thank you for all the time you put in these reviews. I wrote most of it, it's not very long but I tried to cover most of the common interactions with the core. It is under internal review, I'll certainly show you its content before publishing in case you have comments or spot any mistake. > If you have time / inclination to help improve the documentation > in the kernel tree that would also be great. This discussion has made it > clear to me that it would be great to have a set of 'patterns' for common > types of device + how we map them onto the model of IIO (particularly > when they don't quite fit that idealised model). There are similar > compromises around when to use multiple buffers for instance. > > It is always on the list of things to work on but somehow there is always > something else more urgent :( Hehe :) I know, documentation is always a good-to-have item that is not often reached... I feel I need more occasions to work in the IIO area (for example with hardware fifos/buffers or events) before being able to fully appreciate the internal design, but I agree having a set of 'usual patterns' that drivers should conform with depending on the devices capabilities/constraints would be a must. In the mean time, let's start by clarifying this 'iio_dev->modes' property :) Thanks, Miquèl
diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c index 84217e18ef70..0fa7b0fbdba0 100644 --- a/drivers/iio/adc/max1027.c +++ b/drivers/iio/adc/max1027.c @@ -270,15 +270,26 @@ struct max1027_state { struct iio_trigger *trig; __be16 *buffer; struct mutex lock; + struct completion complete; u8 reg ____cacheline_aligned; }; 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; - usleep_range(conversion_time, conversion_time * 2); + if (st->spi->irq) { + ret = wait_for_completion_timeout(&st->complete, + msecs_to_jiffies(1000)); + reinit_completion(&st->complete); + if (!ret) + return ret; + } else { + usleep_range(conversion_time, conversion_time * 2); + } return 0; } @@ -473,6 +484,30 @@ static int max1027_read_scan(struct iio_dev *indio_dev) return 0; } +static irqreturn_t max1027_handler(int irq, void *private) +{ + struct iio_dev *indio_dev = private; + struct max1027_state *st = iio_priv(indio_dev); + + /* + * If buffers are disabled (raw read), we just need to unlock the + * waiters which will then handle the data. + * + * When using the internal trigger, we must hand-off the choice of the + * handler to the core which will then lookup through the interrupt tree + * for the right handler registered with iio_triggered_buffer_setup() + * to execute, as this trigger might very well be used in conjunction + * with another device. The core will then call the relevant handler to + * perform the data processing step. + */ + if (!iio_buffer_enabled(indio_dev)) + complete(&st->complete); + else + iio_trigger_poll(indio_dev->trig); + + return IRQ_HANDLED; +} + static irqreturn_t max1027_trigger_handler(int irq, void *private) { struct iio_poll_func *pf = private; @@ -517,6 +552,7 @@ static int max1027_probe(struct spi_device *spi) st->info = &max1027_chip_info_tbl[spi_get_device_id(spi)->driver_data]; mutex_init(&st->lock); + init_completion(&st->complete); indio_dev->name = spi_get_device_id(spi)->name; indio_dev->info = &max1027_info; @@ -560,10 +596,9 @@ static int max1027_probe(struct spi_device *spi) return ret; } - ret = devm_request_irq(&spi->dev, spi->irq, - iio_trigger_generic_data_rdy_poll, + ret = devm_request_irq(&spi->dev, spi->irq, max1027_handler, IRQF_TRIGGER_FALLING, - spi->dev.driver->name, st->trig); + spi->dev.driver->name, indio_dev); if (ret < 0) { dev_err(&indio_dev->dev, "Failed to allocate IRQ.\n"); return ret;
So far the End-Of-Conversion interrupt was only used in conjunction with the internal trigger to process the data. Let's extend the use of this interrupt handler to support regular single-shot conversions as well. Doing so requires writing our own hard IRQ handler. This handler has to check if buffers are enabled or not: *** Buffers disabled condition *** This means the user requested a single conversion and the sample is ready to be retrieved. -> This implies adding the relevant completion boilerplate. *** Buffers enabled condition *** Triggers are used. So far there is only support for the internal trigger but this trigger might soon be attached to another device as well so it is the core duty to decide which handler to call in order to process the data. The core will decide to either: * Call the internal trigger handler which will extract the data that is already present in the ADC FIFOs or * Call the trigger handler of another driver when using this trigger with another device, even though this call will be slightly delayed by the fact that the max1027 IRQ is a data-ready interrupt rather than a real trigger: -> The new handler will manually inform the core about the trigger having transitioned by directly calling iio_trigger_poll() (which iio_trigger_generic_data_rdy_poll() initially did). In order for the handler to be "source" agnostic, we also need to change the private pointer and provide the IIO device instead of the trigger object. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- Jonathan, I hope this fits the IIO model now. In order to be sure I got the big picture I first refused to look at your code snippets. Just with your "plain english" explanations I wrote most of these three patches, before checking back that they were indeed fully aligned with your examples. I truly hope they do now, but do not hesitate if I missed something. Cheers, Miquèl drivers/iio/adc/max1027.c | 43 +++++++++++++++++++++++++++++++++++---- 1 file changed, 39 insertions(+), 4 deletions(-)