Re: [PATCH v2 3/3] iio: adc: update ad7779 to use IIO backend

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 6 Aug 2025 22:25:00 +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 Ionna,

>  
> +static int ad7779_set_data_lines(struct iio_dev *indio_dev,
> +				 unsigned int num_lanes)
> +{
> +	struct ad7779_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = ad7779_set_sampling_frequency(st, num_lanes * AD7779_DEFAULT_SAMPLING_1LINE);
> +	if (ret)
> +		return ret;
> +
> +	ret = iio_backend_num_lanes_set(st->back, num_lanes);
> +	if (ret)
> +		return ret;
> +
> +	return ad7779_spi_write_mask(st, AD7779_REG_DOUT_FORMAT,
> +				    AD7779_DOUT_FORMAT_MSK,
> +				    FIELD_PREP(AD7779_DOUT_FORMAT_MSK, 2 - ilog2(num_lanes)));
> +}
> +
> +static int ad7779_conf_channels(struct iio_dev *indio_dev, const struct ad7779_state *st)

As per other branch of thread.  This needs a name that makes it more
obvious it's a probe time setup rather that anything to do with configuring
channels at runtime.

> +{
> +	struct iio_chan_spec *channels;
> +	struct device *dev = &st->spi->dev;
> +	int num_channels = ARRAY_SIZE(ad7779_channels);
> +
> +	channels = devm_kcalloc(dev, num_channels, sizeof(*channels), GFP_KERNEL);
devm_kmemdup_array()?

> +	if (!channels)
> +		return -ENOMEM;
> +
> +	memcpy(channels, st->chip_info->channels, num_channels * sizeof(struct iio_chan_spec));
> +
> +	for (int i = 0; i < 8; i++)

Andy got this, but make sure to get that 8 from somewhere. It's num_channels I think. 
Though given most of this code collapses into a single call to devm_kmemdup_array()
I think that local variable may not make much sense. ARRAY_SIZE may be clearer.

> +		channels[i].scan_type.endianness = IIO_CPU;
> +
> +	indio_dev->channels = channels;
> +
> +	return 0;
> +}
> +
> +static int setup_irq(struct ad7779_state *st, struct iio_dev *indio_dev)

As below. Do this in a precursor patch and give it a better name!
Pretty much everything in IIO driver gets namespaced with a driver
specific prefix. Makes future problems with new generic functions
much less likely and makes it clear what is local to the driver.

> +{
> +	int ret;
> +	struct device *dev = &st->spi->dev;
> +
> +	indio_dev->info = &ad7779_info;
> +	indio_dev->channels = st->chip_info->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, st->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));

return ad7779_spi_write_mask...

> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int setup_back(struct ad7779_state *st, struct iio_dev *indio_dev)
> +{
> +	struct device *dev = &st->spi->dev;
> +	int ret = -EINVAL;
> +	int num_lanes;
> +
> +	indio_dev->info = &ad7779_info_data;
> +
> +	ret = ad7779_conf_channels(indio_dev, st);
> +	if (ret)
> +		return ret;
> +
> +	st->back = devm_iio_backend_get(dev, NULL);
> +	if (IS_ERR(st->back)) {
> +		dev_err_probe(dev, ret, "failed to get iio backend");
ret == 0 which isn't what you want.
> +		return PTR_ERR(st->back);

I think Andy pointed this out as well.  I'm not being too careful to not
duplicate unless I happen to remember he covered something in the earlier
review.

> +	}
> +
> +	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 = device_property_read_u32(dev, "adi,num-lanes", &num_lanes);
> +	if (ret < 0)

Whilst it's a bit fiddly as this is an array read underneath, the fact
num_lanes is not null is enough to ensure if (ret) is fine here.

> +		return ad7779_set_data_lines(indio_dev, 4);
> +
> +	return ad7779_set_data_lines(indio_dev, num_lanes);
> +}
> +
>  static int ad7779_probe(struct spi_device *spi)
>  {
>  	struct iio_dev *indio_dev;
> @@ -760,9 +917,6 @@ 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");
> -
>  	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
>  	if (!indio_dev)
>  		return -ENOMEM;
> @@ -804,45 +958,14 @@ 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;
> +	if (device_property_present(dev, "io-backends"))
> +		ret = setup_back(st, indio_dev);
> +	else
> +		ret = setup_irq(st, indio_dev);

Do the factoring out of the setup_irq stuff in a precursor patch.
Then this one will just make it conditional as part of adding the new backend stuff.

setup_irq isn't a good name. It does a bunch of things that aren't
directly related to the irq.  Prefix the function as well
ad7779_setup_without_backend() maybe?


>  
> -	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;
>  
> @@ -936,3 +1059,4 @@ module_spi_driver(ad7779_driver);
>  MODULE_AUTHOR("Ramona Alexandra Nechita <ramona.nechita@analog.com>");
>  MODULE_DESCRIPTION("Analog Devices AD7779 ADC");
>  MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS("IIO_BACKEND");