Re: [PATCH v9 3/6] iio: adc: ad4691: add triggered buffer support

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 7 May 2026 11:37:25 +0000
"Sabau, Radu bogdan" <Radu.Sabau@analog.com> wrote:

> Addressing Sashiko's review for triggered buffer patch.
> 
> > -----Original Message-----
> > From: Radu Sabau via B4 Relay <devnull+radu.sabau.analog.com@kernel.org>
> > Sent: Thursday, April 30, 2026 1:17 PM  
> 
> ...
> 
> > +static int ad4691_manual_buffer_preenable(struct iio_dev *indio_dev)
> > +{
> > +	struct ad4691_state *st = iio_priv(indio_dev);
> > +	unsigned int prev_i, k, i;
> > +	bool first;
> > +	int ret;
> > +
> > +	memset(st->scan_xfers, 0, sizeof(st->scan_xfers));
> > +	memset(st->scan_tx, 0, sizeof(st->scan_tx));
> > +
> > +	spi_message_init(&st->scan_msg);
> > +
> > +	first = true;
> > +	prev_i = 0;
> > +	k = 0;
> > +	iio_for_each_active_channel(indio_dev, i) {
> > +		st->scan_tx[k] = cpu_to_be16(AD4691_ADC_CHAN(i));
> > +		st->scan_xfers[k].tx_buf = &st->scan_tx[k];
> > +		/*
> > +		 * The pipeline means xfer[0] receives the residual from the
> > +		 * previous sequence, not a valid sample for channel i. Point
> > +		 * it at vals[i] anyway; xfer[1] (or the NOOP when only one
> > +		 * channel is active) will overwrite that slot with the real
> > +		 * result, so no separate dummy buffer is needed.
> > +		 */
> > +		if (first) {
> > +			st->scan_xfers[k].rx_buf = &st->vals[i];
> > +			first = false;
> > +		} else {
> > +			st->scan_xfers[k].rx_buf = &st->vals[prev_i];
> > +		}  
> 
> 
> "The IIO subsystem expects data pushed to the buffer to be densely packed
> according to the active channels in the scan mask.
> If only a subset of channels are enabled, does assigning the rx_buf pointer
> directly to absolute array indices at &st->vals[i] leave holes in the buffer?
> When iio_push_to_buffers_with_ts() is called, this might cause it to read
> uninitialized memory instead of the expected samples."
> 
> I would say there is no change needed. Writing to &st->vals[scan_index] and
> passing the full array to iio_push_to_buffers_with_ts() is the standard IIO kfifo
> pattern: the core demultiplexes by reading data[scan_index * storagebits/8]
> for each active channel; holes at inactive indices are silently ignored.
> The same pattern is used in ad4695, ad_sigma_delta, and others. The
> pipeline residual in the first manual-mode transfer is overwritten by the
> subsequent transfer before the scan is pushed, as the comment explains.

This looks wrong to me.

What holes?  If available_scan_masks is set we will do a bunch of
demux work - but then this code would see the mask picked from that
list. If it's not then typically we won't (subject to multiple consumers
forcing it - but that still won't close up holes here).

If the active_scan_mask == the one requested, there is no demux at all
and I think that's the case here - the code pushes the data passed in
directly to the kfifo.

Perhaps given an illustration of what the layout of resulting data
is if only even numbered channels are enabled.



> 
> > +		st->scan_xfers[k].len = sizeof(__be16);
> > +		st->scan_xfers[k].cs_change = 1;
> > +		spi_message_add_tail(&st->scan_xfers[k], &st->scan_msg);
> > +		prev_i = i;
> > +		k++;
> > +	}
> > +  
> 
>
> > +	st->scan_xfers[2 * k + 1].len = sizeof(__be16);
> > +	st->scan_xfers[2 * k + 1].cs_change = 1;
> > +	spi_message_add_tail(&st->scan_xfers[2 * k + 1], &st->scan_msg);
> > +
> > +	ret = spi_optimize_message(st->spi, &st->scan_msg);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_write(st->regmap, AD4691_STD_SEQ_CONFIG,
> > +			   bitmap_read(indio_dev->active_scan_mask, 0,
> > +				       iio_get_masklength(indio_dev)));
> > +	if (ret)
> > +		goto err_unoptimize;
> > +
> > +	ret = regmap_write(st->regmap, AD4691_ACC_MASK_REG,
> > +			   ~bitmap_read(indio_dev->active_scan_mask, 0,
> > +				iio_get_masklength(indio_dev)) &
> > GENMASK(15, 0));
> > +	if (ret)
> > +		goto err_unoptimize;
> > +
> > +	ret = ad4691_enter_conversion_mode(st);
> > +	if (ret)
> > +		goto err_unoptimize;
> > +
> > +	ret = ad4691_sampling_enable(st, true);
> > +	if (ret)
> > +		goto err_exit_conv;
> > +
> > +	enable_irq(st->irq);
> > +	return 0;  
> 
> "Is there a race condition introduced by enabling the PWM and unmasking the
> IRQ here?
> If a hardware interrupt fires before the IIO core attaches the trigger's poll
> function, iio_trigger_poll() drops the event. Will the IRQ handler then call
> disable_irq_nosync() without ever running the consumer thread to re-enable it?"
> 
> Valid. preenable is called before the IIO core attaches the trigger
> poll function; if a DATA_READY IRQ fires in that window, iio_trigger_poll()
> is dropped, disable_irq_nosync() disables the IRQ, and enable_irq() is
> never called, leaving the IRQ stuck. Although the delay would need to be 
> very great for this to happen, I moved sampling_enable(true) and
> enable_irq() to a new postenable callback which the IIO core calls only
> after the trigger poll function is attached.

Make sure to add a comment on why that is there.
Otherwise makes sense.

> 

Rest look fine to me.