[PATCH v1 0/2] NXP SAR ADC IIO driver for s32g2/3 platforms

[PATCH v1 0/2] NXP SAR ADC IIO driver for s32g2/3 platforms

[Daniel Lezcano]

The S32G2 and S32G3 platforms have a couple of successive
approximation register (SAR) ADCs with eight channels and 12-bit
resolution. These changes provide the driver support for these ADCs
and the bindings describing them.

The driver is derived from the BSP driver version. It has been partly
rewritten to conform to upstream criteria.

https://github.com/nxp-auto-linux/linux/blob/release/bsp44.0-6.6.85-rt/drivers/iio/adc/s32cc_adc.c

Daniel Lezcano (2):
  dt-bindings: iio: Add the NXP SAR ADC for s32g2/3 platforms
  iio: adc: Add the NXP SAR ADC support for the s32g2/3 platforms

 .../bindings/iio/adc/nxp,s32g2-sar-adc.yaml   |   68 ++
 drivers/iio/adc/Kconfig                       |   13 +
 drivers/iio/adc/Makefile                      |    1 +
 drivers/iio/adc/nxp-sar-adc.c                 | 1046 +++++++++++++++++
 4 files changed, 1128 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/nxp,s32g2-sar-adc.yaml
 create mode 100644 drivers/iio/adc/nxp-sar-adc.c

-- 
2.43.0

[PATCH v1 1/2] dt-bindings: iio: adc: Add the NXP SAR ADC for s32g2/3 platforms

[Daniel Lezcano]

The s32g2 and s32g3 NXP platforms have two instances of a Successive
Approximation Register ADC. It supports the raw, trigger and scan
modes which involves the DMA. Add their descriptions.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 .../bindings/iio/adc/nxp,s32g2-sar-adc.yaml   | 68 +++++++++++++++++++
 1 file changed, 68 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/nxp,s32g2-sar-adc.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/nxp,s32g2-sar-adc.yaml b/Documentation/devicetree/bindings/iio/adc/nxp,s32g2-sar-adc.yaml
new file mode 100644
index 000000000000..dc6ec240f816
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/nxp,s32g2-sar-adc.yaml
@@ -0,0 +1,68 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/nxp,s32g2-sar-adc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NXP Successive Approximation ADC
+
+description:
+  The NXP SAR ADC provides fast and accurate analog-to-digital
+  conversion using the Successive Approximation Register (SAR) method.
+  It has 12-bit resolution with 8 input channels. Conversions can be
+  launched in software or using hardware triggers. It supports
+  continuous and one-shot modes with separate registers.
+
+maintainers:
+  - Daniel Lezcano <daniel.lezcano@kernel.org>
+
+properties:
+  compatible:
+    oneOf:
+      - const: nxp,s32g2-sar-adc
+      - items:
+          - const: nxp,s32g3-sar-adc
+          - const: nxp,s32g2-sar-adc
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    minItems: 1
+
+  clock-names:
+    minItems: 1
+
+  dmas:
+    minItems: 1
+
+  dma-names:
+    const: rx
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+  - dmas
+  - dma-names
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    adc@401f8000 {
+        compatible = "nxp,s32g2-sar-adc";
+        reg = <0x401f8000 0x1000>;
+        interrupts = <GIC_SPI 70 IRQ_TYPE_LEVEL_HIGH>;
+        clocks = <&clks 0x41>;
+        clock-names = "adc";
+        dmas = <&edma0 0 32>;
+        dma-names = "rx";
+    };
-- 
2.43.0

Re: [PATCH v1 1/2] dt-bindings: iio: adc: Add the NXP SAR ADC for s32g2/3 platforms

[Rob Herring (Arm)]

On Wed, 03 Sep 2025 12:27:55 +0200, Daniel Lezcano wrote:
> The s32g2 and s32g3 NXP platforms have two instances of a Successive
> Approximation Register ADC. It supports the raw, trigger and scan
> modes which involves the DMA. Add their descriptions.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  .../bindings/iio/adc/nxp,s32g2-sar-adc.yaml   | 68 +++++++++++++++++++
>  1 file changed, 68 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/nxp,s32g2-sar-adc.yaml
> 

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

Re: [PATCH v1 1/2] dt-bindings: iio: adc: Add the NXP SAR ADC for s32g2/3 platforms

[David Lechner]

On 9/3/25 5:27 AM, Daniel Lezcano wrote:

...

> +  clock-names:
> +    minItems: 1

Usually if there is only 1 clock, we don't bother with clock-names.

If we keep clock names though, I think it should be:

  clock-names:
    items:
      const: adc

Since that is the only allowed name.

Although the reference manual calls this the "bus" clock, so that might be a
better name.

> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    adc@401f8000 {
> +        compatible = "nxp,s32g2-sar-adc";
> +        reg = <0x401f8000 0x1000>;
> +        interrupts = <GIC_SPI 70 IRQ_TYPE_LEVEL_HIGH>;
> +        clocks = <&clks 0x41>;
> +        clock-names = "adc";
> +        dmas = <&edma0 0 32>;
> +        dma-names = "rx";
> +    };

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

[Daniel Lezcano]

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>
---
 drivers/iio/adc/Kconfig       |   13 +
 drivers/iio/adc/Makefile      |    1 +
 drivers/iio/adc/nxp-sar-adc.c | 1046 +++++++++++++++++++++++++++++++++
 3 files changed, 1060 insertions(+)
 create mode 100644 drivers/iio/adc/nxp-sar-adc.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 6de2abad0197..4c2473a1fa20 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1168,6 +1168,19 @@ config NPCM_ADC
 	  This driver can also be built as a module. If so, the module
 	  will be called npcm_adc.
 
+config NXP_SAR_ADC
+	tristate "NXP S32G SAR-ADC driver"
+	depends on ARCH_S32 || COMPILE_TEST
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	select IIO_SYSFS_TRIGGER
+	help
+	  Say yes here to build support for S32G platforms
+	  analog-to-digital converter.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called nxp_sar_adc.
+
 config PAC1921
 	tristate "Microchip Technology PAC1921 driver"
 	depends on I2C
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 1c6ca5fd4b6d..d1d939753bcc 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -119,6 +119,7 @@ obj-$(CONFIG_RN5T618_ADC) += rn5t618-adc.o
 obj-$(CONFIG_ROHM_BD79124) += rohm-bd79124.o
 obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
 obj-$(CONFIG_RZG2L_ADC) += rzg2l_adc.o
+obj-$(CONFIG_NXP_SAR_ADC) += nxp-sar-adc.o
 obj-$(CONFIG_SC27XX_ADC) += sc27xx_adc.o
 obj-$(CONFIG_SD_ADC_MODULATOR) += sd_adc_modulator.o
 obj-$(CONFIG_SOPHGO_CV1800B_ADC) += sophgo-cv1800b-adc.o
