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

[PATCH v5 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

After the V1 posting there were some discussions around the DMA code
to be converted to use the IIO DMA API [1]. Unfortunately this one is
not yet fully implemented and merged in the framework to support the
cyclic DMA. The current DMA code in the driver has been used in
production since several years and even if I agree it can be improved
with a dedicated IIO DMA API in the future, IMO, it sounds reasonable
to keep it as is until the IIO DMA API supporting the cyclic DMA is
merged. I'll be glad to convert the driver code if such an API exists
and allows to remove code inside the driver.

[1] https://lore.kernel.org/all/c30bb4b6328d15a9c213c0fa64b909035dc7bf40.camel@gmail.com/

Changelog:
	* V5:
	  - Rebased against v6.18-rc1

	  ** Jonathan Cameron
	  - Replace DRIVER_NAME macro with its literal string
	  - Used FIELD_MODIFY() wherever it is possible
	  - Complied with the 80 chars convention
	  - Combined two variables in a single line declaration
	  - Removed the 'remove' function as it is useless
	  - Changed s32g2_sar_adc_data structure indentation / format

	* V4:
	  ** Christophe Jaillet **
	  - Used dmam_alloc_coherent() instead of dma_alloc_coherent()

	* V3:
	  ** Jonathan Cameron **
	  - Removed specific IIO_SYSFS_TRIGGER dependency in Kconfig
	  - Fixed headers
	  - Avoided macro generic names
	  - Used IIO_DECLARE_BUFFER_WITH_TS
	  - Documented buffer and buffer_chan
	  - Fixed single line comment
	  - Commented why channel 32 is the timestamp
	  - Renamed __<prefixed> functions
	  - Factored out the raw read function to prevent nested goto in the switch
	  - Returned -EINVAL instead of break
	  - Removed explict pointer cast
	  - Used iio_push_to_buffers_with_ts variant
	  - Fixed ordering operations in postenable / predisable
	  - Return IRQ_HANDLED even if there is an error in the isr
	  - Fixed devm_add_action_or_reset() to return directly
	  - Used sizeof(*var) instead of sizeof(struct myvar)
	  - Used model name instead of dev_name()
	  - Used dev_err_probe() in any case in the probe function
	  - Fixed indentation

	  ** David Lechner **
	  - Kept alphabetical order in Makefile
	  - Changed explicit GPL-2.0-only
	  - Removed clock name in when calling devm_clk_get_enabled()

	  ** Andriy Shevchenko **
	  - Fixed headers ordering and added the missing ones
	  - Fixed constant numeric format
	  - Ran pahole and consolidated the nxp_sar_adc structure
	  - Fixed semi-column in comments and typos
	  - Fixed indentation
	  - Moved data assignment before iio_dev allocation

	* V2:
	  - Massaged the cover letter changelog to explain the DMA
	  ** Andriy Shevchenko **
	  - Added missing headers and use proper header for of.h
	  - Changed macro offset zero to be consistent
	  - Remove macros REG_ADC_MCR_NRSMPL_* as they are unused
	  - Changed delays macro under the form 100000 => 100 * USEC_PER_MSEC
	  - Replaced PAGE_SIZE by a NXP_PAGE_SIZE = SZ_4K macro
	  - Replaced read_poll_timeout() by readl_poll_timeout()
	  - Changed error pattern "error first"
	  - Replaced variable type 'int' to 'unsigned int'
	  - Fixed bug right instead of left shift, use BIT(channel)
	  - Returned directly from switch-case
	  - Used guard(spinlock_irqsave)()
	  - One liner function call
	  - Remove redundant {}
	  - Write default values litterals instead of temporary variables
	  - Changed variable name vref -> vref_mV
	  - Removed unneeded error message
	  - Used dev_err_probe() consistently
	  - Removed successful driver probe message
	  - Removed redundant blank line

	  ** Nuno Sa **
	  - Replaced of_device_get_match_data() by device_get_match_data()
	  - Removed iio_device_unregister() because devm_iio_device_register() is used
	  - Removed "/* sentinel */" comment
	  - Removed CONFIG_PM_SLEEP defiries

	  ** Krzysztof Kozlowski / David Lechner **
	  - Removed clock-names in DT bindings
	  - Fixed minItems by maxItems

	* V1:
	  - Initial post

Daniel Lezcano (2):
  dt-bindings: iio: adc: 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   |   63 ++
 drivers/iio/adc/Kconfig                       |   12 +
 drivers/iio/adc/Makefile                      |    1 +
 drivers/iio/adc/nxp-sar-adc.c                 | 1006 +++++++++++++++++
 4 files changed, 1082 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 v5 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>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
---
 .../bindings/iio/adc/nxp,s32g2-sar-adc.yaml   | 63 +++++++++++++++++++
 1 file changed, 63 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..ec258f224df8
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/nxp,s32g2-sar-adc.yaml
@@ -0,0 +1,63 @@
+# 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:
+    maxItems: 1
+
+  dmas:
+    maxItems: 1
+
+  dma-names:
+    const: rx
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - 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>;
+        dmas = <&edma0 0 32>;
+        dma-names = "rx";
+    };
-- 
2.43.0

