Re: [PATCH v5 2/2] iio: adc: Add the NXP SAR ADC support for the s32g2/3 platforms
From: Daniel Lezcano
Date: Thu Oct 30 2025 - 04:27:24 EST
Hi Andy,
On 10/18/25 22:12, Andy Shevchenko wrote:
On Fri, Oct 17, 2025 at 06:42:38PM +0200, Daniel Lezcano wrote:
From: Stefan-Gabriel Mirea <stefan-gabriel.mirea@xxxxxxx>
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.
...
+#include <linux/circ_buf.h>
Why not kfifo?
I'm sorry but I don't get your comment.
Do you mean why not use kfifo.h or use kfifo API and change all the code using the circ_buf by the kfifo ?
...
+#define NXP_SAR_ADC_IIO_BUFF_SZ (NXP_SAR_ADC_NR_CHANNELS + (sizeof(u64) / sizeof(u16)))
Hmm... Don't we have some macros so we can avoid this kind of hard coding?
I don't find such a macro, do you have a pointer ?
...
+ ndelay(div64_u64(NSEC_PER_SEC, clk_get_rate(info->clk)) * 80U);
Do you need those 'U':s? clk_get_rate() already returns unsigned value of the
same or higher rank than int. No?
May be not needed, but harmless. I can remove them if you want
+static int nxp_sar_adc_start_conversion(struct nxp_sar_adc *info, bool raw)
+{
+ u32 mcr;
+
+ mcr = readl(NXP_SAR_ADC_MCR(info->regs));
+
+ FIELD_MODIFY(NXP_SAR_ADC_MCR_NSTART, &mcr, 0x1);
+ FIELD_MODIFY(NXP_SAR_ADC_MCR_MODE, &mcr, !raw);
!raw, which is boolean, as a parameter to FIELD_MODIFY() seems a bit odd to me,
perhaps simple
raw ? 0 : 1
would work better?
Sure
(Note, optimizer of the complier will avoid any branching)
+ writel(mcr, NXP_SAR_ADC_MCR(info->regs));
+
+ return 0;
+}
...
+ dma_samples = (u32 *)dma_buf->buf;
Is it aligned properly for this type of casting?
TBH, I don't know the answer :/
How can I check that ?
+ dmaengine_tx_status(info->dma_chan, info->cookie, &state);
No return value check?
The return value is not necessary here because the caller of the callback will check with dma_submit_error() in case of error which covers the DMA_ERROR case and the other cases are not useful because the residue is taken into account right after.
It could be written differently with the DMA_COMPLETE check but the result will be the same. I prefer to keep the current implementation which has been tested.
Ok, I'll add it+static const struct nxp_sar_adc_data s32g2_sar_adc_data = {
+ .vref_mV = 1800,
+ .model = "s32g2-sar-adc"
Keep a trailing comma as here it's not a termination member.
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog