Re: [PATCH v5 2/2] iio: adc: Add the NXP SAR ADC support for the s32g2/3 platforms

Jonathan Cameron
2025-11-09T12:52:12Z

On Fri, 7 Nov 2025 12:15:31 +0100
Vinod Koul <vkoul@kernel.org> wrote:

> On 19-10-25, 09:42, Jonathan Cameron wrote:
> > On Fri, 17 Oct 2025 18:42:38 +0200
> > Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> >
> > > From: Stefan-Gabriel Mirea <stefan-gabriel.mirea@nxp.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@nxp.com>
> > > Signed-off-by: Alexandru-Catalin Ionita <alexandru-catalin.ionita@nxp.com>
> > > Co-developed-by: Ciprian Costea <ciprianmarian.costea@nxp.com>
> > > Signed-off-by: Ciprian Costea <ciprianmarian.costea@nxp.com>
> > > Co-developed-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@oss.nxp.com>
> > > Signed-off-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@oss.nxp.com>
> > > Signed-off-by: Stefan-Gabriel Mirea <stefan-gabriel.mirea@nxp.com>
> > > Co-developed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.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 to me is confusing two things. The mapping establishment (in the dma
engine driver) and what driver looks after the mapping/triggers cleaning it up
(here the ADC driver). This dmam_alloc_coherent() seems to be allocating stuff
in the probe of this driver, but unless more complex management is going on
than I think (always possible!) the remove won't happen until he dmaengine
driver is unbound - so very much out of order with the tear down in this driver.

That is different from what we'd have if we just had
dma_alloc_coherent(dev_dma, ...) in probe
and dma_free_coherent(dev_dma ...) in remove() for this driver.

There are other kernel interfaces that provide a management related struct device
and a struct device needed to perform the operation. Seems like the same should
be true here as they don't seem to be the same struct device.

Jonathan

>
> > 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;
> > > +}
>