| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20251019094246.38daf7bf@jic23-huawei>
Date: Sun, 19 Oct 2025 09:42:46 +0100
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, Vinod Koul <vkoul@...nel.org>,
dmaengine@...r.kernel.org
Subject: Re: [PATCH v5 2/2] iio: adc: Add the NXP SAR ADC support for the
s32g2/3 platforms
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 ;)
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.
This particular pattern with devm_dma_request_chan() is vanishingly rare
so not much prior art to rely on.
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;
> +}
Powered by blists - more mailing lists