[PATCH v5 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       |   12 +
 drivers/iio/adc/Makefile      |    1 +
 drivers/iio/adc/nxp-sar-adc.c | 1006 +++++++++++++++++++++++++++++++++
 3 files changed, 1019 insertions(+)
 create mode 100644 drivers/iio/adc/nxp-sar-adc.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 58a14e6833f6..363eb8a3a6f2 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1212,6 +1212,18 @@ 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
+	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 d008f78dc010..c9d5bc976fd3 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -107,6 +107,7 @@ obj-$(CONFIG_MXS_LRADC_ADC) += mxs-lradc-adc.o
 obj-$(CONFIG_NAU7802) += nau7802.o
 obj-$(CONFIG_NCT7201) += nct7201.o
 obj-$(CONFIG_NPCM_ADC) += npcm_adc.o
+obj-$(CONFIG_NXP_SAR_ADC) += nxp-sar-adc.o
 obj-$(CONFIG_PAC1921) += pac1921.o
 obj-$(CONFIG_PAC1934) += pac1934.o
 obj-$(CONFIG_PALMAS_GPADC) += palmas_gpadc.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..fa390c9d911f
--- /dev/null
+++ b/drivers/iio/adc/nxp-sar-adc.c
@@ -0,0 +1,1006 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * 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/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/circ_buf.h>
+#include <linux/cleanup.h>
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/dma-mapping.h>
+#include <linux/dmaengine.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/iopoll.h>
+#include <linux/math64.h>
+#include <linux/minmax.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/time.h>
+#include <linux/types.h>
+#include <linux/units.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
+
+/* SAR ADC registers. */
+#define NXP_SAR_ADC_CDR(__base, __channel)	(((__base) + 0x100) + ((__channel) * 0x4))
+
+#define NXP_SAR_ADC_CDR_CDATA_MASK	GENMASK(11, 0)
+#define NXP_SAR_ADC_CDR_VALID		BIT(19)
+
+/* Main Configuration Register */
+#define NXP_SAR_ADC_MCR(__base)		((__base) + 0x00)
+
+#define NXP_SAR_ADC_MCR_PWDN		BIT(0)
+#define NXP_SAR_ADC_MCR_ACKO		BIT(5)
+#define NXP_SAR_ADC_MCR_ADCLKSEL	BIT(8)
+#define NXP_SAR_ADC_MCR_TSAMP_MASK	GENMASK(10, 9)
+#define NXP_SAR_ADC_MCR_NRSMPL_MASK	GENMASK(12, 11)
+#define NXP_SAR_ADC_MCR_AVGEN		BIT(13)
+#define NXP_SAR_ADC_MCR_CALSTART	BIT(14)
+#define NXP_SAR_ADC_MCR_NSTART		BIT(24)
+#define NXP_SAR_ADC_MCR_MODE		BIT(29)
+#define NXP_SAR_ADC_MCR_OWREN		BIT(31)
+
+/* Main Status Register */
+#define NXP_SAR_ADC_MSR(__base)		((__base) + 0x04)
+
+#define NXP_SAR_ADC_MSR_CALBUSY		BIT(29)
+#define NXP_SAR_ADC_MSR_CALFAIL		BIT(30)
+
+/* Interrupt Status Register */
+#define NXP_SAR_ADC_ISR(__base)		((__base) + 0x10)
+
+#define NXP_SAR_ADC_ISR_ECH		BIT(0)
+
+/*  Channel Pending Register */
+#define NXP_SAR_ADC_CEOCFR0(__base)	((__base) + 0x14)
+#define NXP_SAR_ADC_CEOCFR1(__base)	((__base) + 0x18)
+
+#define NXP_SAR_ADC_EOC_CH(c)		BIT((c) % 32)
+
+/* Interrupt Mask Register */
+#define NXP_SAR_ADC_IMR(__base)		((__base) + 0x20)
+
+/* Channel Interrupt Mask Register */
+#define NXP_SAR_ADC_CIMR0(__base)	((__base) + 0x24)
+#define NXP_SAR_ADC_CIMR1(__base)	((__base) + 0x28)
+
+/* DMA Setting Register */
+#define NXP_SAR_ADC_DMAE(__base)	((__base) + 0x40)
+
+#define NXP_SAR_ADC_DMAE_DMAEN		BIT(0)
+#define NXP_SAR_ADC_DMAE_DCLR		BIT(1)
+
+/* DMA Control register */
+#define NXP_SAR_ADC_DMAR0(__base)	((__base) + 0x44)
+#define NXP_SAR_ADC_DMAR1(__base)	((__base) + 0x48)
+
+/* Conversion Timing Register */
+#define NXP_SAR_ADC_CTR0(__base)	((__base) + 0x94)
+#define NXP_SAR_ADC_CTR1(__base)	((__base) + 0x98)
+
+#define NXP_SAR_ADC_CTR_INPSAMP_MIN	0x08
+#define NXP_SAR_ADC_CTR_INPSAMP_MAX	0xFF
+
+/* Normal Conversion Mask Register */
+#define NXP_SAR_ADC_NCMR0(__base)	((__base) + 0xa4)
+#define NXP_SAR_ADC_NCMR1(__base)	((__base) + 0xa8)
+
+/* Normal Conversion Mask Register field define */
+#define NXP_SAR_ADC_CH_MASK		GENMASK(7, 0)
+
+/* Other field define */
+#define NXP_SAR_ADC_CONV_TIMEOUT_MS	100
+#define NXP_SAR_ADC_CONV_TIMEOUT_JF	(msecs_to_jiffies(NXP_SAR_ADC_CONV_TIMEOUT_MS))
+#define NXP_SAR_ADC_CAL_TIMEOUT_US	(100 * USEC_PER_MSEC)
+#define NXP_SAR_ADC_WAIT_US		(2 * USEC_PER_MSEC)
+#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_PAGE_SIZE			SZ_4K
+#define NXP_SAR_ADC_DMA_SAMPLE_SZ	DMA_SLAVE_BUSWIDTH_4_BYTES
+#define NXP_SAR_ADC_DMA_BUFF_SZ		(NXP_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;
+	u8 current_channel;
+	u8 channels_used;
+	u16 value;
+	u32 vref_mV;
+
+	/* Save and restore context. */
+	u32 inpsamp;
+	u32 pwdn;
+
+	struct clk *clk;
+	struct dma_chan	*dma_chan;
+	struct completion completion;
+	struct circ_buf dma_buf;
+
+	dma_addr_t rx_dma_buf;
+	dma_cookie_t cookie;
+
+	/* Protect circular buffers access. */
+	spinlock_t lock;
+
+	/* Array of enabled channels. */
+	u16 buffered_chan[NXP_SAR_ADC_NR_CHANNELS];
+
+	/* Buffer to be filled by the DMA. */
+	IIO_DECLARE_BUFFER_WITH_TS(u16, buffer, NXP_SAR_ADC_NR_CHANNELS);
+};
+
+struct nxp_sar_adc_data {
+	u32 vref_mV;
+	const char *model;
+};
+
+#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),
+	/*
+	 * The NXP SAR ADC documentation marks the channels 8 to 31 as
+	 * "Reserved". Reflect the same in the driver in case new ADC
+	 * variants comes with more channels.
+	 */
+	IIO_CHAN_SOFT_TIMESTAMP(32),
+};
+
+static void nxp_sar_adc_irq_cfg(struct nxp_sar_adc *info, bool enable)
+{
+	if (enable)
+		writel(NXP_SAR_ADC_ISR_ECH, NXP_SAR_ADC_IMR(info->regs));
+	else
+		writel(0, NXP_SAR_ADC_IMR(info->regs));
+}
+
+static bool nxp_sar_adc_set_enabled(struct nxp_sar_adc *info, bool enable)
+{
+	u32 mcr;
+	bool pwdn;
+
+	mcr = readl(NXP_SAR_ADC_MCR(info->regs));
+
+	/* Return the current state. */
+	pwdn = FIELD_GET(NXP_SAR_ADC_MCR_PWDN, mcr);
+
+	/* When the enabled flag is not set, we set the power down bit */
+	FIELD_MODIFY(NXP_SAR_ADC_MCR_PWDN, &mcr, !enable);
+
+	writel(mcr, NXP_SAR_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) * 3));
+
+	return pwdn;
+}
+
+static inline bool nxp_sar_adc_enable(struct nxp_sar_adc *info)
+{
+	return nxp_sar_adc_set_enabled(info, true);
+}
+
+static inline bool nxp_sar_adc_disable(struct nxp_sar_adc *info)
+{
+	return nxp_sar_adc_set_enabled(info, false);
+}
+
+static inline void nxp_sar_adc_calibration_start(void __iomem *base)
+{
+	u32 mcr = readl(NXP_SAR_ADC_MCR(base));
+
+	FIELD_MODIFY(NXP_SAR_ADC_MCR_CALSTART, &mcr, 0x1);
+
+	writel(mcr, NXP_SAR_ADC_MCR(base));
+}
+
+static inline int nxp_sar_adc_calibration_wait(void __iomem *base)
+{
+	u32 msr, ret;
+
+	ret = readl_poll_timeout(NXP_SAR_ADC_MSR(base), msr,
+				 !FIELD_GET(NXP_SAR_ADC_MSR_CALBUSY, msr),
+				 NXP_SAR_ADC_WAIT_US,
+				 NXP_SAR_ADC_CAL_TIMEOUT_US);
+	if (ret)
+		return ret;
+
+	if (FIELD_GET(NXP_SAR_ADC_MSR_CALFAIL, msr)) {
+		/*
+		 * If the calibration fails, the status register bit
+		 * must be cleared.
+		 */
+		FIELD_MODIFY(NXP_SAR_ADC_MSR_CALFAIL, &msr, 0x0);
+		writel(msr, NXP_SAR_ADC_MSR(base));
+
+		return -EAGAIN;
+	}
+
+	return 0;
+}
+
+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, NXP_SAR_ADC_CTR_INPSAMP_MIN, NXP_SAR_ADC_CTR_INPSAMP_MAX);
+
+	writel(inpsamp, NXP_SAR_ADC_CTR0(info->regs));
+}
+
+static u32 nxp_sar_adc_conversion_timing_get(struct nxp_sar_adc *info)
+{
+	return readl(NXP_SAR_ADC_CTR0(info->regs));
+}
+
+static void nxp_sar_adc_read_notify(struct nxp_sar_adc *info)
+{
+	writel(NXP_SAR_ADC_CH_MASK, NXP_SAR_ADC_CEOCFR0(info->regs));
+	writel(NXP_SAR_ADC_CH_MASK, NXP_SAR_ADC_CEOCFR1(info->regs));
+}
+
+static int nxp_sar_adc_read_data(struct nxp_sar_adc *info, unsigned int chan)
+{
+	u32 ceocfr, cdr;
+
+	ceocfr = readl(NXP_SAR_ADC_CEOCFR0(info->regs));
+
+	/* FIELD_GET() can not be used here because EOC_CH is not constant */
+	if (!(NXP_SAR_ADC_EOC_CH(chan) & ceocfr))
+		return -EIO;
+
+	cdr = readl(NXP_SAR_ADC_CDR(info->regs, chan));
+	if (!(FIELD_GET(NXP_SAR_ADC_CDR_VALID, cdr)))
+		return -EIO;
+
+	return FIELD_GET(NXP_SAR_ADC_CDR_CDATA_MASK, cdr);
+}
+
+static void nxp_sar_adc_isr_buffer(struct iio_dev *indio_dev)
+{
+	struct nxp_sar_adc *info = iio_priv(indio_dev);
+	unsigned int i;
+	int 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_ts(indio_dev, info->buffer, sizeof(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(NXP_SAR_ADC_ISR(info->regs));
+	if (!(FIELD_GET(NXP_SAR_ADC_ISR_ECH, isr)))
+		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(NXP_SAR_ADC_ISR_ECH, NXP_SAR_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(NXP_SAR_ADC_NCMR0(info->regs));
+	cimr = readl(NXP_SAR_ADC_CIMR0(info->regs));
+
+	/* FIELD_MODIFY() can not be used because the mask is not constant */
+	ncmr &= ~mask;
+	cimr &= ~mask;
+
+	writel(ncmr, NXP_SAR_ADC_NCMR0(info->regs));
+	writel(cimr, NXP_SAR_ADC_CIMR0(info->regs));
+}
+
+static void nxp_sar_adc_channels_enable(struct nxp_sar_adc *info, u32 mask)
+{
+	u32 ncmr, cimr;
+
+	ncmr = readl(NXP_SAR_ADC_NCMR0(info->regs));
+	cimr = readl(NXP_SAR_ADC_CIMR0(info->regs));
+
+	ncmr |= mask;
+	cimr |= mask;
+
+	writel(ncmr, NXP_SAR_ADC_NCMR0(info->regs));
+	writel(cimr, NXP_SAR_ADC_CIMR0(info->regs));
+}
+
+static void nxp_sar_adc_dma_channels_enable(struct nxp_sar_adc *info, u32 mask)
+{
+	u32 dmar;
+
+	dmar = readl(NXP_SAR_ADC_DMAR0(info->regs));
+
+	dmar |= mask;
+
+	writel(dmar, NXP_SAR_ADC_DMAR0(info->regs));
+}
+
+static void nxp_sar_adc_dma_channels_disable(struct nxp_sar_adc *info, u32 mask)
+{
+	u32 dmar;
+
+	dmar = readl(NXP_SAR_ADC_DMAR0(info->regs));
+
+	dmar &= ~mask;
+
+	writel(dmar, NXP_SAR_ADC_DMAR0(info->regs));
+}
+
+static void nxp_sar_adc_dma_cfg(struct nxp_sar_adc *info, bool enable)
+{
+	u32 dmae;
+
+	dmae = readl(NXP_SAR_ADC_DMAE(info->regs));
+
+	FIELD_MODIFY(NXP_SAR_ADC_DMAE_DMAEN, &dmae, enable);
+
+	writel(dmae, NXP_SAR_ADC_DMAE(info->regs));
+}
+
+static void nxp_sar_adc_stop_conversion(struct nxp_sar_adc *info)
+{
+	u32 mcr;
+
+	mcr = readl(NXP_SAR_ADC_MCR(info->regs));
+
+	FIELD_MODIFY(NXP_SAR_ADC_MCR_NSTART, &mcr, 0x0);
+
+	writel(mcr, NXP_SAR_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(NXP_SAR_ADC_MCR(info->regs));
+
+	FIELD_MODIFY(NXP_SAR_ADC_MCR_NSTART, &mcr, 0x1);
+	FIELD_MODIFY(NXP_SAR_ADC_MCR_MODE, &mcr, !raw);
+
+	writel(mcr, NXP_SAR_ADC_MCR(info->regs));
+
+	return 0;
+}
+
+static int nxp_sar_adc_read_channel(struct nxp_sar_adc *info, int channel)
+{
+	int ret;
+
+	info->current_channel = channel;
+	nxp_sar_adc_channels_enable(info, BIT(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_disable;
+
+	ret = wait_for_completion_interruptible_timeout(&info->completion,
+							NXP_SAR_ADC_CONV_TIMEOUT_JF);
+	if (ret == 0)
+		ret = -ETIMEDOUT;
+	if (ret > 0)
+		ret = 0;
+
+	nxp_sar_adc_stop_conversion(info);
+
+out_disable:
+	nxp_sar_adc_channels_disable(info, BIT(channel));
+	nxp_sar_adc_irq_cfg(info, false);
+	nxp_sar_adc_disable(info);
+
+	return ret;
+}
+
+static int nxp_sar_adc_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;
+
+		ret = nxp_sar_adc_read_channel(info, chan->channel);
+
+		iio_device_release_direct(indio_dev);
+
+		if (ret)
+			return ret;
+
+		*val = info->value;
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		*val = info->vref_mV;
+		*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:
+		return -EINVAL;
+	}
+}
+
+static int nxp_sar_adc_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 it 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);
+		return 0;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static void nxp_sar_adc_dma_cb(void *data)
+{
+	struct nxp_sar_adc *info = iio_priv(data);
+	struct iio_dev *indio_dev = data;
+	struct dma_tx_state state;
+	struct circ_buf *dma_buf;
+	struct device *dev_dma;
+	u32 *dma_samples;
+	s64 timestamp;
+	int idx, ret;
+
+	guard(spinlock_irqsave)(&info->lock);
+
+	dma_buf = &info->dma_buf;
+	dma_samples = (u32 *)dma_buf->buf;
+	dev_dma = info->dma_chan->device->dev;
+
+	dmaengine_tx_status(info->dma_chan, info->cookie, &state);
+
+	dma_sync_single_for_cpu(dev_dma, info->rx_dma_buf,
+				NXP_SAR_ADC_DMA_BUFF_SZ, DMA_FROM_DEVICE);
+
+	/* Current head position. */
+	dma_buf->head = (NXP_SAR_ADC_DMA_BUFF_SZ - state.residue) /
+			NXP_SAR_ADC_DMA_SAMPLE_SZ;
+
+	/* If everything was transferred, avoid an off by one error. */
+	if (!state.residue)
+		dma_buf->head--;
+
+	/* Something went wrong and nothing transferred. */
+	if (state.residue == NXP_SAR_ADC_DMA_BUFF_SZ)
+		goto out;
+
+	/* Make sure that head is multiple of info->channels_used. */
+	dma_buf->head -= dma_buf->head % info->channels_used;
+
+	/*
+	 * dma_buf->tail != dma_buf->head condition will become false
+	 * because dma_buf->tail will be incremented with 1.
+	 */
+	while (dma_buf->tail != dma_buf->head) {
+		idx = dma_buf->tail % info->channels_used;
+		info->buffer[idx] = dma_samples[dma_buf->tail];
+		dma_buf->tail = (dma_buf->tail + 1) % NXP_SAR_ADC_DMA_SAMPLE_CNT;
+		if (idx != info->channels_used - 1)
+			continue;
+
+		/*
+		 * iio_push_to_buffers_with_timestamp should not be
+		 * called with dma_samples as parameter. The samples
+		 * will be smashed if timestamp is enabled.
+		 */
+		timestamp = iio_get_time_ns(indio_dev);
+		ret = iio_push_to_buffers_with_ts(indio_dev, info->buffer,
+						  sizeof(info->buffer),
+						  timestamp);
+		if (ret < 0 && ret != -EBUSY)
+			dev_err_ratelimited(&indio_dev->dev,
+					    "failed to push iio buffer: %d",
+					    ret);
+	}
+
+	dma_buf->tail = dma_buf->head;
+out:
+	dma_sync_single_for_device(dev_dma, info->rx_dma_buf,
+				   NXP_SAR_ADC_DMA_BUFF_SZ, DMA_FROM_DEVICE);
+}
+
+static int nxp_sar_adc_start_cyclic_dma(struct iio_dev *indio_dev)
+{
+	struct nxp_sar_adc *info = iio_priv(indio_dev);
+	struct dma_slave_config config;
+	struct dma_async_tx_descriptor *desc;
+	int ret;
+
+	info->dma_buf.head = 0;
+	info->dma_buf.tail = 0;
+
+	config.direction = DMA_DEV_TO_MEM;
+	config.src_addr_width = NXP_SAR_ADC_DMA_SAMPLE_SZ;
+	config.src_addr = NXP_SAR_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_do_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_stop_conversion(info);
+	dmaengine_terminate_sync(info->dma_chan);
+	nxp_sar_adc_dma_cfg(info, false);
+	nxp_sar_adc_dma_channels_disable(info, *indio_dev->active_scan_mask);
+}
+
+static int nxp_sar_adc_buffer_software_do_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_do_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_do_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_do_postenable(indio_dev);
+	else
+		ret = nxp_sar_adc_buffer_trigger_do_postenable(indio_dev);
+	if (ret)
+		goto out_postenable;
+
+	return 0;
+
+out_postenable:
+	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);
+
+	if (currentmode == INDIO_BUFFER_SOFTWARE)
+		nxp_sar_adc_buffer_software_do_predisable(indio_dev);
+	else
+		nxp_sar_adc_buffer_trigger_do_predisable(indio_dev);
+
+	nxp_sar_adc_disable(info);
+
+	nxp_sar_adc_channels_disable(info, *indio_dev->active_scan_mask);
+
+	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)
+		dev_dbg(&indio_dev->dev, "Failed to start conversion\n");
+
+	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  = nxp_sar_adc_read_raw,
+	.write_raw = nxp_sar_adc_write_raw,
+};
+
+static int nxp_sar_adc_dma_probe(struct device *dev, struct nxp_sar_adc *info)
+{
+	struct device *dev_dma;
+	u8 *rx_buf;
+
+	info->dma_chan = devm_dma_request_chan(dev, "rx");
+	if (IS_ERR(info->dma_chan))
+		return PTR_ERR(info->dma_chan);
+
+	dev_dma = info->dma_chan->device->dev;
+	rx_buf = dmam_alloc_coherent(dev_dma, NXP_SAR_ADC_DMA_BUFF_SZ,
+				     &info->rx_dma_buf, GFP_KERNEL);
+	if (!rx_buf)
+		return -ENOMEM;
+
+	info->dma_buf.buf = rx_buf;
+
+	return 0;
+}
+
+/*
+ * The documentation describes the reset values for the registers.
+ * However some registers do not have these values after a reset. It
+ * is not a desirable situation. In some other SoC family
+ * documentation NXP recommends not assuming 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)
+{
+	writel(0x00003901, NXP_SAR_ADC_MCR(info->regs));
+	writel(0x00000001, NXP_SAR_ADC_MSR(info->regs));
+	writel(0x00000014, NXP_SAR_ADC_CTR0(info->regs));
+	writel(0x00000014, NXP_SAR_ADC_CTR1(info->regs));
+	writel(0x00000000, NXP_SAR_ADC_CIMR0(info->regs));
+	writel(0x00000000, NXP_SAR_ADC_CIMR1(info->regs));
+	writel(0x00000000, NXP_SAR_ADC_NCMR0(info->regs));
+	writel(0x00000000, NXP_SAR_ADC_NCMR1(info->regs));
+}
+
+static int nxp_sar_adc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	const struct nxp_sar_adc_data *data = device_get_match_data(dev);
+	struct nxp_sar_adc *info;
+	struct iio_dev *indio_dev;
+	struct resource *mem;
+	int irq, ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*info));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	info = iio_priv(indio_dev);
+
+	info->vref_mV = data->vref_mV;
+
+	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");
+
+	info->regs_phys = mem->start;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	ret = devm_request_irq(dev, irq, nxp_sar_adc_isr, 0,
+			       dev_name(dev), indio_dev);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "failed requesting irq, irq = %d\n", irq);
+
+	spin_lock_init(&info->lock);
+
+	info->clk = devm_clk_get_enabled(dev, NULL);
+	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 = data->model;
+	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_probe(dev, ret, "Calibration failed: %d\n", ret);
+
+	ret = nxp_sar_adc_dma_probe(dev, info);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to initialize the dma\n");
+
+	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)
+		return dev_err_probe(dev, ret, "Couldn't initialise the buffer\n");
+
+	ret = devm_iio_device_register(dev, indio_dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Couldn't register the device\n");
+
+	return 0;
+}
+
+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;
+}
+
+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_mV = 1800,
+	.model = "s32g2-sar-adc"
+};
+
+static const struct of_device_id nxp_sar_adc_match[] = {
+	{ .compatible = "nxp,s32g2-sar-adc", .data = &s32g2_sar_adc_data },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, nxp_sar_adc_match);
+
+static struct platform_driver nxp_sar_adc_driver = {
+	.probe = nxp_sar_adc_probe,
+	.driver = {
+		.name = "nxp-sar-adc",
+		.of_match_table = nxp_sar_adc_match,
+		.pm = pm_ptr(&nxp_sar_adc_pm_ops),
+	},
+};
+module_platform_driver(nxp_sar_adc_driver);
+
+MODULE_AUTHOR("NXP");
+MODULE_DESCRIPTION("NXP SAR-ADC driver");
+MODULE_LICENSE("GPL");
-- 
2.43.0

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

[Andy Shevchenko]

On Fri, Oct 17, 2025 at 06:42:38PM +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?

...

> +#define NXP_SAR_ADC_IIO_BUFF_SZ		(NXP_SAR_ADC_NR_CHANNELS + (sizeof(u64) / sizeof(u16)))

Hmm... Don't we have some macros so we can avoid this kind of hard coding?

...

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

Do you need those 'U':s? clk_get_rate() already returns unsigned value of the
same or higher rank than int. No?

...

> +static int nxp_sar_adc_start_conversion(struct nxp_sar_adc *info, bool raw)
> +{
> +	u32 mcr;
> +
> +	mcr = readl(NXP_SAR_ADC_MCR(info->regs));
> +
> +	FIELD_MODIFY(NXP_SAR_ADC_MCR_NSTART, &mcr, 0x1);
> +	FIELD_MODIFY(NXP_SAR_ADC_MCR_MODE, &mcr, !raw);

!raw, which is boolean, as a parameter to FIELD_MODIFY() seems a bit odd to me,
perhaps simple

	raw ? 0 : 1

would work better?

(Note, optimizer of the complier will avoid any branching)

> +	writel(mcr, NXP_SAR_ADC_MCR(info->regs));
> +
> +	return 0;
> +}

...

> +	dma_samples = (u32 *)dma_buf->buf;

Is it aligned properly for this type of casting?

...

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

No return value check?

...

> +static const struct nxp_sar_adc_data s32g2_sar_adc_data = {
> +	.vref_mV = 1800,
> +	.model = "s32g2-sar-adc"

Keep a trailing comma as here it's not a termination member.

> +};

-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Cameron]

On Fri, 17 Oct 2025 18:42:38 +0200
Daniel Lezcano <daniel.lezcano@linaro.org> wrote:

> From: Stefan-Gabriel Mirea <stefan-gabriel.mirea@nxp.com>
> 
> The NXP S32G2 and S32G3 platforms integrate a successive approximation
> register (SAR) ADC. Two instances are available, each providing 8
> multiplexed input channels with 12-bit resolution. The conversion rate
> is up to 1 Msps depending on the configuration and sampling window.
> 
> The SAR ADC supports raw, buffer, and trigger modes. It can operate
> in both single-shot and continuous conversion modes, with optional
> hardware triggering through the cross-trigger unit (CTU) or external
> events. An internal prescaler allows adjusting the sampling clock,
> while per-channel programmable sampling times provide fine-grained
> trade-offs between accuracy and latency. Automatic calibration is
> performed at probe time to minimize offset and gain errors.
> 
> The driver is derived from the BSP implementation and has been partly
> rewritten to comply with upstream requirements. For this reason, all
> contributors are listed as co-developers, while the author refers to
> the initial BSP driver file creator.
> 
> All modes have been validated on the S32G274-RDB2 platform using an
> externally generated square wave captured by the ADC. Tests covered
> buffered streaming via IIO, trigger synchronization, and accuracy
> verification against a precision laboratory signal source.
> 
> Co-developed-by: Alexandru-Catalin Ionita <alexandru-catalin.ionita@nxp.com>
> Signed-off-by: Alexandru-Catalin Ionita <alexandru-catalin.ionita@nxp.com>
> Co-developed-by: Ciprian Costea <ciprianmarian.costea@nxp.com>
> Signed-off-by: Ciprian Costea <ciprianmarian.costea@nxp.com>
> Co-developed-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@oss.nxp.com>
> Signed-off-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@oss.nxp.com>
> Signed-off-by: Stefan-Gabriel Mirea <stefan-gabriel.mirea@nxp.com>
> Co-developed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Hi Daniel,

Only significant question in here is around lifetimes of the
dma buffer.

+CC Vinod and dmaengine list. Hopefully someone will rapidly tell me
my concern is garbage ;)

IIO folk who are familiar with dmaengine channels etc please take
a look at this as well.  I think all the upstream drivers we have doing
similar things to this predate devm_ management being a common thing.

Jonathan


> diff --git a/drivers/iio/adc/nxp-sar-adc.c b/drivers/iio/adc/nxp-sar-adc.c
> new file mode 100644
> index 000000000000..fa390c9d911f
> --- /dev/null
> +++ b/drivers/iio/adc/nxp-sar-adc.c
> @@ -0,0 +1,1006 @@

> +
> +static void nxp_sar_adc_dma_cb(void *data)
> +{
> +	struct nxp_sar_adc *info = iio_priv(data);
> +	struct iio_dev *indio_dev = data;

Trivial but it would slightly more intuitive to do.
	struct iio_dev *indio_dev = data;
	struct nxp_sar_adc *info = iio_priv(indio_dev);

> +	struct dma_tx_state state;
> +	struct circ_buf *dma_buf;
> +	struct device *dev_dma;
> +	u32 *dma_samples;
> +	s64 timestamp;
> +	int idx, ret;
> +
> +	guard(spinlock_irqsave)(&info->lock);
> +
> +	dma_buf = &info->dma_buf;
> +	dma_samples = (u32 *)dma_buf->buf;
> +	dev_dma = info->dma_chan->device->dev;
> +
> +	dmaengine_tx_status(info->dma_chan, info->cookie, &state);
> +
> +	dma_sync_single_for_cpu(dev_dma, info->rx_dma_buf,
> +				NXP_SAR_ADC_DMA_BUFF_SZ, DMA_FROM_DEVICE);
> +
> +	/* Current head position. */
> +	dma_buf->head = (NXP_SAR_ADC_DMA_BUFF_SZ - state.residue) /
> +			NXP_SAR_ADC_DMA_SAMPLE_SZ;
> +
> +	/* If everything was transferred, avoid an off by one error. */
> +	if (!state.residue)
> +		dma_buf->head--;
> +
> +	/* Something went wrong and nothing transferred. */
> +	if (state.residue == NXP_SAR_ADC_DMA_BUFF_SZ)
> +		goto out;
> +
> +	/* Make sure that head is multiple of info->channels_used. */
> +	dma_buf->head -= dma_buf->head % info->channels_used;
> +
> +	/*
> +	 * dma_buf->tail != dma_buf->head condition will become false
> +	 * because dma_buf->tail will be incremented with 1.
> +	 */
> +	while (dma_buf->tail != dma_buf->head) {
> +		idx = dma_buf->tail % info->channels_used;
> +		info->buffer[idx] = dma_samples[dma_buf->tail];
> +		dma_buf->tail = (dma_buf->tail + 1) % NXP_SAR_ADC_DMA_SAMPLE_CNT;
> +		if (idx != info->channels_used - 1)
> +			continue;
> +
> +		/*
> +		 * iio_push_to_buffers_with_timestamp should not be

Comment needs an update as using with_ts()


> +		 * called with dma_samples as parameter. The samples
> +		 * will be smashed if timestamp is enabled.
> +		 */
> +		timestamp = iio_get_time_ns(indio_dev);
> +		ret = iio_push_to_buffers_with_ts(indio_dev, info->buffer,
> +						  sizeof(info->buffer),
> +						  timestamp);
> +		if (ret < 0 && ret != -EBUSY)
> +			dev_err_ratelimited(&indio_dev->dev,
> +					    "failed to push iio buffer: %d",
> +					    ret);
> +	}
> +
> +	dma_buf->tail = dma_buf->head;
> +out:
> +	dma_sync_single_for_device(dev_dma, info->rx_dma_buf,
> +				   NXP_SAR_ADC_DMA_BUFF_SZ, DMA_FROM_DEVICE);
> +}



