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

[PATCH v2 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:
	* 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                       |   13 +
 drivers/iio/adc/Makefile                      |    1 +
 drivers/iio/adc/nxp-sar-adc.c                 | 1026 +++++++++++++++++
 4 files changed, 1103 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 v2 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 v2 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 | 1026 +++++++++++++++++++++++++++++++++
 3 files changed, 1040 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..421ce42ad9ec
--- /dev/null
+++ b/drivers/iio/adc/nxp-sar-adc.c
@@ -0,0 +1,1026 @@
+// 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/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/interrupt.h>
+#include <linux/iopoll.h>
+#include <linux/math64.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/spinlock.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>
+
+/* 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) + 0x00)
+
+#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_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	(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;
+	struct clk *clk;
+
+	u16 value;
+	u32 vref_mV;
+	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_mV;
+};
+
+#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 = readl_poll_timeout(REG_ADC_MSR(base), msr, !(msr & REG_ADC_MSR_CALBUSY),
+				 NXP_SAR_ADC_WAIT_US,
+				 NXP_SAR_ADC_CAL_TIMEOUT_US);
+	if (ret)
+		return ret;
+
+	if (msr & REG_ADC_MSR_CALFAIL) {
+		/*
+		 * If the calibration fails, the status register bit
+		 * must be cleared
+		 */
+		msr &= ~REG_ADC_MSR_CALFAIL;
+		writel(msr, REG_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, 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);
+	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_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, BIT(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, BIT(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_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:
+		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);
+		return 0;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+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;
+	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 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);
+}
+
+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;
+}
+
+/*
+ * 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)
+{
+	writel(0x00003901, REG_ADC_MCR(info->regs));
+	writel(0x00000001, REG_ADC_MSR(info->regs));
+	writel(0x00000014, REG_ADC_CTR0(info->regs));
+	writel(0x00000014, REG_ADC_CTR1(info->regs));
+	writel(0x00000000, REG_ADC_CIMR0(info->regs));
+	writel(0x00000000, REG_ADC_CIMR1(info->regs));
+	writel(0x00000000, REG_ADC_NCMR0(info->regs));
+	writel(0x00000000, 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 = device_get_match_data(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");
+
+	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);
+
+	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)
+		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 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);
+}
+
+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 };
+
+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,
+	.remove         = nxp_sar_adc_remove,
+	.driver         = {
+		.name   = DRIVER_NAME,
+		.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 v2 2/2] iio: adc: Add the NXP SAR ADC support for the s32g2/3 platforms

[Jonathan Cameron]

On Wed, 10 Sep 2025 17:57:56 +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,

Various feedback inline.  I got a bit side tracked in the v1
timescale by the dma buffer discussion so sorry this is only on v2.


Jonathan

> ---
>  drivers/iio/adc/Kconfig       |   13 +
>  drivers/iio/adc/Makefile      |    1 +
>  drivers/iio/adc/nxp-sar-adc.c | 1026 +++++++++++++++++++++++++++++++++
>  3 files changed, 1040 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

Why?  Should work fine with the hrtimer trigger if someone preferred that
or as a client of many device specific triggers.

Shouldn't be any need to select a specific 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.
> +

> diff --git a/drivers/iio/adc/nxp-sar-adc.c b/drivers/iio/adc/nxp-sar-adc.c
> new file mode 100644
> index 000000000000..421ce42ad9ec
> --- /dev/null
> +++ b/drivers/iio/adc/nxp-sar-adc.c
> @@ -0,0 +1,1026 @@
> +// 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/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/interrupt.h>
> +#include <linux/iopoll.h>
> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/of.h>

Why of.h?  Probably want mod_devicetable.h instead.
Most of the time a modern driver shouldn't need to use
of specific interfaces. Instead use property.h or subsystem
specific firmware queries.

> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.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>
> +
> +/* 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))
Avoid generic names for defines by prefixing with something appropriate
eg NXP_SADC_CDR_REG(__channel)

> +
> +#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) + 0x00)

I'm not really convinced these macros help over just having
readl(info->regs + NXP_SADC_MCR_REG);


> +
> +#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_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	(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)))

IIO_DECLARE_BUFFER_WITH_TS() should handle this and is less reliant on
everything being round numbers than this is.

> +
> +#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;
> +	struct clk *clk;
> +
> +	u16 value;
> +	u32 vref_mV;
> +	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];

Perhaps documentation for these two as their relationship isn't entirely obvious from
the names.

> +
> +	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
Single line comment format preferred.
> +	 */
> +	u32 inpsamp;
> +	u32 pwdn;
> +};

> +
> +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),

Whilst we only insist on monotonic numbering, putting it all the way down
at 32 seems excessive. Why not 8?  Perhaps a comment if this is to avoid
moving it for some future feature.

> +};
> +
> +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)
Similar to comments below.  I'd avoid the __ prefix for helper functions.
Often a bit of adjusting of the name is better.
nxp_sar_adc_set_enabled() perhaps here?

> +{
> +	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 int nxp_sar_adc_calibration(struct nxp_sar_adc *info)
> +{
> +	int ret;
> +
> +	/*
> +	 * Calibration works only if the adc is powered up
> +	 */

Where they fit under 80 chars, stick to single line comments.

> +	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 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, BIT(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, BIT(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;

I'm not a bit fan of gotos within case statements as I find them hard
to read.  Please factor out the bit that does the work here so that
we can do direct returns in a new function and just call
iio_device_release_direct() before checking that return value.

> +		}
> +
> +		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_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:
> +		break;

return -EINVAL here. and avoid breaks.


> +	}
> +
> +	return -EINVAL;
> +}

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

iio_priv(data)
is fine. void * casts don't need to be explicit (see the C spec for
more on that).

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

	/*
 	 * dma_buf->tail ...

for consistency with IIO driver comments in general.


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

Same on multiline comment syntax.

> +		 * with dma_samples as parameter. The samples will be smashed
> +		 * if timestamp is enabled.

So, whilst not entirely there for this reason we do have:
iio_push_to_buffers_with_ts_unaligned()  which bounces the data on the
way to the buffer to avoid fiddly issues.  Perhaps doesn't have the most
helpful of names but the docs do refer to it not needing the extra space.

> +		 */
> +		timestamp = iio_get_time_ns(indio_dev);
> +		ret = iio_push_to_buffers_with_timestamp(indio_dev,

If possible use with_ts() variant.  It does some size santity checking.



> +							 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 void __nxp_sar_adc_buffer_trigger_predisable(struct iio_dev *indio_dev)
Not sure the __ makes much sense here. Normally that indicates weird locking
rules or similar.  Maybe nxp_sar_adc_buffer_trigger_do_predisable() or something like that?
> +{
> +	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);
There is an ordering difference here from what happens in the postenable
callback.  I'd normally expect to see this as an exact mirror.
If that is required, then please add some comments on why.  otherwise
move this stop_conversion after the predisable() calls.
> +
> +	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;

Hmm. Fun corner.  If this hardware fails to respond to a trigger
interrupt (which may be a software faked thing anyway) that doesn't
mean there was no interrupt.  So I'd print a debug message and return
IRQ_HANDLED even on error.

> +
> +	return IRQ_HANDLED;
> +}

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

Look again at what devm_add_action_or_reset() does on failure.
This has already been called. So 
	return devm_add_action_or_reset(dev, nxp_sar_adc_dma_remove, info);
should be fine.

