Message-ID: <20251207192145.646e3d2d@jic23-huawei>
Date: Sun, 7 Dec 2025 19:21:45 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Daniel Lezcano <daniel.lezcano@...aro.org>
Cc: dlechner@...libre.com, nuno.sa@...log.com, andy@...nel.org,
 robh@...nel.org, conor+dt@...nel.org, krzk+dt@...nel.org,
 linux-iio@...r.kernel.org, s32@....com, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org, chester62515@...il.com, mbrugger@...e.com,
 ghennadi.procopciuc@....nxp.com, vkoul@...nel.org
Subject: Re: [PATCH v8 2/2] iio: adc: Add the NXP SAR ADC support for the
 s32g2/3 platforms

On Wed, 19 Nov 2025 23:39:05 +0100
Daniel Lezcano <daniel.lezcano@...aro.org> wrote:

> The NXP S32G2 and S32G3 platforms integrate a successive approximation
> register (SAR) ADC. Two instances are available, each providing 8
> multiplexed input channels with 12-bit resolution. The conversion rate
> is up to 1 Msps depending on the configuration and sampling window.
> 
> The SAR ADC supports raw, buffer, and trigger modes. It can operate
> in both single-shot and continuous conversion modes, with optional
> hardware triggering through the cross-trigger unit (CTU) or external
> events. An internal prescaler allows adjusting the sampling clock,
> while per-channel programmable sampling times provide fine-grained
> trade-offs between accuracy and latency. Automatic calibration is
> performed at probe time to minimize offset and gain errors.
> 
> All modes have been validated on the S32G274-RDB2 platform using an
> externally generated square wave captured by the ADC. Tests covered
> buffered streaming via IIO, trigger synchronization, and accuracy
> verification against a precision laboratory signal source.
> 
> One potential scenario, not detected during testing, is that in some
> corner cases the DMA may already have been armed for the next
> transfer, which can lead dmaengine_tx_status() to return an incorrect
> residue.  The callback_result() operation—intended to supply the
> residue directly and eliminate the need to call
> dmaengine_tx_status()—also does not work.  Attempting to use
> dmaengine_pause() and dmaengine_resume() to prevent the residue from
> being updated does not work either.
> 
> This potential scenario should apply to any driver using cyclic DMA.
> However, no current driver actually handles this case, and they all rely
> on the same acquisition routine (e.g., the STM32 implementation).
> The NXP SAR acquisition routine has been used in production for several
> years, which is a good indication of its robustness.
> 
> As the IIO is implementing the cyclic DMA support API, it is not worth
> to do more spins to the current routine as it will go away when the
> new API will be available.
> 
> The driver is derived from the BSP implementation and has been partly
> rewritten to comply with upstream requirements. For this reason, all
> contributors to the original code are listed as co-developers.
> 
> Originally-by: Stefan-Gabriel Mirea <stefan-gabriel.mirea@....com>
> Co-developed-by: Alexandru-Catalin Ionita <alexandru-catalin.ionita@....com>
> Signed-off-by: Alexandru-Catalin Ionita <alexandru-catalin.ionita@....com>
> Co-developed-by: Ciprian Costea <ciprianmarian.costea@....com>
> Signed-off-by: Ciprian Costea <ciprianmarian.costea@....com>
> Co-developed-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@....nxp.com>
> Signed-off-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@....nxp.com>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@...aro.org>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@...el.com>

Hi Daniel,

Sorry for delay - I had to send the pull a tiny bit early to increase chance
of it landing before I went offline for 10 days. This would have been very
marginal anyway to make it with 1 week before merge window (which is my normal
cut off +- a day or two). In practice that means near final code on list before
rc6 so that I'm sure others have had time to review.  I do sympathise though
as I'm often the one trying to sneak code into the kernel very late in cycles.

On a final read I noticed one thing (see below).  I'm fine making that change
whilst applying the series if you don't mind me doing so.  It is just above
the level that I'd tweak without asking first!

Jonathan


> +++ b/drivers/iio/adc/nxp-sar-adc.c


> +static inline int nxp_sar_adc_calibration_wait(void __iomem *base)
> +{
> +	u32 msr, ret;
> +
> +	ret = readl_poll_timeout(NXP_SAR_ADC_MSR(base), msr,
> +				 !FIELD_GET(NXP_SAR_ADC_MSR_CALBUSY, msr),
> +				 NXP_SAR_ADC_WAIT_US,
> +				 NXP_SAR_ADC_CAL_TIMEOUT_US);
> +	if (ret)
> +		return ret;
> +
> +	if (FIELD_GET(NXP_SAR_ADC_MSR_CALFAIL, msr)) {
> +		/*
> +		 * If the calibration fails, the status register bit
> +		 * must be cleared.

Really trivial but slightly short wrap for standard 80 chars.
So trivial I might no bother changing it.
> +		 */
> +		FIELD_MODIFY(NXP_SAR_ADC_MSR_CALFAIL, &msr, 0x0);
> +		writel(msr, NXP_SAR_ADC_MSR(base));
> +
> +		return -EAGAIN;
> +	}
> +
> +	return 0;
> +}

