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

[Guillaume Stols <gstols@baylibre.com>]

On 10/5/24 13:53, Jonathan Cameron wrote:
> 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.

Hi Jonathan,

The sampling frequency can be adjusted only for the backend version, 
otherwise (including pwm+interrupt), there is no sysfs access to the 
sampling frequency (only available for AD7606_BI_CHANNEL).

>>   	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

mmm actually this should not be in a condition in the PWM  patch. Will 
fix this directly there.

>
>
>> @@ -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.
Will do this in a next series where support is added for the other chips.
>
>> +	indio_dev->num_channels = 8;
>> +
>> +	return 0;
>> +}