> +
> +	return ret;
> +}
> +
> +/*
> + * The documentation describes the reset values for the
wrap consistently to 80 chars. This first line has ended up short enough
registers. fits.
> + * 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)
> +{
> +	writel(0x00003901, REG_ADC_MCR(info->regs));
> +	writel(0x00000001, REG_ADC_MSR(info->regs));
> +	writel(0x00000014, REG_ADC_CTR0(info->regs));
> +	writel(0x00000014, REG_ADC_CTR1(info->regs));
> +	writel(0x00000000, REG_ADC_CIMR0(info->regs));
> +	writel(0x00000000, REG_ADC_CIMR1(info->regs));
> +	writel(0x00000000, REG_ADC_NCMR0(info->regs));
> +	writel(0x00000000, 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));

sizeof(*info) preferred given you then get it back via iio_priv() so it
is easier if a reader doesn't need to check the types match.

> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	info = iio_priv(indio_dev);
> +
> +	data = device_get_match_data(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");
> +
> +	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);
> +
> +	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);

This should be the 'part number'.  That is a little ill defined
for a SoC integrated ADC, but generally not what we get from dev_name()
on the platform_device.

> +	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;
		return dev_err_probe(dev, ret, "Calibration failed\n");

Whilst we don't get the deferred probe debugging in this case the dev_err_probe()
does some pretty printing and saves a few lines of code, so it is fine to
use it everywhere in probe() or code just called from probe()

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

> +
> +static struct platform_driver nxp_sar_adc_driver = {
> +	.probe          = nxp_sar_adc_probe,
Completely trivial feedback.

I'd not do this aligning as to my eyes having the nested = under
the other ones is actively misleading.  A single space the = is
to me easier to read and less likely to lead to churn in the long
run.  I don't mind careful alignment for closely related values
like lists of register addresses but don't see a strong advantage
to doing it here.


> +	.remove         = nxp_sar_adc_remove,
> +	.driver         = {
> +		.name   = DRIVER_NAME,

This pattern is one of those things that is completely trivial but
makes no sense to me. I'd rather see the string directly here.

> +		.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");

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

[Daniel Lezcano]

Hi Jonathan,

thanks for the review

On 10/09/2025 19:32, Jonathan Cameron wrote:
> On Wed, 10 Sep 2025 17:57:56 +0200
> Daniel Lezcano <daniel.lezcano@linaro.org> wrote:

[ ... ]

>> +/* Main Configuration Register */
>> +#define REG_ADC_MCR(__base)		((__base) + 0x00)
> 
> I'm not really convinced these macros help over just having
> readl(info->regs + NXP_SADC_MCR_REG);

That is really a matter of taste :)

I used to create this format in order to stick the macros with the 
debugfs register code which is not part of these changes. There is a 
similar format in drivers/clocksource/timer-nxp-stm.c or 
driver/thermal/mediatek/lvts.c IMHO is less prone to error than base + 
REG all around the code.

Do you want me to convert all the macros to info->__base + MACRO ?

[ ... ]

>> +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),
> 
> Whilst we only insist on monotonic numbering, putting it all the way down
> at 32 seems excessive. Why not 8?  Perhaps a comment if this is to avoid
> moving it for some future feature.

The ADC has 8 channels for external acquisition however others channels 
8->31 are described as reserved. They may evolve in the future to more 
channels. That is probably the reason why 32 is used here.

[ ... ]

>> +	indio_dev->name = dev_name(dev);
> 
> This should be the 'part number'.  That is a little ill defined
> for a SoC integrated ADC, but generally not what we get from dev_name()
> on the platform_device.

Sorry, I don't get the comment. If I refer to the different drivers 
there is not consistency with the iio_dev->name.

rtq6056.c:      indio_dev->name = "rtq6056";
rzg2l_adc.c:    indio_dev->name = DRIVER_NAME;
sc27xx_adc.c:   indio_dev->name = dev_name(dev);
mt6359-auxadc.c:  indio_dev->name = adc_dev->chip_info->model_name;
mcp3911.c:      indio_dev->name = spi_get_device_id(spi)->name;

Are you suggesting to use the compatible part number ?

	indio->name = "s32g2-sar-adc";




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

[David Lechner]

On 9/11/25 7:55 AM, Daniel Lezcano wrote:
> 
> Hi Jonathan,
> 
> thanks for the review
> 
> On 10/09/2025 19:32, Jonathan Cameron wrote:
>> On Wed, 10 Sep 2025 17:57:56 +0200
>> Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> 
> [ ... ]
> 

...

> 
>>> +    indio_dev->name = dev_name(dev);
>>
>> This should be the 'part number'.  That is a little ill defined
>> for a SoC integrated ADC, but generally not what we get from dev_name()
>> on the platform_device.
> 
> Sorry, I don't get the comment. If I refer to the different drivers there is not consistency with the iio_dev->name.

dev_name() will be something like adc@12345678 from the devicetree,
so not the "part number".

> 
> rtq6056.c:      indio_dev->name = "rtq6056";

This style is preferred if there is only one supported part.