diff --git a/drivers/iio/adc/nxp-sar-adc.c b/drivers/iio/adc/nxp-sar-adc.c
new file mode 100644
index 000000000000..666f907aa03b
--- /dev/null
+++ b/drivers/iio/adc/nxp-sar-adc.c
@@ -0,0 +1,1046 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * NXP SAR-ADC driver (adapted from Freescale Vybrid vf610 ADC driver
+ * by Fugang Duan <B38611@freescale.com>)
+ *
+ * Copyright 2013 Freescale Semiconductor, Inc.
+ * Copyright 2017, 2020-2025 NXP
+ * Copyright 2025, Linaro Ltd
+ */
+#include <linux/circ_buf.h>
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/dma-mapping.h>
+#include <linux/dmaengine.h>
+#include <linux/interrupt.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
+
+/* This will be the driver name the kernel reports */
+#define DRIVER_NAME "nxp-sar-adc"
+
+/* SAR ADC registers */
+#define REG_ADC_CDR(__base, __channel)	(((__base) + 0x100) + ((__channel) * 0x4))
+
+#define REG_ADC_CDR_CDATA_MASK		GENMASK(11, 0)
+#define REG_ADC_CDR_VALID		BIT(19)
+
+/* Main Configuration Register */
+#define REG_ADC_MCR(__base)		(__base)
+
+#define REG_ADC_MCR_PWDN		BIT(0)
+#define REG_ADC_MCR_ACKO		BIT(5)
+#define REG_ADC_MCR_ADCLKSEL		BIT(8)
+#define REG_ADC_MCR_TSAMP_MASK		GENMASK(10, 9)
+#define REG_ADC_MCR_NRSMPL_32		BIT(11)
+#define REG_ADC_MCR_NRSMPL_128		BIT(12)
+#define REG_ADC_MCR_NRSMPL_512		(BIT(11) | BIT(12))
+#define REG_ADC_MCR_NRSMPL_MASK		GENMASK(12, 11)
+#define REG_ADC_MCR_AVGEN		BIT(13)
+#define REG_ADC_MCR_CALSTART		BIT(14)
+#define REG_ADC_MCR_NSTART		BIT(24)
+#define REG_ADC_MCR_MODE		BIT(29)
+#define REG_ADC_MCR_OWREN		BIT(31)
+
+/* Main Status Register */
+#define REG_ADC_MSR(__base)		((__base) + 0x04)
+
+#define REG_ADC_MSR_CALBUSY		BIT(29)
+#define REG_ADC_MSR_CALFAIL		BIT(30)
+
+/* Interrupt Status Register */
+#define REG_ADC_ISR(__base)		((__base) + 0x10)
+
+#define REG_ADC_ISR_ECH			BIT(0)
+
+/*  Channel Pending Register */
+#define REG_ADC_CEOCFR0(__base)		((__base) + 0x14)
+#define REG_ADC_CEOCFR1(__base)		((__base) + 0x18)
+
+#define REG_ADC_EOC_CH(c)		BIT((c) % 32)
+
+/* Interrupt Mask Register */
+#define REG_ADC_IMR(__base)		((__base) + 0x20)
+
+/* Channel Interrupt Mask Register */
+#define REG_ADC_CIMR0(__base)		((__base) + 0x24)
+#define REG_ADC_CIMR1(__base)		((__base) + 0x28)
+
+/* DMA Setting Register */
+#define REG_ADC_DMAE(__base)		((__base) + 0x40)
+
+#define REG_ADC_DMAE_DMAEN		BIT(0)
+#define REG_ADC_DMAE_DCLR		BIT(1)
+
+/* DMA Control register */
+#define REG_ADC_DMAR0(__base)		((__base) + 0x44)
+#define REG_ADC_DMAR1(__base)		((__base) + 0x48)
+
+/* Conversion Timing Register */
+#define REG_ADC_CTR0(__base)		((__base) + 0x94)
+#define REG_ADC_CTR1(__base)		((__base) + 0x98)
+
+#define REG_ADC_CTR_INPSAMP_MIN		8
+#define REG_ADC_CTR_INPSAMP_MAX		0xFF
+
+/* Normal Conversion Mask Register */
+#define REG_ADC_NCMR0(__base)		((__base) + 0xa4)
+#define REG_ADC_NCMR1(__base)		((__base) + 0xa8)
+
+/* Normal Conversion Mask Register field define */
+#define REG_ADC_CH_MASK			GENMASK(7, 0)
+
+/* Other field define */
+#define NXP_SAR_ADC_CONV_TIMEOUT_MS	100
+#define NXP_SAR_ADC_CAL_TIMEOUT_US	100000
+#define NXP_SAR_ADC_WAIT_US		2000
+#define NXP_SAR_ADC_RESOLUTION		12
+
+/* Duration of conversion phases */
+#define NXP_SAR_ADC_TPT			2
+#define NXP_SAR_ADC_DP			2
+#define NXP_SAR_ADC_CT			((NXP_SAR_ADC_RESOLUTION + 2) * 4)
+#define NXP_SAR_ADC_CONV_TIME		(NXP_SAR_ADC_TPT + NXP_SAR_ADC_CT + NXP_SAR_ADC_DP)
+
+#define NXP_SAR_ADC_NR_CHANNELS	8
+
+#define NXP_SAR_ADC_IIO_BUFF_SZ		(NXP_SAR_ADC_NR_CHANNELS + (sizeof(u64) / sizeof(u16)))
+
+#define NXP_SAR_ADC_DMA_SAMPLE_SZ	DMA_SLAVE_BUSWIDTH_4_BYTES
+#define NXP_SAR_ADC_DMA_BUFF_SZ		(PAGE_SIZE * NXP_SAR_ADC_DMA_SAMPLE_SZ)
+#define NXP_SAR_ADC_DMA_SAMPLE_CNT	(NXP_SAR_ADC_DMA_BUFF_SZ / NXP_SAR_ADC_DMA_SAMPLE_SZ)
+
+struct nxp_sar_adc {
+	void __iomem *regs;
+	phys_addr_t regs_phys;
+	struct clk *clk;
+
+	u16 value;
+	u32 vref;
+	u8 current_channel;
+	u8 channels_used;
+
+	struct completion completion;
+
+	u16 buffer[NXP_SAR_ADC_IIO_BUFF_SZ];
+	u16 buffered_chan[NXP_SAR_ADC_NR_CHANNELS];
+
+	struct circ_buf dma_buf;
+	struct dma_chan	*dma_chan;
+	struct dma_slave_config dma_config;
+	dma_addr_t rx_dma_buf;
+	dma_cookie_t cookie;
+
+	/* Protect circular buffers access. */
+	spinlock_t lock;
+
+	/*
+	 * Save and restore context
+	 */
+	u32 inpsamp;
+	u32 pwdn;
+};
+
+struct nxp_sar_adc_data {
+	u32 vref;
+};
+
+#define ADC_CHAN(_idx, _chan_type) {				\
+	.type = (_chan_type),					\
+	.indexed = 1,						\
+	.channel = (_idx),					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |	\
+				BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
+	.scan_index = (_idx),					\
+	.scan_type = {						\
+		.sign = 'u',					\
+		.realbits = 12,					\
+		.storagebits = 16,				\
+	},							\
+}
+
+static const struct iio_chan_spec nxp_sar_adc_iio_channels[] = {
+	ADC_CHAN(0, IIO_VOLTAGE),
+	ADC_CHAN(1, IIO_VOLTAGE),
+	ADC_CHAN(2, IIO_VOLTAGE),
+	ADC_CHAN(3, IIO_VOLTAGE),
+	ADC_CHAN(4, IIO_VOLTAGE),
+	ADC_CHAN(5, IIO_VOLTAGE),
+	ADC_CHAN(6, IIO_VOLTAGE),
+	ADC_CHAN(7, IIO_VOLTAGE),
+	IIO_CHAN_SOFT_TIMESTAMP(32),
+};
+
+static void nxp_sar_adc_irq_cfg(struct nxp_sar_adc *info, bool enable)
+{
+	if (enable)
+		writel(REG_ADC_ISR_ECH, REG_ADC_IMR(info->regs));
+	else
+		writel(0, REG_ADC_IMR(info->regs));
+}
+
+static bool __nxp_sar_adc_enable(struct nxp_sar_adc *info, bool enable)
+{
+	u32 mcr;
+	bool pwdn;
+
+	mcr = readl(REG_ADC_MCR(info->regs));
+
+	/*
+	 * Return the current state
+	 */
+	pwdn = mcr & REG_ADC_MCR_PWDN;
+
+	if (enable)
+		mcr &= ~REG_ADC_MCR_PWDN;
+	else
+		mcr |= REG_ADC_MCR_PWDN;
+
+	writel(mcr, REG_ADC_MCR(info->regs));
+
+	/*
+	 * Ensure there are at least three cycles between the
+	 * configuration of NCMR and the setting of NSTART
+	 */
+	if (enable)
+		ndelay(div64_u64(NSEC_PER_SEC, clk_get_rate(info->clk) * 3U));
+
+	return pwdn;
+}
+
+static inline bool nxp_sar_adc_enable(struct nxp_sar_adc *info)
+{
+	return __nxp_sar_adc_enable(info, true);
+}
+
+static inline bool nxp_sar_adc_disable(struct nxp_sar_adc *info)
+{
+	return __nxp_sar_adc_enable(info, false);
+}
+
+static inline void nxp_sar_adc_calibration_start(void __iomem *base)
+{
+	u32 mcr = readl(REG_ADC_MCR(base));
+
+	mcr |= REG_ADC_MCR_CALSTART;
+
+	writel(mcr, REG_ADC_MCR(base));
+}
+
+static inline int nxp_sar_adc_calibration_wait(void __iomem *base)
+{
+	u32 msr, ret;
+
+	ret = read_poll_timeout(readl, msr, !(msr & REG_ADC_MSR_CALBUSY),
+				NXP_SAR_ADC_WAIT_US,
+				NXP_SAR_ADC_CAL_TIMEOUT_US,
+				true, REG_ADC_MSR(base));
+	if (ret)
+		return ret;
+
+	if (!(msr & REG_ADC_MSR_CALFAIL))
+		return 0;
+
+	/*
+	 * If the calibration fails, the status register bit must be
+	 * cleared
+	 */
+	msr &= ~REG_ADC_MSR_CALFAIL;
+
+	writel(msr, REG_ADC_MSR(base));
+
+	return -EAGAIN;
+}
+
+static int nxp_sar_adc_calibration(struct nxp_sar_adc *info)
+{
+	int ret;
+
+	/*
+	 * Calibration works only if the adc is powered up
+	 */
+	nxp_sar_adc_enable(info);
+
+	/*
+	 * The calibration operation starts
+	 */
+	nxp_sar_adc_calibration_start(info->regs);
+
+	ret = nxp_sar_adc_calibration_wait(info->regs);
+
+	/*
+	 * Calibration works only if the adc is powered up. However
+	 * the calibration is called from the probe function where the
+	 * iio is not enabled, so we disable after the calibration
+	 */
+	nxp_sar_adc_disable(info);
+
+	return ret;
+}
+
+static void nxp_sar_adc_conversion_timing_set(struct nxp_sar_adc *info, u32 inpsamp)
+{
+	inpsamp = clamp(inpsamp, REG_ADC_CTR_INPSAMP_MIN, REG_ADC_CTR_INPSAMP_MAX);
+
+	writel(inpsamp, REG_ADC_CTR0(info->regs));
+}
+
+static u32 nxp_sar_adc_conversion_timing_get(struct nxp_sar_adc *info)
+{
+	return readl(REG_ADC_CTR0(info->regs));
+}
+
+static void nxp_sar_adc_read_notify(struct nxp_sar_adc *info)
+{
+	writel(REG_ADC_CH_MASK, REG_ADC_CEOCFR0(info->regs));
+	writel(REG_ADC_CH_MASK, REG_ADC_CEOCFR1(info->regs));
+}
+
+static int nxp_sar_adc_read_data(struct nxp_sar_adc *info, unsigned int chan)
+{
+	u32 ceocfr, cdr;
+
+	ceocfr = readl(REG_ADC_CEOCFR0(info->regs));
+	if (!(ceocfr & REG_ADC_EOC_CH(chan)))
+		return -EIO;
+
+	cdr = readl(REG_ADC_CDR(info->regs, chan));
+	if (!(cdr & REG_ADC_CDR_VALID))
+		return -EIO;
+
+	return cdr & REG_ADC_CDR_CDATA_MASK;
+}
+
+static void nxp_sar_adc_isr_buffer(struct iio_dev *indio_dev)
+{
+	struct nxp_sar_adc *info = iio_priv(indio_dev);
+	int i, ret;
+
+	for (i = 0; i < info->channels_used; i++) {
+		ret = nxp_sar_adc_read_data(info, info->buffered_chan[i]);
+		if (ret < 0) {
+			nxp_sar_adc_read_notify(info);
+			return;
+		}
+
+		info->buffer[i] = ret;
+	}
+
+	nxp_sar_adc_read_notify(info);
+	iio_push_to_buffers_with_timestamp(indio_dev,
+					   info->buffer,
+					   iio_get_time_ns(indio_dev));
+	iio_trigger_notify_done(indio_dev->trig);
+}
+
+static void nxp_sar_adc_isr_read_raw(struct iio_dev *indio_dev)
+{
+	struct nxp_sar_adc *info = iio_priv(indio_dev);
+	int ret;
+
+	ret = nxp_sar_adc_read_data(info, info->current_channel);
+	nxp_sar_adc_read_notify(info);
+	if (ret < 0)
+		return;
+
+	info->value = ret;
+	complete(&info->completion);
+}
+
+static irqreturn_t nxp_sar_adc_isr(int irq, void *dev_id)
+{
+	struct iio_dev *indio_dev = (struct iio_dev *)dev_id;
+	struct nxp_sar_adc *info = iio_priv(indio_dev);
+	int isr;
+
+	isr = readl(REG_ADC_ISR(info->regs));
+	if (!(isr & REG_ADC_ISR_ECH))
+		return IRQ_NONE;
+
+	if (iio_buffer_enabled(indio_dev))
+		nxp_sar_adc_isr_buffer(indio_dev);
+	else
+		nxp_sar_adc_isr_read_raw(indio_dev);
+
+	writel(REG_ADC_ISR_ECH, REG_ADC_ISR(info->regs));
+
+	return IRQ_HANDLED;
+}
+
+static void nxp_sar_adc_channels_disable(struct nxp_sar_adc *info, u32 mask)
+{
+	u32 ncmr, cimr;
+
+	ncmr = readl(REG_ADC_NCMR0(info->regs));
+	cimr = readl(REG_ADC_CIMR0(info->regs));
+
+	ncmr &= ~mask;
+	cimr &= ~mask;
+
+	writel(ncmr, REG_ADC_NCMR0(info->regs));
+	writel(cimr, REG_ADC_CIMR0(info->regs));
+}
+
+static void nxp_sar_adc_channels_enable(struct nxp_sar_adc *info, u32 mask)
+{
+	u32 ncmr, cimr;
+
+	ncmr = readl(REG_ADC_NCMR0(info->regs));
+	cimr = readl(REG_ADC_CIMR0(info->regs));
+
+	ncmr |= mask;
+	cimr |= mask;
+
+	writel(ncmr, REG_ADC_NCMR0(info->regs));
+	writel(cimr, REG_ADC_CIMR0(info->regs));
+}
+
+static void nxp_sar_adc_dma_channels_enable(struct nxp_sar_adc *info, u32 mask)
+{
+	u32 dmar;
+
+	dmar = readl(REG_ADC_DMAR0(info->regs));
+
+	dmar |= mask;
+
+	writel(dmar, REG_ADC_DMAR0(info->regs));
+}
+
+static void nxp_sar_adc_dma_channels_disable(struct nxp_sar_adc *info, u32 mask)
+{
+	u32 dmar;
+
+	dmar = readl(REG_ADC_DMAR0(info->regs));
+
+	dmar &= ~mask;
+
+	writel(dmar, REG_ADC_DMAR0(info->regs));
+}
+
+static void nxp_sar_adc_dma_cfg(struct nxp_sar_adc *info, bool enable)
+{
+	u32 dmae;
+
+	dmae = readl(REG_ADC_DMAE(info->regs));
+	if (enable)
+		dmae |= REG_ADC_DMAE_DMAEN;
+	else
+		dmae &= ~REG_ADC_DMAE_DMAEN;
+	writel(dmae, REG_ADC_DMAE(info->regs));
+}
+
+static void nxp_sar_adc_stop_conversion(struct nxp_sar_adc *info)
+{
+	u32 mcr;
+
+	mcr = readl(REG_ADC_MCR(info->regs));
+	mcr &= ~REG_ADC_MCR_NSTART;
+	writel(mcr, REG_ADC_MCR(info->regs));
+
+	/*
+	 * On disable, we have to wait for the transaction to finish.
+	 * ADC does not abort the transaction if a chain conversion
+	 * is in progress.
+	 * Wait for the worst case scenario - 80 ADC clk cycles.
+	 */
+	ndelay(div64_u64(NSEC_PER_SEC, clk_get_rate(info->clk)) * 80U);
+}
+
+static int nxp_sar_adc_start_conversion(struct nxp_sar_adc *info, bool raw)
+{
+	u32 mcr;
+
+	mcr = readl(REG_ADC_MCR(info->regs));
+	mcr |= REG_ADC_MCR_NSTART;
+
+	if (raw)
+		mcr &= ~REG_ADC_MCR_MODE;
+	else
+		mcr |= REG_ADC_MCR_MODE;
+
+	writel(mcr, REG_ADC_MCR(info->regs));
+
+	return 0;
+}
+
+static int sar_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
+			int *val, int *val2, long mask)
+{
+	struct nxp_sar_adc *info = iio_priv(indio_dev);
+	u32 inpsamp;
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		if (!iio_device_claim_direct(indio_dev))
+			return -EBUSY;
+
+		info->current_channel = chan->channel;
+		nxp_sar_adc_channels_enable(info, 1 >> chan->channel);
+		nxp_sar_adc_irq_cfg(info, true);
+		nxp_sar_adc_enable(info);
+
+		reinit_completion(&info->completion);
+		ret = nxp_sar_adc_start_conversion(info, true);
+		if (ret < 0)
+			goto out_iio_chan_info_raw;
+
+		ret = wait_for_completion_interruptible_timeout
+			(&info->completion,
+			msecs_to_jiffies(NXP_SAR_ADC_CONV_TIMEOUT_MS));
+
+		nxp_sar_adc_channels_disable(info, 1 >> chan->channel);
+		nxp_sar_adc_irq_cfg(info, false);
+		nxp_sar_adc_stop_conversion(info);
+		nxp_sar_adc_disable(info);
+
+		if (ret == 0) {
+			ret = -ETIMEDOUT;
+			goto out_iio_chan_info_raw;
+		}
+
+		if (ret < 0)
+			goto out_iio_chan_info_raw;
+
+		*val = info->value;
+		ret = IIO_VAL_INT;
+
+out_iio_chan_info_raw:
+		iio_device_release_direct(indio_dev);
+		return ret;
+
+	case IIO_CHAN_INFO_SCALE:
+		*val = info->vref;
+		*val2 = NXP_SAR_ADC_RESOLUTION;
+		return IIO_VAL_FRACTIONAL_LOG2;
+
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		inpsamp = nxp_sar_adc_conversion_timing_get(info);
+		*val = clk_get_rate(info->clk) / (inpsamp + NXP_SAR_ADC_CONV_TIME);
+		return IIO_VAL_INT;
+
+	default:
+		break;
+	}
+
+	return -EINVAL;
+}
+
+static int sar_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
+			 int val, int val2, long mask)
+{
+	struct nxp_sar_adc *info = iio_priv(indio_dev);
+	u32 inpsamp;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		/*
+		 * Configures the sample period duration in terms of
+		 * the SAR controller clock. The minimum acceptable
+		 * value is 8. Configuring to a value lower than 8
+		 * sets the sample period to 8 cycles.  We read the
+		 * clock value and divide by the sampling timing which
+		 * gives us the number of cycles expected. The value
+		 * is 8 bits wide, consequently the max value is 0xFF
+		 */
+		inpsamp = clk_get_rate(info->clk) / val - NXP_SAR_ADC_CONV_TIME;
+		nxp_sar_adc_conversion_timing_set(info, inpsamp);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void nxp_sar_adc_dma_cb(void *data)
+{
+	struct nxp_sar_adc *info = iio_priv((struct iio_dev *)data);
+	struct iio_dev *indio_dev = data;
+	struct dma_tx_state state;
+	struct circ_buf *dma_buf;
+	struct device *dev_dma;
+	unsigned long flags;
+	u32 *dma_samples;
+	s64 timestamp;
+	int idx, ret;
+
+	dma_buf = &info->dma_buf;
+	dma_samples = (u32 *)dma_buf->buf;
+	dev_dma = info->dma_chan->device->dev;
+	spin_lock_irqsave(&info->lock, flags);
+	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 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 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_timestamp(indio_dev,
+							 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);
+	spin_unlock_irqrestore(&info->lock, flags);
+}
+
+static int nxp_sar_adc_start_cyclic_dma(struct iio_dev *indio_dev)
+{
+	struct nxp_sar_adc *info = iio_priv(indio_dev);
+	struct dma_slave_config *config = &info->dma_config;
+	struct dma_async_tx_descriptor *desc;
+	int ret;
+
+	info->dma_buf.head = 0;
+	info->dma_buf.tail = 0;
+
+	config->src_addr = REG_ADC_CDR(info->regs_phys, info->buffered_chan[0]);
+	config->src_port_window_size = info->channels_used;
+	config->src_maxburst = info->channels_used;
+	ret = dmaengine_slave_config(info->dma_chan, config);
+	if (ret < 0)
+		return ret;
+
+	desc = dmaengine_prep_dma_cyclic(info->dma_chan,
+					 info->rx_dma_buf,
+					 NXP_SAR_ADC_DMA_BUFF_SZ,
+					 NXP_SAR_ADC_DMA_BUFF_SZ / 2,
+					 DMA_DEV_TO_MEM, DMA_PREP_INTERRUPT);
+	if (!desc)
+		return -EINVAL;
+
+	desc->callback = nxp_sar_adc_dma_cb;
+	desc->callback_param = indio_dev;
+	info->cookie = dmaengine_submit(desc);
+	ret = dma_submit_error(info->cookie);
+	if (ret) {
+		dmaengine_terminate_async(info->dma_chan);
+		return ret;
+	}
+
+	dma_async_issue_pending(info->dma_chan);
+
+	return 0;
+}
+
+static void __nxp_sar_adc_buffer_software_predisable(struct iio_dev *indio_dev)
+{
+	struct nxp_sar_adc *info = iio_priv(indio_dev);
+
+	/*
+	 * The ADC DMAEN bit should be cleared before DMA transaction
+	 * is canceled.
+	 */
+	nxp_sar_adc_dma_channels_disable(info, *indio_dev->active_scan_mask);
+	nxp_sar_adc_dma_cfg(info, false);
+	dmaengine_terminate_sync(info->dma_chan);
+}
+
+static int __nxp_sar_adc_buffer_software_postenable(struct iio_dev *indio_dev)
+{
+	struct nxp_sar_adc *info = iio_priv(indio_dev);
+	int ret;
+
+	nxp_sar_adc_dma_channels_enable(info, *indio_dev->active_scan_mask);
+
+	nxp_sar_adc_dma_cfg(info, true);
+
+	ret = nxp_sar_adc_start_cyclic_dma(indio_dev);
+	if (ret)
+		goto out_dma_channels_disable;
+
+	ret = nxp_sar_adc_start_conversion(info, false);
+	if (ret)
+		goto out_stop_cyclic_dma;
+
+	return 0;
+
+out_stop_cyclic_dma:
+	dmaengine_terminate_sync(info->dma_chan);
+
+out_dma_channels_disable:
+	nxp_sar_adc_dma_cfg(info, false);
+	nxp_sar_adc_dma_channels_disable(info, *indio_dev->active_scan_mask);
+
+	return ret;
+}
+
+static void __nxp_sar_adc_buffer_trigger_predisable(struct iio_dev *indio_dev)
+{
+	struct nxp_sar_adc *info = iio_priv(indio_dev);
+
+	nxp_sar_adc_irq_cfg(info, false);
+}
+
+static int __nxp_sar_adc_buffer_trigger_postenable(struct iio_dev *indio_dev)
+{
+	struct nxp_sar_adc *info = iio_priv(indio_dev);
+
+	nxp_sar_adc_irq_cfg(info, true);
+
+	return 0;
+}
+
+static int nxp_sar_adc_buffer_postenable(struct iio_dev *indio_dev)
+{
+	struct nxp_sar_adc *info = iio_priv(indio_dev);
+	int current_mode = iio_device_get_current_mode(indio_dev);
+	unsigned long channel;
+	int ret;
+
+	info->channels_used = 0;
+
+	/*
+	 * The SAR-ADC has two groups of channels.
+	 *
+	 *	- Group #0:
+	 *	* bit 0-7  : channel 0 -> channel 7
+	 *	* bit 8-31 : reserved
+	 *
+	 *	- Group #32:
+	 *	* bit 0-7  : Internal
+	 *	* bit 8-31 : reserved
+	 *
+	 * The 8 channels from group #0 are used in this driver for
+	 * ADC as described when declaring the IIO device and the
+	 * mapping is the same. That means the active_scan_mask can be
+	 * used directly to write the channel interrupt mask.
+	 */
+	nxp_sar_adc_channels_enable(info, *indio_dev->active_scan_mask);
+
+	for_each_set_bit(channel, indio_dev->active_scan_mask, NXP_SAR_ADC_NR_CHANNELS) {
+		info->buffered_chan[info->channels_used++] = channel;
+	}
+
+	nxp_sar_adc_enable(info);
+
+	if (current_mode == INDIO_BUFFER_SOFTWARE)
+		ret = __nxp_sar_adc_buffer_software_postenable(indio_dev);
+	else
+		ret = __nxp_sar_adc_buffer_trigger_postenable(indio_dev);
+	if (ret)
+		goto out_postenable;
+
+	return 0;
+
+out_postenable:
+	nxp_sar_adc_stop_conversion(info);
+	nxp_sar_adc_disable(info);
+	nxp_sar_adc_channels_disable(info, *indio_dev->active_scan_mask);
+
+	return ret;
+}
+
+static int nxp_sar_adc_buffer_predisable(struct iio_dev *indio_dev)
+{
+	struct nxp_sar_adc *info = iio_priv(indio_dev);
+	int currentmode = iio_device_get_current_mode(indio_dev);
+
+	nxp_sar_adc_stop_conversion(info);
+
+	if (currentmode == INDIO_BUFFER_SOFTWARE)
+		__nxp_sar_adc_buffer_software_predisable(indio_dev);
+	else
+		__nxp_sar_adc_buffer_trigger_predisable(indio_dev);
+
+	nxp_sar_adc_channels_disable(info, *indio_dev->active_scan_mask);
+	nxp_sar_adc_disable(info);
+
+	return 0;
+}
+
+static irqreturn_t nxp_sar_adc_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct nxp_sar_adc *info = iio_priv(indio_dev);
+	int ret;
+
+	ret = nxp_sar_adc_start_conversion(info, true);
+	if (ret < 0)
+		return IRQ_NONE;
+
+	return IRQ_HANDLED;
+}
+
+static const struct iio_buffer_setup_ops iio_triggered_buffer_setup_ops = {
+	.postenable = nxp_sar_adc_buffer_postenable,
+	.predisable = nxp_sar_adc_buffer_predisable,
+};
+
+static const struct iio_info nxp_sar_adc_iio_info = {
+	.read_raw  = sar_read_raw,
+	.write_raw = sar_write_raw,
+};
+
+static void nxp_sar_adc_dma_remove(void *data)
+{
+	struct nxp_sar_adc *info = data;
+
+	dma_free_coherent(info->dma_chan->device->dev, NXP_SAR_ADC_DMA_BUFF_SZ,
+			  info->dma_buf.buf, info->rx_dma_buf);
+}
+
+static int nxp_sar_adc_dma_probe(struct device *dev, struct nxp_sar_adc *info)
+{
+	struct device *dev_dma;
+	int ret;
+	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 = dma_alloc_coherent(dev_dma, NXP_SAR_ADC_DMA_BUFF_SZ,
+				    &info->rx_dma_buf, GFP_KERNEL);
+	if (!rx_buf)
+		return -ENOMEM;
+
+	info->dma_buf.buf = rx_buf;
+
+	info->dma_config.direction = DMA_DEV_TO_MEM;
+	info->dma_config.src_addr_width = NXP_SAR_ADC_DMA_SAMPLE_SZ;
+	info->dma_config.src_maxburst = 1;
+
+	ret = devm_add_action_or_reset(dev, nxp_sar_adc_dma_remove, info);
+	if (ret) {
+		nxp_sar_adc_dma_remove(info);
+		return ret;
+	}
+
+	return 0;
+}
+
+/*
+ * The documentation describes the reset values for the
+ * registers. However some registers do not have these values after a
+ * reset. It is a not desirable situation. In some other SoC family
+ * documentation NXP recommend to not assume the default values are
+ * set and to initialize the registers conforming to the documentation
+ * reset information to prevent this situation. Assume the same rule
+ * applies here as there is a discrepancy between what is read from
+ * the registers at reset time and the documentation.
+ */
+static void nxp_sar_adc_set_default_values(struct nxp_sar_adc *info)
+{
+	const u32 mcr_default	= 0x00003901;
+	const u32 msr_default	= 0x00000001;
+	const u32 ctr_default	= 0x00000014;
+	const u32 cimr_default	= 0x00000000;
+	const u32 ncmr_default	= 0x00000000;
+
+	writel(mcr_default, REG_ADC_MCR(info->regs));
+	writel(msr_default, REG_ADC_MSR(info->regs));
+	writel(ctr_default, REG_ADC_CTR0(info->regs));
+	writel(ctr_default, REG_ADC_CTR1(info->regs));
+	writel(cimr_default, REG_ADC_CIMR0(info->regs));
+	writel(cimr_default, REG_ADC_CIMR1(info->regs));
+	writel(ncmr_default, REG_ADC_NCMR0(info->regs));
+	writel(ncmr_default, REG_ADC_NCMR1(info->regs));
+}
+
+static int nxp_sar_adc_probe(struct platform_device *pdev)
+{
+	const struct nxp_sar_adc_data *data;
+	struct nxp_sar_adc *info;
+	struct iio_dev *indio_dev;
+	struct resource *mem;
+	struct device *dev = &pdev->dev;
+	int irq;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(struct nxp_sar_adc));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	info = iio_priv(indio_dev);
+
+	data = of_device_get_match_data(dev);
+
+	info->vref = data->vref;
+
+	info->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &mem);
+	if (IS_ERR(info->regs))
+		return dev_err_probe(dev, PTR_ERR(info->regs),
+				     "failed to get and remap resource");
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return dev_err_probe(dev, irq, "no irq resource\n");
+
+	ret = devm_request_irq(dev, irq, nxp_sar_adc_isr, 0,
+			       dev_name(dev), indio_dev);
+	if (ret < 0)
+		dev_err_probe(dev, ret, "failed requesting irq, irq = %d\n", irq);
+
+	info->regs_phys = mem->start;
+	spin_lock_init(&info->lock);
+
+	info->clk = devm_clk_get_enabled(dev, "adc");
+	if (IS_ERR(info->clk))
+		return dev_err_probe(dev, PTR_ERR(info->clk),
+				     "failed to get the clock\n");
+
+	platform_set_drvdata(pdev, indio_dev);
+
+	init_completion(&info->completion);
+
+	indio_dev->name = dev_name(dev);
+	indio_dev->info = &nxp_sar_adc_iio_info;
+	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
+	indio_dev->channels = nxp_sar_adc_iio_channels;
+	indio_dev->num_channels = ARRAY_SIZE(nxp_sar_adc_iio_channels);
+
+	nxp_sar_adc_set_default_values(info);
+
+	ret = nxp_sar_adc_calibration(info);
+	if (ret) {
+		dev_err(dev, "Calibration failed: %d\n", ret);
+		return ret;
+	}
+
+	ret = nxp_sar_adc_dma_probe(dev, info);
+	if (ret) {
+		dev_err(dev, "Failed to initialize the dma\n");
+		return ret;
+	}
+
+	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
+					      &iio_pollfunc_store_time,
+					      &nxp_sar_adc_trigger_handler,
+					      &iio_triggered_buffer_setup_ops);
+	if (ret < 0) {
+		dev_err(dev, "Couldn't initialise the buffer\n");
+		return ret;
+	}
+
+	ret = devm_iio_device_register(dev, indio_dev);
+	if (ret) {
+		dev_err(dev, "Couldn't register the device.\n");
+		return ret;
+	}
+
+	dev_info(dev, "Device initialized successfully.\n");
+
+	return 0;
+}
+
+static void nxp_sar_adc_remove(struct platform_device *pdev)
+{
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct nxp_sar_adc *info = iio_priv(indio_dev);
+
+	nxp_sar_adc_stop_conversion(info);
+	nxp_sar_adc_channels_disable(info, REG_ADC_CH_MASK);
+	nxp_sar_adc_dma_channels_disable(info, REG_ADC_CH_MASK);
+	nxp_sar_adc_dma_cfg(info, false);
+	nxp_sar_adc_disable(info);
+	dmaengine_terminate_sync(info->dma_chan);
+	iio_device_unregister(indio_dev);
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int nxp_sar_adc_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct nxp_sar_adc *info = iio_priv(indio_dev);
+
+	info->pwdn = nxp_sar_adc_disable(info);
+	info->inpsamp = nxp_sar_adc_conversion_timing_get(info);
+
+	clk_disable_unprepare(info->clk);
+
+	return 0;
+}
+
+static int nxp_sar_adc_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct nxp_sar_adc *info = iio_priv(indio_dev);
+	int ret;
+
+	ret = clk_prepare_enable(info->clk);
+	if (ret)
+		return ret;
+
+	nxp_sar_adc_conversion_timing_set(info, info->inpsamp);
+
+	if (!info->pwdn)
+		nxp_sar_adc_enable(info);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(nxp_sar_adc_pm_ops, nxp_sar_adc_suspend, nxp_sar_adc_resume);
+
+static const struct nxp_sar_adc_data s32g2_sar_adc_data = { .vref = 1800 };
+
+static const struct of_device_id nxp_sar_adc_match[] = {
+	{ .compatible = "nxp,s32g2-sar-adc", .data = &s32g2_sar_adc_data },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, nxp_sar_adc_match);
+
+static struct platform_driver nxp_sar_adc_driver = {
+	.probe          = nxp_sar_adc_probe,
+	.remove         = nxp_sar_adc_remove,
+	.driver         = {
+		.name   = DRIVER_NAME,
+		.of_match_table = nxp_sar_adc_match,
+#ifdef CONFIG_PM_SLEEP
+		.pm     = &nxp_sar_adc_pm_ops,
+#endif
+	},
+};
+
+module_platform_driver(nxp_sar_adc_driver);
+
+MODULE_AUTHOR("NXP");
+MODULE_DESCRIPTION("NXP SAR-ADC driver");
+MODULE_LICENSE("GPL");
-- 
2.43.0

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

