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

[Jonathan Cameron]

> 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@xxxxxxxxxxx>


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


> diff --git a/drivers/iio/adc/rzt2h_adc.c b/drivers/iio/adc/rzt2h_adc.c
> index 95bcebdc02cb..0460dffe189e 100644
> --- a/drivers/iio/adc/rzt2h_adc.c

> +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 <jonathan.cameron@xxxxxxxxxxxxxxxx>