> rzg2l_adc.c:    indio_dev->name = DRIVER_NAME;

We try to avoid using a macro for the driver name like this.

> sc27xx_adc.c:   indio_dev->name = dev_name(dev);

Looks like we missed catching this one in review.

> mt6359-auxadc.c:  indio_dev->name = adc_dev->chip_info->model_name;

This is preferred if there is more than one part supported in the driver.

> mcp3911.c:      indio_dev->name = spi_get_device_id(spi)->name;

This is fine too in cases where there isn't chip_info.

> 
> Are you suggesting to use the compatible part number ?
> 
>     indio->name = "s32g2-sar-adc";
> 

That works.

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

[David Lechner]

On 9/10/25 10:57 AM, 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>
> ---
>  drivers/iio/adc/Kconfig       |   13 +
>  drivers/iio/adc/Makefile      |    1 +
>  drivers/iio/adc/nxp-sar-adc.c | 1026 +++++++++++++++++++++++++++++++++
>  3 files changed, 1040 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

We try to keep these in alphabetical order.

>  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..421ce42ad9ec
> --- /dev/null
> +++ b/drivers/iio/adc/nxp-sar-adc.c
> @@ -0,0 +1,1026 @@
> +// SPDX-License-Identifier: GPL-2.0

Prefer explicit GPL-2.0-only or GPL-2.0-or-later

...

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

Is it OK to call this with spinlock held? It looks like it can call
devm_krealloc() which may sleep.

> +		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_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 = device_get_match_data(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");
> +
> +	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);
> +
> +	info->regs_phys = mem->start;
> +	spin_lock_init(&info->lock);
> +
> +	info->clk = devm_clk_get_enabled(dev, "adc");

clock-names was dropped from bindings, so name should be 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 = 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)
> +		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 const struct nxp_sar_adc_data s32g2_sar_adc_data = { .vref_mV = 1800 };

Why have this if there is only one option?

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

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

[Daniel Lezcano]

Hi David,


On 11/09/2025 22:10, David Lechner wrote:
> On 9/10/25 10:57 AM, Daniel Lezcano wrote:

[ ... ]

>> +		/* 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);
> 
> Is it OK to call this with spinlock held? It looks like it can call
> devm_krealloc() which may sleep.
> 

It should be ok, devm_krealloc is in the code path of 
iio_push_to_buffers_with_ts_unaligned(), not in 
iio_push_to_buffers_with_timestamp()

> ...
> 
>> +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 = device_get_match_data(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");
>> +
>> +	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);
>> +
>> +	info->regs_phys = mem->start;
>> +	spin_lock_init(&info->lock);
>> +
>> +	info->clk = devm_clk_get_enabled(dev, "adc");
> 
> clock-names was dropped from bindings, so name should be NULL.

Right, I found it when I removed the clock-names from the DT :)

> 
>> +static const struct nxp_sar_adc_data s32g2_sar_adc_data = { .vref_mV = 1800 };
> 
> Why have this if there is only one option?

There will be the ADC model name/variant in V3.

Thanks!

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

[Andy Shevchenko]

On Fri, Sep 12, 2025 at 2:03 AM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> On 11/09/2025 22:10, David Lechner wrote:
> > On 9/10/25 10:57 AM, Daniel Lezcano wrote:

[ ... ]

> >> +            /* iio_push_to_buffers_with_timestamp should not be called
> >> +             * with dma_samples as parameter. The samples will be smashed
> >> +             * if timestamp is enabled.
> >> +             */

/*
 * Btw, comment style for multi-line
 * comments is wrong for this subsystem.
 * Use this as an example, Also, refer to
 * the function as func(), i.e. mind the parentheses.
 */

> >> +            timestamp = iio_get_time_ns(indio_dev);
> >> +            ret = iio_push_to_buffers_with_timestamp(indio_dev,
> >> +                                                     info->buffer,
> >> +                                                     timestamp);
> >
> > Is it OK to call this with spinlock held? It looks like it can call
> > devm_krealloc() which may sleep.
>
> It should be ok, devm_krealloc is in the code path of
> iio_push_to_buffers_with_ts_unaligned(), not in
> iio_push_to_buffers_with_timestamp()