[Nuno Sá]

On Wed, 2025-09-03 at 12:27 +0200, Daniel Lezcano 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 David,

Just some minor review for now...

>  drivers/iio/adc/Kconfig       |   13 +
>  drivers/iio/adc/Makefile      |    1 +
>  drivers/iio/adc/nxp-sar-adc.c | 1046 +++++++++++++++++++++++++++++++++
>  3 files changed, 1060 insertions(+)
>  create mode 100644 drivers/iio/adc/nxp-sar-adc.c
> 
> 

...

> +
> +static int nxp_sar_adc_dma_probe(struct device *dev, struct nxp_sar_adc
> *info)
> +{
> +	struct device *dev_dma;
> +	int ret;
> +	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 = dma_alloc_coherent(dev_dma, NXP_SAR_ADC_DMA_BUFF_SZ,
> +				    &info->rx_dma_buf, GFP_KERNEL);
> +	if (!rx_buf)
> +		return -ENOMEM;
> +

The above needs some discussion at the very least. Have you considered the IIO
DMA buffer interface? It should be extendable to accommodate any particularity
of your usecase (or we should at least discuss it).

With it, you also gain a userspace interface where you can actually share DMA
buffers in a zero copy fashion. You can also share these buffers with USB
gadgets. For instance, with libiio, you would be able to fetch samples from your
host machine (through USB) in a very fast way (zero copy between IIO and USB).

Setting up DMA to then "having" to push it to a SW buffer and needing a syscall
to retrieve the data seems counter-productive. 

> +	info->dma_buf.buf = rx_buf;
> +
> +	info->dma_config.direction = DMA_DEV_TO_MEM;
> +	info->dma_config.src_addr_width = NXP_SAR_ADC_DMA_SAMPLE_SZ;
> +	info->dma_config.src_maxburst = 1;
> +
> +	ret = devm_add_action_or_reset(dev, nxp_sar_adc_dma_remove, info);
> +	if (ret) {
> +		nxp_sar_adc_dma_remove(info);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * The documentation describes the reset values for the
> + * registers. However some registers do not have these values after a
> + * reset. It is a not desirable situation. In some other SoC family
> + * documentation NXP recommend to not assume the default values are
> + * set and to initialize the registers conforming to the documentation
> + * reset information to prevent this situation. Assume the same rule
> + * applies here as there is a discrepancy between what is read from
> + * the registers at reset time and the documentation.
> + */
> +static void nxp_sar_adc_set_default_values(struct nxp_sar_adc *info)
> +{
> +	const u32 mcr_default	= 0x00003901;
> +	const u32 msr_default	= 0x00000001;
> +	const u32 ctr_default	= 0x00000014;
> +	const u32 cimr_default	= 0x00000000;
> +	const u32 ncmr_default	= 0x00000000;
> +

const does not really bring much here. I would rather have them as #defines.

> +	writel(mcr_default, REG_ADC_MCR(info->regs));
> +	writel(msr_default, REG_ADC_MSR(info->regs));
> +	writel(ctr_default, REG_ADC_CTR0(info->regs));
> +	writel(ctr_default, REG_ADC_CTR1(info->regs));
> +	writel(cimr_default, REG_ADC_CIMR0(info->regs));
> +	writel(cimr_default, REG_ADC_CIMR1(info->regs));
> +	writel(ncmr_default, REG_ADC_NCMR0(info->regs));
> +	writel(ncmr_default, REG_ADC_NCMR1(info->regs));
> +}
> +
> +static int nxp_sar_adc_probe(struct platform_device *pdev)
> +{
> +	const struct nxp_sar_adc_data *data;
> +	struct nxp_sar_adc *info;
> +	struct iio_dev *indio_dev;
> +	struct resource *mem;
> +	struct device *dev = &pdev->dev;
> +	int irq;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(struct nxp_sar_adc));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	info = iio_priv(indio_dev);
> +
> +	data = of_device_get_match_data(dev);
> +

device_get_match_data()
> +	info->vref = data->vref;
> +
> +	info->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &mem);
> +	if (IS_ERR(info->regs))
> +		return dev_err_probe(dev, PTR_ERR(info->regs),
> +				     "failed to get and remap resource");
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return dev_err_probe(dev, irq, "no irq resource\n");
> +
> +	ret = devm_request_irq(dev, irq, nxp_sar_adc_isr, 0,
> +			       dev_name(dev), indio_dev);
> +	if (ret < 0)
> +		dev_err_probe(dev, ret, "failed requesting irq, irq = %d\n",
> irq);
> +
> +	info->regs_phys = mem->start;
> +	spin_lock_init(&info->lock);
> +
> +	info->clk = devm_clk_get_enabled(dev, "adc");
> +	if (IS_ERR(info->clk))
> +		return dev_err_probe(dev, PTR_ERR(info->clk),
> +				     "failed to get the clock\n");
> +
> +	platform_set_drvdata(pdev, indio_dev);
> +
> +	init_completion(&info->completion);
> +
> +	indio_dev->name = dev_name(dev);
> +	indio_dev->info = &nxp_sar_adc_iio_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
> +	indio_dev->channels = nxp_sar_adc_iio_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(nxp_sar_adc_iio_channels);
> +
> +	nxp_sar_adc_set_default_values(info);
> +
> +	ret = nxp_sar_adc_calibration(info);
> +	if (ret) {
> +		dev_err(dev, "Calibration failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = nxp_sar_adc_dma_probe(dev, info);
> +	if (ret) {
> +		dev_err(dev, "Failed to initialize the dma\n");
> +		return ret;
> +	}
> +
> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> +					      &iio_pollfunc_store_time,
> +					      &nxp_sar_adc_trigger_handler,
> +					     
> &iio_triggered_buffer_setup_ops);
> +	if (ret < 0) {
> +		dev_err(dev, "Couldn't initialise the buffer\n");
> +		return ret;
> +	}
> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret) {
> +		dev_err(dev, "Couldn't register the device.\n");

dev_err_probe(). Ditto for all other places.

> +		return ret;
> +	}
> +
> +	dev_info(dev, "Device initialized successfully.\n");
> 
dev_dbg() if you want to keep it but kind of useless IMHO.

> +	return 0;
> +}
> +
> +static void nxp_sar_adc_remove(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct nxp_sar_adc *info = iio_priv(indio_dev);
> +
> +	nxp_sar_adc_stop_conversion(info);
> +	nxp_sar_adc_channels_disable(info, REG_ADC_CH_MASK);
> +	nxp_sar_adc_dma_channels_disable(info, REG_ADC_CH_MASK);
> +	nxp_sar_adc_dma_cfg(info, false);
> +	nxp_sar_adc_disable(info);
> +	dmaengine_terminate_sync(info->dma_chan);
> +	iio_device_unregister(indio_dev);

You're using devm_iio_device_register().

> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int nxp_sar_adc_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct nxp_sar_adc *info = iio_priv(indio_dev);
> +
> +	info->pwdn = nxp_sar_adc_disable(info);
> +	info->inpsamp = nxp_sar_adc_conversion_timing_get(info);
> +
> +	clk_disable_unprepare(info->clk);
> +
> +	return 0;
> +}
> +
> +static int nxp_sar_adc_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct nxp_sar_adc *info = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = clk_prepare_enable(info->clk);
> +	if (ret)
> +		return ret;
> +
> +	nxp_sar_adc_conversion_timing_set(info, info->inpsamp);
> +
> +	if (!info->pwdn)
> +		nxp_sar_adc_enable(info);
> +
> +	return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(nxp_sar_adc_pm_ops, nxp_sar_adc_suspend,
> nxp_sar_adc_resume);
> +
> +static const struct nxp_sar_adc_data s32g2_sar_adc_data = { .vref = 1800 };
> +
> +static const struct of_device_id nxp_sar_adc_match[] = {
> +	{ .compatible = "nxp,s32g2-sar-adc", .data = &s32g2_sar_adc_data },
> +	{ /* sentinel */ }

No need for the comment. Kind of obvious :)

