Re: [PATCH v3 7/9] iio: adc: ti-ads1262: support triggered buffer sampling

[Kurt Borja]

On Sat Aug 8, 2026 at 1:39 PM -05, David Lechner wrote:
> On 8/7/26 10:58 PM, Kurt Borja wrote:
>> Add triggered buffer support and a data-ready (DRDY) hardware trigger.
>> 
>> Signed-off-by: Kurt Borja <kuurtb@xxxxxxxxx>
>> ---
>>  drivers/iio/adc/Kconfig      |   2 +
>>  drivers/iio/adc/ti-ads1262.c | 264 +++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 266 insertions(+)
>> 
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index dbf76427912b..b9b561be8347 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig

[...]

>> @@ -241,8 +245,16 @@ struct ads1262 {
>>  	bool need_avss_uV;
>>  	bool bipolar_supply;
>>  
>> +	IIO_DECLARE_BUFFER_WITH_TS(__be32, scan_buffer,
>> +				   ADS1262_MAX_CHANNEL_COUNT);
>> +
>>  	/* Protects transfer buffers and concurrent SPI transfers */
>>  	struct mutex xfer_lock;
>> +	struct spi_message msg;
>> +	struct spi_transfer xfer;
>> +
>
> Where does the 11 come from?

Size needed to hold both transfers for multi channel read. I can use a
macro here.

>
>> +	u8 tx[11] __aligned(IIO_DMA_MINALIGN);
>> +	u8 rx[11] __aligned(IIO_DMA_MINALIGN);
>
> don't need second one to be aligned, they aren't indepedant.

Ah, I forgot this observation in the last version. These are used in a
full-duplex transfer, wouldn't that require for both to be on its own
cache line? I just started learning about DMA.

[...]

>> +static int ads1262_enable_and_read_last(struct ads1262 *st,
>> +					const struct iio_chan_spec *spec,
>> +					__be32 *val)
>> +{
>> +	struct ads1262_channel *chan;
>> +	int ret;
>> +
>> +	lockdep_assert_held(&st->xfer_lock);
>
> What happens if something else (e.g. gpio in the future) decides to do a
> register write here. If it wins the race, will it unintentially read the
> data? So do we also need to read the stored data via command here too?

On each trigger, we are holding the lock before we enable the first
channel, until after we read the final conversion. So we don't really
care if there's concurrent activity in-between triggers. Am I missing
something?

>
>> +
>> +	if (spec) {
>> +		guard(mutex)(&st->chan_lock);
>> +
>> +		chan = &st->channels[spec->scan_index];
>> +
>> +		/* Group 1: MODE0, MODE1, MODE2, INPMUX */
>> +		st->tx[0] = ADS1262_MODE0_REG | ADS1262_OPCODE_WREG;
>> +		st->tx[1] = ADS1262_INPMUX_REG - ADS1262_MODE0_REG;
>> +		st->tx[2] = FIELD_PREP(ADS1262_MODE0_INPUT_CHOP_MASK, chan->input_chop) |
>> +			    FIELD_PREP(ADS1262_MODE0_IDAC_CHOP_MASK, chan->idac_chop) |
>> +			    FIELD_PREP(ADS1262_MODE0_RUNMODE_MASK, ADS1262_RUNMODE_CONTINUOUS) |
>> +			    FIELD_PREP(ADS1262_MODE0_REFREV_MASK, chan->ref_reversal);
>> +		st->tx[3] = FIELD_PREP(ADS1262_MODE1_FILTER_MASK, ADS1262_FILTER_FIR);
>> +		st->tx[4] = FIELD_PREP(ADS1262_MODE2_DR_MASK, chan->data_rate) |
>> +			    FIELD_PREP(ADS1262_MODE2_GAIN_MASK, chan->gain);
>> +		st->tx[5] = FIELD_PREP(ADS1262_INPMUX_MUXP_MASK, spec->channel) |
>> +			    FIELD_PREP(ADS1262_INPMUX_MUXN_MASK, spec->channel2);
>> +
>> +		/* Group 2: IDACMUX, IDACMAG, REFMUX */
>> +		st->tx[6] = ADS1262_IDACMUX_REG | ADS1262_OPCODE_WREG;
>> +		st->tx[7] = ADS1262_REFMUX_REG - ADS1262_IDACMUX_REG;
>> +		st->tx[8] = FIELD_PREP(ADS1262_IDACMUX_MUX1_MASK, chan->idac_mux[0]) |
>> +			    FIELD_PREP(ADS1262_IDACMUX_MUX2_MASK, chan->idac_mux[1]);
>> +		st->tx[9] = FIELD_PREP(ADS1262_IDACMAG_MAG1_MASK, chan->idac_mag[0]) |
>> +			    FIELD_PREP(ADS1262_IDACMAG_MAG2_MASK, chan->idac_mag[1]);
>> +		st->tx[10] = FIELD_PREP(ADS1262_REFMUX_RMUXP_MASK, chan->ref_p) |
>> +			     FIELD_PREP(ADS1262_REFMUX_RMUXN_MASK, chan->ref_n);
>> +	} else {
>> +		memset(st->tx, 0, sizeof(st->tx));
>> +	}
>> +
>> +	ret = spi_sync(st->spi, &st->msg);
>> +	if (ret)
>> +		return ret;
>> +
>> +	memcpy(val, st->rx, sizeof(*val));
>> +
>> +	return 0;
>> +}
>> +
>> +static int ads1262_fill_buffer_mult(struct iio_dev *indio_dev)
>> +{
>> +	struct ads1262 *st = iio_priv(indio_dev);
>> +	unsigned int chan;
>> +	__be32 val;
>> +	int i = -1;
>> +	int ret;
>> +
>> +	/*
>> +	 * This routine enables and reads channels in a full-duplex fashion.
>> +	 *
>> +	 * When a channel is enabled, the previous conversion is clocked out of
>> +	 * the shift data register on the same transfer (Section 9.4.7.1). This
>> +	 * allows for low latency software sequencing but forbids any
>> +	 * communication with the chip in-between or data corruption may occur,
>> +	 * hence the need to take the xfer_lock for the whole operation.
>> +	 */
>> +	guard(mutex)(&st->xfer_lock);
>> +
>> +	iio_for_each_active_channel(indio_dev, chan) {
>> +		ret = ads1262_enable_and_read_last(st, &indio_dev->channels[chan],
>> +						   &val);
>> +		if (ret)
>> +			return ret;
>> +
>> +		/*
>> +		 * After writing to the channel configuration registers, the
>> +		 * conversion-cycle is restarted and the data registers are
>> +		 * cleared. This means we have to reinit the completion after
>> +		 * enabling to avoid reading stale data.
>> +		 */
>> +		reinit_completion(&st->drdy);
>
> This seems racy still as DRDY could have been triggered already, in which case
> we would time out waiting for the interrupt. Or does the DRDY toggle again
> even if we don't read the data to trigger another conversion?

Yep, in continuous mode it just keeps toggling. However, data corruption
may occur if we read just before DRDY is about to toggle again. Which is
why...

>
> Would it be possible to make one big SPI messsage that contains all enabled
> channels and just run that instead? Insted of waiting for drdy, it would have
> to add a delay at the end of the sequence of xfers for setting up each channel
> that was long enough to ensure that the conversion will be done when we read
> it.
>
> Or we could just do similar to the start_one() function and don't leave
> it in continuous conversion mode. Using the delay option of spi xfers, we
> could just tack on two more commands to start and stop the conversion after
> after writing all of the mode stuff so that it still all happens in one SPI
> message.

...this gave me an idea. Instead of relying on delays, which are a bit
of a pain to calculate because the datasheet only gives latency values
for the nominal clock speed (I'll have to reverse engineer the formulas
for settlingtime :]). I can actually read in pulse mode here and stuff
the start commands inside the same transfer. The buffer would look like:

	6 bytes       | 5 bytes       | 1 byte
	--------------+---------------+----------
	channel_cfg_1 | channel_cfg_2 | start cmd

Thankfully we can chain commands without having to lift the CS line so
this is efficient. I didn't know this when I first started developing
the driver.

The buffer of course can be further optimized if say, all channels share
the same IDAC and reference configuration, but that can be done later if
needed.

-- 
Thanks,
 ~ Kurt