> +static int nxp_sar_adc_dma_probe(struct device *dev, struct nxp_sar_adc *info)
> +{
> +	struct device *dev_dma;
> +	u8 *rx_buf;
> +
> +	info->dma_chan = devm_dma_request_chan(dev, "rx");
> +	if (IS_ERR(info->dma_chan))
> +		return PTR_ERR(info->dma_chan);
> +
> +	dev_dma = info->dma_chan->device->dev;
> +	rx_buf = dmam_alloc_coherent(dev_dma, NXP_SAR_ADC_DMA_BUFF_SZ,
> +				     &info->rx_dma_buf, GFP_KERNEL);

Is this setting up the right life time?  Superficially it looks to be
associating the buffer lifetime with a device related to the dma engine rather
than the device we are dealing with here.

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

If the info->dma_chan->device->dev is instantiated by devm_dma_request_chan()
and hence torn down as that is unwound it will be fine as this is simply
nested devm handling, but it seems a struct dma_device has many chans so
I think that isn't the case.

Given that device parameter is also needed for the buffer allocation and
making sure we have the right properties / iommu magic etc, I'm not sure
how to make this work. One option would be to use dma_alloc_coherent() and
tear down with a devm_add_action_or_reset() handler on dev rather than
dev_dma.

> +	if (!rx_buf)
> +		return -ENOMEM;
> +
> +	info->dma_buf.buf = rx_buf;
> +
> +	return 0;
> +}

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