> +};
> +MODULE_DEVICE_TABLE(of, nxp_sar_adc_match);
> +
> +static struct platform_driver nxp_sar_adc_driver = {
> +	.probe          = nxp_sar_adc_probe,
> +	.remove         = nxp_sar_adc_remove,
> +	.driver         = {
> +		.name   = DRIVER_NAME,
> +		.of_match_table = nxp_sar_adc_match,
> +#ifdef CONFIG_PM_SLEEP

You should not need the above. Look at pm_ptr() and friends.

> +		.pm     = &nxp_sar_adc_pm_ops,
> +#endif
> +	},
> +};
> +
> +module_platform_driver(nxp_sar_adc_driver);
> +
> +MODULE_AUTHOR("NXP");
> +MODULE_DESCRIPTION("NXP SAR-ADC driver");
> +MODULE_LICENSE("GPL");

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

[Daniel Lezcano]

Hi Nuno,

On 03/09/2025 13:20, Nuno Sá wrote:
> On Wed, 2025-09-03 at 12:27 +0200, Daniel Lezcano 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 David,

s/David/Daniel/ :)

> Just some minor review for now...

Whow, thanks for fast review !

[ ... ]

>> +static int nxp_sar_adc_dma_probe(struct device *dev, struct nxp_sar_adc
>> *info)
>> +{
>> +	struct device *dev_dma;
>> +	int ret;
>> +	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 = dma_alloc_coherent(dev_dma, NXP_SAR_ADC_DMA_BUFF_SZ,
>> +				    &info->rx_dma_buf, GFP_KERNEL);
>> +	if (!rx_buf)
>> +		return -ENOMEM;
>> +
> 
> The above needs some discussion at the very least. Have you considered the IIO
> DMA buffer interface? It should be extendable to accommodate any particularity
> of your usecase (or we should at least discuss it).
> 
> With it, you also gain a userspace interface where you can actually share DMA
> buffers in a zero copy fashion. You can also share these buffers with USB
> gadgets. For instance, with libiio, you would be able to fetch samples from your
> host machine (through USB) in a very fast way (zero copy between IIO and USB).
> 
> Setting up DMA to then "having" to push it to a SW buffer and needing a syscall
> to retrieve the data seems counter-productive.

I'm not very used to dma. If there is a better implementation to put in 
place I'll be glad to take any suggestion to understand the approach.

Is there any driver using the IIO DMA buffer interface I can refer to ?

[ ... ]


-- 
<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

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

[Jonathan Cameron]

On Wed, 03 Sep 2025 12:20:39 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Wed, 2025-09-03 at 12:27 +0200, Daniel Lezcano 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 David,
> 
> Just some minor review for now...
A couple of follow ups (ignoring the DMA buf as others are better
than I am to comment on that!)

> > +/*
> > + * The documentation describes the reset values for the
> > + * registers. However some registers do not have these values after a
> > + * reset. It is a not desirable situation. In some other SoC family
> > + * documentation NXP recommend to not assume the default values are
> > + * set and to initialize the registers conforming to the documentation
> > + * reset information to prevent this situation. Assume the same rule
> > + * applies here as there is a discrepancy between what is read from
> > + * the registers at reset time and the documentation.
> > + */
> > +static void nxp_sar_adc_set_default_values(struct nxp_sar_adc *info)
> > +{
> > +	const u32 mcr_default	= 0x00003901;
> > +	const u32 msr_default	= 0x00000001;
> > +	const u32 ctr_default	= 0x00000014;
> > +	const u32 cimr_default	= 0x00000000;
> > +	const u32 ncmr_default	= 0x00000000;
> > +  
> 
> const does not really bring much here. I would rather have them as #defines.

Unless they can be broken down into meaningful fields I'd
not bother with defines. Just put the values in the writel()
as their meaning is clear from what is being registered.

> 
> > +	writel(mcr_default, REG_ADC_MCR(info->regs));
> > +	writel(msr_default, REG_ADC_MSR(info->regs));
> > +	writel(ctr_default, REG_ADC_CTR0(info->regs));
> > +	writel(ctr_default, REG_ADC_CTR1(info->regs));
> > +	writel(cimr_default, REG_ADC_CIMR0(info->regs));
> > +	writel(cimr_default, REG_ADC_CIMR1(info->regs));
> > +	writel(ncmr_default, REG_ADC_NCMR0(info->regs));
> > +	writel(ncmr_default, REG_ADC_NCMR1(info->regs));
> > +}

> > +};
> > +MODULE_DEVICE_TABLE(of, nxp_sar_adc_match);
> > +
> > +static struct platform_driver nxp_sar_adc_driver = {
> > +	.probe          = nxp_sar_adc_probe,
> > +	.remove         = nxp_sar_adc_remove,
> > +	.driver         = {
> > +		.name   = DRIVER_NAME,
> > +		.of_match_table = nxp_sar_adc_match,
> > +#ifdef CONFIG_PM_SLEEP  
> 
> You should not need the above. Look at pm_ptr() and friends.

Further to that, DEFINE_SIMPLE_DEV_PM_OPS() and drop the guards
around the functions.  The trick here is that it exposes
the functions to the compiler but lets it figure out they aren't
actually used and drop them.  All with out ifdef or __maybe_unused
etc.

> 
> > +		.pm     = &nxp_sar_adc_pm_ops,
> > +#endif
> > +	},
> > +};
> > +
> > +module_platform_driver(nxp_sar_adc_driver);
> > +
> > +MODULE_AUTHOR("NXP");
> > +MODULE_DESCRIPTION("NXP SAR-ADC driver");
> > +MODULE_LICENSE("GPL");  
> 

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

