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

[Sabau, Radu bogdan]

> -----Original Message-----
> From: Jonathan Cameron <jic23@xxxxxxxxxx>
> Sent: Thursday, May 7, 2026 5:26 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.
> 

Correct. scan_bytes is a dense count (channels 0, 2, 4 active → 3 × 2 = 6
bytes). With the old sparse layout (rx_buf = &vals[scan_index]):

  vals[0] = ch0 result  (correct)
  vals[1] = hole (incorrect)  <- userspace reads as ch2
  vals[2] = ch2 result  (correct)  <-userspace reads as ch4

Fixed by using the slot counter k as the rx_buf index rather than the channel
index i, giving a densely packed vals[]. In manual mode xfer[k] delivers the
previous channel's result (pipelined), so subsequent transfers write into
vals[k-1]; vals[0] serves as a throwaway slot for the first transfer's garbage.