[Daniel Lezcano]

Hi Andy,

On 10/18/25 22:12, Andy Shevchenko wrote:
> On Fri, Oct 17, 2025 at 06:42:38PM +0200, Daniel Lezcano wrote:
>> From: Stefan-Gabriel Mirea <stefan-gabriel.mirea@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?

I'm sorry but I don't get your comment.

Do you mean why not use kfifo.h or use kfifo API and change all the code 
using the circ_buf by the kfifo ?
> ...
> 
>> +#define NXP_SAR_ADC_IIO_BUFF_SZ		(NXP_SAR_ADC_NR_CHANNELS + (sizeof(u64) / sizeof(u16)))
> 
> Hmm... Don't we have some macros so we can avoid this kind of hard coding?

I don't find such a macro, do you have a pointer ?

> ...
> 
>> +	ndelay(div64_u64(NSEC_PER_SEC, clk_get_rate(info->clk)) * 80U);
> 
> Do you need those 'U':s? clk_get_rate() already returns unsigned value of the
> same or higher rank than int. No?

May be not needed, but harmless. I can remove them if you want

>> +static int nxp_sar_adc_start_conversion(struct nxp_sar_adc *info, bool raw)
>> +{
>> +	u32 mcr;
>> +
>> +	mcr = readl(NXP_SAR_ADC_MCR(info->regs));
>> +
>> +	FIELD_MODIFY(NXP_SAR_ADC_MCR_NSTART, &mcr, 0x1);
>> +	FIELD_MODIFY(NXP_SAR_ADC_MCR_MODE, &mcr, !raw);
> 
> !raw, which is boolean, as a parameter to FIELD_MODIFY() seems a bit odd to me,
> perhaps simple
> 
> 	raw ? 0 : 1
> 
> would work better?

Sure
> (Note, optimizer of the complier will avoid any branching)
> 
>> +	writel(mcr, NXP_SAR_ADC_MCR(info->regs));
>> +
>> +	return 0;
>> +}
> 
> ...
> 
>> +	dma_samples = (u32 *)dma_buf->buf;
> 
> Is it aligned properly for this type of casting?