[Daniel Lezcano]

Hi Nuno,

On 03/09/2025 13:20, Nuno Sá wrote:
> On Wed, 2025-09-03 at 12:27 +0200, Daniel Lezcano 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>
>> ---

[ ... ]

> The above needs some discussion at the very least. Have you considered the IIO
> DMA buffer interface? It should be extendable to accommodate any particularity
> of your usecase (or we should at least discuss it).
> 
> With it, you also gain a userspace interface where you can actually share DMA
> buffers in a zero copy fashion. You can also share these buffers with USB
> gadgets. For instance, with libiio, you would be able to fetch samples from your
> host machine (through USB) in a very fast way (zero copy between IIO and USB).
> 
> Setting up DMA to then "having" to push it to a SW buffer and needing a syscall
> to retrieve the data seems counter-productive.

I've read a bit about the DMA engine. It is unclear how to use it and 
there are very few examples in the different drivers to refer to.

This proposed driver supports the RAW, BUFFER and TRIGGERED.

Shall I create an IIO device with the modes:

indio_dev->modes =
	INDIO_DIRECT_MODE |
	INDIO_BUFFER_HARDWARE |
	INDIO_BUFFER_TRIGGERED

And then use:

devm_iio_triggered_buffer_setup()

and

devm_iio_dmaengine_buffer_setup_with_handle

?



-- 
<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

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

[David Lechner]

On 9/4/25 12:40 PM, Daniel Lezcano wrote:
> 
> Hi Nuno,
> 
> On 03/09/2025 13:20, Nuno Sá wrote:
>> On Wed, 2025-09-03 at 12:27 +0200, Daniel Lezcano 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>
>>> ---
> 
> [ ... ]
> 
>> The above needs some discussion at the very least. Have you considered the IIO
>> DMA buffer interface? It should be extendable to accommodate any particularity
>> of your usecase (or we should at least discuss it).
>>
>> With it, you also gain a userspace interface where you can actually share DMA
>> buffers in a zero copy fashion. You can also share these buffers with USB
>> gadgets. For instance, with libiio, you would be able to fetch samples from your
>> host machine (through USB) in a very fast way (zero copy between IIO and USB).
>>
>> Setting up DMA to then "having" to push it to a SW buffer and needing a syscall
>> to retrieve the data seems counter-productive.
> 
> I've read a bit about the DMA engine. It is unclear how to use it and there are very few examples in the different drivers to refer to.
> 
> This proposed driver supports the RAW, BUFFER and TRIGGERED.
> 
> Shall I create an IIO device with the modes:
> 
> indio_dev->modes =
>     INDIO_DIRECT_MODE |

Only INDIO_DIRECT_MODE needs to be set here.

>     INDIO_BUFFER_HARDWARE |
>     INDIO_BUFFER_TRIGGERED
> 
> And then use:
> 
> devm_iio_triggered_buffer_setup()

Yes, use this and it will add INDIO_BUFFER_TRIGGERED to the flags.

> 
> and
> 
> devm_iio_dmaengine_buffer_setup_with_handle

Likewise, this will add INDIO_BUFFER_HARDWARE.

And you likely only need to call devm_iio_dmaengine_buffer_setup() which will
save some boilerplate code.

> 
> ?
> 
> 
> 

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

[Daniel Lezcano]