This is a good observation, can we document this in the respective
kernel-doc:s please? Also add might_sleep().might_sleep_if() in the
appropriate functions.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, Sep 10, 2025 at 6:58 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> 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/bitops.h>

> +#include <linux/circ_buf.h>

Is it used?

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

+ err.h

> +#include <linux/interrupt.h>
> +#include <linux/iopoll.h>
> +#include <linux/math64.h>

+ minmax.h

> +#include <linux/module.h>

> +#include <linux/of.h>

Wrong header, should be property.h.

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

+ time.h

> +#include <linux/types.h>
> +#include <linux/units.h>

...

> +#define REG_ADC_CTR_INPSAMP_MIN                8
> +#define REG_ADC_CTR_INPSAMP_MAX                0xFF

First of all, the value is written in a different style. Be
consistent. Second, is this limited by a bit field resolution? If so,
form (BIT($x) - 1) is better as it shows the limit in $x ($x should be
replaced with the actual value).

...

> +struct nxp_sar_adc {
> +       void __iomem *regs;
> +       phys_addr_t regs_phys;
> +       struct clk *clk;
> +
> +       u16 value;
> +       u32 vref_mV;
> +       u8 current_channel;
> +       u8 channels_used;

Run `pahole` and act accordingly.

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

...

> +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
> +        */

This is perfectly a single line comment. Same applies to a few
comments in the code.

> +       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
> +        */

Missing period at the end. Same applies to a few comments in the code.

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

Do you need 'U'? AFAIR frequency can't be negative. Same applies to a
few cases in the code.

> +       return pwdn;
> +}

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

...

> +               ret = wait_for_completion_interruptible_timeout
> +                       (&info->completion,
> +                       msecs_to_jiffies(NXP_SAR_ADC_CONV_TIMEOUT_MS));

This is bad style, please reindent in a better way.

...

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

Configuring it to

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

...

> +       /* If everything transferred, avoid an off by one error. */

was/is transfered.

...

> +       /* dma_buf->tail != dma_buf->head condition will become false
> +        * because dma_buf->tail will be incremented with 1.
> +        */

Wrong multi-line comment style. Driver was written by a few people?

...

> +/*
> + * 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

It is not a

> + * documentation NXP recommend to not assume the default values are

recommends
assuming

> + * 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 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 = device_get_match_data(dev);
> +
> +       info->vref_mV = data->vref_mV;

Instead of split, move data assignment to be before indio_dev allocation.

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

return dev_err_probe(...);

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

-- 
With Best Regards,
Andy Shevchenko

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

[Nuno Sá]

On Fri, 2025-09-12 at 08:38 +0300, Andy Shevchenko wrote:
> On Fri, Sep 12, 2025 at 2:03 AM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
> > On 11/09/2025 22:10, David Lechner wrote:
> > > On 9/10/25 10:57 AM, Daniel Lezcano wrote:
> 
> [ ... ]
> 
> > > > +            /* iio_push_to_buffers_with_timestamp should not be called
> > > > +             * with dma_samples as parameter. The samples will be
> > > > smashed
> > > > +             * if timestamp is enabled.
> > > > +             */
> 
> /*
>  * Btw, comment style for multi-line
>  * comments is wrong for this subsystem.
>  * Use this as an example, Also, refer to
>  * the function as func(), i.e. mind the parentheses.
>  */
> 
> > > > +            timestamp = iio_get_time_ns(indio_dev);
> > > > +            ret = iio_push_to_buffers_with_timestamp(indio_dev,
> > > > +                                                     info->buffer,
> > > > +                                                     timestamp);
> > > 
> > > Is it OK to call this with spinlock held? It looks like it can call
> > > devm_krealloc() which may sleep.
> > 
> > It should be ok, devm_krealloc is in the code path of
> > iio_push_to_buffers_with_ts_unaligned(), not in
> > iio_push_to_buffers_with_timestamp()
> 
> This is a good observation, can we document this in the respective
> kernel-doc:s please? Also add might_sleep().might_sleep_if() in the
> appropriate functions.

That's a good idea!

- Nuno Sá

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

[Jonathan Cameron]

