Re: [PATCH v2 5/9] iio: adc: rzt2h: implement DMA buffer support

Implement buffered capture using a cyclic DMA transfer into a kfifo
buffer to support continuous high-rate sampling.

On buffer enable, switch the ADC to continuous conversion mode and start
a cyclic DMA transfer over the active channels.

Because the DMA controller does not support native scatter-gather, and
because of the cyclic DMA setup, transfers must be done in widths
covering all the enabled channels.

Since DMA transfer width must be a power of two and aligned to its size,
cover the smallest power-of-two-aligned group of channel registers
spanning the enabled channels.

Split the cyclic buffer into fixed-size periods. On each period
completion, bump a pending counter and wake a consumer kthread from the
DMA callback.

For every completed period, gather the enabled channels out of the DMA
layout into the scan layout the IIO core expects and push each scan
with iio_push_to_buffers().

If the consumer kthread falls behind by a full buffer, drop the oldest
periods.

Because the DMA transfer must cover all channels between the first and
last enabled ones, skip disabled channels while compacting.

Also, the DMA controller transfers data in 32-bit words, but the ADC's
data registers are 16-bit wide, causing adjacent channel data to be
swapped. Swap consecutive channels while compacting to account for this.

Allocate the DMA buffer via dma_alloc_noncoherent() and synchronise it
per period to allow it to be cached by the CPU while compacting.

Disable the completion IRQ for the duration of the DMA transfer, as the
ICU does not mask this event from reaching the GIC even if it is being
used to drive the DMA capture.

Signed-off-by: Cosmin Tanislav <cosmin-gabriel.tanislav.xa@renesas.com>

Hi Cosmin.

A couple of comments inline but so far nothing to stop me applying
this series and one of them is random musings :)

Jonathan

+static void rzt2h_adc_dma_calc_layout(struct iio_dev *indio_dev)
+{
+	struct rzt2h_adc *adc = iio_priv(indio_dev);
+	unsigned int hi = 0, lo = RZT2H_ADC_MAX_CHANNELS - 1;
+	const struct iio_chan_spec *chan;
+	unsigned int sample_chans;
+	unsigned int first_chan;
+	unsigned int scan_index;
+	unsigned int swap;
+	unsigned int idx;
+
+	/* Find the lowest and highest enabled channel. */
+	iio_for_each_active_channel(indio_dev, scan_index) {
+		chan = &indio_dev->channels[scan_index];
+
+		lo = min_t(unsigned int, lo, chan->channel);
+		hi = max_t(unsigned int, hi, chan->channel);

I'd normally be fussy about use of max_t etc but here it is
highlighting something that came up in another review - why is
channel an signed?  We use negative in the scan index but
I don't think we ever do for channel.  Anyhow, unrelated to this
series other than we might be able to tidy this up shortly if
I get round to checking if we can just make chan->channel unsigned.

+	}
+
+	/*
+	 * The DMA has no scatter-gather and transfers must have a power-of-two
+	 * width, so pick the smallest power-of-two-aligned block of channels
+	 * that covers all enabled channels.
+	 */
+	for (sample_chans = 1; sample_chans < RZT2H_ADC_MAX_CHANNELS; sample_chans <<= 1) {
+		first_chan = round_down(lo, sample_chans);
+
+		if (first_chan + sample_chans > hi)
+			break;
+	}
+
+	/*
+	 * Build a table to map each enabled channel to its position in the
+	 * transferred block, it will be used later to extract only the enabled
+	 * channels out of it.
+	 * The DMA moves data in 32-bit words, which swaps each pair of adjacent
+	 * 16-bit channels. Undo it.
+	 */
+	adc->dma.gather_len = 0;
+	swap = sample_chans > 1;
+	iio_for_each_active_channel(indio_dev, scan_index) {
+		chan = &indio_dev->channels[scan_index];
+		idx = chan->channel - first_chan;
+
+		adc->dma.gather[adc->dma.gather_len++] = idx ^ swap;
+	}
+
+	adc->dma.first_chan = first_chan;
+	adc->dma.sample_chans = sample_chans;
+	adc->dma.period_bytes = RZT2H_ADC_DMA_PERIOD_SAMPLES * sample_chans *
+				RZT2H_ADC_CHANNEL_BYTES;
+}
+
+static int rzt2h_adc_start_dma(struct iio_dev *indio_dev)
+{
+	struct rzt2h_adc *adc = iio_priv(indio_dev);
+	struct dma_async_tx_descriptor *desc;
+	struct dma_slave_config config;
+	unsigned int buffer_bytes;
+	dma_cookie_t cookie;
+	int ret;
+
+	rzt2h_adc_dma_calc_layout(indio_dev);
+
+	config = (struct dma_slave_config) {
+		.src_addr = adc->phys_base + RZT2H_ADDR_REG(adc->dma.first_chan),
+		.src_addr_width = adc->dma.sample_chans * RZT2H_ADC_CHANNEL_BYTES,
+	};
+
+	buffer_bytes = RZT2H_ADC_DMA_PERIODS * adc->dma.period_bytes;
+
+	ret = dmaengine_slave_config(adc->dma.chan, &config);
+	if (ret)
+		return ret;
+
+	desc = dmaengine_prep_dma_cyclic(adc->dma.chan, adc->dma.addr,
+					 buffer_bytes, adc->dma.period_bytes,
+					 DMA_DEV_TO_MEM, DMA_PREP_INTERRUPT);
+	if (!desc)
+		return -EBUSY;
+
+	desc->callback = rzt2h_adc_dma_callback;
+	desc->callback_param = indio_dev;
+
+	cookie = dmaengine_submit(desc);
+	ret = dma_submit_error(cookie);

It is fairly common to combine these
	ret = dma_submit_error(dmaengine_submit(desc))

If you happen to respin for other reasons consider doing this

-- 
Jonathan Cameron [off-list ref]