On 04/09/2025 19:49, David Lechner wrote:
> On 9/4/25 12:40 PM, Daniel Lezcano wrote:
>>
>> Hi Nuno,
>>
>> On 03/09/2025 13:20, Nuno Sá wrote:
>>> On Wed, 2025-09-03 at 12:27 +0200, Daniel Lezcano 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>
>>>> ---
>>
>> [ ... ]
>>
>>> The above needs some discussion at the very least. Have you considered the IIO
>>> DMA buffer interface? It should be extendable to accommodate any particularity
>>> of your usecase (or we should at least discuss it).
>>>
>>> With it, you also gain a userspace interface where you can actually share DMA
>>> buffers in a zero copy fashion. You can also share these buffers with USB
>>> gadgets. For instance, with libiio, you would be able to fetch samples from your
>>> host machine (through USB) in a very fast way (zero copy between IIO and USB).
>>>
>>> Setting up DMA to then "having" to push it to a SW buffer and needing a syscall
>>> to retrieve the data seems counter-productive.
>>
>> I've read a bit about the DMA engine. It is unclear how to use it and there are very few examples in the different drivers to refer to.
>>
>> This proposed driver supports the RAW, BUFFER and TRIGGERED.
>>
>> Shall I create an IIO device with the modes:
>>
>> indio_dev->modes =
>>      INDIO_DIRECT_MODE |
> 
> Only INDIO_DIRECT_MODE needs to be set here.
> 
>>      INDIO_BUFFER_HARDWARE |
>>      INDIO_BUFFER_TRIGGERED
>>
>> And then use:
>>
>> devm_iio_triggered_buffer_setup()
> 
> Yes, use this and it will add INDIO_BUFFER_TRIGGERED to the flags.
> 
>>
>> and
>>
>> devm_iio_dmaengine_buffer_setup_with_handle
> 
> Likewise, this will add INDIO_BUFFER_HARDWARE.
> 
> And you likely only need to call devm_iio_dmaengine_buffer_setup() which will
> save some boilerplate code.

What is still unclear for me is the trigger and the dma modes.

If the dma engine is supported, I should use 
devm_iio_dmaengine_buffer_setup_with_handle(), but will the trigger mode 
be supported also automatically (I don't see a clear answer in the 
documentation neither in the drivers) ?

If not, shall I call devm_iio_triggered_buffer_setup() and 
devm_iio_dmaengine_buffer_setup_with_handle() ?






-- 
<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

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

[David Lechner]

On 9/5/25 4:44 AM, Daniel Lezcano wrote:
> On 04/09/2025 19:49, David Lechner wrote:
>> On 9/4/25 12:40 PM, Daniel Lezcano wrote:
>>>
>>> Hi Nuno,
>>>
>>> On 03/09/2025 13:20, Nuno Sá wrote:
>>>> On Wed, 2025-09-03 at 12:27 +0200, Daniel Lezcano 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>
>>>>> ---
>>>
>>> [ ... ]
>>>
>>>> The above needs some discussion at the very least. Have you considered the IIO
>>>> DMA buffer interface? It should be extendable to accommodate any particularity
>>>> of your usecase (or we should at least discuss it).
>>>>
>>>> With it, you also gain a userspace interface where you can actually share DMA
>>>> buffers in a zero copy fashion. You can also share these buffers with USB
>>>> gadgets. For instance, with libiio, you would be able to fetch samples from your
>>>> host machine (through USB) in a very fast way (zero copy between IIO and USB).
>>>>
>>>> Setting up DMA to then "having" to push it to a SW buffer and needing a syscall
>>>> to retrieve the data seems counter-productive.
>>>
>>> I've read a bit about the DMA engine. It is unclear how to use it and there are very few examples in the different drivers to refer to.
>>>
>>> This proposed driver supports the RAW, BUFFER and TRIGGERED.
>>>
>>> Shall I create an IIO device with the modes:
>>>
>>> indio_dev->modes =
>>>      INDIO_DIRECT_MODE |
>>
>> Only INDIO_DIRECT_MODE needs to be set here.
>>
>>>      INDIO_BUFFER_HARDWARE |
>>>      INDIO_BUFFER_TRIGGERED
>>>
>>> And then use:
>>>
>>> devm_iio_triggered_buffer_setup()
>>
>> Yes, use this and it will add INDIO_BUFFER_TRIGGERED to the flags.
>>
>>>
>>> and
>>>
>>> devm_iio_dmaengine_buffer_setup_with_handle
>>
>> Likewise, this will add INDIO_BUFFER_HARDWARE.
>>
>> And you likely only need to call devm_iio_dmaengine_buffer_setup() which will
>> save some boilerplate code.
> 
> What is still unclear for me is the trigger and the dma modes.
> 
> If the dma engine is supported, I should use devm_iio_dmaengine_buffer_setup_with_handle(), but will the trigger mode be supported also automatically (I don't see a clear answer in the documentation neither in the drivers) ?
> 
> If not, shall I call devm_iio_triggered_buffer_setup() and devm_iio_dmaengine_buffer_setup_with_handle() ?
> 

Taking a step back, what sort of real-world uses cases do you need to support?
Or are you just trying to implement everything that the ADC can do? The latter
can be a bit risky because you might end making something where you can't do
a buffered read and a single channel read at the same time, but later find out
you have a real-world application that needs to do this.

It looks like it would be possible to implement buffered reads in lots of ways.
IIO devices can have more than one buffer per device so we can add more in the
future if we need to. So I would just drop the DMA part of the implementation
for now and implement the basic triggered buffer using MCR[NSTART] and ECH
(End of Chain) interrupt request and just reading data from the ICDR registers.

I would wait to have a real-world application that requires DMA to decide the
best way to implement that. There are lots of possibilities, like does it need
an external trigger or is continuous mode good enough? Does it need to be cyclic
(something the IIO subsystem doesn't really support yet) or not. Is exact sample
timing important or do we just need a big buffer? These questions we can't
really answer without a specific application to use it.

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

[Andy Shevchenko]

On Wed, Sep 03, 2025 at 12:27:56PM +0200, Daniel Lezcano 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.

...

> +#include <linux/circ_buf.h>

Why not kfifo?

> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/dmaengine.h>
> +#include <linux/interrupt.h>
> +#include <linux/iopoll.h>

+ match.h and more are missing...

> +#include <linux/module.h>

> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>

Misuse of headers, and please make driver agnostic. There is none OF specifics
in the code AFAICT.

> +#include <linux/platform_device.h>
> +#include <linux/slab.h>

...

> +/* Main Configuration Register */
> +#define REG_ADC_MCR(__base)		(__base)

Useless macro. Perhaps (looking the others) this should be
((__base) + 0x00) which makes much more sense.

...

> +#define REG_ADC_MCR_NRSMPL_32		BIT(11)
> +#define REG_ADC_MCR_NRSMPL_128		BIT(12)
> +#define REG_ADC_MCR_NRSMPL_512		(BIT(11) | BIT(12))

These are not bits, please use them in a form of 0, 1, 2, 3 and why not using
bitfield.h?

...

> +#define NXP_SAR_ADC_CONV_TIMEOUT_MS	100
> +#define NXP_SAR_ADC_CAL_TIMEOUT_US	100000

(100 * USEC_PER_MSEC)

> +#define NXP_SAR_ADC_WAIT_US		2000

(2 * USEC_PER_MSEC)

...

> +#define NXP_SAR_ADC_DMA_BUFF_SZ		(PAGE_SIZE * NXP_SAR_ADC_DMA_SAMPLE_SZ)

Oh, PAGE_SIZE is not good to use. I believe this HW is not tolerant to any page size.
(Note, we made similar mistake in Intel IPU3 camera driver, which is now fixed)

...

> +	/* Protect circular buffers access. */
> +	spinlock_t lock;

+ spinlock.h

> +	/*
> +	 * Save and restore context
> +	 */
> +	u32 inpsamp;
> +	u32 pwdn;

+ types.h

...

> +		ndelay(div64_u64(NSEC_PER_SEC, clk_get_rate(info->clk) * 3U));

+ delay.h

Actually + math64.h (no need to include math.h which I mentioned elsewhere).

...

> +static inline int nxp_sar_adc_calibration_wait(void __iomem *base)
> +{
> +	u32 msr, ret;
> +
> +	ret = read_poll_timeout(readl, msr, !(msr & REG_ADC_MSR_CALBUSY),

Why not readl_poll_timeout()?

> +				NXP_SAR_ADC_WAIT_US,
> +				NXP_SAR_ADC_CAL_TIMEOUT_US,
> +				true, REG_ADC_MSR(base));
> +	if (ret)
> +		return ret;

> +	if (!(msr & REG_ADC_MSR_CALFAIL))
> +		return 0;

I would expect standard pattern — "errors first", but here either works.

> +	/*
> +	 * If the calibration fails, the status register bit must be
> +	 * cleared
> +	 */
> +	msr &= ~REG_ADC_MSR_CALFAIL;
> +
> +	writel(msr, REG_ADC_MSR(base));
> +
> +	return -EAGAIN;
> +}

...

> +{
> +	struct nxp_sar_adc *info = iio_priv(indio_dev);
> +	int i, ret;

Why is 'i' signed?

> +
> +	for (i = 0; i < info->channels_used; i++) {
> +		ret = nxp_sar_adc_read_data(info, info->buffered_chan[i]);
> +		if (ret < 0) {
> +			nxp_sar_adc_read_notify(info);
> +			return;
> +		}
> +
> +		info->buffer[i] = ret;
> +	}
> +
> +	nxp_sar_adc_read_notify(info);
> +	iio_push_to_buffers_with_timestamp(indio_dev,
> +					   info->buffer,
> +					   iio_get_time_ns(indio_dev));
> +	iio_trigger_notify_done(indio_dev->trig);
> +}

...

> +	/*
> +	 * On disable, we have to wait for the transaction to finish.
> +	 * ADC does not abort the transaction if a chain conversion
> +	 * is in progress.
> +	 * Wait for the worst case scenario - 80 ADC clk cycles.
> +	 */
> +	ndelay(div64_u64(NSEC_PER_SEC, clk_get_rate(info->clk)) * 80U);

Could it possible go wrong and with low rate clocks (kHz:ish) this will go into
lo-o-o-o-ng *atomic* delay?

> +}

...

> +		nxp_sar_adc_channels_enable(info, 1 >> chan->channel);

1 >> ?!? Did you want BIT(channel)? Or simply channel != 0?

...

> +static int sar_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> +			 int val, int val2, long mask)
> +{
> +	struct nxp_sar_adc *info = iio_priv(indio_dev);
> +	u32 inpsamp;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		/*
> +		 * Configures the sample period duration in terms of
> +		 * the SAR controller clock. The minimum acceptable
> +		 * value is 8. Configuring to a value lower than 8
> +		 * sets the sample period to 8 cycles.  We read the
> +		 * clock value and divide by the sampling timing which
> +		 * gives us the number of cycles expected. The value
> +		 * is 8 bits wide, consequently the max value is 0xFF
> +		 */
> +		inpsamp = clk_get_rate(info->clk) / val - NXP_SAR_ADC_CONV_TIME;
> +		nxp_sar_adc_conversion_timing_set(info, inpsamp);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

> +	return 0;

Return directly from switch-case.

> +}

...

> +static void nxp_sar_adc_dma_cb(void *data)
> +{
> +	struct nxp_sar_adc *info = iio_priv((struct iio_dev *)data);
> +	struct iio_dev *indio_dev = data;
> +	struct dma_tx_state state;
> +	struct circ_buf *dma_buf;
> +	struct device *dev_dma;
> +	unsigned long flags;
> +	u32 *dma_samples;
> +	s64 timestamp;
> +	int idx, ret;
> +
> +	dma_buf = &info->dma_buf;
> +	dma_samples = (u32 *)dma_buf->buf;
> +	dev_dma = info->dma_chan->device->dev;

> +	spin_lock_irqsave(&info->lock, flags);

Why not guard()() from cleanup.h?

> +	dmaengine_tx_status(info->dma_chan,
> +			    info->cookie, &state);

Perfectly one line. No return check?

> +	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 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 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_timestamp(indio_dev,
> +							 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);
> +	spin_unlock_irqrestore(&info->lock, flags);
> +}

...

