Re: [PATCH 4/4] iio: adc: Update ad7779 to use IIO backend

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 28 Jul 2025 16:43:36 +0300
Ioana Risteiu <Ioana.Risteiu@analog.com> wrote:

> Add a new functionality to ad7779 driver that streams data through data
> output interface using IIO backend interface.
> 
> Signed-off-by: Ioana Risteiu <Ioana.Risteiu@analog.com>

Hi Ioana,

There is some new ABI (I think) in here.  I'm not clear what it is for and
ABI documentation is missing.

A few other minor comments inline.

Thanks,

Jonathan

> +static int ad7779_set_data_lines(struct iio_dev *indio_dev,
> +				 struct iio_chan_spec const *chan,
> +				 unsigned int mode)
> +{
> +	struct ad7779_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = ad7779_spi_write_mask(st, AD7779_REG_DOUT_FORMAT,
> +				    AD7779_DOUT_FORMAT_MSK,
> +				    FIELD_PREP(AD7779_DOUT_FORMAT_MSK, mode));
> +	switch (mode) {
> +	case AD7779_4LINES:
> +		ret = ad7779_set_sampling_frequency(st, AD7779_DEFAULT_SAMPLING_FREQ);
> +		if (ret)
> +			return ret;
> +		ret = iio_backend_num_lanes_set(st->back, 4);
> +		break;
> +	case AD7779_2LINES:
> +		ret = ad7779_set_sampling_frequency(st, AD7779_DEFAULT_SAMPLING_2LINE);
> +		if (ret)
> +			return ret;
> +		ret = iio_backend_num_lanes_set(st->back, 2);
> +		break;
> +	case AD7779_1LINE:
> +		ret = ad7779_set_sampling_frequency(st, AD7779_DEFAULT_SAMPLING_1LINE);
> +		if (ret)
> +			return ret;
> +		ret = iio_backend_num_lanes_set(st->back, 1);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +

	return ret;
or better yet, return in the various cases above.


> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}

t struct iio_chan_spec_ext_info ad7779_ext_info_filter[] = {
> +	IIO_ENUM("data_lines", IIO_SHARED_BY_ALL, &ad7779_data_lines_enum),

New ABI.  Needs documentation in Documentation/ABI/testing/sysfs-bus-iio*

I'm not clear why we need to expose it to userspace, and documentation
might explain that to me.


> +	IIO_ENUM_AVAILABLE("data_lines", IIO_SHARED_BY_ALL,
> +			   &ad7779_data_lines_enum),
> +	IIO_ENUM("filter_type", IIO_SHARED_BY_ALL, &ad7779_filter_enum),
> +	IIO_ENUM_AVAILABLE("filter_type", IIO_SHARED_BY_ALL,
> +			   &ad7779_filter_enum),
> +	{ }
> +};
>
> +
> +static int ad7779_register_back(struct ad7779_state *st, struct iio_dev *indio_dev)
> +{
> +	struct device *dev = &st->spi->dev;
> +	int ret = -EINVAL;
> +
> +	indio_dev->info = &ad7779_info_data;
> +
> +	if (strcmp(st->chip_info->name, "ad7771") == 0) {

Don't do string matching.  Much preferred to add another pointer + num_channels
to chip_info.  Just add backend_channels, num_backend_channels or something like that.


> +		indio_dev->channels = ad7779_channels_filter_data;
> +		indio_dev->num_channels = ARRAY_SIZE(ad7779_channels_filter_data);
> +	} else {
> +		indio_dev->channels = ad7779_channels_data;
> +		indio_dev->num_channels = ARRAY_SIZE(ad7779_channels_data);
> +	}
> +
> +	st->back = devm_iio_backend_get(dev, NULL);
> +	if (IS_ERR(st->back)) {
> +		dev_err_probe(dev, ret, "failed to get iio backend");
> +		return PTR_ERR(st->back);
return dev_err_probe() key being that your message here has ret
different to what you return.

> +	}
> +
> +	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;
> +
> +	ret = iio_backend_num_lanes_set(st->back, 4);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}
> +
>  static int ad7779_probe(struct spi_device *spi)
>  {
>  	struct iio_dev *indio_dev;
> @@ -760,8 +1009,8 @@ static int ad7779_probe(struct spi_device *spi)
>  	struct device *dev = &spi->dev;
>  	int ret = -EINVAL;
>  
> -	if (!spi->irq)
> -		return dev_err_probe(dev, ret, "DRDY irq not present\n");
> +	if (!spi->irq && !device_property_present(dev, "io-backends"))
> +		return dev_err_probe(dev, ret, "Either DRDY interrupt or io-backends property required\n");

Very long line. break before the string at least.

In the binding you aren't disallowing both the irq and io-backends.
That's fine but I wonder here if we should prioritize io-backends
if both are set? 

>  
>  	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
>  	if (!indio_dev)
> @@ -804,49 +1053,12 @@ static int ad7779_probe(struct spi_device *spi)
>  		return ret;
>  
>  	indio_dev->name = st->chip_info->name;
> -	indio_dev->info = &ad7779_info;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
> -	indio_dev->channels = st->chip_info->channels;
> -	indio_dev->num_channels = ARRAY_SIZE(ad7779_channels);
> -
> -	st->trig = devm_iio_trigger_alloc(dev, "%s-dev%d", indio_dev->name,
> -					  iio_device_id(indio_dev));
> -	if (!st->trig)
> -		return -ENOMEM;
> -
> -	st->trig->ops = &ad7779_trigger_ops;
> -
> -	iio_trigger_set_drvdata(st->trig, st);
> -
> -	ret = devm_request_irq(dev, spi->irq, iio_trigger_generic_data_rdy_poll,
> -			       IRQF_ONESHOT | IRQF_NO_AUTOEN, indio_dev->name,
> -			       st->trig);
> -	if (ret)
> -		return dev_err_probe(dev, ret, "request IRQ %d failed\n",
> -				     st->spi->irq);
> -
> -	ret = devm_iio_trigger_register(dev, st->trig);
> -	if (ret)
> -		return ret;
> -
> -	indio_dev->trig = iio_trigger_get(st->trig);
> -
> -	init_completion(&st->completion);
> -
> -	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> -					      &iio_pollfunc_store_time,
> -					      &ad7779_trigger_handler,
> -					      &ad7779_buffer_setup_ops);
> -	if (ret)
> -		return ret;
>  
> -	ret = ad7779_spi_write_mask(st, AD7779_REG_DOUT_FORMAT,
> -				    AD7779_DCLK_CLK_DIV_MSK,
> -				    FIELD_PREP(AD7779_DCLK_CLK_DIV_MSK, 7));
> -	if (ret)
> -		return ret;
> -
> -	return devm_iio_device_register(dev, indio_dev);

This call happens in bother cases. I'd be tempted to rename the functions
as setup_irq() / setup_back() and keep this call outside of either
function.

> +	if (spi->irq)
> +		return ad7779_register_irq(st, indio_dev);
> +	else
> +		return ad7779_register_back(st, indio_dev);
>  }