TBH, I don't know the answer :/

How can I check that ?

>> +	dmaengine_tx_status(info->dma_chan, info->cookie, &state);
> 
> No return value check?

The return value is not necessary here because the caller of the 
callback will check with dma_submit_error() in case of error which 
covers the DMA_ERROR case and the other cases are not useful because the 
residue is taken into account right after.

It could be written differently with the DMA_COMPLETE check but the 
result will be the same. I prefer to keep the current implementation 
which has been tested.

>> +static const struct nxp_sar_adc_data s32g2_sar_adc_data = {
>> +	.vref_mV = 1800,
>> +	.model = "s32g2-sar-adc"
> 
> Keep a trailing comma as here it's not a termination member.
Ok, I'll add it

-- 
<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 v5 2/2] iio: adc: Add the NXP SAR ADC support for the s32g2/3 platforms

[Andy Shevchenko]

On Thu, Oct 30, 2025 at 09:27:21AM +0100, Daniel Lezcano wrote:
> On 10/18/25 22:12, Andy Shevchenko wrote:
> > On Fri, Oct 17, 2025 at 06:42:38PM +0200, Daniel Lezcano wrote:

...

> > > +#include <linux/circ_buf.h>
> > 
> > Why not kfifo?
> 
> I'm sorry but I don't get your comment.
> 
> Do you mean why not use kfifo.h or use kfifo API and change all the code
> using the circ_buf by the kfifo ?

Yes, I mean why use circ_buf API and not kfifo API.

...

> > > +#define NXP_SAR_ADC_IIO_BUFF_SZ		(NXP_SAR_ADC_NR_CHANNELS + (sizeof(u64) / sizeof(u16)))
> > 
> > Hmm... Don't we have some macros so we can avoid this kind of hard coding?
> 
> I don't find such a macro, do you have a pointer ?

If I got the use case correctly, I was thinking of IIO_DECLARE_BUFFER_WITH_TS().

...

> > > +	ndelay(div64_u64(NSEC_PER_SEC, clk_get_rate(info->clk)) * 80U);
> > 
> > Do you need those 'U':s? clk_get_rate() already returns unsigned value of the
> > same or higher rank than int. No?
> 
> May be not needed, but harmless. I can remove them if you want

I think using them in cases that have no side-effects are not needed.

...

> > > +	dma_samples = (u32 *)dma_buf->buf;
> > 
> > Is it aligned properly for this type of casting?
> 
> TBH, I don't know the answer :/
> 
> How can I check that ?

Is buf defined as a pointer to u32 / int or bigger? or is it just byte buffer?
If the latter, how does the address of it being formed? Does it come from a heap
(memory allocator)? If yes, we are fine, as this is usually the case for all
(k)malloc'ed memory.

...

> > > +	dmaengine_tx_status(info->dma_chan, info->cookie, &state);
> > 
> > No return value check?
> 
> The return value is not necessary here because the caller of the callback
> will check with dma_submit_error() in case of error which covers the
> DMA_ERROR case and the other cases are not useful because the residue is
> taken into account right after.

In some cases it might return DMA_PAUSE (and actually this is the correct way
to get residue, one needs to pause the channel to read it, otherwise it will
give outdated / incorrect information).

> It could be written differently with the DMA_COMPLETE check but the result
> will be the same. I prefer to keep the current implementation which has been
> tested.

-- 
With Best Regards,
Andy Shevchenko

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

[Daniel Lezcano]

On 10/30/25 10:28, Andy Shevchenko wrote:
> On Thu, Oct 30, 2025 at 09:27:21AM +0100, Daniel Lezcano wrote:
>> On 10/18/25 22:12, Andy Shevchenko wrote:
>>> On Fri, Oct 17, 2025 at 06:42:38PM +0200, Daniel Lezcano wrote:
> 
> ...
> 
>>>> +#include <linux/circ_buf.h>
>>>
>>> Why not kfifo?
>>
>> I'm sorry but I don't get your comment.
>>
>> Do you mean why not use kfifo.h or use kfifo API and change all the code
>> using the circ_buf by the kfifo ?
> 
> Yes, I mean why use circ_buf API and not kfifo API.

Ok, thanks for the clarification.

At the first glance, the resulting code with the kfifo API would look 
nicer. However, on one side, the current code was largely tested and it 
is running on a production system, on the other side the IIO framework 
will evolve with the cyclic DMA support and this driver would be 
converted to it when ready.

I can convert to kfifo but if it is changed again for the new IIO cyclic 
DMA, I'm not sure it is worth the effort.


[ ... ]


-- 
<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 v5 2/2] iio: adc: Add the NXP SAR ADC support for the s32g2/3 platforms

[Daniel Lezcano]

On 10/30/25 10:28, Andy Shevchenko wrote:
> On Thu, Oct 30, 2025 at 09:27:21AM +0100, Daniel Lezcano wrote:
>> On 10/18/25 22:12, Andy Shevchenko wrote:
>>> On Fri, Oct 17, 2025 at 06:42:38PM +0200, Daniel Lezcano wrote:

[ ... ]

>>>> +#define NXP_SAR_ADC_IIO_BUFF_SZ		(NXP_SAR_ADC_NR_CHANNELS + (sizeof(u64) / sizeof(u16)))
>>>
>>> Hmm... Don't we have some macros so we can avoid this kind of hard coding?
>>
>> I don't find such a macro, do you have a pointer ?
> 
> If I got the use case correctly, I was thinking of IIO_DECLARE_BUFFER_WITH_TS().

Oh right ! Actually, IIO_DECLARE_BUFFER_WITH_TS() is used but the macro 
above was not removed :)

[ ... ]

>>>> +	dma_samples = (u32 *)dma_buf->buf;
>>>
>>> Is it aligned properly for this type of casting?
>>
>> TBH, I don't know the answer :/
>>
>> How can I check that ?
> 
> Is buf defined as a pointer to u32 / int or bigger? or is it just byte buffer?
> If the latter, how does the address of it being formed? Does it come from a heap
> (memory allocator)? If yes, we are fine, as this is usually the case for all
> (k)malloc'ed memory.

buf is a byte buffer allocated with dmam_alloc_coherent(..., GFP_KERNEL)


> ...
> 
>>>> +	dmaengine_tx_status(info->dma_chan, info->cookie, &state);
>>>
>>> No return value check?
>>
>> The return value is not necessary here because the caller of the callback
>> will check with dma_submit_error() in case of error which covers the
>> DMA_ERROR case and the other cases are not useful because the residue is
>> taken into account right after.
> 
> In some cases it might return DMA_PAUSE (and actually this is the correct way
> to get residue, one needs to pause the channel to read it, otherwise it will
> give outdated / incorrect information).

But if the residue is checked in the callback routine without checking 
DMA_PAUSED, the result is the same no ?


-- 
<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 v5 2/2] iio: adc: Add the NXP SAR ADC support for the s32g2/3 platforms

[Andy Shevchenko]

On Fri, Oct 31, 2025 at 12:32:03PM +0100, Daniel Lezcano wrote:
> On 10/30/25 10:28, Andy Shevchenko wrote:
> > On Thu, Oct 30, 2025 at 09:27:21AM +0100, Daniel Lezcano wrote:
> > > On 10/18/25 22:12, Andy Shevchenko wrote:
> > > > On Fri, Oct 17, 2025 at 06:42:38PM +0200, Daniel Lezcano wrote:

[ ... ]

> > > > > +	dma_samples = (u32 *)dma_buf->buf;
> > > > 
> > > > Is it aligned properly for this type of casting?
> > > 
> > > TBH, I don't know the answer :/
> > > 
> > > How can I check that ?
> > 
> > Is buf defined as a pointer to u32 / int or bigger? or is it just byte buffer?
> > If the latter, how does the address of it being formed? Does it come from a heap
> > (memory allocator)? If yes, we are fine, as this is usually the case for all
> > (k)malloc'ed memory.
> 
> buf is a byte buffer allocated with dmam_alloc_coherent(..., GFP_KERNEL)

We are fine :-)

...