> +	/*
> +	 * The SAR-ADC has two groups of channels.
> +	 *
> +	 *	- Group #0:
> +	 *	* bit 0-7  : channel 0 -> channel 7
> +	 *	* bit 8-31 : reserved
> +	 *
> +	 *	- Group #32:
> +	 *	* bit 0-7  : Internal
> +	 *	* bit 8-31 : reserved
> +	 *
> +	 * The 8 channels from group #0 are used in this driver for
> +	 * ADC as described when declaring the IIO device and the
> +	 * mapping is the same. That means the active_scan_mask can be
> +	 * used directly to write the channel interrupt mask.
> +	 */
> +	nxp_sar_adc_channels_enable(info, *indio_dev->active_scan_mask);
> +
> +	for_each_set_bit(channel, indio_dev->active_scan_mask, NXP_SAR_ADC_NR_CHANNELS) {

+ bitops.h

> +		info->buffered_chan[info->channels_used++] = channel;
> +	}

{} are redundant.

...

> +static int nxp_sar_adc_dma_probe(struct device *dev, struct nxp_sar_adc *info)
> +{
> +	struct device *dev_dma;
> +	int ret;
> +	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 = dma_alloc_coherent(dev_dma, NXP_SAR_ADC_DMA_BUFF_SZ,
> +				    &info->rx_dma_buf, GFP_KERNEL);
> +	if (!rx_buf)
> +		return -ENOMEM;
> +
> +	info->dma_buf.buf = rx_buf;
> +
> +	info->dma_config.direction = DMA_DEV_TO_MEM;
> +	info->dma_config.src_addr_width = NXP_SAR_ADC_DMA_SAMPLE_SZ;
> +	info->dma_config.src_maxburst = 1;
> +
> +	ret = devm_add_action_or_reset(dev, nxp_sar_adc_dma_remove, info);
> +	if (ret) {
> +		nxp_sar_adc_dma_remove(info);

> +		return ret;
> +	}
> +
> +	return 0;

	return ret;

> +}

...

> +/*
> + * The documentation describes the reset values for the
> + * registers. However some registers do not have these values after a
> + * reset. It is a not desirable situation. In some other SoC family
> + * documentation NXP recommend to not assume the default values are
> + * set and to initialize the registers conforming to the documentation
> + * reset information to prevent this situation. Assume the same rule
> + * applies here as there is a discrepancy between what is read from
> + * the registers at reset time and the documentation.
> + */
> +static void nxp_sar_adc_set_default_values(struct nxp_sar_adc *info)
> +{
> +	const u32 mcr_default	= 0x00003901;
> +	const u32 msr_default	= 0x00000001;
> +	const u32 ctr_default	= 0x00000014;
> +	const u32 cimr_default	= 0x00000000;
> +	const u32 ncmr_default	= 0x00000000;

What is the purpose of having these constant to be temporary variables in the
code?

> +	writel(mcr_default, REG_ADC_MCR(info->regs));
> +	writel(msr_default, REG_ADC_MSR(info->regs));
> +	writel(ctr_default, REG_ADC_CTR0(info->regs));
> +	writel(ctr_default, REG_ADC_CTR1(info->regs));
> +	writel(cimr_default, REG_ADC_CIMR0(info->regs));
> +	writel(cimr_default, REG_ADC_CIMR1(info->regs));
> +	writel(ncmr_default, REG_ADC_NCMR0(info->regs));
> +	writel(ncmr_default, REG_ADC_NCMR1(info->regs));
> +}

...

> +static int nxp_sar_adc_probe(struct platform_device *pdev)
> +{
> +	const struct nxp_sar_adc_data *data;
> +	struct nxp_sar_adc *info;
> +	struct iio_dev *indio_dev;
> +	struct resource *mem;
> +	struct device *dev = &pdev->dev;
> +	int irq;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(struct nxp_sar_adc));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	info = iio_priv(indio_dev);

> +	data = of_device_get_match_data(dev);

We have an agnostic alternative in property.h.

> +	info->vref = data->vref;

vref_uV / vref_mV in both cases?

> +	info->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &mem);
> +	if (IS_ERR(info->regs))
> +		return dev_err_probe(dev, PTR_ERR(info->regs),
> +				     "failed to get and remap resource");
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)

> +		return dev_err_probe(dev, irq, "no irq resource\n");

No need, it prints message if fails.

> +	ret = devm_request_irq(dev, irq, nxp_sar_adc_isr, 0,
> +			       dev_name(dev), indio_dev);
> +	if (ret < 0)
> +		dev_err_probe(dev, ret, "failed requesting irq, irq = %d\n", irq);
> +
> +	info->regs_phys = mem->start;
> +	spin_lock_init(&info->lock);
> +
> +	info->clk = devm_clk_get_enabled(dev, "adc");
> +	if (IS_ERR(info->clk))
> +		return dev_err_probe(dev, PTR_ERR(info->clk),
> +				     "failed to get the clock\n");

> +	platform_set_drvdata(pdev, indio_dev);

> +	init_completion(&info->completion);
> +
> +	indio_dev->name = dev_name(dev);
> +	indio_dev->info = &nxp_sar_adc_iio_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
> +	indio_dev->channels = nxp_sar_adc_iio_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(nxp_sar_adc_iio_channels);
> +
> +	nxp_sar_adc_set_default_values(info);
> +
> +	ret = nxp_sar_adc_calibration(info);
> +	if (ret) {
> +		dev_err(dev, "Calibration failed: %d\n", ret);
> +		return ret;

Be consistent. It looks like driver is written by 2+ people who do not
communicate with each other.

		return dev_err_probe(...);

> +	}
> +
> +	ret = nxp_sar_adc_dma_probe(dev, info);
> +	if (ret) {
> +		dev_err(dev, "Failed to initialize the dma\n");
> +		return ret;
> +	}
> +
> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> +					      &iio_pollfunc_store_time,
> +					      &nxp_sar_adc_trigger_handler,
> +					      &iio_triggered_buffer_setup_ops);
> +	if (ret < 0) {
> +		dev_err(dev, "Couldn't initialise the buffer\n");
> +		return ret;
> +	}
> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret) {
> +		dev_err(dev, "Couldn't register the device.\n");
> +		return ret;
> +	}

> +	dev_info(dev, "Device initialized successfully.\n");

Noise. This should be dropped.

> +	return 0;
> +}

> +};

...

> +

Redundant blank line.

> +module_platform_driver(nxp_sar_adc_driver);

-- 
With Best Regards,
Andy Shevchenko

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

[Daniel Lezcano]

Hi Andy,

thank you for the review


On 03/09/2025 13:48, Andy Shevchenko wrote:
> On Wed, Sep 03, 2025 at 12:27:56PM +0200, Daniel Lezcano 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.
> 
> ...
> 
>> +#include <linux/circ_buf.h>
> 
> Why not kfifo?

Are you suggesting to use kfifo instead of the circular buffer in the code ?

>> +#include <linux/clk.h>
>> +#include <linux/completion.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/dmaengine.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/iopoll.h>
> 
> + match.h and more are missing...
> 
>> +#include <linux/module.h>
> 
>> +#include <linux/of_irq.h>
>> +#include <linux/of_platform.h>
> 
> Misuse of headers, and please make driver agnostic. There is none OF specifics
> in the code AFAICT.
> 
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
> 
> ...
> 
>> +/* Main Configuration Register */
>> +#define REG_ADC_MCR(__base)		(__base)
> 
> Useless macro. Perhaps (looking the others) this should be
> ((__base) + 0x00) which makes much more sense.

Sure

>> +#define REG_ADC_MCR_NRSMPL_32		BIT(11)
>> +#define REG_ADC_MCR_NRSMPL_128		BIT(12)
>> +#define REG_ADC_MCR_NRSMPL_512		(BIT(11) | BIT(12))
> 
> These are not bits, please use them in a form of 0, 1, 2, 3 and why not using
> bitfield.h?
>
> ...
> 
>> +#define NXP_SAR_ADC_CONV_TIMEOUT_MS	100
>> +#define NXP_SAR_ADC_CAL_TIMEOUT_US	100000
> 
> (100 * USEC_PER_MSEC)
> 
>> +#define NXP_SAR_ADC_WAIT_US		2000
> 
> (2 * USEC_PER_MSEC)

Why is this more understandable than the raw value ?

> 
>> +#define NXP_SAR_ADC_DMA_BUFF_SZ		(PAGE_SIZE * NXP_SAR_ADC_DMA_SAMPLE_SZ)
> 
> Oh, PAGE_SIZE is not good to use. I believe this HW is not tolerant to any page size.
> (Note, we made similar mistake in Intel IPU3 camera driver, which is now fixed)

Is it acceptable to put a hardcoded 4096 value ?

> ...
> 
>> +	/* Protect circular buffers access. */
>> +	spinlock_t lock;
> 
> + spinlock.h
> 
>> +	/*
>> +	 * Save and restore context
>> +	 */
>> +	u32 inpsamp;
>> +	u32 pwdn;
> 
> + types.h
> 
> ...
> 
>> +		ndelay(div64_u64(NSEC_PER_SEC, clk_get_rate(info->clk) * 3U));
> 
> + delay.h
> 
> Actually + math64.h (no need to include math.h which I mentioned elsewhere).
> 
> ...
> 
>> +static inline int nxp_sar_adc_calibration_wait(void __iomem *base)
>> +{
>> +	u32 msr, ret;
>> +
>> +	ret = read_poll_timeout(readl, msr, !(msr & REG_ADC_MSR_CALBUSY),
> 
> Why not readl_poll_timeout()?
> 
>> +				NXP_SAR_ADC_WAIT_US,
>> +				NXP_SAR_ADC_CAL_TIMEOUT_US,
>> +				true, REG_ADC_MSR(base));
>> +	if (ret)
>> +		return ret;
> 
>> +	if (!(msr & REG_ADC_MSR_CALFAIL))
>> +		return 0;
> 
> I would expect standard pattern — "errors first", but here either works.

Does it mean this chunk of code can be preserved or do you prefer an 
error block followed with a return 0 ?

>> +	/*
>> +	 * If the calibration fails, the status register bit must be
>> +	 * cleared
>> +	 */
>> +	msr &= ~REG_ADC_MSR_CALFAIL;
>> +
>> +	writel(msr, REG_ADC_MSR(base));
>> +
>> +	return -EAGAIN;
>> +}
> 
> ...
> 
>> +{
>> +	struct nxp_sar_adc *info = iio_priv(indio_dev);
>> +	int i, ret;
> 
> Why is 'i' signed?
> 
>> +
>> +	for (i = 0; i < info->channels_used; i++) {
>> +		ret = nxp_sar_adc_read_data(info, info->buffered_chan[i]);
>> +		if (ret < 0) {
>> +			nxp_sar_adc_read_notify(info);
>> +			return;
>> +		}
>> +
>> +		info->buffer[i] = ret;
>> +	}
>> +
>> +	nxp_sar_adc_read_notify(info);
>> +	iio_push_to_buffers_with_timestamp(indio_dev,
>> +					   info->buffer,
>> +					   iio_get_time_ns(indio_dev));
>> +	iio_trigger_notify_done(indio_dev->trig);
>> +}
> 
> ...
> 
>> +	/*
>> +	 * On disable, we have to wait for the transaction to finish.
>> +	 * ADC does not abort the transaction if a chain conversion
>> +	 * is in progress.
>> +	 * Wait for the worst case scenario - 80 ADC clk cycles.
>> +	 */
>> +	ndelay(div64_u64(NSEC_PER_SEC, clk_get_rate(info->clk)) * 80U);
> 
> Could it possible go wrong and with low rate clocks (kHz:ish) this will go into
> lo-o-o-o-ng *atomic* delay?

It is the ADC clock where we need to wait for 80 cycles. The lowest 
clock rate is 40MHz, but on this platform it is 80MHz IIRC. This routine 
is called only when the capture finishes. Except I'm missing something, 
this scenario should not happen.

