Re: [PATCH 15/16] iio: adc: max1027: Support software triggers

[Miquel Raynal]

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