Message-ID: <aQ3U0_6RM0KMP_od@vaman>
Date: Fri, 7 Nov 2025 12:15:31 +0100
From: Vinod Koul <vkoul@...nel.org>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Daniel Lezcano <daniel.lezcano@...aro.org>, 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,
	dmaengine@...r.kernel.org
Subject: Re: [PATCH v5 2/2] iio: adc: Add the NXP SAR ADC support for the
 s32g2/3 platforms

On 19-10-25, 09:42, Jonathan Cameron wrote:
> On Fri, 17 Oct 2025 18:42:38 +0200
> Daniel Lezcano <daniel.lezcano@...aro.org> wrote:
> 
> > From: Stefan-Gabriel Mirea <stefan-gabriel.mirea@....com>
> > 
> > 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.
> > 
> > The driver is derived from the BSP implementation and has been partly
> > rewritten to comply with upstream requirements. For this reason, all
> > contributors are listed as co-developers, while the author refers to
> > the initial BSP driver file creator.
> > 
> > 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.
> > 
> > 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: Stefan-Gabriel Mirea <stefan-gabriel.mirea@....com>
> > Co-developed-by: Daniel Lezcano <daniel.lezcano@...aro.org>
> > Signed-off-by: Daniel Lezcano <daniel.lezcano@...aro.org>
> 
> Hi Daniel,
> 
> Only significant question in here is around lifetimes of the
> dma buffer.
> 
> +CC Vinod and dmaengine list. Hopefully someone will rapidly tell me
> my concern is garbage ;)

Thanks for looping me in
> 
> IIO folk who are familiar with dmaengine channels etc please take
> a look at this as well.  I think all the upstream drivers we have doing
> similar things to this predate devm_ management being a common thing.
> 
> Jonathan
> 
> 
> > diff --git a/drivers/iio/adc/nxp-sar-adc.c b/drivers/iio/adc/nxp-sar-adc.c
> > new file mode 100644
> > index 000000000000..fa390c9d911f
> > --- /dev/null
> > +++ b/drivers/iio/adc/nxp-sar-adc.c
> > @@ -0,0 +1,1006 @@
> 
> > +
> > +static void nxp_sar_adc_dma_cb(void *data)
> > +{
> > +	struct nxp_sar_adc *info = iio_priv(data);
> > +	struct iio_dev *indio_dev = data;
> 
> Trivial but it would slightly more intuitive to do.
> 	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;
> > +
> > +	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;
> > +
> > +	/* 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_timestamp should not be
> 
> Comment needs an update as using with_ts()
> 
> 
> > +		 * 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);
> > +}
> 
> 
> 
> > +static int nxp_sar_adc_dma_probe(struct device *dev, struct nxp_sar_adc *info)
> > +{
> > +	struct device *dev_dma;
> > +	u8 *rx_buf;
> > +
> > +	info->dma_chan = devm_dma_request_chan(dev, "rx");
> > +	if (IS_ERR(info->dma_chan))
> > +		return PTR_ERR(info->dma_chan);
> > +
> > +	dev_dma = info->dma_chan->device->dev;
> > +	rx_buf = dmam_alloc_coherent(dev_dma, NXP_SAR_ADC_DMA_BUFF_SZ,
> > +				     &info->rx_dma_buf, GFP_KERNEL);
> 
> Is this setting up the right life time?  Superficially it looks to be
> associating the buffer lifetime with a device related to the dma engine rather
> than the device we are dealing with here.

Absolutely! the buffer ought to be mapped always with dmaengine device.
The transaction are performed by the dmaengine device and that is why
mapping should always be done with dmaengine device, it will not work
otherwise.

> This particular pattern with devm_dma_request_chan() is vanishingly rare
> so not much prior art to rely on.

I personally do not like that, dmaengine channel is a shared resource
and it should be grabbed at runtime and freed up after use. Ideally
would be good if we grab the channel when we need i

But I know people like to grab channels at startup :-(

> If the info->dma_chan->device->dev is instantiated by devm_dma_request_chan()
> and hence torn down as that is unwound it will be fine as this is simply
> nested devm handling, but it seems a struct dma_device has many chans so
> I think that isn't the case.
> 
> Given that device parameter is also needed for the buffer allocation and
> making sure we have the right properties / iommu magic etc, I'm not sure
> how to make this work. One option would be to use dma_alloc_coherent() and
> tear down with a devm_add_action_or_reset() handler on dev rather than
> dev_dma.
> 
> > +	if (!rx_buf)
> > +		return -ENOMEM;
> > +
> > +	info->dma_buf.buf = rx_buf;
> > +
> > +	return 0;
> > +}

-- 
~Vinod

Powered by blists - more mailing lists