RE: [PATCH v6 4/4] iio: adc: ad4691: add SPI offload support

[Sabau, Radu bogdan]

> -----Original Message-----
> From: David Lechner <dlechner@xxxxxxxxxxxx>
> Sent: Saturday, April 4, 2026 6:57 PM

...

> > +
> >  #define AD4691_CHANNEL(ch)
> 	\
> >  	{								\
> >  		.type = IIO_VOLTAGE,					\
> > @@ -122,11 +155,9 @@ struct ad4691_chip_info {
> >  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE),
> 	\
> >  		.channel = ch,						\
> >  		.scan_index = ch,					\
> > -		.scan_type = {						\
> > -			.sign = 'u',					\
> > -			.realbits = 16,					\
> > -			.storagebits = 16,				\
> > -		},							\
> > +		.has_ext_scan_type = 1,
> 	\
> > +		.ext_scan_type = ad4691_scan_types,			\
> > +		.num_ext_scan_type = ARRAY_SIZE(ad4691_scan_types),
> 	\
> 
> Usually, we just make two separte ad4691_chip_info structs for offload
> vs. not offload.
> 
> ext_scan_type is generally only used when the scan type can change
> dynamically after probe.
> 

So, just to be clear, you are saying I should have different chip_info structs
and change the triggered-buffer for offload ones if offload is present?
I am asking since offload has different scan types as well, and this would
mean 3 different chip_info structs for each chip -> total of 12 chip_info structs,
each with a different channel array, or perhaps there is a more compact way
to have this implemented.
I could make the channel arrays use the same macro and have the scan_type
reversed to storage and shift done as parameters.

Please let me know your thoughts on this.

> >  	}
> >
> >  static const struct iio_chan_spec ad4691_channels[] = {
> > @@ -221,6 +252,17 @@ static const struct ad4691_chip_info
> ad4694_chip_info = {
> >  	.max_rate = 1 * HZ_PER_MHZ,
> >  };
> >

...

> >  }
> >
> > @@ -883,6 +1184,20 @@ static ssize_t sampling_frequency_store(struct
> device *dev,
> >  	if (iio_buffer_enabled(indio_dev))
> >  		return -EBUSY;
> >
> > +	if (st->manual_mode && st->offload) {
> > +		struct spi_offload_trigger_config config = {
> > +			.type = SPI_OFFLOAD_TRIGGER_PERIODIC,
> > +			.periodic = { .frequency_hz = freq },
> > +		};
> 
> Same comment as other patches. This needs to account for oversampling
> ratio.
> 

I am thinking that since we would have different chip_info structs, manual
mode channels could omit the oversampling attribute, since it is not supported
by the chip on this mode.

> > +
> > +		ret = spi_offload_trigger_validate(st->offload->trigger,
> &config);
> > +		if (ret)
> > +			return ret;
> > +
> > +		st->offload->trigger_hz = config.periodic.frequency_hz;
> > +		return len;
> > +	}
> > +
> >  	ret = ad4691_set_pwm_freq(st, freq);
> >  	if (ret)
> >  		return ret;
> > @@ -968,10 +1283,23 @@ static irqreturn_t ad4691_trigger_handler(int
> irq, void *p)
> >  	return IRQ_HANDLED;
> >  }
> >
> > +static int ad4691_get_current_scan_type(const struct iio_dev *indio_dev,
> > +					 const struct iio_chan_spec *chan)
> > +{
> > +	struct ad4691_state *st = iio_priv(indio_dev);
> > +
> > +	if (!st->offload)
> > +		return AD4691_SCAN_TYPE_NORMAL;
> > +	if (st->manual_mode)
> > +		return AD4691_SCAN_TYPE_OFFLOAD_MANUAL;
> > +	return AD4691_SCAN_TYPE_OFFLOAD_CNV;
> > +}
> > +
> >  static const struct iio_info ad4691_info = {
> >  	.read_raw = &ad4691_read_raw,
> >  	.write_raw = &ad4691_write_raw,
> >  	.read_avail = &ad4691_read_avail,
> > +	.get_current_scan_type = &ad4691_get_current_scan_type,
> >  	.debugfs_reg_access = &ad4691_reg_access,
> >  };
> >
> > @@ -1195,9 +1523,75 @@ static int ad4691_setup_triggered_buffer(struct
> iio_dev *indio_dev,
> >
> &ad4691_manual_buffer_setup_ops);
> >  }
> >
> > +static int ad4691_setup_offload(struct iio_dev *indio_dev,
> > +				struct ad4691_state *st,
> > +				struct spi_offload *spi_offload)
> > +{
> > +	struct device *dev = regmap_get_device(st->regmap);
> > +	struct ad4691_offload_state *offload;
> > +	struct dma_chan *rx_dma;
> > +	int ret;
> > +
> > +	offload = devm_kzalloc(dev, sizeof(*offload), GFP_KERNEL);
> > +	if (!offload)
> > +		return -ENOMEM;
> > +
> > +	offload->spi = spi_offload;
> > +	st->offload = offload;
> > +
> > +	if (st->manual_mode) {
> > +		offload->trigger =
> > +			devm_spi_offload_trigger_get(dev, offload->spi,
> > +
> SPI_OFFLOAD_TRIGGER_PERIODIC);
> > +		if (IS_ERR(offload->trigger))
> > +			return dev_err_probe(dev, PTR_ERR(offload->trigger),
> > +					     "Failed to get periodic offload
> trigger\n");
> > +
> > +		offload->trigger_hz = st->info->max_rate;
> 
> I think I mentioned this elsewhere, but can we really get max_rate in manual
> mode
> due to the extra SPI overhead? Probably safer to start with a lower rate.

You are right a slower rate would be nicer, from my tests 311kHz worked perfect
with a 10MHz SPI frequency, but perhaps these numbers are a bit "odd".

How do you feel about 100kHz for a starting sample rate?

> 
> > +	} else {
> > +		struct spi_offload_trigger_info trigger_info = {
> > +			.fwnode = dev_fwnode(dev),
> > +			.ops    = &ad4691_offload_trigger_ops,
> > +			.priv   = st,
> > +		};
> > +
> > +		ret = devm_spi_offload_trigger_register(dev, &trigger_info);
> > +		if (ret)
> > +			return dev_err_probe(dev, ret,
> > +					     "Failed to register offload
> trigger\n");
> > +
> > +		offload->trigger =
> > +			devm_spi_offload_trigger_get(dev, offload->spi,
> > +
> SPI_OFFLOAD_TRIGGER_DATA_READY);
> > +		if (IS_ERR(offload->trigger))
> > +			return dev_err_probe(dev, PTR_ERR(offload->trigger),
> > +					     "Failed to get DATA_READY offload
> trigger\n");
> > +	}
> > +
> > +	rx_dma = devm_spi_offload_rx_stream_request_dma_chan(dev,
> offload->spi);
> > +	if (IS_ERR(rx_dma))
> > +		return dev_err_probe(dev, PTR_ERR(rx_dma),
> > +				     "Failed to get offload RX DMA channel\n");
> > +
> > +	if (st->manual_mode)
> > +		indio_dev->setup_ops =
> &ad4691_manual_offload_buffer_setup_ops;
> > +	else
> > +		indio_dev->setup_ops =
> &ad4691_cnv_burst_offload_buffer_setup_ops;
> > +
> > +	ret = devm_iio_dmaengine_buffer_setup_with_handle(dev, indio_dev,
> rx_dma,
> > +
> IIO_BUFFER_DIRECTION_IN);
> > +	if (ret)
> > +		return ret;
> > +
> > +	indio_dev->buffer->attrs = ad4691_buffer_attrs;
> 
> Should including ad4691_buffer_attrs depend on st->manual_mode?
> 
> I thought it was only used when PWM is connected to CNV.
> 

For offload manual mode, I thought buffer sampling frequency should also be available,
since the offload trigger's frequency is accessible.

> > +
> > +	return 0;
> > +}
> > +
> >  static int ad4691_probe(struct spi_device *spi)
> >  {
> >  	struct device *dev = &spi->dev;
> > +	struct spi_offload *spi_offload;
> >  	struct iio_dev *indio_dev;
> >  	struct ad4691_state *st;
> >  	int ret;
> > @@ -1232,6 +1626,13 @@ static int ad4691_probe(struct spi_device *spi)
> >  	if (ret)
> >  		return ret;
> >
> > +	spi_offload = devm_spi_offload_get(dev, spi,
> &ad4691_offload_config);
> > +	ret = PTR_ERR_OR_ZERO(spi_offload);
> > +	if (ret == -ENODEV)
> > +		spi_offload = NULL;
> > +	else if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to get SPI offload\n");
> > +
> >  	indio_dev->name = st->info->name;
> >  	indio_dev->info = &ad4691_info;
> >  	indio_dev->modes = INDIO_DIRECT_MODE;
> > @@ -1239,7 +1640,10 @@ static int ad4691_probe(struct spi_device *spi)
> >  	indio_dev->channels = st->info->channels;
> >  	indio_dev->num_channels = st->info->num_channels;
> 
> As mentioned earlier, we generally want separate channel structs
> for SPI offload. These will also have different num_channels because
> there is no timestamp channel in SPI offload.

If different chip_info structs will be used, wouldn't they already have specific
channels attached to them?