> > > > > +	dmaengine_tx_status(info->dma_chan, info->cookie, &state);
> > > > 
> > > > No return value check?
> > > 
> > > The return value is not necessary here because the caller of the callback
> > > will check with dma_submit_error() in case of error which covers the
> > > DMA_ERROR case and the other cases are not useful because the residue is
> > > taken into account right after.
> > 
> > In some cases it might return DMA_PAUSE (and actually this is the correct way
> > to get residue, one needs to pause the channel to read it, otherwise it will
> > give outdated / incorrect information).
> 
> But if the residue is checked in the callback routine without checking
> DMA_PAUSED, the result is the same no ?

DMA in some corner cases might have already be charged for the next transfer.
Do you have a synchronisation between DMA start and residue check?

I.o.w. this may work for your case, but in general it's not guaranteed. The proper
read of residue is to: pause DMA --> read residue --> resume DMA.

-- 
With Best Regards,
Andy Shevchenko

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

[Vinod Koul]

On 19-10-25, 09:42, Jonathan Cameron wrote:
> On Fri, 17 Oct 2025 18:42:38 +0200
> Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> 
> > From: Stefan-Gabriel Mirea <stefan-gabriel.mirea@nxp.com>
> > 
> > The NXP S32G2 and S32G3 platforms integrate a successive approximation
> > register (SAR) ADC. Two instances are available, each providing 8
> > multiplexed input channels with 12-bit resolution. The conversion rate
> > is up to 1 Msps depending on the configuration and sampling window.
> > 
> > The SAR ADC supports raw, buffer, and trigger modes. It can operate
> > in both single-shot and continuous conversion modes, with optional
> > hardware triggering through the cross-trigger unit (CTU) or external
> > events. An internal prescaler allows adjusting the sampling clock,
> > while per-channel programmable sampling times provide fine-grained
> > trade-offs between accuracy and latency. Automatic calibration is
> > performed at probe time to minimize offset and gain errors.
> > 
> > The driver is derived from the BSP implementation and has been partly
> > rewritten to comply with upstream requirements. For this reason, all
> > contributors are listed as co-developers, while the author refers to
> > the initial BSP driver file creator.
> > 
> > All modes have been validated on the S32G274-RDB2 platform using an
> > externally generated square wave captured by the ADC. Tests covered
> > buffered streaming via IIO, trigger synchronization, and accuracy
> > verification against a precision laboratory signal source.
> > 
> > Co-developed-by: Alexandru-Catalin Ionita <alexandru-catalin.ionita@nxp.com>
> > Signed-off-by: Alexandru-Catalin Ionita <alexandru-catalin.ionita@nxp.com>
> > Co-developed-by: Ciprian Costea <ciprianmarian.costea@nxp.com>
> > Signed-off-by: Ciprian Costea <ciprianmarian.costea@nxp.com>
> > Co-developed-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@oss.nxp.com>
> > Signed-off-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@oss.nxp.com>
> > Signed-off-by: Stefan-Gabriel Mirea <stefan-gabriel.mirea@nxp.com>
> > Co-developed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> 
> Hi Daniel,
> 
> Only significant question in here is around lifetimes of the
> dma buffer.
> 
> +CC Vinod and dmaengine list. Hopefully someone will rapidly tell me
> my concern is garbage ;)

Thanks for looping me in
> 
> IIO folk who are familiar with dmaengine channels etc please take
> a look at this as well.  I think all the upstream drivers we have doing
> similar things to this predate devm_ management being a common thing.
> 
> Jonathan
> 
> 
> > diff --git a/drivers/iio/adc/nxp-sar-adc.c b/drivers/iio/adc/nxp-sar-adc.c
> > new file mode 100644
> > index 000000000000..fa390c9d911f
> > --- /dev/null
> > +++ b/drivers/iio/adc/nxp-sar-adc.c
> > @@ -0,0 +1,1006 @@
> 
> > +
> > +static void nxp_sar_adc_dma_cb(void *data)
> > +{
> > +	struct nxp_sar_adc *info = iio_priv(data);
> > +	struct iio_dev *indio_dev = data;
> 
> Trivial but it would slightly more intuitive to do.
> 	struct iio_dev *indio_dev = data;
> 	struct nxp_sar_adc *info = iio_priv(indio_dev);
> 
> > +	struct dma_tx_state state;
> > +	struct circ_buf *dma_buf;
> > +	struct device *dev_dma;
> > +	u32 *dma_samples;
> > +	s64 timestamp;
> > +	int idx, ret;
> > +
> > +	guard(spinlock_irqsave)(&info->lock);
> > +
> > +	dma_buf = &info->dma_buf;
> > +	dma_samples = (u32 *)dma_buf->buf;
> > +	dev_dma = info->dma_chan->device->dev;
> > +
> > +	dmaengine_tx_status(info->dma_chan, info->cookie, &state);
> > +
> > +	dma_sync_single_for_cpu(dev_dma, info->rx_dma_buf,
> > +				NXP_SAR_ADC_DMA_BUFF_SZ, DMA_FROM_DEVICE);
> > +
> > +	/* Current head position. */
> > +	dma_buf->head = (NXP_SAR_ADC_DMA_BUFF_SZ - state.residue) /
> > +			NXP_SAR_ADC_DMA_SAMPLE_SZ;
> > +
> > +	/* If everything was transferred, avoid an off by one error. */
> > +	if (!state.residue)
> > +		dma_buf->head--;
> > +
> > +	/* Something went wrong and nothing transferred. */
> > +	if (state.residue == NXP_SAR_ADC_DMA_BUFF_SZ)
> > +		goto out;
> > +
> > +	/* Make sure that head is multiple of info->channels_used. */
> > +	dma_buf->head -= dma_buf->head % info->channels_used;
> > +
> > +	/*
> > +	 * dma_buf->tail != dma_buf->head condition will become false
> > +	 * because dma_buf->tail will be incremented with 1.
> > +	 */
> > +	while (dma_buf->tail != dma_buf->head) {
> > +		idx = dma_buf->tail % info->channels_used;
> > +		info->buffer[idx] = dma_samples[dma_buf->tail];
> > +		dma_buf->tail = (dma_buf->tail + 1) % NXP_SAR_ADC_DMA_SAMPLE_CNT;
> > +		if (idx != info->channels_used - 1)
> > +			continue;
> > +
> > +		/*
> > +		 * iio_push_to_buffers_with_timestamp should not be
> 
> Comment needs an update as using with_ts()
> 
> 
> > +		 * called with dma_samples as parameter. The samples
> > +		 * will be smashed if timestamp is enabled.
> > +		 */
> > +		timestamp = iio_get_time_ns(indio_dev);
> > +		ret = iio_push_to_buffers_with_ts(indio_dev, info->buffer,
> > +						  sizeof(info->buffer),
> > +						  timestamp);
> > +		if (ret < 0 && ret != -EBUSY)
> > +			dev_err_ratelimited(&indio_dev->dev,
> > +					    "failed to push iio buffer: %d",
> > +					    ret);
> > +	}
> > +
> > +	dma_buf->tail = dma_buf->head;
> > +out:
> > +	dma_sync_single_for_device(dev_dma, info->rx_dma_buf,
> > +				   NXP_SAR_ADC_DMA_BUFF_SZ, DMA_FROM_DEVICE);
> > +}
> 
> 
> 
> > +static int nxp_sar_adc_dma_probe(struct device *dev, struct nxp_sar_adc *info)
> > +{
> > +	struct device *dev_dma;
> > +	u8 *rx_buf;
> > +
> > +	info->dma_chan = devm_dma_request_chan(dev, "rx");
> > +	if (IS_ERR(info->dma_chan))
> > +		return PTR_ERR(info->dma_chan);
> > +
> > +	dev_dma = info->dma_chan->device->dev;
> > +	rx_buf = dmam_alloc_coherent(dev_dma, NXP_SAR_ADC_DMA_BUFF_SZ,
> > +				     &info->rx_dma_buf, GFP_KERNEL);
> 
> Is this setting up the right life time?  Superficially it looks to be
> associating the buffer lifetime with a device related to the dma engine rather
> than the device we are dealing with here.

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

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

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

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

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

-- 
~Vinod

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

[Daniel Lezcano]

On 10/31/25 13:45, Andy Shevchenko wrote:
> On Fri, Oct 31, 2025 at 12:32:03PM +0100, Daniel Lezcano wrote:
>> On 10/30/25 10:28, Andy Shevchenko wrote:
>>> On Thu, Oct 30, 2025 at 09:27:21AM +0100, Daniel Lezcano wrote:
>>>> On 10/18/25 22:12, Andy Shevchenko wrote:
>>>>> On Fri, Oct 17, 2025 at 06:42:38PM +0200, Daniel Lezcano wrote:
> 
> [ ... ]
> 
>>>>>> +	dma_samples = (u32 *)dma_buf->buf;
>>>>>
>>>>> Is it aligned properly for this type of casting?
>>>>
>>>> TBH, I don't know the answer :/
>>>>
>>>> How can I check that ?
>>>
>>> Is buf defined as a pointer to u32 / int or bigger? or is it just byte buffer?
>>> If the latter, how does the address of it being formed? Does it come from a heap
>>> (memory allocator)? If yes, we are fine, as this is usually the case for all
>>> (k)malloc'ed memory.
>>
>> buf is a byte buffer allocated with dmam_alloc_coherent(..., GFP_KERNEL)
> 
> We are fine :-)
> 
> ...
> 
>>>>>> +	dmaengine_tx_status(info->dma_chan, info->cookie, &state);
>>>>>
>>>>> No return value check?
>>>>
>>>> The return value is not necessary here because the caller of the callback
>>>> will check with dma_submit_error() in case of error which covers the
>>>> DMA_ERROR case and the other cases are not useful because the residue is
>>>> taken into account right after.
>>>
>>> In some cases it might return DMA_PAUSE (and actually this is the correct way
>>> to get residue, one needs to pause the channel to read it, otherwise it will
>>> give outdated / incorrect information).
>>
>> But if the residue is checked in the callback routine without checking
>> DMA_PAUSED, the result is the same no ?
> 
> DMA in some corner cases might have already be charged for the next transfer.
> Do you have a synchronisation between DMA start and residue check?
> 
> I.o.w. this may work for your case, but in general it's not guaranteed. The proper
> read of residue is to: pause DMA --> read residue --> resume DMA.
> 