> +static void nxp_sar_adc_dma_cb(void *data)
> +{
> +	struct iio_dev *indio_dev = data;
> +	struct nxp_sar_adc *info = iio_priv(indio_dev);
> +	struct dma_tx_state state;
> +	struct circ_buf *dma_buf;
> +	struct device *dev_dma;
> +	u32 *dma_samples;
> +	s64 timestamp;
> +	int idx, ret;
> +
> +	guard(spinlock_irqsave)(&info->lock);
> +
> +	dma_buf = &info->dma_buf;
> +	dma_samples = (u32 *)dma_buf->buf;
> +	dev_dma = info->dma_chan->device->dev;
> +
> +	/*
> +	 * DMA in some corner cases might have already be charged for
> +	 * the next transfer. Potentially there can be a race where
> +	 * the residue changes while the dma engine updates the
> +	 * buffer. That could be handled by using the
> +	 * callback_result() instead of callback() because the residue
> +	 * will be passed as a parameter to the function. However this
> +	 * new callback is pretty new and the backend does not update
> +	 * the residue. So let's stick to the version other drivers do
> +	 * which has proven running well in production since several
> +	 * years.
> +	 */
> +	dmaengine_tx_status(info->dma_chan, info->cookie, &state);
> +
> +	dma_sync_single_for_cpu(dev_dma, info->rx_dma_buf,
> +				NXP_SAR_ADC_DMA_BUFF_SZ, DMA_FROM_DEVICE);
> +
> +	/* Current head position. */
> +	dma_buf->head = (NXP_SAR_ADC_DMA_BUFF_SZ - state.residue) /
> +			NXP_SAR_ADC_DMA_SAMPLE_SZ;
> +
> +	/* If everything was transferred, avoid an off by one error. */
> +	if (!state.residue)
> +		dma_buf->head--;
> +
> +	/* Something went wrong and nothing transferred. */
> +	if (state.residue == NXP_SAR_ADC_DMA_BUFF_SZ)
> +		goto out;

This doesn't align with the guidance in cleanup.h about not mixing
stuff from there with goto style error handling.  However
that guidance is just an easy way to describe how to avoid issues
that can occur around gotos that cross the definition of new stuff
buried in guard() etc.  So this is (I believe anyway) not actually a bug
just something considered bad practice because it leaves a footgun that
someone may encounter in later refactoring of this driver.

Easiest route here is just cope with the extra indent and do

	/* Transfer occurred as expected */
	if (state.residue != NXP_SAR_ADC_DMA_BUF_SZ) {
		/* Make sure that head is multiple of info->channels_used. */
		dma_buf->head -= dma_buf->head % info->channels_used;

		/*
		 * dma_buf->tail != dma_buf->head condition will become false
		 * because dma_buf->tail will be incremented with 1.
		 */
		while (dma_buf->tail != dma_buf->head) {
			idx = dma_buf->tail % info->channels_used;
			info->buffer[idx] = dma_samples[dma_buf->tail];
			dma_buf->tail = (dma_buf->tail + 1) % NXP_SAR_ADC_DMA_SAMPLE_CNT;
			if (idx != info->channels_used - 1)
				continue;
	
			/*
			 * iio_push_to_buffers_with_ts() should not be
			 * called with dma_samples as parameter. The samples
			 * will be smashed if timestamp is enabled.
			 */
			timestamp = iio_get_time_ns(indio_dev);
			ret = iio_push_to_buffers_with_ts(indio_dev,
							  info->buffer,
							  sizeof(info->buffer),
							  timestamp);
			if (ret < 0 && ret != -EBUSY)
				dev_err_ratelimited(&indio_dev->dev,
						    "failed to push iio buffer: %d",
						    ret);
		}
	}
	dma_sync_single_for_device(dev_dma, info->rx_dma_buf,
				   NXP_SAR_ADC_DMA_BUFF_SZ, DMA_FROM_DEVICE);

Not that bad, and avoids potential grumpy Linus (he contributed that particular
statement in cleanup.h IIRC)

> +
> +	/* Make sure that head is multiple of info->channels_used. */
> +	dma_buf->head -= dma_buf->head % info->channels_used;
> +
> +	/*
> +	 * dma_buf->tail != dma_buf->head condition will become false
> +	 * because dma_buf->tail will be incremented with 1.
> +	 */
> +	while (dma_buf->tail != dma_buf->head) {
> +		idx = dma_buf->tail % info->channels_used;
> +		info->buffer[idx] = dma_samples[dma_buf->tail];
> +		dma_buf->tail = (dma_buf->tail + 1) % NXP_SAR_ADC_DMA_SAMPLE_CNT;
> +		if (idx != info->channels_used - 1)
> +			continue;
> +
> +		/*
> +		 * iio_push_to_buffers_with_ts() should not be
> +		 * called with dma_samples as parameter. The samples
> +		 * will be smashed if timestamp is enabled.
> +		 */
> +		timestamp = iio_get_time_ns(indio_dev);
> +		ret = iio_push_to_buffers_with_ts(indio_dev, info->buffer,
> +						  sizeof(info->buffer),
> +						  timestamp);
> +		if (ret < 0 && ret != -EBUSY)
> +			dev_err_ratelimited(&indio_dev->dev,
> +					    "failed to push iio buffer: %d",
> +					    ret);
> +	}
> +
> +	dma_buf->tail = dma_buf->head;
> +out:
> +	dma_sync_single_for_device(dev_dma, info->rx_dma_buf,
> +				   NXP_SAR_ADC_DMA_BUFF_SZ, DMA_FROM_DEVICE);
> +}

Powered by blists - more mailing lists