>> +}
> 
> ...
> 
>> +		nxp_sar_adc_channels_enable(info, 1 >> chan->channel);
> 
> 1 >> ?!? Did you want BIT(channel)? Or simply channel != 0?

Yeah, BIT(chan->channel) is better

>> +static void nxp_sar_adc_dma_cb(void *data)
>> +{
>> +	struct nxp_sar_adc *info = iio_priv((struct iio_dev *)data);
>> +	struct iio_dev *indio_dev = data;
>> +	struct dma_tx_state state;
>> +	struct circ_buf *dma_buf;
>> +	struct device *dev_dma;
>> +	unsigned long flags;
>> +	u32 *dma_samples;
>> +	s64 timestamp;
>> +	int idx, ret;
>> +
>> +	dma_buf = &info->dma_buf;
>> +	dma_samples = (u32 *)dma_buf->buf;
>> +	dev_dma = info->dma_chan->device->dev;
> 
>> +	spin_lock_irqsave(&info->lock, flags);
> 
> Why not guard()() from cleanup.h?

sure

>> +	dmaengine_tx_status(info->dma_chan,
>> +			    info->cookie, &state);
> 
> Perfectly one line. No return check?

Ok, will see if the IIO DMA API has an impact on this portion of code 
before checking the return code. However, the status is often ignored in 
the other drivers.

[ ... ]

>> +/*
>> + * The documentation describes the reset values for the
>> + * registers. However some registers do not have these values after a
>> + * reset. It is a not desirable situation. In some other SoC family
>> + * documentation NXP recommend to not assume the default values are
>> + * set and to initialize the registers conforming to the documentation
>> + * reset information to prevent this situation. Assume the same rule
>> + * applies here as there is a discrepancy between what is read from
>> + * the registers at reset time and the documentation.
>> + */
>> +static void nxp_sar_adc_set_default_values(struct nxp_sar_adc *info)
>> +{
>> +	const u32 mcr_default	= 0x00003901;
>> +	const u32 msr_default	= 0x00000001;
>> +	const u32 ctr_default	= 0x00000014;
>> +	const u32 cimr_default	= 0x00000000;
>> +	const u32 ncmr_default	= 0x00000000;
> 
> What is the purpose of having these constant to be temporary variables in the
> code?

The purpose is to group everything in a single function. As the default 
values are only used in this function, creating the macros where we have 
to roll up and down to check their values seemed to me pointless. But if 
you prefer macros, it is fine I can convert them.

>> +	writel(mcr_default, REG_ADC_MCR(info->regs));
>> +	writel(msr_default, REG_ADC_MSR(info->regs));
>> +	writel(ctr_default, REG_ADC_CTR0(info->regs));
>> +	writel(ctr_default, REG_ADC_CTR1(info->regs));
>> +	writel(cimr_default, REG_ADC_CIMR0(info->regs));
>> +	writel(cimr_default, REG_ADC_CIMR1(info->regs));
>> +	writel(ncmr_default, REG_ADC_NCMR0(info->regs));
>> +	writel(ncmr_default, REG_ADC_NCMR1(info->regs));
>> +}
> 
> ...
> 
>> +static int nxp_sar_adc_probe(struct platform_device *pdev)
>> +{
>> +	const struct nxp_sar_adc_data *data;
>> +	struct nxp_sar_adc *info;
>> +	struct iio_dev *indio_dev;
>> +	struct resource *mem;
>> +	struct device *dev = &pdev->dev;
>> +	int irq;
>> +	int ret;
>> +
>> +	indio_dev = devm_iio_device_alloc(dev, sizeof(struct nxp_sar_adc));
>> +	if (!indio_dev)
>> +		return -ENOMEM;
>> +
>> +	info = iio_priv(indio_dev);
> 
>> +	data = of_device_get_match_data(dev);
> 
> We have an agnostic alternative in property.h.
> 
>> +	info->vref = data->vref;
> 
> vref_uV / vref_mV in both cases?

Good suggestion, it makes sense

>> +	info->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &mem);
>> +	if (IS_ERR(info->regs))
>> +		return dev_err_probe(dev, PTR_ERR(info->regs),
>> +				     "failed to get and remap resource");
>> +
>> +	irq = platform_get_irq(pdev, 0);
>> +	if (irq < 0)
> 
>> +		return dev_err_probe(dev, irq, "no irq resource\n");
> 
> No need, it prints message if fails.
> 
>> +	ret = devm_request_irq(dev, irq, nxp_sar_adc_isr, 0,
>> +			       dev_name(dev), indio_dev);
>> +	if (ret < 0)
>> +		dev_err_probe(dev, ret, "failed requesting irq, irq = %d\n", irq);
>> +
>> +	info->regs_phys = mem->start;
>> +	spin_lock_init(&info->lock);
>> +
>> +	info->clk = devm_clk_get_enabled(dev, "adc");
>> +	if (IS_ERR(info->clk))
>> +		return dev_err_probe(dev, PTR_ERR(info->clk),
>> +				     "failed to get the clock\n");
> 
>> +	platform_set_drvdata(pdev, indio_dev);
> 
>> +	init_completion(&info->completion);
>> +
>> +	indio_dev->name = dev_name(dev);
>> +	indio_dev->info = &nxp_sar_adc_iio_info;
>> +	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
>> +	indio_dev->channels = nxp_sar_adc_iio_channels;
>> +	indio_dev->num_channels = ARRAY_SIZE(nxp_sar_adc_iio_channels);
>> +
>> +	nxp_sar_adc_set_default_values(info);
>> +
>> +	ret = nxp_sar_adc_calibration(info);
>> +	if (ret) {
>> +		dev_err(dev, "Calibration failed: %d\n", ret);
>> +		return ret;
> 
> Be consistent. It looks like driver is written by 2+ people who do not
> communicate with each other.

Indeed ... :)

>> +	}
>> +
>> +	ret = nxp_sar_adc_dma_probe(dev, info);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to initialize the dma\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
>> +					      &iio_pollfunc_store_time,
>> +					      &nxp_sar_adc_trigger_handler,
>> +					      &iio_triggered_buffer_setup_ops);
>> +	if (ret < 0) {
>> +		dev_err(dev, "Couldn't initialise the buffer\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = devm_iio_device_register(dev, indio_dev);
>> +	if (ret) {
>> +		dev_err(dev, "Couldn't register the device.\n");
>> +		return ret;
>> +	}
> 
>> +	dev_info(dev, "Device initialized successfully.\n");
> 
> Noise. This should be dropped.

Ok, thanks again for the review

   -- Daniel

-- 
<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

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

[Andy Shevchenko]

On Wed, Sep 03, 2025 at 05:28:09PM +0200, Daniel Lezcano wrote:
> On 03/09/2025 13:48, Andy Shevchenko wrote:
> > On Wed, Sep 03, 2025 at 12:27:56PM +0200, Daniel Lezcano wrote:

...

> > > +#include <linux/circ_buf.h>
> > 
> > Why not kfifo?
> 
> Are you suggesting to use kfifo instead of the circular buffer in the code ?

Asking, due to relatively recent move in the whole serial subsystem.

...

> > > +#define NXP_SAR_ADC_CAL_TIMEOUT_US	100000
> > 
> > (100 * USEC_PER_MSEC)
> > 
> > > +#define NXP_SAR_ADC_WAIT_US		2000
> > 
> > (2 * USEC_PER_MSEC)
> 
> Why is this more understandable than the raw value ?

Because instead of counting 0:s and checking the suffix, I can immediately read
from the value like:

"This is 2 milliseconds in microseconds units."

It's more understandable and robust against possible typos.

...

> > > +#define NXP_SAR_ADC_DMA_BUFF_SZ		(PAGE_SIZE * NXP_SAR_ADC_DMA_SAMPLE_SZ)
> > 
> > Oh, PAGE_SIZE is not good to use. I believe this HW is not tolerant to any page size.
> > (Note, we made similar mistake in Intel IPU3 camera driver, which is now fixed)
> 
> Is it acceptable to put a hardcoded 4096 value ?

Sure, define it like HW page size

/* This ADC relies on the page size to be 4kB */
NXP_SAR_PAGE_SIZE		0x1000 // or SZ_4K, or ...

...

> > > +	ret = read_poll_timeout(readl, msr, !(msr & REG_ADC_MSR_CALBUSY),
> > 
> > Why not readl_poll_timeout()?
> > 
> > > +				NXP_SAR_ADC_WAIT_US,
> > > +				NXP_SAR_ADC_CAL_TIMEOUT_US,
> > > +				true, REG_ADC_MSR(base));
> > > +	if (ret)
> > > +		return ret;
> > 
> > > +	if (!(msr & REG_ADC_MSR_CALFAIL))
> > > +		return 0;
> > 
> > I would expect standard pattern — "errors first", but here either works.
> 
> Does it mean this chunk of code can be preserved or do you prefer an error
> block followed with a return 0 ?

Up to you. Only the question above (readl_poll_timeout() use) stays unanswered
so far.

...

> > > +	/*
> > > +	 * On disable, we have to wait for the transaction to finish.
> > > +	 * ADC does not abort the transaction if a chain conversion
> > > +	 * is in progress.
> > > +	 * Wait for the worst case scenario - 80 ADC clk cycles.
> > > +	 */
> > > +	ndelay(div64_u64(NSEC_PER_SEC, clk_get_rate(info->clk)) * 80U);
> > 
> > Could it possible go wrong and with low rate clocks (kHz:ish) this will go into
> > lo-o-o-o-ng *atomic* delay?
> 
> It is the ADC clock where we need to wait for 80 cycles. The lowest clock
> rate is 40MHz, but on this platform it is 80MHz IIRC. This routine is called
> only when the capture finishes. Except I'm missing something, this scenario
> should not happen.

So, you guarantee that clk_get_rate() never returns, let's say 1000, right?
(Personally I don't see how it's possible to guarantee...)

...

> > > +		nxp_sar_adc_channels_enable(info, 1 >> chan->channel);
> > 
> > 1 >> ?!? Did you want BIT(channel)? Or simply channel != 0?
> 
> Yeah, BIT(chan->channel) is better

But is the above a bug in the original proposal or not? I mean one wanted left
instead of right shift.

...

> > > +	dmaengine_tx_status(info->dma_chan,
> > > +			    info->cookie, &state);
> > 
> > Perfectly one line. No return check?
> 
> Ok, will see if the IIO DMA API has an impact on this portion of code before
> checking the return code. However, the status is often ignored in the other
> drivers.

...which doesn't mean it's a good example to follow.

-- 
With Best Regards,
Andy Shevchenko

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

[Daniel Lezcano]

On 04/09/2025 09:33, Andy Shevchenko wrote:
> On Wed, Sep 03, 2025 at 05:28:09PM +0200, Daniel Lezcano wrote:
>> On 03/09/2025 13:48, Andy Shevchenko wrote:
>>> On Wed, Sep 03, 2025 at 12:27:56PM +0200, Daniel Lezcano wrote:

[ ... ]

>>>> +	ret = read_poll_timeout(readl, msr, !(msr & REG_ADC_MSR_CALBUSY),
>>>
>>> Why not readl_poll_timeout()?
>>>
>>>> +				NXP_SAR_ADC_WAIT_US,
>>>> +				NXP_SAR_ADC_CAL_TIMEOUT_US,
>>>> +				true, REG_ADC_MSR(base));
>>>> +	if (ret)
>>>> +		return ret;
>>>
>>>> +	if (!(msr & REG_ADC_MSR_CALFAIL))
>>>> +		return 0;
>>>
>>> I would expect standard pattern — "errors first", but here either works.
>>
>> Does it mean this chunk of code can be preserved or do you prefer an error
>> block followed with a return 0 ?
> 
> Up to you. Only the question above (readl_poll_timeout() use) stays unanswered
> so far.

It is a typo, it should be readl_poll_timeout() as you suggested

[ ... ]


Thanks

   -- Daniel


-- 
<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