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

[Sa, Nuno]

> -----Original Message-----
> From: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
> Sent: Wednesday, August 18, 2021 1:12 PM
> To: Jonathan Cameron <jic23@xxxxxxxxxx>; Lars-Peter Clausen
> <lars@xxxxxxxxxx>
> Cc: Thomas Petazzoni <thomas.petazzoni@xxxxxxxxxxx>; linux-
> iio@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Miquel Raynal
> <miquel.raynal@xxxxxxxxxxx>
> Subject: [PATCH 15/16] iio: adc: max1027: Support software triggers
> 
> [External]
> 
> 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. 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);
> +	if (ret)
> +		goto out;
> +
> +	ret = max1027_configure_chans_to_scan(indio_dev);
> +	if (ret)
> +		goto out;
> +
> +	ret = max1027_wait_eoc(indio_dev);
> +	if (ret)
> +		goto out;
> +
>  	ret = max1027_read_scan(indio_dev);

There's something that I'm not getting... How are we checking that
we have software triggers? This API is called only if the device
allocates it's own trigger which will happen if there's a spi IRQ.

I'm probably missing something as this series is fairly big but the way
I would do it is (in the probe):

- always call 'devm_iio_triggered_buffer_setup()' function and properly use
buffer ops [1] (for example, you can use 'validate_scan_mask()' to setup the
channels to read);  
- only allocate a trigger if an IRQ is present in which case, we assume HW 
triggering is supported.

Does this makes sense?

- Nuno Sá