I'll use the new callback function dma_async_tx_callback_result() which 
should prevent that and allows to remove the spinlock at the same time


-- 
<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 v5 2/2] iio: adc: Add the NXP SAR ADC support for the s32g2/3 platforms

[Jonathan Cameron]

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

> On 19-10-25, 09:42, Jonathan Cameron wrote:
> > On Fri, 17 Oct 2025 18:42:38 +0200
> > Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> >   
> > > From: Stefan-Gabriel Mirea <stefan-gabriel.mirea@nxp.com>
> > > 
> > > The NXP S32G2 and S32G3 platforms integrate a successive approximation
> > > register (SAR) ADC. Two instances are available, each providing 8
> > > multiplexed input channels with 12-bit resolution. The conversion rate
> > > is up to 1 Msps depending on the configuration and sampling window.
> > > 
> > > The SAR ADC supports raw, buffer, and trigger modes. It can operate
> > > in both single-shot and continuous conversion modes, with optional
> > > hardware triggering through the cross-trigger unit (CTU) or external
> > > events. An internal prescaler allows adjusting the sampling clock,
> > > while per-channel programmable sampling times provide fine-grained
> > > trade-offs between accuracy and latency. Automatic calibration is
> > > performed at probe time to minimize offset and gain errors.
> > > 
> > > The driver is derived from the BSP implementation and has been partly
> > > rewritten to comply with upstream requirements. For this reason, all
> > > contributors are listed as co-developers, while the author refers to
> > > the initial BSP driver file creator.
> > > 
> > > All modes have been validated on the S32G274-RDB2 platform using an
> > > externally generated square wave captured by the ADC. Tests covered
> > > buffered streaming via IIO, trigger synchronization, and accuracy
> > > verification against a precision laboratory signal source.
> > > 
> > > Co-developed-by: Alexandru-Catalin Ionita <alexandru-catalin.ionita@nxp.com>
> > > Signed-off-by: Alexandru-Catalin Ionita <alexandru-catalin.ionita@nxp.com>
> > > Co-developed-by: Ciprian Costea <ciprianmarian.costea@nxp.com>
> > > Signed-off-by: Ciprian Costea <ciprianmarian.costea@nxp.com>
> > > Co-developed-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@oss.nxp.com>
> > > Signed-off-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@oss.nxp.com>
> > > Signed-off-by: Stefan-Gabriel Mirea <stefan-gabriel.mirea@nxp.com>
> > > Co-developed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>  
> > 
> > Hi Daniel,
> > 
> > Only significant question in here is around lifetimes of the
> > dma buffer.
> > 
> > +CC Vinod and dmaengine list. Hopefully someone will rapidly tell me
> > my concern is garbage ;)  
> 
> Thanks for looping me in
> > 
> > IIO folk who are familiar with dmaengine channels etc please take
> > a look at this as well.  I think all the upstream drivers we have doing
> > similar things to this predate devm_ management being a common thing.
> > 
> > Jonathan
> > 
> >   
> > > diff --git a/drivers/iio/adc/nxp-sar-adc.c b/drivers/iio/adc/nxp-sar-adc.c
> > > new file mode 100644
> > > index 000000000000..fa390c9d911f
> > > --- /dev/null
> > > +++ b/drivers/iio/adc/nxp-sar-adc.c
> > > @@ -0,0 +1,1006 @@  
> >   
> > > +
> > > +static void nxp_sar_adc_dma_cb(void *data)
> > > +{
> > > +	struct nxp_sar_adc *info = iio_priv(data);
> > > +	struct iio_dev *indio_dev = data;  
> > 
> > Trivial but it would slightly more intuitive to do.
> > 	struct iio_dev *indio_dev = data;
> > 	struct nxp_sar_adc *info = iio_priv(indio_dev);
> >   
> > > +	struct dma_tx_state state;
> > > +	struct circ_buf *dma_buf;
> > > +	struct device *dev_dma;
> > > +	u32 *dma_samples;
> > > +	s64 timestamp;
> > > +	int idx, ret;
> > > +
> > > +	guard(spinlock_irqsave)(&info->lock);
> > > +
> > > +	dma_buf = &info->dma_buf;
> > > +	dma_samples = (u32 *)dma_buf->buf;
> > > +	dev_dma = info->dma_chan->device->dev;
> > > +
> > > +	dmaengine_tx_status(info->dma_chan, info->cookie, &state);
> > > +
> > > +	dma_sync_single_for_cpu(dev_dma, info->rx_dma_buf,
> > > +				NXP_SAR_ADC_DMA_BUFF_SZ, DMA_FROM_DEVICE);
> > > +
> > > +	/* Current head position. */
> > > +	dma_buf->head = (NXP_SAR_ADC_DMA_BUFF_SZ - state.residue) /
> > > +			NXP_SAR_ADC_DMA_SAMPLE_SZ;
> > > +
> > > +	/* If everything was transferred, avoid an off by one error. */
> > > +	if (!state.residue)
> > > +		dma_buf->head--;
> > > +
> > > +	/* Something went wrong and nothing transferred. */
> > > +	if (state.residue == NXP_SAR_ADC_DMA_BUFF_SZ)
> > > +		goto out;
> > > +
> > > +	/* Make sure that head is multiple of info->channels_used. */
> > > +	dma_buf->head -= dma_buf->head % info->channels_used;
> > > +
> > > +	/*
> > > +	 * dma_buf->tail != dma_buf->head condition will become false
> > > +	 * because dma_buf->tail will be incremented with 1.
> > > +	 */
> > > +	while (dma_buf->tail != dma_buf->head) {
> > > +		idx = dma_buf->tail % info->channels_used;
> > > +		info->buffer[idx] = dma_samples[dma_buf->tail];
> > > +		dma_buf->tail = (dma_buf->tail + 1) % NXP_SAR_ADC_DMA_SAMPLE_CNT;
> > > +		if (idx != info->channels_used - 1)
> > > +			continue;
> > > +
> > > +		/*
> > > +		 * iio_push_to_buffers_with_timestamp should not be  
> > 
> > Comment needs an update as using with_ts()
> > 
> >   
> > > +		 * called with dma_samples as parameter. The samples
> > > +		 * will be smashed if timestamp is enabled.
> > > +		 */
> > > +		timestamp = iio_get_time_ns(indio_dev);
> > > +		ret = iio_push_to_buffers_with_ts(indio_dev, info->buffer,
> > > +						  sizeof(info->buffer),
> > > +						  timestamp);
> > > +		if (ret < 0 && ret != -EBUSY)
> > > +			dev_err_ratelimited(&indio_dev->dev,
> > > +					    "failed to push iio buffer: %d",
> > > +					    ret);
> > > +	}
> > > +
> > > +	dma_buf->tail = dma_buf->head;
> > > +out:
> > > +	dma_sync_single_for_device(dev_dma, info->rx_dma_buf,
> > > +				   NXP_SAR_ADC_DMA_BUFF_SZ, DMA_FROM_DEVICE);
> > > +}  
> > 
> > 
> >   
> > > +static int nxp_sar_adc_dma_probe(struct device *dev, struct nxp_sar_adc *info)
> > > +{
> > > +	struct device *dev_dma;
> > > +	u8 *rx_buf;
> > > +
> > > +	info->dma_chan = devm_dma_request_chan(dev, "rx");
> > > +	if (IS_ERR(info->dma_chan))
> > > +		return PTR_ERR(info->dma_chan);
> > > +
> > > +	dev_dma = info->dma_chan->device->dev;
> > > +	rx_buf = dmam_alloc_coherent(dev_dma, NXP_SAR_ADC_DMA_BUFF_SZ,
> > > +				     &info->rx_dma_buf, GFP_KERNEL);  
> > 
> > Is this setting up the right life time?  Superficially it looks to be
> > associating the buffer lifetime with a device related to the dma engine rather
> > than the device we are dealing with here.  
> 
> Absolutely! the buffer ought to be mapped always with dmaengine device.
> The transaction are performed by the dmaengine device and that is why
> mapping should always be done with dmaengine device, it will not work
> otherwise.

This to me is confusing two things. The mapping establishment (in the dma
engine driver) and what driver looks after the mapping/triggers cleaning it up
(here the ADC driver). This dmam_alloc_coherent() seems to be allocating stuff
in the probe of this driver, but unless more complex management is going on
than I think (always possible!) the remove won't happen until he dmaengine
driver is unbound - so very much out of order with the tear down in this driver.

That is different from what we'd have if we just had
dma_alloc_coherent(dev_dma, ...) in probe
and dma_free_coherent(dev_dma ...)  in remove() for this driver.
 
There are other kernel interfaces that provide a management related struct device
and a struct device needed to perform the operation. Seems like the same should
be true here as they don't seem to be the same struct device.

Jonathan

> 
> > This particular pattern with devm_dma_request_chan() is vanishingly rare
> > so not much prior art to rely on.  
> 
> I personally do not like that, dmaengine channel is a shared resource
> and it should be grabbed at runtime and freed up after use. Ideally
> would be good if we grab the channel when we need i
> 
> But I know people like to grab channels at startup :-(
> 
> > If the info->dma_chan->device->dev is instantiated by devm_dma_request_chan()
> > and hence torn down as that is unwound it will be fine as this is simply
> > nested devm handling, but it seems a struct dma_device has many chans so
> > I think that isn't the case.
> > 
> > Given that device parameter is also needed for the buffer allocation and
> > making sure we have the right properties / iommu magic etc, I'm not sure
> > how to make this work. One option would be to use dma_alloc_coherent() and
> > tear down with a devm_add_action_or_reset() handler on dev rather than
> > dev_dma.
> >   
> > > +	if (!rx_buf)
> > > +		return -ENOMEM;
> > > +
> > > +	info->dma_buf.buf = rx_buf;
> > > +
> > > +	return 0;
> > > +}  
> 

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

[Daniel Lezcano]

Hi Andy,

On 10/31/25 13:45, Andy Shevchenko wrote:
> On Fri, Oct 31, 2025 at 12:32:03PM +0100, Daniel Lezcano wrote:
>> On 10/30/25 10:28, Andy Shevchenko wrote:
>>> On Thu, Oct 30, 2025 at 09:27:21AM +0100, Daniel Lezcano wrote:
>>>> On 10/18/25 22:12, Andy Shevchenko wrote:
>>>>> On Fri, Oct 17, 2025 at 06:42:38PM +0200, Daniel Lezcano wrote:
> 
> [ ... ]
> 
>>>>>> +	dma_samples = (u32 *)dma_buf->buf;
>>>>>
>>>>> Is it aligned properly for this type of casting?
>>>>
>>>> TBH, I don't know the answer :/
>>>>
>>>> How can I check that ?
>>>
>>> Is buf defined as a pointer to u32 / int or bigger? or is it just byte buffer?
>>> If the latter, how does the address of it being formed? Does it come from a heap
>>> (memory allocator)? If yes, we are fine, as this is usually the case for all
>>> (k)malloc'ed memory.
>>
>> buf is a byte buffer allocated with dmam_alloc_coherent(..., GFP_KERNEL)
> 
> We are fine :-)
> 
> ...
> 
>>>>>> +	dmaengine_tx_status(info->dma_chan, info->cookie, &state);
>>>>>
>>>>> No return value check?
>>>>
>>>> The return value is not necessary here because the caller of the callback
>>>> will check with dma_submit_error() in case of error which covers the
>>>> DMA_ERROR case and the other cases are not useful because the residue is
>>>> taken into account right after.
>>>
>>> In some cases it might return DMA_PAUSE (and actually this is the correct way
>>> to get residue, one needs to pause the channel to read it, otherwise it will
>>> give outdated / incorrect information).
>>
>> But if the residue is checked in the callback routine without checking
>> DMA_PAUSED, the result is the same no ?
> 
> DMA in some corner cases might have already be charged for the next transfer.
> Do you have a synchronisation between DMA start and residue check?
> 
> I.o.w. this may work for your case, but in general it's not guaranteed. The proper
> read of residue is to: pause DMA --> read residue --> resume DMA.

