Hi Jonathan,
Jonathan Cameron <jic23@xxxxxxxxxx> wrote on Mon, 30 Aug 2021 11:50:46
+0100:
> On Wed, 18 Aug 2021 13:11:38 +0200
> Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
>
> > Now that max1027_trigger_handler() has been freed from handling hardware
> > triggers EOC situations, we can use it for what it has been designed in
> > the first place: trigger software originated conversions.
>
> As mentioned earlier, this is not how I'd normally expect this sort of
> case to be handled. I'd be expecting the cnvst trigger to still be calling
> this function and the function to do the relevant check to ensure it
> knows the data is already available in that case.
I tried to follow your advice and Nuno's regarding this, I hope the new
version will match your expectations (new version coming soon).
However if my changes do not match, I will probably need more guidance
to understand in deep what you suggest.
> > In other
> > words, when userspace initiates a conversion with a sysfs trigger or a
> > hrtimer trigger, we must do all configuration steps, ie:
> > 1- Configuring the trigger
> > 2- Configuring the channels to scan
> > 3- Starting the conversion (actually done automatically by step 2 in
> > this case)
> > 4- Waiting for the conversion to end
> > 5- Retrieving the data from the ADC
> > 6- Push the data to the IIO core and notify it
> >
> > Add the missing steps to this helper and drop the trigger verification
> > hook otherwise software triggers would simply not be accepted at all.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
> > ---
> > drivers/iio/adc/max1027.c | 26 ++++++++++++++------------
> > 1 file changed, 14 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> > index 8c5995ae59f2..bb437e43adaf 100644
> > --- a/drivers/iio/adc/max1027.c
> > +++ b/drivers/iio/adc/max1027.c
> > @@ -413,17 +413,6 @@ static int max1027_debugfs_reg_access(struct iio_dev *indio_dev,
> > return spi_write(st->spi, val, 1);
> > }
> >
> > -static int max1027_validate_trigger(struct iio_dev *indio_dev,
> > - struct iio_trigger *trig)
> > -{
> > - struct max1027_state *st = iio_priv(indio_dev);
> > -
> > - if (st->trig != trig)
> > - return -EINVAL;
> > -
> > - return 0;
> > -}
> > -
> > static int max1027_set_cnvst_trigger_state(struct iio_trigger *trig, bool state)
> > {
> > struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> > @@ -512,7 +501,21 @@ static irqreturn_t max1027_trigger_handler(int irq, void *private)
> >
> > pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq, private);
> >
> > + ret = max1027_configure_trigger(indio_dev);
>
> I'd not expect to see this ever time. The configuration shouldn't change
> from one call of this function to the next.
True, this is not needed.
> > + if (ret)
> > + goto out;
> > +
> > + ret = max1027_configure_chans_to_scan(indio_dev);
>
> This should also not change unless it is also responsible for the 'go' signal.
> If that's true then it is badly named.
It's responsible for the go signal, I renamed it "configure_and_start".
However, just for my own understanding, when would I be supposed to
configure the channels requested by the user otherwise?
Thanks,
Miquèl