On Thu, 11 Sep 2025 08:26:06 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 9/11/25 7:55 AM, Daniel Lezcano wrote:
> > 
> > Hi Jonathan,
> > 
> > thanks for the review
> > 
> > On 10/09/2025 19:32, Jonathan Cameron wrote:  
> >> On Wed, 10 Sep 2025 17:57:56 +0200
> >> Daniel Lezcano <daniel.lezcano@linaro.org> wrote:  
> > 
> > [ ... ]
> >   
> 
> ...
> 
> >   
> >>> +    indio_dev->name = dev_name(dev);  
> >>
> >> This should be the 'part number'.  That is a little ill defined
> >> for a SoC integrated ADC, but generally not what we get from dev_name()
> >> on the platform_device.  
> > 
> > Sorry, I don't get the comment. If I refer to the different drivers there is not consistency with the iio_dev->name.  
> 
> dev_name() will be something like adc@12345678 from the devicetree,
> so not the "part number".
> 
> > 
> > rtq6056.c:      indio_dev->name = "rtq6056";  
> 
> This style is preferred if there is only one supported part.
> 
> > rzg2l_adc.c:    indio_dev->name = DRIVER_NAME;  
> 
> We try to avoid using a macro for the driver name like this.
> 
> > sc27xx_adc.c:   indio_dev->name = dev_name(dev);  
> 
> Looks like we missed catching this one in review.

There are a bunch of historical drivers (and maybe some more recent
ones that snuck in). The risk of changing that ABI seemed too high to fix them
up.  I guess this happens often enough I should add a comment to the ones
that still do this about it being wrong but left alone as the ABI was in use.


> 
> > mt6359-auxadc.c:  indio_dev->name = adc_dev->chip_info->model_name;  
> 
> This is preferred if there is more than one part supported in the driver.
> 
> > mcp3911.c:      indio_dev->name = spi_get_device_id(spi)->name;  
> 
> This is fine too in cases where there isn't chip_info.
> 
> > 
> > Are you suggesting to use the compatible part number ?
> > 
> >     indio->name = "s32g2-sar-adc";
> >   
> 
> That works.
> 
> 
> 

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

[Jonathan Cameron]

On Thu, 11 Sep 2025 14:55:00 +0200
Daniel Lezcano <daniel.lezcano@linaro.org> wrote:

> Hi Jonathan,
> 
> thanks for the review
> 
> On 10/09/2025 19:32, Jonathan Cameron wrote:
> > On Wed, 10 Sep 2025 17:57:56 +0200
> > Daniel Lezcano <daniel.lezcano@linaro.org> wrote:  
> 
> [ ... ]
> 
> >> +/* Main Configuration Register */
> >> +#define REG_ADC_MCR(__base)		((__base) + 0x00)  
> > 
> > I'm not really convinced these macros help over just having
> > readl(info->regs + NXP_SADC_MCR_REG);  
> 
> That is really a matter of taste :)
> 
> I used to create this format in order to stick the macros with the 
> debugfs register code which is not part of these changes. There is a 
> similar format in drivers/clocksource/timer-nxp-stm.c or 
> driver/thermal/mediatek/lvts.c IMHO is less prone to error than base + 
> REG all around the code.
> 
> Do you want me to convert all the macros to info->__base + MACRO ?

I'm not that fussed if there is other code for related devices using this
style.  To me it adds little benefit but it doesn't hurt that much either!

> 
> [ ... ]
> 
> >> +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),  
> > 
> > Whilst we only insist on monotonic numbering, putting it all the way down
> > at 32 seems excessive. Why not 8?  Perhaps a comment if this is to avoid
> > moving it for some future feature.  
> 
> The ADC has 8 channels for external acquisition however others channels 
> 8->31 are described as reserved. They may evolve in the future to more 
> channels. That is probably the reason why 32 is used here.

Add a comment on that so we don't forget the reasoning.

Thanks,

Jonathan

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

[Jonathan Cameron]