I discussed with Vinod about this change and he suggested to use the 
callback_result() to get the residue as a parameter so the 
dmaengine_txstatus() call won't be needed anymore.

Unfortunately, it does not work. I had a look in the DMA driver and the 
internals but my knowledge is limited in this area so I was unable to 
find out what is going on. Moreover there are no so many driver using 
this API I can use as an example. The best I was able to do was 
propagating the residue to the result in the vchan_complete() but it 
does not work.

Then I stepped back by not using the callback_result() and used 
dmaengine_pause(), read the residue, dmaengine_resume() but there are no 
result after these calls. I don't know why.

The issue you are mentioning above should be handled in other drivers 
doing the same kind of acquisition but the routine is similar to the one 
proposed here (eg. stm32).

The NXP SAR acquisition routine is running since several years in 
production AFAICT.

I investigated the different solutions without success, while I can run 
the acquisition routine without problem here with my hardware. A signal 
generator captured by the ADC, plotted and compared with the 
oscilloscope display.

The circ buffer is working well here and no bug was spotted with the 
current routine. I think I did my best to make the driver better from 
its initial submission. The best is the enemy of the good, and I would 
like to make some progress here in the driver acceptance. Changing the 
entire driver for the sake of replacing the circ_buffer by the kfifo and 
change the code for a scenario which is not happening is not really 
worth. Especially that the DMA engine is being modified to take into the 
cyclic DMA in its API, thus the circ_buffer and the routine will go away 
once the driver is changed to take into account this new API.

IOW, can we keep this routine as it is for now as it works fine and go 
forward for a v6 ?





-- 
<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 v5 2/2] iio: adc: Add the NXP SAR ADC support for the s32g2/3 platforms

[Andy Shevchenko]

On Fri, Nov 07, 2025 at 12:36:14PM +0100, Daniel Lezcano wrote:
> On 10/31/25 13:45, Andy Shevchenko wrote:
> > On Fri, Oct 31, 2025 at 12:32:03PM +0100, Daniel Lezcano wrote:
> > > On 10/30/25 10:28, Andy Shevchenko wrote:
> > > > On Thu, Oct 30, 2025 at 09:27:21AM +0100, Daniel Lezcano wrote:
> > > > > On 10/18/25 22:12, Andy Shevchenko wrote:
> > > > > > On Fri, Oct 17, 2025 at 06:42:38PM +0200, Daniel Lezcano wrote:

[ ... ]

> > > > > > > +	dmaengine_tx_status(info->dma_chan, info->cookie, &state);
> > > > > > 
> > > > > > No return value check?
> > > > > 
> > > > > The return value is not necessary here because the caller of the callback
> > > > > will check with dma_submit_error() in case of error which covers the
> > > > > DMA_ERROR case and the other cases are not useful because the residue is
> > > > > taken into account right after.
> > > > 
> > > > In some cases it might return DMA_PAUSE (and actually this is the correct way
> > > > to get residue, one needs to pause the channel to read it, otherwise it will
> > > > give outdated / incorrect information).
> > > 
> > > But if the residue is checked in the callback routine without checking
> > > DMA_PAUSED, the result is the same no ?
> > 
> > DMA in some corner cases might have already be charged for the next transfer.
> > Do you have a synchronisation between DMA start and residue check?
> > 
> > I.o.w. this may work for your case, but in general it's not guaranteed. The proper
> > read of residue is to: pause DMA --> read residue --> resume DMA.
> 
> I'll use the new callback function dma_async_tx_callback_result() which
> should prevent that and allows to remove the spinlock at the same time

So, AFAIU this doesn't work for you.


-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Nov 18, 2025 at 02:57:41PM +0100, Daniel Lezcano wrote:
> On 10/31/25 13:45, Andy Shevchenko wrote:
> > On Fri, Oct 31, 2025 at 12:32:03PM +0100, Daniel Lezcano wrote:
> > > On 10/30/25 10:28, Andy Shevchenko wrote:
> > > > On Thu, Oct 30, 2025 at 09:27:21AM +0100, Daniel Lezcano wrote:
> > > > > On 10/18/25 22:12, Andy Shevchenko wrote:
> > > > > > On Fri, Oct 17, 2025 at 06:42:38PM +0200, Daniel Lezcano wrote:

[ ... ]

> > > > > > > +	dmaengine_tx_status(info->dma_chan, info->cookie, &state);
> > > > > > 
> > > > > > No return value check?
> > > > > 
> > > > > The return value is not necessary here because the caller of the callback
> > > > > will check with dma_submit_error() in case of error which covers the
> > > > > DMA_ERROR case and the other cases are not useful because the residue is
> > > > > taken into account right after.
> > > > 
> > > > In some cases it might return DMA_PAUSE (and actually this is the correct way
> > > > to get residue, one needs to pause the channel to read it, otherwise it will
> > > > give outdated / incorrect information).
> > > 
> > > But if the residue is checked in the callback routine without checking
> > > DMA_PAUSED, the result is the same no ?
> > 
> > DMA in some corner cases might have already be charged for the next transfer.
> > Do you have a synchronisation between DMA start and residue check?
> > 
> > I.o.w. this may work for your case, but in general it's not guaranteed. The proper
> > read of residue is to: pause DMA --> read residue --> resume DMA.
> 
> I discussed with Vinod about this change and he suggested to use the
> callback_result() to get the residue as a parameter so the
> dmaengine_txstatus() call won't be needed anymore.
> 
> Unfortunately, it does not work. I had a look in the DMA driver and the
> internals but my knowledge is limited in this area so I was unable to find
> out what is going on. Moreover there are no so many driver using this API I
> can use as an example. The best I was able to do was propagating the residue
> to the result in the vchan_complete() but it does not work.
> 
> Then I stepped back by not using the callback_result() and used
> dmaengine_pause(), read the residue, dmaengine_resume() but there are no
> result after these calls. I don't know why.
> 
> The issue you are mentioning above should be handled in other drivers doing
> the same kind of acquisition but the routine is similar to the one proposed
> here (eg. stm32).
> 
> The NXP SAR acquisition routine is running since several years in production
> AFAICT.
> 
> I investigated the different solutions without success, while I can run the
> acquisition routine without problem here with my hardware. A signal
> generator captured by the ADC, plotted and compared with the oscilloscope
> display.
> 
> The circ buffer is working well here and no bug was spotted with the current
> routine. I think I did my best to make the driver better from its initial
> submission. The best is the enemy of the good, and I would like to make some
> progress here in the driver acceptance. Changing the entire driver for the
> sake of replacing the circ_buffer by the kfifo and change the code for a
> scenario which is not happening is not really worth. Especially that the DMA
> engine is being modified to take into the cyclic DMA in its API, thus the
> circ_buffer and the routine will go away once the driver is changed to take
> into account this new API.
> 
> IOW, can we keep this routine as it is for now as it works fine and go
> forward for a v6 ?

Thanks for trying and getting to this. Please, make a summary of the research
and pack it into a paragraph in the commit message, so we will have this in
the history. Maybe even a short comment in the code.

-- 
With Best Regards,
Andy Shevchenko