Re: [PATCH v3 09/10] iio: adc: ad7606: Add iio-backend support

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 04 Oct 2024 21:48:43 +0000
Guillaume Stols <gstols@baylibre.com> wrote:

> - Basic support for iio backend.
> - Supports IIO_CHAN_INFO_SAMP_FREQ R/W.
> - Only hardware mode is available, and that IIO_CHAN_INFO_RAW is not
>   supported if iio-backend mode is selected.
I don't much like the trivial window between this patch and the next
where the emulated mode is still there but the sleeps aren't adapting with sampling frequency.

Maybe it's worth a dance of leaving the write_raw support
until after this one so the frequency remains fixed until after
the fsleep(2) calls are gone?

There is another bit that I'm unsure is technically correct until after
the next patch.  Maybe I'm reading the diff wrong though!

Thanks,

J

> 
> Signed-off-by: Guillaume Stols <gstols@baylibre.com>
> ---
>  drivers/iio/adc/Kconfig      |   2 +
>  drivers/iio/adc/ad7606.c     | 124 +++++++++++++++++++++++++++++++++++++------
>  drivers/iio/adc/ad7606.h     |  15 ++++++
>  drivers/iio/adc/ad7606_par.c |  94 +++++++++++++++++++++++++++++++-
>  4 files changed, 219 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 4ab1a3092d88..9b52d5b2c592 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -224,9 +224,11 @@ config AD7606_IFACE_PARALLEL
>  	tristate "Analog Devices AD7606 ADC driver with parallel interface support"
>  	depends on HAS_IOPORT
>  	select AD7606
> +	select IIO_BACKEND
>  	help
>  	  Say yes here to build parallel interface support for Analog Devices:
>  	  ad7605-4, ad7606, ad7606-6, ad7606-4 analog to digital converters (ADC).
> +	  It also support iio_backended devices for AD7606B.
>  
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called ad7606_par.
> diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
> index 3666a58f8a6f..d86eb7c3e4f7 100644
> --- a/drivers/iio/adc/ad7606.c
> +++ b/drivers/iio/adc/ad7606.c
> @@ -21,6 +21,7 @@

> @@ -737,6 +773,10 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
>  			return ret;
>  
>  		return 0;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		if (val < 0 && val2 != 0)
> +			return -EINVAL;
> +		return ad7606_set_sampling_freq(st, val);

Currently I think  for the !backend + pwm case this can go out of
range for which that code works (fsleep removed in next patch).
Perhaps delay adding this until after that patch.
>  	default:
>  		return -EINVAL;
>  	}

> @@ -1108,7 +1186,24 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
>  					       st->cnvst_pwm);
>  		if (ret)
>  			return ret;
> +	}
> +
> +	if (st->bops->iio_backend_config) {
> +		/*
> +		 * If there is a backend, the PWM should not overpass the maximum sampling
> +		 * frequency the chip supports.
> +		 */
> +		ret = ad7606_set_sampling_freq(st,
> +					       chip_info->max_samplerate ? : 2 * KILO);
> +		if (ret)
> +			return ret;
> +
> +		ret = st->bops->iio_backend_config(dev, indio_dev);
> +		if (ret)
> +			return ret;
> +		indio_dev->setup_ops = &ad7606_pwm_buffer_ops;
>  	} else {
> +		init_completion(&st->completion);
>  		st->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
>  						  indio_dev->name,
>  						  iio_device_id(indio_dev));
It's a little hard to unwind the patches, but this was previously in the !pwm case.
At this point in the series we still allow the pwm case to work with ! backend.
So is this now running in that case?   Do we need a temporary additional check
on !pwm


> @@ -1126,15 +1221,14 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
>  						      &ad7606_buffer_ops);
>  		if (ret)
>  			return ret;
> +		ret = devm_request_threaded_irq(dev, irq,
> +						NULL,
> +						&ad7606_interrupt,
> +						IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +						chip_info->name, indio_dev);
> +		if (ret)
> +			return ret;
>  	}
> -	ret = devm_request_threaded_irq(dev, irq,
> -					NULL,
> -					&ad7606_interrupt,
> -					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> -					chip_info->name, indio_dev);
> -	if (ret)
> -		return ret;
> -
>  	return devm_iio_device_register(dev, indio_dev);
>  }
>  EXPORT_SYMBOL_NS_GPL(ad7606_probe, IIO_AD7606);

> diff --git a/drivers/iio/adc/ad7606_par.c b/drivers/iio/adc/ad7606_par.c
> index b87be2f1ca04..6042f6799272 100644
> --- a/drivers/iio/adc/ad7606_par.c
> +++ b/drivers/iio/adc/ad7606_par.c
> @@ -2,7 +2,8 @@
> +
> +static int ad7606_bi_setup_iio_backend(struct device *dev, struct iio_dev *indio_dev)
> +{
> +	struct ad7606_state *st = iio_priv(indio_dev);
> +	unsigned int ret, c;
> +	struct iio_backend_data_fmt data = {
> +		.sign_extend = true,
> +		.enable = true,
> +	};
> +
> +	st->back = devm_iio_backend_get(dev, NULL);
> +	if (IS_ERR(st->back))
> +		return PTR_ERR(st->back);
> +
> +	/* If the device is iio_backend powered the PWM is mandatory */
> +	if (!st->cnvst_pwm)
> +		return dev_err_probe(st->dev, -EINVAL,
> +				     "A PWM is mandatory when using backend.\n");
> +
> +	ret = devm_iio_backend_request_buffer(dev, st->back, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_iio_backend_enable(dev, st->back);
> +	if (ret)
> +		return ret;
> +
> +	for (c = 0; c < indio_dev->num_channels; c++) {
> +		ret = iio_backend_data_format_set(st->back, c, &data);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	indio_dev->channels = ad7606b_bi_channels;

Ultimately this may want to move into the chip_info structures as more devices are added
but this is fine for now I suppose.

> +	indio_dev->num_channels = 8;
> +
> +	return 0;
> +}