On Fri, 12 Sep 2025 09:19:43 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Fri, 2025-09-12 at 08:38 +0300, Andy Shevchenko wrote:
> > On Fri, Sep 12, 2025 at 2:03 AM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:  
> > > On 11/09/2025 22:10, David Lechner wrote:  
> > > > On 9/10/25 10:57 AM, Daniel Lezcano wrote:  
> > 
> > [ ... ]
> >   
> > > > > +            /* iio_push_to_buffers_with_timestamp should not be called
> > > > > +             * with dma_samples as parameter. The samples will be
> > > > > smashed
> > > > > +             * if timestamp is enabled.
> > > > > +             */  
> > 
> > /*
> >  * Btw, comment style for multi-line
> >  * comments is wrong for this subsystem.
> >  * Use this as an example, Also, refer to
> >  * the function as func(), i.e. mind the parentheses.
> >  */
> >   
> > > > > +            timestamp = iio_get_time_ns(indio_dev);
> > > > > +            ret = iio_push_to_buffers_with_timestamp(indio_dev,
> > > > > +                                                     info->buffer,
> > > > > +                                                     timestamp);  
> > > > 
> > > > Is it OK to call this with spinlock held? It looks like it can call
> > > > devm_krealloc() which may sleep.  
> > > 
> > > It should be ok, devm_krealloc is in the code path of
> > > iio_push_to_buffers_with_ts_unaligned(), not in
> > > iio_push_to_buffers_with_timestamp()  
> > 
> > This is a good observation, can we document this in the respective
> > kernel-doc:s please? Also add might_sleep().might_sleep_if() in the
> > appropriate functions.  
> 
> That's a good idea!

Agreed. I'd forgotten that hidden allocation was there in the unaligned()
variant.  A might_sleep() seems wise.

> 
> - Nuno Sá

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

[Nuno Sá]

On Fri, 2025-09-12 at 15:17 +0100, Jonathan Cameron wrote:
> On Thu, 11 Sep 2025 08:26:06 -0500
> David Lechner <dlechner@baylibre.com> wrote:
> 
> > On 9/11/25 7:55 AM, Daniel Lezcano wrote:
> > > 
> > > Hi Jonathan,
> > > 
> > > thanks for the review
> > > 
> > > On 10/09/2025 19:32, Jonathan Cameron wrote:  
> > > > On Wed, 10 Sep 2025 17:57:56 +0200
> > > > Daniel Lezcano <daniel.lezcano@linaro.org> wrote:  
> > > 
> > > [ ... ]
> > >   
> > 
> > ...
> > 
> > >   
> > > > > +    indio_dev->name = dev_name(dev);  
> > > > 
> > > > This should be the 'part number'.  That is a little ill defined
> > > > for a SoC integrated ADC, but generally not what we get from dev_name()
> > > > on the platform_device.  
> > > 
> > > Sorry, I don't get the comment. If I refer to the different drivers there
> > > is not consistency with the iio_dev->name.  
> > 
> > dev_name() will be something like adc@12345678 from the devicetree,
> > so not the "part number".
> > 
> > > 
> > > rtq6056.c:      indio_dev->name = "rtq6056";  
> > 
> > This style is preferred if there is only one supported part.
> > 
> > > rzg2l_adc.c:    indio_dev->name = DRIVER_NAME;  
> > 
> > We try to avoid using a macro for the driver name like this.
> > 
> > > sc27xx_adc.c:   indio_dev->name = dev_name(dev);  
> > 
> > Looks like we missed catching this one in review.
> 
> There are a bunch of historical drivers (and maybe some more recent
> ones that snuck in). The risk of changing that ABI seemed too high to fix them
> up.  I guess this happens often enough I should add a comment to the ones
> that still do this about it being wrong but left alone as the ABI was in use.
> 

Yeah, we (at ADI) also had the highly questionable practise of using the OF node
name until we the label property got in. So I would not be surprised if some of
those are still around.

- Nuno Sá 

> 
> > 
> > > mt6359-auxadc.c:  indio_dev->name = adc_dev->chip_info->model_name;  
> > 
> > This is preferred if there is more than one part supported in the driver.
> > 
> > > mcp3911.c:      indio_dev->name = spi_get_device_id(spi)->name;  
> > 
> > This is fine too in cases where there isn't chip_info.
> > 
> > > 
> > > Are you suggesting to use the compatible part number ?
> > > 
> > >     indio->name = "s32g2-sar-adc";
> > >   
> > 
> > That works.
> > 
> > 
> >