[PATCH 0/2] iio: adc: Add Nuvoton MA35D1 EADC support

[PATCH 0/2] iio: adc: Add Nuvoton MA35D1 EADC support

[Chi-Wen Weng]

From: Chi-Wen Weng <cwweng@nuvoton.com>

This series adds devicetree binding and IIO driver support for the
Nuvoton MA35D1 Enhanced ADC controller.

The MA35D1 EADC controller supports multiple ADC input channels. This
initial upstream driver supports direct raw reads and triggered buffered
capture using the controller end-of-conversion interrupt as the IIO
device trigger.

ADC channels are described using standard firmware child nodes. Both
single-ended and differential channels are supported. Since the
differential enable bit is global in the controller, mixed single-ended
and differential buffered scans are rejected.

DMA support is intentionally not included in this initial version. The
driver uses the interrupt-driven conversion path to keep the first
upstream submission small and easier to review.

Patch 1 adds the devicetree binding.
Patch 2 adds the MA35D1 EADC IIO driver.

Chi-Wen Weng (2):
  dt-bindings: iio: adc: Add Nuvoton MA35D1 EADC
  iio: adc: Add Nuvoton MA35D1 EADC driver

 .../bindings/iio/adc/nuvoton,ma35d1-eadc.yaml | 100 +++
 drivers/iio/adc/Kconfig                       |  10 +
 drivers/iio/adc/Makefile                      |   1 +
 drivers/iio/adc/ma35d1_eadc.c                 | 636 ++++++++++++++++++
 4 files changed, 747 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml
 create mode 100644 drivers/iio/adc/ma35d1_eadc.c

-- 
2.25.1

[PATCH 1/2] dt-bindings: iio: adc: Add Nuvoton MA35D1 EADC

[Chi-Wen Weng]

From: Chi-Wen Weng <cwweng@nuvoton.com>

Add devicetree binding for the Enhanced ADC controller found on
Nuvoton MA35D1 SoCs.

The controller has one register region, one interrupt and one functional
clock. ADC inputs are described using standard channel child nodes,
including optional differential channel pairs.

Signed-off-by: Chi-Wen Weng <cwweng@nuvoton.com>
---
 .../bindings/iio/adc/nuvoton,ma35d1-eadc.yaml | 100 ++++++++++++++++++
 1 file changed, 100 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml b/Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml
new file mode 100644
index 000000000000..ae7ad0f7689a
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml
@@ -0,0 +1,100 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/nuvoton,ma35d1-eadc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Nuvoton MA35D1 Enhanced Analog to Digital Converter
+
+maintainers:
+  - Chi-Wen Weng <cwweng@nuvoton.com>
+
+description: |
+  The Nuvoton MA35D1 Enhanced Analog to Digital Converter (EADC) is a
+  12-bit ADC controller integrated in the MA35D1 SoC. Each enabled ADC
+  input is described by a child channel node.
+
+properties:
+  compatible:
+    const: nuvoton,ma35d1-eadc
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+patternProperties:
+  '^channel@[0-8]$':
+    type: object
+    $ref: adc.yaml
+    unevaluatedProperties: false
+
+    properties:
+      reg:
+        minimum: 0
+        maximum: 8
+
+      diff-channels:
+        minItems: 2
+        maxItems: 2
+        items:
+          minimum: 0
+          maximum: 8
+
+    required:
+      - reg
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - '#address-cells'
+  - '#size-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/nuvoton,ma35d1-clk.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        adc@40430000 {
+            compatible = "nuvoton,ma35d1-eadc";
+            reg = <0x0 0x40430000 0x0 0x10000>;
+            interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>;
+            clocks = <&clk EADC_GATE>;
+
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            channel@0 {
+                reg = <0>;
+            };
+
+            channel@1 {
+                reg = <1>;
+            };
+
+            channel@2 {
+                reg = <2>;
+                diff-channels = <2 3>;
+            };
+        };
+    };
+...
-- 
2.25.1

[PATCH 2/2] iio: adc: Add Nuvoton MA35D1 EADC driver

[Chi-Wen Weng]

From: Chi-Wen Weng <cwweng@nuvoton.com>

Add an IIO driver for the Nuvoton MA35D1 Enhanced ADC controller.

The driver supports direct raw reads and triggered buffered capture. The
controller end-of-conversion interrupt is exposed as the device trigger
and is used to push samples into the IIO buffer.

Channels are described by firmware child nodes and can be configured as
single-ended or differential inputs. Since the differential enable bit is
global, mixed single-ended and differential buffered scans are rejected.

DMA support is intentionally not included in this initial upstream driver;
conversions are handled through the interrupt-driven path.

Signed-off-by: Chi-Wen Weng <cwweng@nuvoton.com>
---
 drivers/iio/adc/Kconfig       |  10 +
 drivers/iio/adc/Makefile      |   1 +
 drivers/iio/adc/ma35d1_eadc.c | 636 ++++++++++++++++++++++++++++++++++
 3 files changed, 647 insertions(+)
 create mode 100644 drivers/iio/adc/ma35d1_eadc.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 1c663c98c6c9..43409999a94b 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -981,6 +981,16 @@ config LTC2497
 	  To compile this driver as a module, choose M here: the module will be
 	  called ltc2497.
 
+config MA35D1_EADC
+	tristate "MA35D1 EADC driver"
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	help
+	  Say yes here to build support for MA35D1 EADC.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called ma35d1.
+
 config MAX1027
 	tristate "Maxim max1027 ADC driver"
 	depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 707dd708912f..7b9b38688223 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -85,6 +85,7 @@ obj-$(CONFIG_LTC2471) += ltc2471.o
 obj-$(CONFIG_LTC2485) += ltc2485.o
 obj-$(CONFIG_LTC2496) += ltc2496.o ltc2497-core.o
 obj-$(CONFIG_LTC2497) += ltc2497.o ltc2497-core.o
+obj-$(CONFIG_MA35D1_EADC) += ma35d1_eadc.o
 obj-$(CONFIG_MAX1027) += max1027.o
 obj-$(CONFIG_MAX11100) += max11100.o
 obj-$(CONFIG_MAX1118) += max1118.o
diff --git a/drivers/iio/adc/ma35d1_eadc.c b/drivers/iio/adc/ma35d1_eadc.c
new file mode 100644
index 000000000000..0c075126e139
--- /dev/null
+++ b/drivers/iio/adc/ma35d1_eadc.c
@@ -0,0 +1,636 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Nuvoton MA35D1 EADC driver
+ *
+ * Copyright (c) 2026 Nuvoton Technology Corp.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/bitmap.h>
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/property.h>
+
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+#define MA35D1_EADC_DAT(n)		(0x00 + (n) * 0x04)
+#define MA35D1_EADC_CTL			0x50
+#define MA35D1_EADC_SWTRG		0x54
+#define MA35D1_EADC_SCTL(n)		(0x80 + (n) * 0x04)
+#define MA35D1_EADC_INTSRC0		0xd0
+#define MA35D1_EADC_STATUS2		0xf8
+#define MA35D1_EADC_SELSMP0		0x140
+#define MA35D1_EADC_REFADJCTL		0x150
+
+#define MA35D1_EADC_CTL_ADCEN		BIT(0)
+#define MA35D1_EADC_CTL_ADCIEN0		BIT(2)
+#define MA35D1_EADC_CTL_DIFFEN		BIT(8)
+
+#define MA35D1_EADC_SCTL_CHSEL_MASK	GENMASK(3, 0)
+#define MA35D1_EADC_SCTL_TRGDLY_MASK	GENMASK(15, 8)
+#define MA35D1_EADC_SCTL_TRGSEL_MASK	GENMASK(21, 16)
+#define MA35D1_EADC_SCTL_TRGSEL_ADINT0	\
+	FIELD_PREP(MA35D1_EADC_SCTL_TRGSEL_MASK, 2)
+
+#define MA35D1_EADC_DAT_MASK		GENMASK(11, 0)
+#define MA35D1_EADC_STATUS2_ADIF0	BIT(0)
+#define MA35D1_EADC_INTSRC0_ADINT0	BIT(0)
+#define MA35D1_EADC_REFADJCTL_EXT_VREF	BIT(0)
+
+#define MA35D1_EADC_MAX_CHANNELS	9
+#define MA35D1_EADC_MAX_SAMPLE_MODULES	16
+#define MA35D1_EADC_CHAN_NAME_LEN	16
+#define MA35D1_EADC_TIMEOUT		msecs_to_jiffies(1000)
+
+struct ma35d1_adc {
+	struct device *dev;
+	void __iomem *regs;
+	struct clk *clk;
+	struct completion completion;
+	/* Protects direct conversions against concurrent register access. */
+	struct mutex lock;
+	struct iio_trigger *trig;
+	unsigned int scan_chancnt;
+	bool scan_differential;
+	char chan_name[MA35D1_EADC_MAX_CHANNELS][MA35D1_EADC_CHAN_NAME_LEN];
+	struct {
+		u16 channels[MA35D1_EADC_MAX_SAMPLE_MODULES];
+		aligned_s64 timestamp;
+	} scan;
+};
+
+static inline u32 ma35d1_adc_read(struct ma35d1_adc *adc, u32 reg)
+{
+	return readl(adc->regs + reg);
+}
+
+static inline void ma35d1_adc_write(struct ma35d1_adc *adc, u32 reg, u32 val)
+{
+	writel(val, adc->regs + reg);
+}
+
+static void ma35d1_adc_rmw(struct ma35d1_adc *adc, u32 reg, u32 mask, u32 val)
+{
+	u32 tmp;
+
+	tmp = ma35d1_adc_read(adc, reg);
+	tmp &= ~mask;
+	tmp |= val;
+	ma35d1_adc_write(adc, reg, tmp);
+}
+
+static void ma35d1_adc_set_diff(struct ma35d1_adc *adc, bool differential)
+{
+	ma35d1_adc_rmw(adc, MA35D1_EADC_CTL, MA35D1_EADC_CTL_DIFFEN,
+		       differential ? MA35D1_EADC_CTL_DIFFEN : 0);
+}
+
+static void ma35d1_adc_config_sample(struct ma35d1_adc *adc,
+				     unsigned int sample, unsigned int channel)
+{
+	u32 reg = MA35D1_EADC_SCTL(sample);
+
+	ma35d1_adc_rmw(adc, reg,
+		       MA35D1_EADC_SCTL_CHSEL_MASK |
+		       MA35D1_EADC_SCTL_TRGSEL_MASK,
+		       FIELD_PREP(MA35D1_EADC_SCTL_CHSEL_MASK, channel) |
+		       MA35D1_EADC_SCTL_TRGSEL_ADINT0);
+}
+
+static void ma35d1_adc_disable_irq(struct ma35d1_adc *adc)
+{
+	ma35d1_adc_rmw(adc, MA35D1_EADC_CTL, MA35D1_EADC_CTL_ADCIEN0, 0);
+}
+
+static void ma35d1_adc_hw_init(struct ma35d1_adc *adc)
+{
+	ma35d1_adc_disable_irq(adc);
+	ma35d1_adc_rmw(adc, MA35D1_EADC_CTL,
+		       MA35D1_EADC_CTL_ADCEN, MA35D1_EADC_CTL_ADCEN);
+	ma35d1_adc_write(adc, MA35D1_EADC_STATUS2, MA35D1_EADC_STATUS2_ADIF0);
+	ma35d1_adc_rmw(adc, MA35D1_EADC_INTSRC0,
+		       MA35D1_EADC_INTSRC0_ADINT0,
+		       MA35D1_EADC_INTSRC0_ADINT0);
+	ma35d1_adc_rmw(adc, MA35D1_EADC_REFADJCTL,
+		       MA35D1_EADC_REFADJCTL_EXT_VREF,
+		       MA35D1_EADC_REFADJCTL_EXT_VREF);
+	ma35d1_adc_rmw(adc, MA35D1_EADC_SELSMP0, GENMASK(1, 0), 3);
+}
+
+static void ma35d1_adc_hw_disable(void *data)
+{
+	struct ma35d1_adc *adc = data;
+
+	ma35d1_adc_disable_irq(adc);
+	ma35d1_adc_rmw(adc, MA35D1_EADC_CTL, MA35D1_EADC_CTL_ADCEN, 0);
+}
+
+static irqreturn_t ma35d1_adc_isr(int irq, void *data)
+{
+	struct iio_dev *indio_dev = data;
+	struct ma35d1_adc *adc = iio_priv(indio_dev);
+	u32 status;
+
+	status = ma35d1_adc_read(adc, MA35D1_EADC_STATUS2);
+	if (!(status & MA35D1_EADC_STATUS2_ADIF0))
+		return IRQ_NONE;
+
+	ma35d1_adc_write(adc, MA35D1_EADC_STATUS2, MA35D1_EADC_STATUS2_ADIF0);
+
+	if (iio_buffer_enabled(indio_dev)) {
+		ma35d1_adc_disable_irq(adc);
+		iio_trigger_poll(adc->trig);
+	} else {
+		complete(&adc->completion);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t ma35d1_adc_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct ma35d1_adc *adc = iio_priv(indio_dev);
+	int i;
+
+	for (i = 0; i < adc->scan_chancnt; i++)
+		adc->scan.channels[i] =
+			ma35d1_adc_read(adc, MA35D1_EADC_DAT(i)) &
+			MA35D1_EADC_DAT_MASK;
+
+	iio_push_to_buffers_with_timestamp(indio_dev, &adc->scan, pf->timestamp);
+	iio_trigger_notify_done(adc->trig);
+
+	ma35d1_adc_rmw(adc, MA35D1_EADC_CTL, MA35D1_EADC_CTL_ADCIEN0,
+		       MA35D1_EADC_CTL_ADCIEN0);
+	ma35d1_adc_write(adc, MA35D1_EADC_SWTRG, 1);
+
+	return IRQ_HANDLED;
+}
+
+static int ma35d1_adc_read_conversion(struct iio_dev *indio_dev,
+				      const struct iio_chan_spec *chan,
+				      int *val)
+{
+	struct ma35d1_adc *adc = iio_priv(indio_dev);
+	long timeout;
+
+	reinit_completion(&adc->completion);
+
+	ma35d1_adc_write(adc, MA35D1_EADC_STATUS2, MA35D1_EADC_STATUS2_ADIF0);
+	ma35d1_adc_rmw(adc, MA35D1_EADC_SCTL(0),
+		       MA35D1_EADC_SCTL_CHSEL_MASK |
+		       MA35D1_EADC_SCTL_TRGSEL_MASK,
+		       FIELD_PREP(MA35D1_EADC_SCTL_CHSEL_MASK,
+				  chan->channel));
+	ma35d1_adc_set_diff(adc, chan->differential);
+	ma35d1_adc_rmw(adc, MA35D1_EADC_CTL, MA35D1_EADC_CTL_ADCIEN0,
+		       MA35D1_EADC_CTL_ADCIEN0);
+	ma35d1_adc_write(adc, MA35D1_EADC_SWTRG, 1);
+
+	timeout = wait_for_completion_interruptible_timeout(&adc->completion,
+							    MA35D1_EADC_TIMEOUT);
+	ma35d1_adc_disable_irq(adc);
+
+	if (timeout < 0)
+		return timeout;
+	if (!timeout)
+		return -ETIMEDOUT;
+
+	*val = ma35d1_adc_read(adc, MA35D1_EADC_DAT(0)) & MA35D1_EADC_DAT_MASK;
+
+	return 0;
+}
+
+static int ma35d1_adc_read_raw(struct iio_dev *indio_dev,
+			       const struct iio_chan_spec *chan,
+			       int *val, int *val2, long mask)
+{
+	struct ma35d1_adc *adc = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		if (!iio_device_claim_direct(indio_dev))
+			return -EBUSY;
+
+		mutex_lock(&adc->lock);
+		ret = ma35d1_adc_read_conversion(indio_dev, chan, val);
+		mutex_unlock(&adc->lock);
+
+		iio_device_release_direct(indio_dev);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ma35d1_adc_validate_scan(struct iio_dev *indio_dev,
+				    const unsigned long *scan_mask)
+{
+	const struct iio_chan_spec *chan;
+	bool have_single = false;
+	bool have_diff = false;
+	unsigned int count = 0;
+	unsigned long bit;
+
+	for_each_set_bit(bit, scan_mask, indio_dev->masklength) {
+		chan = &indio_dev->channels[bit];
+
+		if (chan->type == IIO_TIMESTAMP)
+			continue;
+		count++;
+		if (chan->differential)
+			have_diff = true;
+		else
+			have_single = true;
+	}
+
+	if (!count || count > MA35D1_EADC_MAX_SAMPLE_MODULES)
+		return -EINVAL;
+
+	if (have_single && have_diff)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int ma35d1_adc_update_scan_mode(struct iio_dev *indio_dev,
+				       const unsigned long *scan_mask)
+{
+	struct ma35d1_adc *adc = iio_priv(indio_dev);
+	const struct iio_chan_spec *chan;
+	unsigned int sample = 0;
+	unsigned long bit;
+	bool differential = false;
+	int ret;
+
+	ret = ma35d1_adc_validate_scan(indio_dev, scan_mask);
+	if (ret)
+		return ret;
+
+	for_each_set_bit(bit, scan_mask, indio_dev->masklength) {
+		chan = &indio_dev->channels[bit];
+		if (chan->type == IIO_TIMESTAMP)
+			continue;
+
+		if (!sample)
+			differential = chan->differential;
+
+		ma35d1_adc_config_sample(adc, sample, chan->channel);
+		sample++;
+	}
+
+	adc->scan_chancnt = sample;
+	adc->scan_differential = differential;
+
+	return 0;
+}
+
+static int ma35d1_adc_buffer_postenable(struct iio_dev *indio_dev)
+{
+	struct ma35d1_adc *adc = iio_priv(indio_dev);
+	int i;
+
+	if (!adc->scan_chancnt)
+		return -EINVAL;
+
+	ma35d1_adc_write(adc, MA35D1_EADC_STATUS2, MA35D1_EADC_STATUS2_ADIF0);
+	ma35d1_adc_rmw(adc, MA35D1_EADC_INTSRC0,
+		       MA35D1_EADC_INTSRC0_ADINT0,
+		       MA35D1_EADC_INTSRC0_ADINT0);
+	ma35d1_adc_rmw(adc, MA35D1_EADC_REFADJCTL,
+		       MA35D1_EADC_REFADJCTL_EXT_VREF,
+		       MA35D1_EADC_REFADJCTL_EXT_VREF);
+	ma35d1_adc_rmw(adc, MA35D1_EADC_SELSMP0, GENMASK(1, 0), 3);
+	ma35d1_adc_set_diff(adc, adc->scan_differential);
+
+	for (i = 0; i < adc->scan_chancnt; i++)
+		ma35d1_adc_rmw(adc, MA35D1_EADC_SCTL(i),
+			       MA35D1_EADC_SCTL_TRGDLY_MASK,
+			       MA35D1_EADC_SCTL_TRGDLY_MASK);
+
+	ma35d1_adc_rmw(adc, MA35D1_EADC_CTL, MA35D1_EADC_CTL_ADCIEN0,
+		       MA35D1_EADC_CTL_ADCIEN0);
+	ma35d1_adc_write(adc, MA35D1_EADC_SWTRG, 1);
+
+	return 0;
+}
+
+static int ma35d1_adc_buffer_predisable(struct iio_dev *indio_dev)
+{
+	struct ma35d1_adc *adc = iio_priv(indio_dev);
+	int i;
+
+	ma35d1_adc_disable_irq(adc);
+	for (i = 0; i < adc->scan_chancnt; i++)
+		ma35d1_adc_rmw(adc, MA35D1_EADC_SCTL(i),
+			       MA35D1_EADC_SCTL_TRGSEL_MASK, 0);
+
+	return 0;
+}
+
+static const struct iio_buffer_setup_ops ma35d1_adc_buffer_ops = {
+	.postenable = ma35d1_adc_buffer_postenable,
+	.predisable = ma35d1_adc_buffer_predisable,
+};
+
+static const struct iio_info ma35d1_adc_info = {
+	.read_raw = ma35d1_adc_read_raw,
+	.update_scan_mode = ma35d1_adc_update_scan_mode,
+};
+
+static const struct iio_trigger_ops ma35d1_adc_trigger_ops = {
+	.validate_device = iio_trigger_validate_own_device,
+};
+
+static void ma35d1_adc_init_channel(struct ma35d1_adc *adc,
+				    struct iio_chan_spec *chan, u32 vinp,
+				    u32 vinn, int scan_index, bool differential)
+{
+	char *name = adc->chan_name[vinp];
+
+	chan->type = IIO_VOLTAGE;
+	chan->indexed = 1;
+	chan->channel = vinp;
+	chan->address = vinp;
+	chan->scan_index = scan_index;
+	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
+	chan->scan_type.sign = 'u';
+	chan->scan_type.realbits = 12;
+	chan->scan_type.storagebits = 16;
+	chan->scan_type.endianness = IIO_CPU;
+
+	if (differential) {
+		chan->differential = 1;
+		chan->channel2 = vinn;
+		snprintf(name, MA35D1_EADC_CHAN_NAME_LEN, "in%d-in%d", vinp,
+			 vinn);
+	} else {
+		snprintf(name, MA35D1_EADC_CHAN_NAME_LEN, "in%d", vinp);
+	}
+
+	chan->datasheet_name = name;
+}
+
+static int ma35d1_adc_parse_channels(struct iio_dev *indio_dev,
+				     struct device *dev)
+{
+	struct ma35d1_adc *adc = iio_priv(indio_dev);
+	DECLARE_BITMAP(used_channels, MA35D1_EADC_MAX_CHANNELS);
+	struct fwnode_handle *child;
+	struct iio_chan_spec *channels;
+	int num_channels;
+	int scan_index = 0;
+	int ret;
+
+	bitmap_zero(used_channels, MA35D1_EADC_MAX_CHANNELS);
+
+	num_channels = device_get_child_node_count(dev);
+	if (!num_channels)
+		return dev_err_probe(dev, -ENODATA,
+				     "no ADC channels configured\n");
+
+	if (num_channels > MA35D1_EADC_MAX_CHANNELS)
+		return dev_err_probe(dev, -EINVAL, "too many ADC channels\n");
+
+	channels = devm_kcalloc(dev, num_channels + 1, sizeof(*channels),
+				GFP_KERNEL);
+	if (!channels)
+		return -ENOMEM;
+
+	device_for_each_child_node(dev, child) {
+		u32 diff[2];
+		u32 reg;
+		bool differential = false;
+
+		ret = fwnode_property_read_u32(child, "reg", &reg);
+		if (ret) {
+			fwnode_handle_put(child);
+			return dev_err_probe(dev, ret,
+					     "missing channel reg property\n");
+		}
+
+		if (reg >= MA35D1_EADC_MAX_CHANNELS) {
+			fwnode_handle_put(child);
+			return dev_err_probe(dev, -EINVAL,
+					     "invalid ADC channel %u\n", reg);
+		}
+
+		if (test_and_set_bit(reg, used_channels)) {
+			fwnode_handle_put(child);
+			return dev_err_probe(dev, -EINVAL,
+					     "duplicate ADC channel %u\n", reg);
+		}
+
+		if (fwnode_property_present(child, "diff-channels")) {
+			ret = fwnode_property_read_u32_array(child,
+							     "diff-channels",
+							     diff,
+							     ARRAY_SIZE(diff));
+			if (ret) {
+				fwnode_handle_put(child);
+				return dev_err_probe(dev, ret,
+						     "invalid diff-channels for channel %u\n",
+						     reg);
+			}
+
+			if (diff[0] != reg ||
+			    diff[1] >= MA35D1_EADC_MAX_CHANNELS ||
+			    diff[0] == diff[1]) {
+				fwnode_handle_put(child);
+				return dev_err_probe(dev, -EINVAL,
+						     "invalid differential ADC channel %u-%u\n",
+						     diff[0], diff[1]);
+			}
+
+			if (test_and_set_bit(diff[1], used_channels)) {
+				fwnode_handle_put(child);
+				return dev_err_probe(dev, -EINVAL,
+						     "ADC channel %u already used\n",
+						     diff[1]);
+			}
+
+			differential = true;
+		}
+
+		ma35d1_adc_init_channel(adc, &channels[scan_index], reg,
+					differential ? diff[1] : 0,
+					scan_index, differential);
+		scan_index++;
+	}
+
+	channels[scan_index] = (struct iio_chan_spec)
+		IIO_CHAN_SOFT_TIMESTAMP(scan_index);
+
+	indio_dev->channels = channels;
+	indio_dev->num_channels = scan_index + 1;
+	indio_dev->masklength = indio_dev->num_channels;
+
+	return 0;
+}
+
+static int ma35d1_adc_setup_trigger(struct iio_dev *indio_dev,
+				    struct device *dev)
+{
+	struct ma35d1_adc *adc = iio_priv(indio_dev);
+	int ret;
+
+	adc->trig = devm_iio_trigger_alloc(dev, "%s-trigger", dev_name(dev));
+	if (!adc->trig)
+		return -ENOMEM;
+
+	adc->trig->ops = &ma35d1_adc_trigger_ops;
+	iio_trigger_set_drvdata(adc->trig, indio_dev);
+
+	ret = devm_iio_trigger_register(dev, adc->trig);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to register trigger\n");
+
+	ret = iio_trigger_set_immutable(indio_dev, adc->trig);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to set trigger\n");
+
+	return 0;
+}
+
+static int ma35d1_adc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct iio_dev *indio_dev;
+	struct ma35d1_adc *adc;
+	int irq;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*adc));
+	if (!indio_dev)
+		return -ENOMEM;
+	adc = iio_priv(indio_dev);
+	adc->dev = dev;
+	mutex_init(&adc->lock);
+	init_completion(&adc->completion);
+
+	adc->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(adc->regs))
+		return dev_err_probe(dev, PTR_ERR(adc->regs),
+				     "failed to map registers\n");
+
+	adc->clk = devm_clk_get_enabled(dev, NULL);
+	if (IS_ERR(adc->clk))
+		return dev_err_probe(dev, PTR_ERR(adc->clk),
+				     "failed to get and enable ADC clock\n");
+
+	indio_dev->name = "ma35d1-eadc";
+	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED;
+	indio_dev->info = &ma35d1_adc_info;
+
+	ret = ma35d1_adc_parse_channels(indio_dev, dev);
+	if (ret)
+		return ret;
+
+	ma35d1_adc_hw_init(adc);
+
+	ret = devm_add_action_or_reset(dev, ma35d1_adc_hw_disable, adc);
+	if (ret)
+		return ret;
+
+	ret = ma35d1_adc_setup_trigger(indio_dev, dev);
+	if (ret)
+		return ret;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	ret = devm_request_irq(dev, irq, ma35d1_adc_isr, 0, dev_name(dev),
+			       indio_dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to request IRQ %d\n", irq);
+
+	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
+					      iio_pollfunc_store_time,
+					      ma35d1_adc_trigger_handler,
+					      &ma35d1_adc_buffer_ops);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to setup triggered buffer\n");
+
+	platform_set_drvdata(pdev, indio_dev);
+
+	ret = devm_iio_device_register(dev, indio_dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to register IIO device\n");
+
+	return 0;
+}
+
+static int ma35d1_adc_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct ma35d1_adc *adc = iio_priv(indio_dev);
+
+	if (iio_buffer_enabled(indio_dev))
+		return -EBUSY;
+
+	ma35d1_adc_hw_disable(adc);
+	clk_disable_unprepare(adc->clk);
+
+	return 0;
+}
+
+static int ma35d1_adc_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct ma35d1_adc *adc = iio_priv(indio_dev);
+	int ret;
+
+	ret = clk_prepare_enable(adc->clk);
+	if (ret)
+		return ret;
+
+	ma35d1_adc_hw_init(adc);
+
+	return 0;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(ma35d1_adc_pm_ops,
+				ma35d1_adc_suspend, ma35d1_adc_resume);
+
+static const struct of_device_id ma35d1_adc_of_match[] = {
+	{ .compatible = "nuvoton,ma35d1-eadc" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ma35d1_adc_of_match);
+
+static struct platform_driver ma35d1_adc_driver = {
+	.probe = ma35d1_adc_probe,
+	.driver = {
+		.name = "ma35d1-eadc",
+		.of_match_table = ma35d1_adc_of_match,
+		.pm = pm_sleep_ptr(&ma35d1_adc_pm_ops),
+	},
+};
+module_platform_driver(ma35d1_adc_driver);
+
+MODULE_AUTHOR("Chi-Wen Weng <cwweng@nuvoton.com>");
+MODULE_DESCRIPTION("Nuvoton MA35D1 EADC driver");
+MODULE_LICENSE("GPL");
-- 
2.25.1

Re: [PATCH 1/2] dt-bindings: iio: adc: Add Nuvoton MA35D1 EADC

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 540 bytes --]

On Thu, Jun 25, 2026 at 07:06:37PM +0800, Chi-Wen Weng wrote:
> From: Chi-Wen Weng <cwweng@nuvoton.com>
> 
> Add devicetree binding for the Enhanced ADC controller found on
> Nuvoton MA35D1 SoCs.
> 
> The controller has one register region, one interrupt and one functional
> clock. ADC inputs are described using standard channel child nodes,
> including optional differential channel pairs.
> 
> Signed-off-by: Chi-Wen Weng <cwweng@nuvoton.com>

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
pw-bot: not-applicable

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/2] iio: adc: Add Nuvoton MA35D1 EADC driver

[Andy Shevchenko]

On Thu, Jun 25, 2026 at 07:06:38PM +0800, Chi-Wen Weng wrote:

> Add an IIO driver for the Nuvoton MA35D1 Enhanced ADC controller.
> 
> The driver supports direct raw reads and triggered buffered capture. The
> controller end-of-conversion interrupt is exposed as the device trigger
> and is used to push samples into the IIO buffer.
> 
> Channels are described by firmware child nodes and can be configured as
> single-ended or differential inputs. Since the differential enable bit is
> global, mixed single-ended and differential buffered scans are rejected.
> 
> DMA support is intentionally not included in this initial upstream driver;
> conversions are handled through the interrupt-driven path.

Nice written driver, some small issues here and there, and I think in a couple
of versions it will stabilize and can be accepted.

...

> +#include <linux/bitfield.h>

> +#include <linux/bits.h>

No need, bitmap.h covers this.

> +#include <linux/bitmap.h>
> +#include <linux/clk.h>
> +#include <linux/completion.h>

> +#include <linux/device.h>

No need, covered by platform_device.h.

> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>

> +#include <linux/kernel.h>

No way this header should be in the mere drivers.

> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm.h>
> +#include <linux/property.h>

Also missing some headers, such as types.h.

...

> +#define MA35D1_EADC_TIMEOUT		msecs_to_jiffies(1000)

+ jiffies.h

...

> +static inline u32 ma35d1_adc_read(struct ma35d1_adc *adc, u32 reg)
> +{
> +	return readl(adc->regs + reg);
> +}
> +
> +static inline void ma35d1_adc_write(struct ma35d1_adc *adc, u32 reg, u32 val)
> +{
> +	writel(val, adc->regs + reg);
> +}
> +
> +static void ma35d1_adc_rmw(struct ma35d1_adc *adc, u32 reg, u32 mask, u32 val)

Name it _update() to be aligned with the _read() and _write() above.

> +{
> +	u32 tmp;
> +
> +	tmp = ma35d1_adc_read(adc, reg);
> +	tmp &= ~mask;
> +	tmp |= val;

Correct pattern is to use

	tmp = (tmp & ~mask) | (val & mask);

> +	ma35d1_adc_write(adc, reg, tmp);
> +}

...

> +static void ma35d1_adc_config_sample(struct ma35d1_adc *adc,
> +				     unsigned int sample, unsigned int channel)
> +{
> +	u32 reg = MA35D1_EADC_SCTL(sample);

I don't see the need of this variable, use the value directly.

> +	ma35d1_adc_rmw(adc, reg,
> +		       MA35D1_EADC_SCTL_CHSEL_MASK |
> +		       MA35D1_EADC_SCTL_TRGSEL_MASK,
> +		       FIELD_PREP(MA35D1_EADC_SCTL_CHSEL_MASK, channel) |
> +		       MA35D1_EADC_SCTL_TRGSEL_ADINT0);
> +}

...

> +static irqreturn_t ma35d1_adc_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct ma35d1_adc *adc = iio_priv(indio_dev);

> +	int i;
> +
> +	for (i = 0; i < adc->scan_chancnt; i++)

	for (unsigned int i = 0; i < adc->scan_chancnt; i++)

> +		adc->scan.channels[i] =
> +			ma35d1_adc_read(adc, MA35D1_EADC_DAT(i)) &
> +			MA35D1_EADC_DAT_MASK;
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, &adc->scan, pf->timestamp);
> +	iio_trigger_notify_done(adc->trig);
> +
> +	ma35d1_adc_rmw(adc, MA35D1_EADC_CTL, MA35D1_EADC_CTL_ADCIEN0,
> +		       MA35D1_EADC_CTL_ADCIEN0);
> +	ma35d1_adc_write(adc, MA35D1_EADC_SWTRG, 1);
> +
> +	return IRQ_HANDLED;
> +}

...

> +static int ma35d1_adc_validate_scan(struct iio_dev *indio_dev,
> +				    const unsigned long *scan_mask)
> +{
> +	const struct iio_chan_spec *chan;
> +	bool have_single = false;
> +	bool have_diff = false;
> +	unsigned int count = 0;
> +	unsigned long bit;
> +
> +	for_each_set_bit(bit, scan_mask, indio_dev->masklength) {
> +		chan = &indio_dev->channels[bit];
> +
> +		if (chan->type == IIO_TIMESTAMP)
> +			continue;

> +		count++;

Make it last in the loop, it will be standard pattern. Otherwise it's hard to
read and find. Also it's recommended to split assignment and definition
for better maintenance.

	unsigned int count;
	...
	count = 0;
	for_each_set_bit(bit, scan_mask, indio_dev->masklength) {
		...
		count++;
	}

> +		if (chan->differential)
> +			have_diff = true;
> +		else
> +			have_single = true;
> +	}
> +
> +	if (!count || count > MA35D1_EADC_MAX_SAMPLE_MODULES)
> +		return -EINVAL;

> +	if (have_single && have_diff)
> +		return -EINVAL;

Is it possible IRL?

> +	return 0;
> +}

...

> +static int ma35d1_adc_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
> +	int i;
> +
> +	if (!adc->scan_chancnt)
> +		return -EINVAL;
> +
> +	ma35d1_adc_write(adc, MA35D1_EADC_STATUS2, MA35D1_EADC_STATUS2_ADIF0);
> +	ma35d1_adc_rmw(adc, MA35D1_EADC_INTSRC0,
> +		       MA35D1_EADC_INTSRC0_ADINT0,
> +		       MA35D1_EADC_INTSRC0_ADINT0);
> +	ma35d1_adc_rmw(adc, MA35D1_EADC_REFADJCTL,
> +		       MA35D1_EADC_REFADJCTL_EXT_VREF,
> +		       MA35D1_EADC_REFADJCTL_EXT_VREF);
> +	ma35d1_adc_rmw(adc, MA35D1_EADC_SELSMP0, GENMASK(1, 0), 3);
> +	ma35d1_adc_set_diff(adc, adc->scan_differential);
> +	for (i = 0; i < adc->scan_chancnt; i++)

	for (unsigned int i = 0; i < adc->scan_chancnt; i++)

> +		ma35d1_adc_rmw(adc, MA35D1_EADC_SCTL(i),
> +			       MA35D1_EADC_SCTL_TRGDLY_MASK,
> +			       MA35D1_EADC_SCTL_TRGDLY_MASK);
> +
> +	ma35d1_adc_rmw(adc, MA35D1_EADC_CTL, MA35D1_EADC_CTL_ADCIEN0,
> +		       MA35D1_EADC_CTL_ADCIEN0);
> +	ma35d1_adc_write(adc, MA35D1_EADC_SWTRG, 1);
> +
> +	return 0;
> +}
> +
> +static int ma35d1_adc_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
> +	int i;
> +
> +	ma35d1_adc_disable_irq(adc);
> +	for (i = 0; i < adc->scan_chancnt; i++)
> +		ma35d1_adc_rmw(adc, MA35D1_EADC_SCTL(i),
> +			       MA35D1_EADC_SCTL_TRGSEL_MASK, 0);

Ditto.

Also looking to the cases of setting 0s, I would rather have a helper
_set_bits() / _clear_bits() in conjunction with _update().

> +	return 0;
> +}

...

> +static void ma35d1_adc_init_channel(struct ma35d1_adc *adc,
> +				    struct iio_chan_spec *chan, u32 vinp,
> +				    u32 vinn, int scan_index, bool differential)
> +{
> +	char *name = adc->chan_name[vinp];
> +
> +	chan->type = IIO_VOLTAGE;
> +	chan->indexed = 1;
> +	chan->channel = vinp;
> +	chan->address = vinp;
> +	chan->scan_index = scan_index;
> +	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> +	chan->scan_type.sign = 'u';
> +	chan->scan_type.realbits = 12;
> +	chan->scan_type.storagebits = 16;
> +	chan->scan_type.endianness = IIO_CPU;
> +
> +	if (differential) {
> +		chan->differential = 1;
> +		chan->channel2 = vinn;

> +		snprintf(name, MA35D1_EADC_CHAN_NAME_LEN, "in%d-in%d", vinp,
> +			 vinn);

Can compiler prove the buffer size is enough?

> +	} else {
> +		snprintf(name, MA35D1_EADC_CHAN_NAME_LEN, "in%d", vinp);
> +	}
> +
> +	chan->datasheet_name = name;

Why not use devm_kasprintf() instead?

> +}

...

> +static int ma35d1_adc_parse_channels(struct iio_dev *indio_dev,
> +				     struct device *dev)
> +{
> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
> +	DECLARE_BITMAP(used_channels, MA35D1_EADC_MAX_CHANNELS);
> +	struct fwnode_handle *child;
> +	struct iio_chan_spec *channels;
> +	int num_channels;
> +	int scan_index = 0;
> +	int ret;
> +
> +	bitmap_zero(used_channels, MA35D1_EADC_MAX_CHANNELS);
> +
> +	num_channels = device_get_child_node_count(dev);
> +	if (!num_channels)
> +		return dev_err_probe(dev, -ENODATA,
> +				     "no ADC channels configured\n");
> +
> +	if (num_channels > MA35D1_EADC_MAX_CHANNELS)

Perhaps >= ?

> +		return dev_err_probe(dev, -EINVAL, "too many ADC channels\n");
> +
> +	channels = devm_kcalloc(dev, num_channels + 1, sizeof(*channels),
> +				GFP_KERNEL);
> +	if (!channels)
> +		return -ENOMEM;
> +
> +	device_for_each_child_node(dev, child) {

Use _scoped() variant.

> +		u32 diff[2];
> +		u32 reg;
> +		bool differential = false;
> +
> +		ret = fwnode_property_read_u32(child, "reg", &reg);
> +		if (ret) {
> +			fwnode_handle_put(child);
> +			return dev_err_probe(dev, ret,
> +					     "missing channel reg property\n");
> +		}
> +
> +		if (reg >= MA35D1_EADC_MAX_CHANNELS) {
> +			fwnode_handle_put(child);
> +			return dev_err_probe(dev, -EINVAL,
> +					     "invalid ADC channel %u\n", reg);
> +		}
> +
> +		if (test_and_set_bit(reg, used_channels)) {
> +			fwnode_handle_put(child);
> +			return dev_err_probe(dev, -EINVAL,
> +					     "duplicate ADC channel %u\n", reg);
> +		}
> +
> +		if (fwnode_property_present(child, "diff-channels")) {
> +			ret = fwnode_property_read_u32_array(child,
> +							     "diff-channels",
> +							     diff,
> +							     ARRAY_SIZE(diff));
> +			if (ret) {
> +				fwnode_handle_put(child);
> +				return dev_err_probe(dev, ret,
> +						     "invalid diff-channels for channel %u\n",
> +						     reg);
> +			}
> +
> +			if (diff[0] != reg ||
> +			    diff[1] >= MA35D1_EADC_MAX_CHANNELS ||
> +			    diff[0] == diff[1]) {
> +				fwnode_handle_put(child);
> +				return dev_err_probe(dev, -EINVAL,
> +						     "invalid differential ADC channel %u-%u\n",
> +						     diff[0], diff[1]);
> +			}
> +
> +			if (test_and_set_bit(diff[1], used_channels)) {
> +				fwnode_handle_put(child);
> +				return dev_err_probe(dev, -EINVAL,
> +						     "ADC channel %u already used\n",
> +						     diff[1]);
> +			}
> +
> +			differential = true;
> +		}
> +
> +		ma35d1_adc_init_channel(adc, &channels[scan_index], reg,
> +					differential ? diff[1] : 0,
> +					scan_index, differential);
> +		scan_index++;
> +	}
> +
> +	channels[scan_index] = (struct iio_chan_spec)
> +		IIO_CHAN_SOFT_TIMESTAMP(scan_index);
> +
> +	indio_dev->channels = channels;
> +	indio_dev->num_channels = scan_index + 1;
> +	indio_dev->masklength = indio_dev->num_channels;
> +
> +	return 0;
> +}

...

> +	ret = devm_request_irq(dev, irq, ma35d1_adc_isr, 0, dev_name(dev),
> +			       indio_dev);

Make it a single line, here it's fine.

> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to request IRQ %d\n", irq);

Remove duplicate error message.

...

> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> +					      iio_pollfunc_store_time,
> +					      ma35d1_adc_trigger_handler,
> +					      &ma35d1_adc_buffer_ops);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to setup triggered buffer\n");

So, it seems this is very rarely can be not -ENOMEM, and hence it's 99.99% dead
code, just

		return ret;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] dt-bindings: iio: adc: Add Nuvoton MA35D1 EADC

[David Lechner]

On 6/25/26 6:06 AM, Chi-Wen Weng wrote:
> From: Chi-Wen Weng <cwweng@nuvoton.com>
> 
> Add devicetree binding for the Enhanced ADC controller found on
> Nuvoton MA35D1 SoCs.
> 
> The controller has one register region, one interrupt and one functional
> clock. ADC inputs are described using standard channel child nodes,
> including optional differential channel pairs.
> 
> Signed-off-by: Chi-Wen Weng <cwweng@nuvoton.com>
> ---
>  .../bindings/iio/adc/nuvoton,ma35d1-eadc.yaml | 100 ++++++++++++++++++
>  1 file changed, 100 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml b/Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml
> new file mode 100644
> index 000000000000..ae7ad0f7689a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml
> @@ -0,0 +1,100 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/nuvoton,ma35d1-eadc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Nuvoton MA35D1 Enhanced Analog to Digital Converter
> +
> +maintainers:
> +  - Chi-Wen Weng <cwweng@nuvoton.com>
> +
> +description: |
> +  The Nuvoton MA35D1 Enhanced Analog to Digital Converter (EADC) is a
> +  12-bit ADC controller integrated in the MA35D1 SoC. Each enabled ADC
> +  input is described by a child channel node.
> +
> +properties:
> +  compatible:
> +    const: nuvoton,ma35d1-eadc
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1

Datasheet says there are 4 interrupts.

> +
> +  clocks:
> +    maxItems: 1

Should there be an optional vref-supply for the V_REF pin?

Should there be a dmas property? Datasheet says it supports
PDMA transfer.

> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +patternProperties:
> +  '^channel@[0-8]$':
> +    type: object
> +    $ref: adc.yaml
> +    unevaluatedProperties: false
> +
> +    properties:
> +      reg:
> +        minimum: 0
> +        maximum: 8

I assume 8 is for the internal batter voltage channel? Often, we don't
include fixed internal channels like this in the devicetree since they
are always the same and don't depend on external wiring.

> +
> +      diff-channels:
> +        minItems: 2
> +        maxItems: 2

adc.yaml already specifies minItems and maxItems, so we don't need to repeat it.

> +        items:
> +          minimum: 0
> +          maximum: 8

This (and reg) are uint32, so don't really need minimum: 0.

Also, I assume that 8 is for the internal battery voltage channel, which
wouldn't make sense as part of a differential input.

> +
> +    required:
> +      - reg
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - '#address-cells'
> +  - '#size-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/nuvoton,ma35d1-clk.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        adc@40430000 {
> +            compatible = "nuvoton,ma35d1-eadc";
> +            reg = <0x0 0x40430000 0x0 0x10000>;
> +            interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>;
> +            clocks = <&clk EADC_GATE>;
> +
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            channel@0 {
> +                reg = <0>;
> +            };
> +
> +            channel@1 {
> +                reg = <1>;
> +            };
> +
> +            channel@2 {
> +                reg = <2>;
> +                diff-channels = <2 3>;
> +            };
> +        };
> +    };
> +...

Re: [PATCH 2/2] iio: adc: Add Nuvoton MA35D1 EADC driver

[David Lechner]

On 6/25/26 6:06 AM, Chi-Wen Weng wrote:
> From: Chi-Wen Weng <cwweng@nuvoton.com>
> 
> Add an IIO driver for the Nuvoton MA35D1 Enhanced ADC controller.
> 
> The driver supports direct raw reads and triggered buffered capture. The
> controller end-of-conversion interrupt is exposed as the device trigger
> and is used to push samples into the IIO buffer.
> 
> Channels are described by firmware child nodes and can be configured as
> single-ended or differential inputs. Since the differential enable bit is
> global, mixed single-ended and differential buffered scans are rejected.
> 
> DMA support is intentionally not included in this initial upstream driver;
> conversions are handled through the interrupt-driven path.
> 
> Signed-off-by: Chi-Wen Weng <cwweng@nuvoton.com>
> ---
>  drivers/iio/adc/Kconfig       |  10 +
>  drivers/iio/adc/Makefile      |   1 +
>  drivers/iio/adc/ma35d1_eadc.c | 636 ++++++++++++++++++++++++++++++++++
>  3 files changed, 647 insertions(+)
>  create mode 100644 drivers/iio/adc/ma35d1_eadc.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 1c663c98c6c9..43409999a94b 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -981,6 +981,16 @@ config LTC2497
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called ltc2497.
>  
> +config MA35D1_EADC
> +	tristate "MA35D1 EADC driver"
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
> +	help
> +	  Say yes here to build support for MA35D1 EADC.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called ma35d1.
> +
>  config MAX1027
>  	tristate "Maxim max1027 ADC driver"
>  	depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 707dd708912f..7b9b38688223 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -85,6 +85,7 @@ obj-$(CONFIG_LTC2471) += ltc2471.o
>  obj-$(CONFIG_LTC2485) += ltc2485.o
>  obj-$(CONFIG_LTC2496) += ltc2496.o ltc2497-core.o
>  obj-$(CONFIG_LTC2497) += ltc2497.o ltc2497-core.o
> +obj-$(CONFIG_MA35D1_EADC) += ma35d1_eadc.o
>  obj-$(CONFIG_MAX1027) += max1027.o
>  obj-$(CONFIG_MAX11100) += max11100.o
>  obj-$(CONFIG_MAX1118) += max1118.o
> diff --git a/drivers/iio/adc/ma35d1_eadc.c b/drivers/iio/adc/ma35d1_eadc.c
> new file mode 100644
> index 000000000000..0c075126e139
> --- /dev/null
> +++ b/drivers/iio/adc/ma35d1_eadc.c
> @@ -0,0 +1,636 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Nuvoton MA35D1 EADC driver
> + *
> + * Copyright (c) 2026 Nuvoton Technology Corp.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/bitmap.h>
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm.h>
> +#include <linux/property.h>
> +
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +
> +#define MA35D1_EADC_DAT(n)		(0x00 + (n) * 0x04)
> +#define MA35D1_EADC_CTL			0x50
> +#define MA35D1_EADC_SWTRG		0x54
> +#define MA35D1_EADC_SCTL(n)		(0x80 + (n) * 0x04)
> +#define MA35D1_EADC_INTSRC0		0xd0
> +#define MA35D1_EADC_STATUS2		0xf8
> +#define MA35D1_EADC_SELSMP0		0x140
> +#define MA35D1_EADC_REFADJCTL		0x150
> +
> +#define MA35D1_EADC_CTL_ADCEN		BIT(0)
> +#define MA35D1_EADC_CTL_ADCIEN0		BIT(2)
> +#define MA35D1_EADC_CTL_DIFFEN		BIT(8)
> +
> +#define MA35D1_EADC_SCTL_CHSEL_MASK	GENMASK(3, 0)
> +#define MA35D1_EADC_SCTL_TRGDLY_MASK	GENMASK(15, 8)
> +#define MA35D1_EADC_SCTL_TRGSEL_MASK	GENMASK(21, 16)
> +#define MA35D1_EADC_SCTL_TRGSEL_ADINT0	\
> +	FIELD_PREP(MA35D1_EADC_SCTL_TRGSEL_MASK, 2)
> +
> +#define MA35D1_EADC_DAT_MASK		GENMASK(11, 0)
> +#define MA35D1_EADC_STATUS2_ADIF0	BIT(0)
> +#define MA35D1_EADC_INTSRC0_ADINT0	BIT(0)
> +#define MA35D1_EADC_REFADJCTL_EXT_VREF	BIT(0)
> +
> +#define MA35D1_EADC_MAX_CHANNELS	9
> +#define MA35D1_EADC_MAX_SAMPLE_MODULES	16
> +#define MA35D1_EADC_CHAN_NAME_LEN	16
> +#define MA35D1_EADC_TIMEOUT		msecs_to_jiffies(1000)
> +
> +struct ma35d1_adc {
> +	struct device *dev;

It looks like this is never used, so we can drop it.

> +	void __iomem *regs;
> +	struct clk *clk;
> +	struct completion completion;
> +	/* Protects direct conversions against concurrent register access. */
> +	struct mutex lock;
> +	struct iio_trigger *trig;
> +	unsigned int scan_chancnt;
> +	bool scan_differential;
> +	char chan_name[MA35D1_EADC_MAX_CHANNELS][MA35D1_EADC_CHAN_NAME_LEN];
> +	struct {
> +		u16 channels[MA35D1_EADC_MAX_SAMPLE_MODULES];
> +		aligned_s64 timestamp;
> +	} scan;

Unless the hardware requires all channels to be read at once, we should
use this instead:

	IIO_DECLARE_BUFFER_WITH_TS(u16, scan, MA35D1_EADC_MAX_SAMPLE_MODULES);

It means an array with enough room for MA35D1_EADC_MAX_SAMPLE_MODULES u16
data points plus an aligned timestamp, but doesn't specify where the
timestamp will be as it could be in a different place depending on how
many channels are read.

Also, this is only used in one function, so can just be stack-allocated
in that function (with ` = { };` to zero it) instead of allocating it here.

> +};
> +
> +static inline u32 ma35d1_adc_read(struct ma35d1_adc *adc, u32 reg)
> +{
> +	return readl(adc->regs + reg);
> +}
> +
> +static inline void ma35d1_adc_write(struct ma35d1_adc *adc, u32 reg, u32 val)
> +{
> +	writel(val, adc->regs + reg);
> +}
> +
> +static void ma35d1_adc_rmw(struct ma35d1_adc *adc, u32 reg, u32 mask, u32 val)
> +{
> +	u32 tmp;
> +
> +	tmp = ma35d1_adc_read(adc, reg);
> +	tmp &= ~mask;
> +	tmp |= val;
> +	ma35d1_adc_write(adc, reg, tmp);
> +}

Why not use regmap?

> +
> +static void ma35d1_adc_set_diff(struct ma35d1_adc *adc, bool differential)
> +{
> +	ma35d1_adc_rmw(adc, MA35D1_EADC_CTL, MA35D1_EADC_CTL_DIFFEN,
> +		       differential ? MA35D1_EADC_CTL_DIFFEN : 0);
> +}
> +
> +static void ma35d1_adc_config_sample(struct ma35d1_adc *adc,
> +				     unsigned int sample, unsigned int channel)
> +{
> +	u32 reg = MA35D1_EADC_SCTL(sample);
> +
> +	ma35d1_adc_rmw(adc, reg,
> +		       MA35D1_EADC_SCTL_CHSEL_MASK |
> +		       MA35D1_EADC_SCTL_TRGSEL_MASK,
> +		       FIELD_PREP(MA35D1_EADC_SCTL_CHSEL_MASK, channel) |
> +		       MA35D1_EADC_SCTL_TRGSEL_ADINT0);
> +}
> +
> +static void ma35d1_adc_disable_irq(struct ma35d1_adc *adc)
> +{
> +	ma35d1_adc_rmw(adc, MA35D1_EADC_CTL, MA35D1_EADC_CTL_ADCIEN0, 0);
> +}
> +
> +static void ma35d1_adc_hw_init(struct ma35d1_adc *adc)
> +{
> +	ma35d1_adc_disable_irq(adc);
> +	ma35d1_adc_rmw(adc, MA35D1_EADC_CTL,
> +		       MA35D1_EADC_CTL_ADCEN, MA35D1_EADC_CTL_ADCEN);
> +	ma35d1_adc_write(adc, MA35D1_EADC_STATUS2, MA35D1_EADC_STATUS2_ADIF0);
> +	ma35d1_adc_rmw(adc, MA35D1_EADC_INTSRC0,
> +		       MA35D1_EADC_INTSRC0_ADINT0,
> +		       MA35D1_EADC_INTSRC0_ADINT0);
> +	ma35d1_adc_rmw(adc, MA35D1_EADC_REFADJCTL,
> +		       MA35D1_EADC_REFADJCTL_EXT_VREF,
> +		       MA35D1_EADC_REFADJCTL_EXT_VREF);

As mentioned elsewhere, external reference doesn't make sense unless
we can get the reference voltage from it.

> +	ma35d1_adc_rmw(adc, MA35D1_EADC_SELSMP0, GENMASK(1, 0), 3);
> +}
> +
> +static void ma35d1_adc_hw_disable(void *data)
> +{
> +	struct ma35d1_adc *adc = data;
> +
> +	ma35d1_adc_disable_irq(adc);

ma35d1_adc_hw_init() doesn't enable the IRQ, so this seems a bit unbalanced.

> +	ma35d1_adc_rmw(adc, MA35D1_EADC_CTL, MA35D1_EADC_CTL_ADCEN, 0);
> +}
> +
> +static irqreturn_t ma35d1_adc_isr(int irq, void *data)
> +{
> +	struct iio_dev *indio_dev = data;
> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
> +	u32 status;
> +
> +	status = ma35d1_adc_read(adc, MA35D1_EADC_STATUS2);
> +	if (!(status & MA35D1_EADC_STATUS2_ADIF0))
> +		return IRQ_NONE;
> +
> +	ma35d1_adc_write(adc, MA35D1_EADC_STATUS2, MA35D1_EADC_STATUS2_ADIF0);
> +
> +	if (iio_buffer_enabled(indio_dev)) {
> +		ma35d1_adc_disable_irq(adc);
> +		iio_trigger_poll(adc->trig);
> +	} else {
> +		complete(&adc->completion);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t ma35d1_adc_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
> +	int i;
> +
> +	for (i = 0; i < adc->scan_chancnt; i++)
> +		adc->scan.channels[i] =
> +			ma35d1_adc_read(adc, MA35D1_EADC_DAT(i)) &
> +			MA35D1_EADC_DAT_MASK;
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, &adc->scan, pf->timestamp);
> +	iio_trigger_notify_done(adc->trig);
> +
> +	ma35d1_adc_rmw(adc, MA35D1_EADC_CTL, MA35D1_EADC_CTL_ADCIEN0,
> +		       MA35D1_EADC_CTL_ADCIEN0);
> +	ma35d1_adc_write(adc, MA35D1_EADC_SWTRG, 1);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int ma35d1_adc_read_conversion(struct iio_dev *indio_dev,
> +				      const struct iio_chan_spec *chan,
> +				      int *val)
> +{
> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
> +	long timeout;
> +
> +	reinit_completion(&adc->completion);
> +
> +	ma35d1_adc_write(adc, MA35D1_EADC_STATUS2, MA35D1_EADC_STATUS2_ADIF0);
> +	ma35d1_adc_rmw(adc, MA35D1_EADC_SCTL(0),
> +		       MA35D1_EADC_SCTL_CHSEL_MASK |
> +		       MA35D1_EADC_SCTL_TRGSEL_MASK,
> +		       FIELD_PREP(MA35D1_EADC_SCTL_CHSEL_MASK,
> +				  chan->channel));
> +	ma35d1_adc_set_diff(adc, chan->differential);
> +	ma35d1_adc_rmw(adc, MA35D1_EADC_CTL, MA35D1_EADC_CTL_ADCIEN0,
> +		       MA35D1_EADC_CTL_ADCIEN0);
> +	ma35d1_adc_write(adc, MA35D1_EADC_SWTRG, 1);
> +
> +	timeout = wait_for_completion_interruptible_timeout(&adc->completion,
> +							    MA35D1_EADC_TIMEOUT);
> +	ma35d1_adc_disable_irq(adc);
> +
> +	if (timeout < 0)
> +		return timeout;
> +	if (!timeout)
> +		return -ETIMEDOUT;
> +
> +	*val = ma35d1_adc_read(adc, MA35D1_EADC_DAT(0)) & MA35D1_EADC_DAT_MASK;
> +
> +	return 0;
> +}
> +
> +static int ma35d1_adc_read_raw(struct iio_dev *indio_dev,
> +			       const struct iio_chan_spec *chan,
> +			       int *val, int *val2, long mask)
> +{
> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (!iio_device_claim_direct(indio_dev))
> +			return -EBUSY;
> +
> +		mutex_lock(&adc->lock);
> +		ret = ma35d1_adc_read_conversion(indio_dev, chan, val);
> +		mutex_unlock(&adc->lock);
> +
> +		iio_device_release_direct(indio_dev);
> +		if (ret)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ma35d1_adc_validate_scan(struct iio_dev *indio_dev,
> +				    const unsigned long *scan_mask)
> +{
> +	const struct iio_chan_spec *chan;
> +	bool have_single = false;
> +	bool have_diff = false;
> +	unsigned int count = 0;
> +	unsigned long bit;
> +
> +	for_each_set_bit(bit, scan_mask, indio_dev->masklength) {
> +		chan = &indio_dev->channels[bit];
> +
> +		if (chan->type == IIO_TIMESTAMP)
> +			continue;

Timestamp channel will never be set (it is handled differently), so we
don't need to check here.

> +		count++;
> +		if (chan->differential)
> +			have_diff = true;
> +		else
> +			have_single = true;
> +	}
> +
> +	if (!count || count > MA35D1_EADC_MAX_SAMPLE_MODULES)
> +		return -EINVAL;
> +
> +	if (have_single && have_diff)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int ma35d1_adc_update_scan_mode(struct iio_dev *indio_dev,
> +				       const unsigned long *scan_mask)
> +{
> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
> +	const struct iio_chan_spec *chan;
> +	unsigned int sample = 0;
> +	unsigned long bit;
> +	bool differential = false;
> +	int ret;
> +
> +	ret = ma35d1_adc_validate_scan(indio_dev, scan_mask);
> +	if (ret)
> +		return ret;
> +
> +	for_each_set_bit(bit, scan_mask, indio_dev->masklength) {
> +		chan = &indio_dev->channels[bit];
> +		if (chan->type == IIO_TIMESTAMP)
> +			continue;

Same here.

> +
> +		if (!sample)
> +			differential = chan->differential;
> +
> +		ma35d1_adc_config_sample(adc, sample, chan->channel);
> +		sample++;
> +	}
> +
> +	adc->scan_chancnt = sample;
> +	adc->scan_differential = differential;
> +
> +	return 0;
> +}
> +
> +static int ma35d1_adc_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
> +	int i;
> +
> +	if (!adc->scan_chancnt)
> +		return -EINVAL;

Should this even be possible because of ma35d1_adc_validate_scan()?

> +
> +	ma35d1_adc_write(adc, MA35D1_EADC_STATUS2, MA35D1_EADC_STATUS2_ADIF0);
> +	ma35d1_adc_rmw(adc, MA35D1_EADC_INTSRC0,
> +		       MA35D1_EADC_INTSRC0_ADINT0,
> +		       MA35D1_EADC_INTSRC0_ADINT0);
> +	ma35d1_adc_rmw(adc, MA35D1_EADC_REFADJCTL,
> +		       MA35D1_EADC_REFADJCTL_EXT_VREF,
> +		       MA35D1_EADC_REFADJCTL_EXT_VREF);

How can we use the external V_REF without knowing what is connected to it?

I would expect this to use internal reference unless the devicetree specified
a vref-supply.

> +	ma35d1_adc_rmw(adc, MA35D1_EADC_SELSMP0, GENMASK(1, 0), 3);

There should be a macro to say what field GENMASK(1, 0) is.

> +	ma35d1_adc_set_diff(adc, adc->scan_differential);
> +
> +	for (i = 0; i < adc->scan_chancnt; i++)
> +		ma35d1_adc_rmw(adc, MA35D1_EADC_SCTL(i),
> +			       MA35D1_EADC_SCTL_TRGDLY_MASK,
> +			       MA35D1_EADC_SCTL_TRGDLY_MASK);
> +
> +	ma35d1_adc_rmw(adc, MA35D1_EADC_CTL, MA35D1_EADC_CTL_ADCIEN0,
> +		       MA35D1_EADC_CTL_ADCIEN0);
> +	ma35d1_adc_write(adc, MA35D1_EADC_SWTRG, 1);
> +
> +	return 0;
> +}
> +
> +static int ma35d1_adc_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
> +	int i;
> +
> +	ma35d1_adc_disable_irq(adc);
> +	for (i = 0; i < adc->scan_chancnt; i++)
> +		ma35d1_adc_rmw(adc, MA35D1_EADC_SCTL(i),
> +			       MA35D1_EADC_SCTL_TRGSEL_MASK, 0);
> +
> +	return 0;
> +}
> +
> +static const struct iio_buffer_setup_ops ma35d1_adc_buffer_ops = {
> +	.postenable = ma35d1_adc_buffer_postenable,
> +	.predisable = ma35d1_adc_buffer_predisable,
> +};
> +
> +static const struct iio_info ma35d1_adc_info = {
> +	.read_raw = ma35d1_adc_read_raw,
> +	.update_scan_mode = ma35d1_adc_update_scan_mode,
> +};
> +
> +static const struct iio_trigger_ops ma35d1_adc_trigger_ops = {
> +	.validate_device = iio_trigger_validate_own_device,
> +};
> +
> +static void ma35d1_adc_init_channel(struct ma35d1_adc *adc,
> +				    struct iio_chan_spec *chan, u32 vinp,
> +				    u32 vinn, int scan_index, bool differential)
> +{
> +	char *name = adc->chan_name[vinp];
> +
> +	chan->type = IIO_VOLTAGE;
> +	chan->indexed = 1;
> +	chan->channel = vinp;
> +	chan->address = vinp;

Setting address is reduant if it is always going to be the same as channel.
We can just use channel directly instead. address doesn't appear to be
used anyway.

> +	chan->scan_index = scan_index;
> +	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);

This should also have IIO_CHAN_INFO_SCALE that is based on the reference source.

> +	chan->scan_type.sign = 'u';

This field has a new name:

	chan->scan_type.format = IIO_SCAN_FORMAT_UNSIGNED_INT;

> +	chan->scan_type.realbits = 12;
> +	chan->scan_type.storagebits = 16;
> +	chan->scan_type.endianness = IIO_CPU;
> +
> +	if (differential) {
> +		chan->differential = 1;
> +		chan->channel2 = vinn;
> +		snprintf(name, MA35D1_EADC_CHAN_NAME_LEN, "in%d-in%d", vinp,
> +			 vinn);
> +	} else {
> +		snprintf(name, MA35D1_EADC_CHAN_NAME_LEN, "in%d", vinp);
> +	}
> +
> +	chan->datasheet_name = name;

We usually don't use datasheet_name. It would make more sense to implement
labels and get the label from devicetree.


> +}
> +
> +static int ma35d1_adc_parse_channels(struct iio_dev *indio_dev,
> +				     struct device *dev)
> +{
> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
> +	DECLARE_BITMAP(used_channels, MA35D1_EADC_MAX_CHANNELS);
> +	struct fwnode_handle *child;
> +	struct iio_chan_spec *channels;
> +	int num_channels;
> +	int scan_index = 0;
> +	int ret;
> +
> +	bitmap_zero(used_channels, MA35D1_EADC_MAX_CHANNELS);
> +
> +	num_channels = device_get_child_node_count(dev);
> +	if (!num_channels)
> +		return dev_err_probe(dev, -ENODATA,
> +				     "no ADC channels configured\n");
> +
> +	if (num_channels > MA35D1_EADC_MAX_CHANNELS)
> +		return dev_err_probe(dev, -EINVAL, "too many ADC channels\n");
> +
> +	channels = devm_kcalloc(dev, num_channels + 1, sizeof(*channels),
> +				GFP_KERNEL);
> +	if (!channels)
> +		return -ENOMEM;
> +
> +	device_for_each_child_node(dev, child) {

Can use device_for_each_child_node_scoped() here to avoid needing to put
handle on all return paths.

> +		u32 diff[2];
> +		u32 reg;
> +		bool differential = false;
> +
> +		ret = fwnode_property_read_u32(child, "reg", &reg);
> +		if (ret) {
> +			fwnode_handle_put(child);
> +			return dev_err_probe(dev, ret,
> +					     "missing channel reg property\n");
> +		}
> +
> +		if (reg >= MA35D1_EADC_MAX_CHANNELS) {
> +			fwnode_handle_put(child);
> +			return dev_err_probe(dev, -EINVAL,
> +					     "invalid ADC channel %u\n", reg);
> +		}
> +
> +		if (test_and_set_bit(reg, used_channels)) {
> +			fwnode_handle_put(child);
> +			return dev_err_probe(dev, -EINVAL,
> +					     "duplicate ADC channel %u\n", reg);
> +		}
> +
> +		if (fwnode_property_present(child, "diff-channels")) {
> +			ret = fwnode_property_read_u32_array(child,
> +							     "diff-channels",
> +							     diff,
> +							     ARRAY_SIZE(diff));
> +			if (ret) {
> +				fwnode_handle_put(child);
> +				return dev_err_probe(dev, ret,
> +						     "invalid diff-channels for channel %u\n",
> +						     reg);
> +			}
> +
> +			if (diff[0] != reg ||
> +			    diff[1] >= MA35D1_EADC_MAX_CHANNELS ||
> +			    diff[0] == diff[1]) {
> +				fwnode_handle_put(child);
> +				return dev_err_probe(dev, -EINVAL,
> +						     "invalid differential ADC channel %u-%u\n",
> +						     diff[0], diff[1]);
> +			}
> +
> +			if (test_and_set_bit(diff[1], used_channels)) {
> +				fwnode_handle_put(child);
> +				return dev_err_probe(dev, -EINVAL,
> +						     "ADC channel %u already used\n",
> +						     diff[1]);
> +			}
> +
> +			differential = true;
> +		}
> +
> +		ma35d1_adc_init_channel(adc, &channels[scan_index], reg,
> +					differential ? diff[1] : 0,
> +					scan_index, differential);
> +		scan_index++;
> +	}
> +
> +	channels[scan_index] = (struct iio_chan_spec)

The macro already includes (struct iio_chan_spec) so we don't need it here.

> +		IIO_CHAN_SOFT_TIMESTAMP(scan_index);
> +
> +	indio_dev->channels = channels;
> +	indio_dev->num_channels = scan_index + 1;
> +	indio_dev->masklength = indio_dev->num_channels;
> +
> +	return 0;
> +}
> +
> +static int ma35d1_adc_setup_trigger(struct iio_dev *indio_dev,
> +				    struct device *dev)
> +{
> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
> +	int ret;
> +
> +	adc->trig = devm_iio_trigger_alloc(dev, "%s-trigger", dev_name(dev));
> +	if (!adc->trig)
> +		return -ENOMEM;
> +
> +	adc->trig->ops = &ma35d1_adc_trigger_ops;
> +	iio_trigger_set_drvdata(adc->trig, indio_dev);
> +
> +	ret = devm_iio_trigger_register(dev, adc->trig);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to register trigger\n");
> +
> +	ret = iio_trigger_set_immutable(indio_dev, adc->trig);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to set trigger\n");
> +
> +	return 0;
> +}
> +
> +static int ma35d1_adc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct iio_dev *indio_dev;
> +	struct ma35d1_adc *adc;
> +	int irq;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*adc));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +	adc = iio_priv(indio_dev);
> +	adc->dev = dev;
> +	mutex_init(&adc->lock);

	devm_mutex_init()

> +	init_completion(&adc->completion);
> +
> +	adc->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(adc->regs))
> +		return dev_err_probe(dev, PTR_ERR(adc->regs),
> +				     "failed to map registers\n");
> +
> +	adc->clk = devm_clk_get_enabled(dev, NULL);
> +	if (IS_ERR(adc->clk))
> +		return dev_err_probe(dev, PTR_ERR(adc->clk),
> +				     "failed to get and enable ADC clock\n");
> +
> +	indio_dev->name = "ma35d1-eadc";
> +	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED;

devm_iio_triggered_buffer_setup() sets the INDIO_BUFFER_TRIGGERED
flag, so we don't need it here.

> +	indio_dev->info = &ma35d1_adc_info;
> +
> +	ret = ma35d1_adc_parse_channels(indio_dev, dev);
> +	if (ret)
> +		return ret;
> +
> +	ma35d1_adc_hw_init(adc);
> +
> +	ret = devm_add_action_or_reset(dev, ma35d1_adc_hw_disable, adc);
> +	if (ret)
> +		return ret;
> +
> +	ret = ma35d1_adc_setup_trigger(indio_dev, dev);
> +	if (ret)
> +		return ret;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;
> +
> +	ret = devm_request_irq(dev, irq, ma35d1_adc_isr, 0, dev_name(dev),
> +			       indio_dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to request IRQ %d\n", irq);
> +
> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> +					      iio_pollfunc_store_time,
> +					      ma35d1_adc_trigger_handler,
> +					      &ma35d1_adc_buffer_ops);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to setup triggered buffer\n");
> +
> +	platform_set_drvdata(pdev, indio_dev);
> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to register IIO device\n");
> +
> +	return 0;
> +}
> +
> +static int ma35d1_adc_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
> +
> +	if (iio_buffer_enabled(indio_dev))
> +		return -EBUSY;
> +
> +	ma35d1_adc_hw_disable(adc);
> +	clk_disable_unprepare(adc->clk);
> +
> +	return 0;
> +}
> +
> +static int ma35d1_adc_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = clk_prepare_enable(adc->clk);
> +	if (ret)
> +		return ret;
> +
> +	ma35d1_adc_hw_init(adc);
> +
> +	return 0;
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(ma35d1_adc_pm_ops,
> +				ma35d1_adc_suspend, ma35d1_adc_resume);
> +
> +static const struct of_device_id ma35d1_adc_of_match[] = {
> +	{ .compatible = "nuvoton,ma35d1-eadc" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, ma35d1_adc_of_match);
> +
> +static struct platform_driver ma35d1_adc_driver = {
> +	.probe = ma35d1_adc_probe,
> +	.driver = {
> +		.name = "ma35d1-eadc",
> +		.of_match_table = ma35d1_adc_of_match,
> +		.pm = pm_sleep_ptr(&ma35d1_adc_pm_ops),
> +	},
> +};
> +module_platform_driver(ma35d1_adc_driver);
> +
> +MODULE_AUTHOR("Chi-Wen Weng <cwweng@nuvoton.com>");
> +MODULE_DESCRIPTION("Nuvoton MA35D1 EADC driver");
> +MODULE_LICENSE("GPL");

Re: [PATCH 2/2] iio: adc: Add Nuvoton MA35D1 EADC driver

[Chi-Wen Weng]

Hi Andy,

Thanks for the review and the kind words.

I will address the cleanup comments in v2:
- trim the include list and add the missing specific headers such as
   linux/types.h and linux/jiffies.h,
- rename the RMW helper to ma35d1_adc_update(),
- mask the update value in the helper,
- add set/clear bits helpers,
- use loop-local unsigned int indices where applicable,
- use devm_kasprintf() or drop the channel name handling if it is no
   longer needed,
- switch to device_for_each_child_node_scoped(),
- simplify the IRQ and triggered-buffer error paths.

 > > +     if (have_single && have_diff)
 > > +             return -EINVAL;
 >
 > Is it possible IRL?

The EADC differential enable bit is global, so the check was intended to
reject buffered scans containing both single-ended and differential
channels.

However, after looking at the hardware constraints and the other review
comments, I plan to simplify v2 and reduce the initial upstream driver
scope. The v2 driver will focus on direct raw reads for the external
single-ended ADC channels only.

Triggered buffered capture, the device trigger and differential channel
support will be dropped from the initial submission. They can be added
later as follow-up patches once the scan sequencing, trigger model and
differential pair constraints are handled properly.

Thanks,
Chi-Wen

Andy Shevchenko 於 2026/6/26 下午 08:54 寫道:
> On Thu, Jun 25, 2026 at 07:06:38PM +0800, Chi-Wen Weng wrote:
>
>> Add an IIO driver for the Nuvoton MA35D1 Enhanced ADC controller.
>>
>> The driver supports direct raw reads and triggered buffered capture. The
>> controller end-of-conversion interrupt is exposed as the device trigger
>> and is used to push samples into the IIO buffer.
>>
>> Channels are described by firmware child nodes and can be configured as
>> single-ended or differential inputs. Since the differential enable bit is
>> global, mixed single-ended and differential buffered scans are rejected.
>>
>> DMA support is intentionally not included in this initial upstream driver;
>> conversions are handled through the interrupt-driven path.
> Nice written driver, some small issues here and there, and I think in a couple
> of versions it will stabilize and can be accepted.
>
> ...
>
>> +#include <linux/bitfield.h>
>> +#include <linux/bits.h>
> No need, bitmap.h covers this.
>
>> +#include <linux/bitmap.h>
>> +#include <linux/clk.h>
>> +#include <linux/completion.h>
>> +#include <linux/device.h>
> No need, covered by platform_device.h.
>
>> +#include <linux/err.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
> No way this header should be in the mere drivers.
>
>> +#include <linux/module.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/mutex.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm.h>
>> +#include <linux/property.h>
> Also missing some headers, such as types.h.
>
> ...
>
>> +#define MA35D1_EADC_TIMEOUT		msecs_to_jiffies(1000)
> + jiffies.h
>
> ...
>
>> +static inline u32 ma35d1_adc_read(struct ma35d1_adc *adc, u32 reg)
>> +{
>> +	return readl(adc->regs + reg);
>> +}
>> +
>> +static inline void ma35d1_adc_write(struct ma35d1_adc *adc, u32 reg, u32 val)
>> +{
>> +	writel(val, adc->regs + reg);
>> +}
>> +
>> +static void ma35d1_adc_rmw(struct ma35d1_adc *adc, u32 reg, u32 mask, u32 val)
> Name it _update() to be aligned with the _read() and _write() above.
>
>> +{
>> +	u32 tmp;
>> +
>> +	tmp = ma35d1_adc_read(adc, reg);
>> +	tmp &= ~mask;
>> +	tmp |= val;
> Correct pattern is to use
>
> 	tmp = (tmp & ~mask) | (val & mask);
>
>> +	ma35d1_adc_write(adc, reg, tmp);
>> +}
> ...
>
>> +static void ma35d1_adc_config_sample(struct ma35d1_adc *adc,
>> +				     unsigned int sample, unsigned int channel)
>> +{
>> +	u32 reg = MA35D1_EADC_SCTL(sample);
> I don't see the need of this variable, use the value directly.
>
>> +	ma35d1_adc_rmw(adc, reg,
>> +		       MA35D1_EADC_SCTL_CHSEL_MASK |
>> +		       MA35D1_EADC_SCTL_TRGSEL_MASK,
>> +		       FIELD_PREP(MA35D1_EADC_SCTL_CHSEL_MASK, channel) |
>> +		       MA35D1_EADC_SCTL_TRGSEL_ADINT0);
>> +}
> ...
>
>> +static irqreturn_t ma35d1_adc_trigger_handler(int irq, void *p)
>> +{
>> +	struct iio_poll_func *pf = p;
>> +	struct iio_dev *indio_dev = pf->indio_dev;
>> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
>> +	int i;
>> +
>> +	for (i = 0; i < adc->scan_chancnt; i++)
> 	for (unsigned int i = 0; i < adc->scan_chancnt; i++)
>
>> +		adc->scan.channels[i] =
>> +			ma35d1_adc_read(adc, MA35D1_EADC_DAT(i)) &
>> +			MA35D1_EADC_DAT_MASK;
>> +
>> +	iio_push_to_buffers_with_timestamp(indio_dev, &adc->scan, pf->timestamp);
>> +	iio_trigger_notify_done(adc->trig);
>> +
>> +	ma35d1_adc_rmw(adc, MA35D1_EADC_CTL, MA35D1_EADC_CTL_ADCIEN0,
>> +		       MA35D1_EADC_CTL_ADCIEN0);
>> +	ma35d1_adc_write(adc, MA35D1_EADC_SWTRG, 1);
>> +
>> +	return IRQ_HANDLED;
>> +}
> ...
>
>> +static int ma35d1_adc_validate_scan(struct iio_dev *indio_dev,
>> +				    const unsigned long *scan_mask)
>> +{
>> +	const struct iio_chan_spec *chan;
>> +	bool have_single = false;
>> +	bool have_diff = false;
>> +	unsigned int count = 0;
>> +	unsigned long bit;
>> +
>> +	for_each_set_bit(bit, scan_mask, indio_dev->masklength) {
>> +		chan = &indio_dev->channels[bit];
>> +
>> +		if (chan->type == IIO_TIMESTAMP)
>> +			continue;
>> +		count++;
> Make it last in the loop, it will be standard pattern. Otherwise it's hard to
> read and find. Also it's recommended to split assignment and definition
> for better maintenance.
>
> 	unsigned int count;
> 	...
> 	count = 0;
> 	for_each_set_bit(bit, scan_mask, indio_dev->masklength) {
> 		...
> 		count++;
> 	}
>
>> +		if (chan->differential)
>> +			have_diff = true;
>> +		else
>> +			have_single = true;
>> +	}
>> +
>> +	if (!count || count > MA35D1_EADC_MAX_SAMPLE_MODULES)
>> +		return -EINVAL;
>> +	if (have_single && have_diff)
>> +		return -EINVAL;
> Is it possible IRL?
>
>> +	return 0;
>> +}
> ...
>
>> +static int ma35d1_adc_buffer_postenable(struct iio_dev *indio_dev)
>> +{
>> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
>> +	int i;
>> +
>> +	if (!adc->scan_chancnt)
>> +		return -EINVAL;
>> +
>> +	ma35d1_adc_write(adc, MA35D1_EADC_STATUS2, MA35D1_EADC_STATUS2_ADIF0);
>> +	ma35d1_adc_rmw(adc, MA35D1_EADC_INTSRC0,
>> +		       MA35D1_EADC_INTSRC0_ADINT0,
>> +		       MA35D1_EADC_INTSRC0_ADINT0);
>> +	ma35d1_adc_rmw(adc, MA35D1_EADC_REFADJCTL,
>> +		       MA35D1_EADC_REFADJCTL_EXT_VREF,
>> +		       MA35D1_EADC_REFADJCTL_EXT_VREF);
>> +	ma35d1_adc_rmw(adc, MA35D1_EADC_SELSMP0, GENMASK(1, 0), 3);
>> +	ma35d1_adc_set_diff(adc, adc->scan_differential);
>> +	for (i = 0; i < adc->scan_chancnt; i++)
> 	for (unsigned int i = 0; i < adc->scan_chancnt; i++)
>
>> +		ma35d1_adc_rmw(adc, MA35D1_EADC_SCTL(i),
>> +			       MA35D1_EADC_SCTL_TRGDLY_MASK,
>> +			       MA35D1_EADC_SCTL_TRGDLY_MASK);
>> +
>> +	ma35d1_adc_rmw(adc, MA35D1_EADC_CTL, MA35D1_EADC_CTL_ADCIEN0,
>> +		       MA35D1_EADC_CTL_ADCIEN0);
>> +	ma35d1_adc_write(adc, MA35D1_EADC_SWTRG, 1);
>> +
>> +	return 0;
>> +}
>> +
>> +static int ma35d1_adc_buffer_predisable(struct iio_dev *indio_dev)
>> +{
>> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
>> +	int i;
>> +
>> +	ma35d1_adc_disable_irq(adc);
>> +	for (i = 0; i < adc->scan_chancnt; i++)
>> +		ma35d1_adc_rmw(adc, MA35D1_EADC_SCTL(i),
>> +			       MA35D1_EADC_SCTL_TRGSEL_MASK, 0);
> Ditto.
>
> Also looking to the cases of setting 0s, I would rather have a helper
> _set_bits() / _clear_bits() in conjunction with _update().
>
>> +	return 0;
>> +}
> ...
>
>> +static void ma35d1_adc_init_channel(struct ma35d1_adc *adc,
>> +				    struct iio_chan_spec *chan, u32 vinp,
>> +				    u32 vinn, int scan_index, bool differential)
>> +{
>> +	char *name = adc->chan_name[vinp];
>> +
>> +	chan->type = IIO_VOLTAGE;
>> +	chan->indexed = 1;
>> +	chan->channel = vinp;
>> +	chan->address = vinp;
>> +	chan->scan_index = scan_index;
>> +	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
>> +	chan->scan_type.sign = 'u';
>> +	chan->scan_type.realbits = 12;
>> +	chan->scan_type.storagebits = 16;
>> +	chan->scan_type.endianness = IIO_CPU;
>> +
>> +	if (differential) {
>> +		chan->differential = 1;
>> +		chan->channel2 = vinn;
>> +		snprintf(name, MA35D1_EADC_CHAN_NAME_LEN, "in%d-in%d", vinp,
>> +			 vinn);
> Can compiler prove the buffer size is enough?
>
>> +	} else {
>> +		snprintf(name, MA35D1_EADC_CHAN_NAME_LEN, "in%d", vinp);
>> +	}
>> +
>> +	chan->datasheet_name = name;
> Why not use devm_kasprintf() instead?
>
>> +}
> ...
>
>> +static int ma35d1_adc_parse_channels(struct iio_dev *indio_dev,
>> +				     struct device *dev)
>> +{
>> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
>> +	DECLARE_BITMAP(used_channels, MA35D1_EADC_MAX_CHANNELS);
>> +	struct fwnode_handle *child;
>> +	struct iio_chan_spec *channels;
>> +	int num_channels;
>> +	int scan_index = 0;
>> +	int ret;
>> +
>> +	bitmap_zero(used_channels, MA35D1_EADC_MAX_CHANNELS);
>> +
>> +	num_channels = device_get_child_node_count(dev);
>> +	if (!num_channels)
>> +		return dev_err_probe(dev, -ENODATA,
>> +				     "no ADC channels configured\n");
>> +
>> +	if (num_channels > MA35D1_EADC_MAX_CHANNELS)
> Perhaps >= ?
>
>> +		return dev_err_probe(dev, -EINVAL, "too many ADC channels\n");
>> +
>> +	channels = devm_kcalloc(dev, num_channels + 1, sizeof(*channels),
>> +				GFP_KERNEL);
>> +	if (!channels)
>> +		return -ENOMEM;
>> +
>> +	device_for_each_child_node(dev, child) {
> Use _scoped() variant.
>
>> +		u32 diff[2];
>> +		u32 reg;
>> +		bool differential = false;
>> +
>> +		ret = fwnode_property_read_u32(child, "reg", &reg);
>> +		if (ret) {
>> +			fwnode_handle_put(child);
>> +			return dev_err_probe(dev, ret,
>> +					     "missing channel reg property\n");
>> +		}
>> +
>> +		if (reg >= MA35D1_EADC_MAX_CHANNELS) {
>> +			fwnode_handle_put(child);
>> +			return dev_err_probe(dev, -EINVAL,
>> +					     "invalid ADC channel %u\n", reg);
>> +		}
>> +
>> +		if (test_and_set_bit(reg, used_channels)) {
>> +			fwnode_handle_put(child);
>> +			return dev_err_probe(dev, -EINVAL,
>> +					     "duplicate ADC channel %u\n", reg);
>> +		}
>> +
>> +		if (fwnode_property_present(child, "diff-channels")) {
>> +			ret = fwnode_property_read_u32_array(child,
>> +							     "diff-channels",
>> +							     diff,
>> +							     ARRAY_SIZE(diff));
>> +			if (ret) {
>> +				fwnode_handle_put(child);
>> +				return dev_err_probe(dev, ret,
>> +						     "invalid diff-channels for channel %u\n",
>> +						     reg);
>> +			}
>> +
>> +			if (diff[0] != reg ||
>> +			    diff[1] >= MA35D1_EADC_MAX_CHANNELS ||
>> +			    diff[0] == diff[1]) {
>> +				fwnode_handle_put(child);
>> +				return dev_err_probe(dev, -EINVAL,
>> +						     "invalid differential ADC channel %u-%u\n",
>> +						     diff[0], diff[1]);
>> +			}
>> +
>> +			if (test_and_set_bit(diff[1], used_channels)) {
>> +				fwnode_handle_put(child);
>> +				return dev_err_probe(dev, -EINVAL,
>> +						     "ADC channel %u already used\n",
>> +						     diff[1]);
>> +			}
>> +
>> +			differential = true;
>> +		}
>> +
>> +		ma35d1_adc_init_channel(adc, &channels[scan_index], reg,
>> +					differential ? diff[1] : 0,
>> +					scan_index, differential);
>> +		scan_index++;
>> +	}
>> +
>> +	channels[scan_index] = (struct iio_chan_spec)
>> +		IIO_CHAN_SOFT_TIMESTAMP(scan_index);
>> +
>> +	indio_dev->channels = channels;
>> +	indio_dev->num_channels = scan_index + 1;
>> +	indio_dev->masklength = indio_dev->num_channels;
>> +
>> +	return 0;
>> +}
> ...
>
>> +	ret = devm_request_irq(dev, irq, ma35d1_adc_isr, 0, dev_name(dev),
>> +			       indio_dev);
> Make it a single line, here it's fine.
>
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "failed to request IRQ %d\n", irq);
> Remove duplicate error message.
>
> ...
>
>> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
>> +					      iio_pollfunc_store_time,
>> +					      ma35d1_adc_trigger_handler,
>> +					      &ma35d1_adc_buffer_ops);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret,
>> +				     "failed to setup triggered buffer\n");
> So, it seems this is very rarely can be not -ENOMEM, and hence it's 99.99% dead
> code, just
>
> 		return ret;
>

Re: [PATCH 1/2] dt-bindings: iio: adc: Add Nuvoton MA35D1 EADC

[Chi-Wen Weng]

Hi David,

Thanks for the review.

 > Datasheet says there are 4 interrupts.

Yes, the controller has four EOC interrupt outputs, ADINT0 to ADINT3.
The initial driver only uses ADINT0, but the hardware does provide four
interrupt lines.

I will update the binding to allow up to four interrupt entries and
document the order as ADINT0, ADINT1, ADINT2 and ADINT3. The example
will keep a single interrupt entry since that is the only one used by
the initial driver.

 > Should there be an optional vref-supply for the V_REF pin?

Yes, I agree. I will add an optional vref-supply property.

I will also update the driver so that it does not force the external
reference path unconditionally. If vref-supply is present, the driver
will enable it and use it to report the ADC scale. Otherwise, the driver
will use the internal reference path.

 > Should there be a dmas property? Datasheet says it supports PDMA 
transfer.

The hardware does support PDMA, but DMA support is intentionally not
included in this initial upstream version. The initial driver will only
support interrupt-driven direct raw reads, and the MA35D1 PDMA provider
is not upstream yet.

I would prefer to leave dmas/dma-names out of the initial binding and
add them later together with DMA support. Please let me know if you
would prefer optional DMA properties to be described now.

 > I assume 8 is for the internal batter voltage channel? Often, we don't
 > include fixed internal channels like this in the devicetree since they
 > are always the same and don't depend on external wiring.

Correct. Channels 0 to 7 are the external ADC input pins, while channel
8 is the internal VBAT input. I will limit the DT child channel nodes to
external channels 0 to 7.

If VBAT support is added later, it can be exposed by the driver as a
fixed internal channel rather than being described by devicetree.

 > adc.yaml already specifies minItems and maxItems, so we don't need to
 > repeat it.

Since I plan to simplify v2 and drop differential channel support from
the initial submission, I will remove diff-channels from the initial
binding.

Differential input support can be added later once the fixed hardware
pair constraints and signed output handling are implemented in the
driver.

 > This (and reg) are uint32, so don't really need minimum: 0.
 >
 > Also, I assume that 8 is for the internal battery voltage channel,
 > which wouldn't make sense as part of a differential input.

Will fix. The v2 binding will restrict child nodes to external channels
0 to 7 and drop the unnecessary minimum: 0.

Thanks,
Chi-Wen


David Lechner 於 2026/6/28 上午 04:05 寫道:
> On 6/25/26 6:06 AM, Chi-Wen Weng wrote:
>> From: Chi-Wen Weng <cwweng@nuvoton.com>
>>
>> Add devicetree binding for the Enhanced ADC controller found on
>> Nuvoton MA35D1 SoCs.
>>
>> The controller has one register region, one interrupt and one functional
>> clock. ADC inputs are described using standard channel child nodes,
>> including optional differential channel pairs.
>>
>> Signed-off-by: Chi-Wen Weng <cwweng@nuvoton.com>
>> ---
>>   .../bindings/iio/adc/nuvoton,ma35d1-eadc.yaml | 100 ++++++++++++++++++
>>   1 file changed, 100 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml b/Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml
>> new file mode 100644
>> index 000000000000..ae7ad0f7689a
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml
>> @@ -0,0 +1,100 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/iio/adc/nuvoton,ma35d1-eadc.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Nuvoton MA35D1 Enhanced Analog to Digital Converter
>> +
>> +maintainers:
>> +  - Chi-Wen Weng <cwweng@nuvoton.com>
>> +
>> +description: |
>> +  The Nuvoton MA35D1 Enhanced Analog to Digital Converter (EADC) is a
>> +  12-bit ADC controller integrated in the MA35D1 SoC. Each enabled ADC
>> +  input is described by a child channel node.
>> +
>> +properties:
>> +  compatible:
>> +    const: nuvoton,ma35d1-eadc
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
> Datasheet says there are 4 interrupts.
>
>> +
>> +  clocks:
>> +    maxItems: 1
> Should there be an optional vref-supply for the V_REF pin?
>
> Should there be a dmas property? Datasheet says it supports
> PDMA transfer.
>
>> +
>> +  '#address-cells':
>> +    const: 1
>> +
>> +  '#size-cells':
>> +    const: 0
>> +
>> +patternProperties:
>> +  '^channel@[0-8]$':
>> +    type: object
>> +    $ref: adc.yaml
>> +    unevaluatedProperties: false
>> +
>> +    properties:
>> +      reg:
>> +        minimum: 0
>> +        maximum: 8
> I assume 8 is for the internal batter voltage channel? Often, we don't
> include fixed internal channels like this in the devicetree since they
> are always the same and don't depend on external wiring.
>
>> +
>> +      diff-channels:
>> +        minItems: 2
>> +        maxItems: 2
> adc.yaml already specifies minItems and maxItems, so we don't need to repeat it.
>
>> +        items:
>> +          minimum: 0
>> +          maximum: 8
> This (and reg) are uint32, so don't really need minimum: 0.
>
> Also, I assume that 8 is for the internal battery voltage channel, which
> wouldn't make sense as part of a differential input.
>
>> +
>> +    required:
>> +      - reg
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - clocks
>> +  - '#address-cells'
>> +  - '#size-cells'
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/nuvoton,ma35d1-clk.h>
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +
>> +    soc {
>> +        #address-cells = <2>;
>> +        #size-cells = <2>;
>> +
>> +        adc@40430000 {
>> +            compatible = "nuvoton,ma35d1-eadc";
>> +            reg = <0x0 0x40430000 0x0 0x10000>;
>> +            interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>;
>> +            clocks = <&clk EADC_GATE>;
>> +
>> +            #address-cells = <1>;
>> +            #size-cells = <0>;
>> +
>> +            channel@0 {
>> +                reg = <0>;
>> +            };
>> +
>> +            channel@1 {
>> +                reg = <1>;
>> +            };
>> +
>> +            channel@2 {
>> +                reg = <2>;
>> +                diff-channels = <2 3>;
>> +            };
>> +        };
>> +    };
>> +...

Re: [PATCH 2/2] iio: adc: Add Nuvoton MA35D1 EADC driver

[Chi-Wen Weng]

Hi David,

Thanks for the detailed review.

After looking at your comments and the other review feedback, I plan to
simplify v2 and limit the initial upstream driver to direct raw reads for
the external single-ended ADC channels.

In v2, I will drop the triggered buffer support, the device trigger and
the differential channel support for now. Buffered capture and
differential inputs can be added later as follow-up patches once the
scan sequencing, trigger model and differential pair constraints are
handled properly.

This also means that the scan buffer layout comments will no longer
apply to v2, since the triggered-buffer path will be removed from the
initial submission.

I will address the other driver comments in v2:
- drop the unused struct device pointer,
- remove the triggered-buffer and trigger-related Kconfig selects,
- switch the register access helpers to regmap,
- avoid forcing the external reference path unconditionally,
- add optional vref-supply handling,
- add IIO_CHAN_INFO_SCALE based on the selected reference source,
- use the internal reference when no vref-supply is provided,
- add a named macro for the sample-time field,
- drop the unused channel address field,
- drop datasheet_name from the initial driver,
- use device_for_each_child_node_scoped(),
- use devm_mutex_init(),
- keep only INDIO_DIRECT_MODE for the initial driver.

For the firmware-described channels, v2 will only accept the external
ADC input channels 0 to 7. The internal VBAT channel and differential
inputs will not be described or exposed by the initial driver.

Thanks,
Chi-Wen

David Lechner 於 2026/6/28 上午 04:52 寫道:
> On 6/25/26 6:06 AM, Chi-Wen Weng wrote:
>> From: Chi-Wen Weng <cwweng@nuvoton.com>
>>
>> Add an IIO driver for the Nuvoton MA35D1 Enhanced ADC controller.
>>
>> The driver supports direct raw reads and triggered buffered capture. The
>> controller end-of-conversion interrupt is exposed as the device trigger
>> and is used to push samples into the IIO buffer.
>>
>> Channels are described by firmware child nodes and can be configured as
>> single-ended or differential inputs. Since the differential enable bit is
>> global, mixed single-ended and differential buffered scans are rejected.
>>
>> DMA support is intentionally not included in this initial upstream driver;
>> conversions are handled through the interrupt-driven path.
>>
>> Signed-off-by: Chi-Wen Weng <cwweng@nuvoton.com>
>> ---
>>   drivers/iio/adc/Kconfig       |  10 +
>>   drivers/iio/adc/Makefile      |   1 +
>>   drivers/iio/adc/ma35d1_eadc.c | 636 ++++++++++++++++++++++++++++++++++
>>   3 files changed, 647 insertions(+)
>>   create mode 100644 drivers/iio/adc/ma35d1_eadc.c
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 1c663c98c6c9..43409999a94b 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -981,6 +981,16 @@ config LTC2497
>>   	  To compile this driver as a module, choose M here: the module will be
>>   	  called ltc2497.
>>   
>> +config MA35D1_EADC
>> +	tristate "MA35D1 EADC driver"
>> +	select IIO_BUFFER
>> +	select IIO_TRIGGERED_BUFFER
>> +	help
>> +	  Say yes here to build support for MA35D1 EADC.
>> +
>> +	  To compile this driver as a module, choose M here: the module will be
>> +	  called ma35d1.
>> +
>>   config MAX1027
>>   	tristate "Maxim max1027 ADC driver"
>>   	depends on SPI
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index 707dd708912f..7b9b38688223 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -85,6 +85,7 @@ obj-$(CONFIG_LTC2471) += ltc2471.o
>>   obj-$(CONFIG_LTC2485) += ltc2485.o
>>   obj-$(CONFIG_LTC2496) += ltc2496.o ltc2497-core.o
>>   obj-$(CONFIG_LTC2497) += ltc2497.o ltc2497-core.o
>> +obj-$(CONFIG_MA35D1_EADC) += ma35d1_eadc.o
>>   obj-$(CONFIG_MAX1027) += max1027.o
>>   obj-$(CONFIG_MAX11100) += max11100.o
>>   obj-$(CONFIG_MAX1118) += max1118.o
>> diff --git a/drivers/iio/adc/ma35d1_eadc.c b/drivers/iio/adc/ma35d1_eadc.c
>> new file mode 100644
>> index 000000000000..0c075126e139
>> --- /dev/null
>> +++ b/drivers/iio/adc/ma35d1_eadc.c
>> @@ -0,0 +1,636 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Nuvoton MA35D1 EADC driver
>> + *
>> + * Copyright (c) 2026 Nuvoton Technology Corp.
>> + */
>> +
>> +#include <linux/bitfield.h>
>> +#include <linux/bits.h>
>> +#include <linux/bitmap.h>
>> +#include <linux/clk.h>
>> +#include <linux/completion.h>
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/mutex.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm.h>
>> +#include <linux/property.h>
>> +
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/trigger.h>
>> +#include <linux/iio/trigger_consumer.h>
>> +#include <linux/iio/triggered_buffer.h>
>> +
>> +#define MA35D1_EADC_DAT(n)		(0x00 + (n) * 0x04)
>> +#define MA35D1_EADC_CTL			0x50
>> +#define MA35D1_EADC_SWTRG		0x54
>> +#define MA35D1_EADC_SCTL(n)		(0x80 + (n) * 0x04)
>> +#define MA35D1_EADC_INTSRC0		0xd0
>> +#define MA35D1_EADC_STATUS2		0xf8
>> +#define MA35D1_EADC_SELSMP0		0x140
>> +#define MA35D1_EADC_REFADJCTL		0x150
>> +
>> +#define MA35D1_EADC_CTL_ADCEN		BIT(0)
>> +#define MA35D1_EADC_CTL_ADCIEN0		BIT(2)
>> +#define MA35D1_EADC_CTL_DIFFEN		BIT(8)
>> +
>> +#define MA35D1_EADC_SCTL_CHSEL_MASK	GENMASK(3, 0)
>> +#define MA35D1_EADC_SCTL_TRGDLY_MASK	GENMASK(15, 8)
>> +#define MA35D1_EADC_SCTL_TRGSEL_MASK	GENMASK(21, 16)
>> +#define MA35D1_EADC_SCTL_TRGSEL_ADINT0	\
>> +	FIELD_PREP(MA35D1_EADC_SCTL_TRGSEL_MASK, 2)
>> +
>> +#define MA35D1_EADC_DAT_MASK		GENMASK(11, 0)
>> +#define MA35D1_EADC_STATUS2_ADIF0	BIT(0)
>> +#define MA35D1_EADC_INTSRC0_ADINT0	BIT(0)
>> +#define MA35D1_EADC_REFADJCTL_EXT_VREF	BIT(0)
>> +
>> +#define MA35D1_EADC_MAX_CHANNELS	9
>> +#define MA35D1_EADC_MAX_SAMPLE_MODULES	16
>> +#define MA35D1_EADC_CHAN_NAME_LEN	16
>> +#define MA35D1_EADC_TIMEOUT		msecs_to_jiffies(1000)
>> +
>> +struct ma35d1_adc {
>> +	struct device *dev;
> It looks like this is never used, so we can drop it.
>
>> +	void __iomem *regs;
>> +	struct clk *clk;
>> +	struct completion completion;
>> +	/* Protects direct conversions against concurrent register access. */
>> +	struct mutex lock;
>> +	struct iio_trigger *trig;
>> +	unsigned int scan_chancnt;
>> +	bool scan_differential;
>> +	char chan_name[MA35D1_EADC_MAX_CHANNELS][MA35D1_EADC_CHAN_NAME_LEN];
>> +	struct {
>> +		u16 channels[MA35D1_EADC_MAX_SAMPLE_MODULES];
>> +		aligned_s64 timestamp;
>> +	} scan;
> Unless the hardware requires all channels to be read at once, we should
> use this instead:
>
> 	IIO_DECLARE_BUFFER_WITH_TS(u16, scan, MA35D1_EADC_MAX_SAMPLE_MODULES);
>
> It means an array with enough room for MA35D1_EADC_MAX_SAMPLE_MODULES u16
> data points plus an aligned timestamp, but doesn't specify where the
> timestamp will be as it could be in a different place depending on how
> many channels are read.
>
> Also, this is only used in one function, so can just be stack-allocated
> in that function (with ` = { };` to zero it) instead of allocating it here.
>
>> +};
>> +
>> +static inline u32 ma35d1_adc_read(struct ma35d1_adc *adc, u32 reg)
>> +{
>> +	return readl(adc->regs + reg);
>> +}
>> +
>> +static inline void ma35d1_adc_write(struct ma35d1_adc *adc, u32 reg, u32 val)
>> +{
>> +	writel(val, adc->regs + reg);
>> +}
>> +
>> +static void ma35d1_adc_rmw(struct ma35d1_adc *adc, u32 reg, u32 mask, u32 val)
>> +{
>> +	u32 tmp;
>> +
>> +	tmp = ma35d1_adc_read(adc, reg);
>> +	tmp &= ~mask;
>> +	tmp |= val;
>> +	ma35d1_adc_write(adc, reg, tmp);
>> +}
> Why not use regmap?
>
>> +
>> +static void ma35d1_adc_set_diff(struct ma35d1_adc *adc, bool differential)
>> +{
>> +	ma35d1_adc_rmw(adc, MA35D1_EADC_CTL, MA35D1_EADC_CTL_DIFFEN,
>> +		       differential ? MA35D1_EADC_CTL_DIFFEN : 0);
>> +}
>> +
>> +static void ma35d1_adc_config_sample(struct ma35d1_adc *adc,
>> +				     unsigned int sample, unsigned int channel)
>> +{
>> +	u32 reg = MA35D1_EADC_SCTL(sample);
>> +
>> +	ma35d1_adc_rmw(adc, reg,
>> +		       MA35D1_EADC_SCTL_CHSEL_MASK |
>> +		       MA35D1_EADC_SCTL_TRGSEL_MASK,
>> +		       FIELD_PREP(MA35D1_EADC_SCTL_CHSEL_MASK, channel) |
>> +		       MA35D1_EADC_SCTL_TRGSEL_ADINT0);
>> +}
>> +
>> +static void ma35d1_adc_disable_irq(struct ma35d1_adc *adc)
>> +{
>> +	ma35d1_adc_rmw(adc, MA35D1_EADC_CTL, MA35D1_EADC_CTL_ADCIEN0, 0);
>> +}
>> +
>> +static void ma35d1_adc_hw_init(struct ma35d1_adc *adc)
>> +{
>> +	ma35d1_adc_disable_irq(adc);
>> +	ma35d1_adc_rmw(adc, MA35D1_EADC_CTL,
>> +		       MA35D1_EADC_CTL_ADCEN, MA35D1_EADC_CTL_ADCEN);
>> +	ma35d1_adc_write(adc, MA35D1_EADC_STATUS2, MA35D1_EADC_STATUS2_ADIF0);
>> +	ma35d1_adc_rmw(adc, MA35D1_EADC_INTSRC0,
>> +		       MA35D1_EADC_INTSRC0_ADINT0,
>> +		       MA35D1_EADC_INTSRC0_ADINT0);
>> +	ma35d1_adc_rmw(adc, MA35D1_EADC_REFADJCTL,
>> +		       MA35D1_EADC_REFADJCTL_EXT_VREF,
>> +		       MA35D1_EADC_REFADJCTL_EXT_VREF);
> As mentioned elsewhere, external reference doesn't make sense unless
> we can get the reference voltage from it.
>
>> +	ma35d1_adc_rmw(adc, MA35D1_EADC_SELSMP0, GENMASK(1, 0), 3);
>> +}
>> +
>> +static void ma35d1_adc_hw_disable(void *data)
>> +{
>> +	struct ma35d1_adc *adc = data;
>> +
>> +	ma35d1_adc_disable_irq(adc);
> ma35d1_adc_hw_init() doesn't enable the IRQ, so this seems a bit unbalanced.
>
>> +	ma35d1_adc_rmw(adc, MA35D1_EADC_CTL, MA35D1_EADC_CTL_ADCEN, 0);
>> +}
>> +
>> +static irqreturn_t ma35d1_adc_isr(int irq, void *data)
>> +{
>> +	struct iio_dev *indio_dev = data;
>> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
>> +	u32 status;
>> +
>> +	status = ma35d1_adc_read(adc, MA35D1_EADC_STATUS2);
>> +	if (!(status & MA35D1_EADC_STATUS2_ADIF0))
>> +		return IRQ_NONE;
>> +
>> +	ma35d1_adc_write(adc, MA35D1_EADC_STATUS2, MA35D1_EADC_STATUS2_ADIF0);
>> +
>> +	if (iio_buffer_enabled(indio_dev)) {
>> +		ma35d1_adc_disable_irq(adc);
>> +		iio_trigger_poll(adc->trig);
>> +	} else {
>> +		complete(&adc->completion);
>> +	}
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static irqreturn_t ma35d1_adc_trigger_handler(int irq, void *p)
>> +{
>> +	struct iio_poll_func *pf = p;
>> +	struct iio_dev *indio_dev = pf->indio_dev;
>> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
>> +	int i;
>> +
>> +	for (i = 0; i < adc->scan_chancnt; i++)
>> +		adc->scan.channels[i] =
>> +			ma35d1_adc_read(adc, MA35D1_EADC_DAT(i)) &
>> +			MA35D1_EADC_DAT_MASK;
>> +
>> +	iio_push_to_buffers_with_timestamp(indio_dev, &adc->scan, pf->timestamp);
>> +	iio_trigger_notify_done(adc->trig);
>> +
>> +	ma35d1_adc_rmw(adc, MA35D1_EADC_CTL, MA35D1_EADC_CTL_ADCIEN0,
>> +		       MA35D1_EADC_CTL_ADCIEN0);
>> +	ma35d1_adc_write(adc, MA35D1_EADC_SWTRG, 1);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int ma35d1_adc_read_conversion(struct iio_dev *indio_dev,
>> +				      const struct iio_chan_spec *chan,
>> +				      int *val)
>> +{
>> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
>> +	long timeout;
>> +
>> +	reinit_completion(&adc->completion);
>> +
>> +	ma35d1_adc_write(adc, MA35D1_EADC_STATUS2, MA35D1_EADC_STATUS2_ADIF0);
>> +	ma35d1_adc_rmw(adc, MA35D1_EADC_SCTL(0),
>> +		       MA35D1_EADC_SCTL_CHSEL_MASK |
>> +		       MA35D1_EADC_SCTL_TRGSEL_MASK,
>> +		       FIELD_PREP(MA35D1_EADC_SCTL_CHSEL_MASK,
>> +				  chan->channel));
>> +	ma35d1_adc_set_diff(adc, chan->differential);
>> +	ma35d1_adc_rmw(adc, MA35D1_EADC_CTL, MA35D1_EADC_CTL_ADCIEN0,
>> +		       MA35D1_EADC_CTL_ADCIEN0);
>> +	ma35d1_adc_write(adc, MA35D1_EADC_SWTRG, 1);
>> +
>> +	timeout = wait_for_completion_interruptible_timeout(&adc->completion,
>> +							    MA35D1_EADC_TIMEOUT);
>> +	ma35d1_adc_disable_irq(adc);
>> +
>> +	if (timeout < 0)
>> +		return timeout;
>> +	if (!timeout)
>> +		return -ETIMEDOUT;
>> +
>> +	*val = ma35d1_adc_read(adc, MA35D1_EADC_DAT(0)) & MA35D1_EADC_DAT_MASK;
>> +
>> +	return 0;
>> +}
>> +
>> +static int ma35d1_adc_read_raw(struct iio_dev *indio_dev,
>> +			       const struct iio_chan_spec *chan,
>> +			       int *val, int *val2, long mask)
>> +{
>> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		if (!iio_device_claim_direct(indio_dev))
>> +			return -EBUSY;
>> +
>> +		mutex_lock(&adc->lock);
>> +		ret = ma35d1_adc_read_conversion(indio_dev, chan, val);
>> +		mutex_unlock(&adc->lock);
>> +
>> +		iio_device_release_direct(indio_dev);
>> +		if (ret)
>> +			return ret;
>> +
>> +		return IIO_VAL_INT;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static int ma35d1_adc_validate_scan(struct iio_dev *indio_dev,
>> +				    const unsigned long *scan_mask)
>> +{
>> +	const struct iio_chan_spec *chan;
>> +	bool have_single = false;
>> +	bool have_diff = false;
>> +	unsigned int count = 0;
>> +	unsigned long bit;
>> +
>> +	for_each_set_bit(bit, scan_mask, indio_dev->masklength) {
>> +		chan = &indio_dev->channels[bit];
>> +
>> +		if (chan->type == IIO_TIMESTAMP)
>> +			continue;
> Timestamp channel will never be set (it is handled differently), so we
> don't need to check here.
>
>> +		count++;
>> +		if (chan->differential)
>> +			have_diff = true;
>> +		else
>> +			have_single = true;
>> +	}
>> +
>> +	if (!count || count > MA35D1_EADC_MAX_SAMPLE_MODULES)
>> +		return -EINVAL;
>> +
>> +	if (have_single && have_diff)
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>> +static int ma35d1_adc_update_scan_mode(struct iio_dev *indio_dev,
>> +				       const unsigned long *scan_mask)
>> +{
>> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
>> +	const struct iio_chan_spec *chan;
>> +	unsigned int sample = 0;
>> +	unsigned long bit;
>> +	bool differential = false;
>> +	int ret;
>> +
>> +	ret = ma35d1_adc_validate_scan(indio_dev, scan_mask);
>> +	if (ret)
>> +		return ret;
>> +
>> +	for_each_set_bit(bit, scan_mask, indio_dev->masklength) {
>> +		chan = &indio_dev->channels[bit];
>> +		if (chan->type == IIO_TIMESTAMP)
>> +			continue;
> Same here.
>
>> +
>> +		if (!sample)
>> +			differential = chan->differential;
>> +
>> +		ma35d1_adc_config_sample(adc, sample, chan->channel);
>> +		sample++;
>> +	}
>> +
>> +	adc->scan_chancnt = sample;
>> +	adc->scan_differential = differential;
>> +
>> +	return 0;
>> +}
>> +
>> +static int ma35d1_adc_buffer_postenable(struct iio_dev *indio_dev)
>> +{
>> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
>> +	int i;
>> +
>> +	if (!adc->scan_chancnt)
>> +		return -EINVAL;
> Should this even be possible because of ma35d1_adc_validate_scan()?
>
>> +
>> +	ma35d1_adc_write(adc, MA35D1_EADC_STATUS2, MA35D1_EADC_STATUS2_ADIF0);
>> +	ma35d1_adc_rmw(adc, MA35D1_EADC_INTSRC0,
>> +		       MA35D1_EADC_INTSRC0_ADINT0,
>> +		       MA35D1_EADC_INTSRC0_ADINT0);
>> +	ma35d1_adc_rmw(adc, MA35D1_EADC_REFADJCTL,
>> +		       MA35D1_EADC_REFADJCTL_EXT_VREF,
>> +		       MA35D1_EADC_REFADJCTL_EXT_VREF);
> How can we use the external V_REF without knowing what is connected to it?
>
> I would expect this to use internal reference unless the devicetree specified
> a vref-supply.
>
>> +	ma35d1_adc_rmw(adc, MA35D1_EADC_SELSMP0, GENMASK(1, 0), 3);
> There should be a macro to say what field GENMASK(1, 0) is.
>
>> +	ma35d1_adc_set_diff(adc, adc->scan_differential);
>> +
>> +	for (i = 0; i < adc->scan_chancnt; i++)
>> +		ma35d1_adc_rmw(adc, MA35D1_EADC_SCTL(i),
>> +			       MA35D1_EADC_SCTL_TRGDLY_MASK,
>> +			       MA35D1_EADC_SCTL_TRGDLY_MASK);
>> +
>> +	ma35d1_adc_rmw(adc, MA35D1_EADC_CTL, MA35D1_EADC_CTL_ADCIEN0,
>> +		       MA35D1_EADC_CTL_ADCIEN0);
>> +	ma35d1_adc_write(adc, MA35D1_EADC_SWTRG, 1);
>> +
>> +	return 0;
>> +}
>> +
>> +static int ma35d1_adc_buffer_predisable(struct iio_dev *indio_dev)
>> +{
>> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
>> +	int i;
>> +
>> +	ma35d1_adc_disable_irq(adc);
>> +	for (i = 0; i < adc->scan_chancnt; i++)
>> +		ma35d1_adc_rmw(adc, MA35D1_EADC_SCTL(i),
>> +			       MA35D1_EADC_SCTL_TRGSEL_MASK, 0);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct iio_buffer_setup_ops ma35d1_adc_buffer_ops = {
>> +	.postenable = ma35d1_adc_buffer_postenable,
>> +	.predisable = ma35d1_adc_buffer_predisable,
>> +};
>> +
>> +static const struct iio_info ma35d1_adc_info = {
>> +	.read_raw = ma35d1_adc_read_raw,
>> +	.update_scan_mode = ma35d1_adc_update_scan_mode,
>> +};
>> +
>> +static const struct iio_trigger_ops ma35d1_adc_trigger_ops = {
>> +	.validate_device = iio_trigger_validate_own_device,
>> +};
>> +
>> +static void ma35d1_adc_init_channel(struct ma35d1_adc *adc,
>> +				    struct iio_chan_spec *chan, u32 vinp,
>> +				    u32 vinn, int scan_index, bool differential)
>> +{
>> +	char *name = adc->chan_name[vinp];
>> +
>> +	chan->type = IIO_VOLTAGE;
>> +	chan->indexed = 1;
>> +	chan->channel = vinp;
>> +	chan->address = vinp;
> Setting address is reduant if it is always going to be the same as channel.
> We can just use channel directly instead. address doesn't appear to be
> used anyway.
>
>> +	chan->scan_index = scan_index;
>> +	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> This should also have IIO_CHAN_INFO_SCALE that is based on the reference source.
>
>> +	chan->scan_type.sign = 'u';
> This field has a new name:
>
> 	chan->scan_type.format = IIO_SCAN_FORMAT_UNSIGNED_INT;
>
>> +	chan->scan_type.realbits = 12;
>> +	chan->scan_type.storagebits = 16;
>> +	chan->scan_type.endianness = IIO_CPU;
>> +
>> +	if (differential) {
>> +		chan->differential = 1;
>> +		chan->channel2 = vinn;
>> +		snprintf(name, MA35D1_EADC_CHAN_NAME_LEN, "in%d-in%d", vinp,
>> +			 vinn);
>> +	} else {
>> +		snprintf(name, MA35D1_EADC_CHAN_NAME_LEN, "in%d", vinp);
>> +	}
>> +
>> +	chan->datasheet_name = name;
> We usually don't use datasheet_name. It would make more sense to implement
> labels and get the label from devicetree.
>
>
>> +}
>> +
>> +static int ma35d1_adc_parse_channels(struct iio_dev *indio_dev,
>> +				     struct device *dev)
>> +{
>> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
>> +	DECLARE_BITMAP(used_channels, MA35D1_EADC_MAX_CHANNELS);
>> +	struct fwnode_handle *child;
>> +	struct iio_chan_spec *channels;
>> +	int num_channels;
>> +	int scan_index = 0;
>> +	int ret;
>> +
>> +	bitmap_zero(used_channels, MA35D1_EADC_MAX_CHANNELS);
>> +
>> +	num_channels = device_get_child_node_count(dev);
>> +	if (!num_channels)
>> +		return dev_err_probe(dev, -ENODATA,
>> +				     "no ADC channels configured\n");
>> +
>> +	if (num_channels > MA35D1_EADC_MAX_CHANNELS)
>> +		return dev_err_probe(dev, -EINVAL, "too many ADC channels\n");
>> +
>> +	channels = devm_kcalloc(dev, num_channels + 1, sizeof(*channels),
>> +				GFP_KERNEL);
>> +	if (!channels)
>> +		return -ENOMEM;
>> +
>> +	device_for_each_child_node(dev, child) {
> Can use device_for_each_child_node_scoped() here to avoid needing to put
> handle on all return paths.
>
>> +		u32 diff[2];
>> +		u32 reg;
>> +		bool differential = false;
>> +
>> +		ret = fwnode_property_read_u32(child, "reg", &reg);
>> +		if (ret) {
>> +			fwnode_handle_put(child);
>> +			return dev_err_probe(dev, ret,
>> +					     "missing channel reg property\n");
>> +		}
>> +
>> +		if (reg >= MA35D1_EADC_MAX_CHANNELS) {
>> +			fwnode_handle_put(child);
>> +			return dev_err_probe(dev, -EINVAL,
>> +					     "invalid ADC channel %u\n", reg);
>> +		}
>> +
>> +		if (test_and_set_bit(reg, used_channels)) {
>> +			fwnode_handle_put(child);
>> +			return dev_err_probe(dev, -EINVAL,
>> +					     "duplicate ADC channel %u\n", reg);
>> +		}
>> +
>> +		if (fwnode_property_present(child, "diff-channels")) {
>> +			ret = fwnode_property_read_u32_array(child,
>> +							     "diff-channels",
>> +							     diff,
>> +							     ARRAY_SIZE(diff));
>> +			if (ret) {
>> +				fwnode_handle_put(child);
>> +				return dev_err_probe(dev, ret,
>> +						     "invalid diff-channels for channel %u\n",
>> +						     reg);
>> +			}
>> +
>> +			if (diff[0] != reg ||
>> +			    diff[1] >= MA35D1_EADC_MAX_CHANNELS ||
>> +			    diff[0] == diff[1]) {
>> +				fwnode_handle_put(child);
>> +				return dev_err_probe(dev, -EINVAL,
>> +						     "invalid differential ADC channel %u-%u\n",
>> +						     diff[0], diff[1]);
>> +			}
>> +
>> +			if (test_and_set_bit(diff[1], used_channels)) {
>> +				fwnode_handle_put(child);
>> +				return dev_err_probe(dev, -EINVAL,
>> +						     "ADC channel %u already used\n",
>> +						     diff[1]);
>> +			}
>> +
>> +			differential = true;
>> +		}
>> +
>> +		ma35d1_adc_init_channel(adc, &channels[scan_index], reg,
>> +					differential ? diff[1] : 0,
>> +					scan_index, differential);
>> +		scan_index++;
>> +	}
>> +
>> +	channels[scan_index] = (struct iio_chan_spec)
> The macro already includes (struct iio_chan_spec) so we don't need it here.
>
>> +		IIO_CHAN_SOFT_TIMESTAMP(scan_index);
>> +
>> +	indio_dev->channels = channels;
>> +	indio_dev->num_channels = scan_index + 1;
>> +	indio_dev->masklength = indio_dev->num_channels;
>> +
>> +	return 0;
>> +}
>> +
>> +static int ma35d1_adc_setup_trigger(struct iio_dev *indio_dev,
>> +				    struct device *dev)
>> +{
>> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	adc->trig = devm_iio_trigger_alloc(dev, "%s-trigger", dev_name(dev));
>> +	if (!adc->trig)
>> +		return -ENOMEM;
>> +
>> +	adc->trig->ops = &ma35d1_adc_trigger_ops;
>> +	iio_trigger_set_drvdata(adc->trig, indio_dev);
>> +
>> +	ret = devm_iio_trigger_register(dev, adc->trig);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "failed to register trigger\n");
>> +
>> +	ret = iio_trigger_set_immutable(indio_dev, adc->trig);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "failed to set trigger\n");
>> +
>> +	return 0;
>> +}
>> +
>> +static int ma35d1_adc_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct iio_dev *indio_dev;
>> +	struct ma35d1_adc *adc;
>> +	int irq;
>> +	int ret;
>> +
>> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*adc));
>> +	if (!indio_dev)
>> +		return -ENOMEM;
>> +	adc = iio_priv(indio_dev);
>> +	adc->dev = dev;
>> +	mutex_init(&adc->lock);
> 	devm_mutex_init()
>
>> +	init_completion(&adc->completion);
>> +
>> +	adc->regs = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(adc->regs))
>> +		return dev_err_probe(dev, PTR_ERR(adc->regs),
>> +				     "failed to map registers\n");
>> +
>> +	adc->clk = devm_clk_get_enabled(dev, NULL);
>> +	if (IS_ERR(adc->clk))
>> +		return dev_err_probe(dev, PTR_ERR(adc->clk),
>> +				     "failed to get and enable ADC clock\n");
>> +
>> +	indio_dev->name = "ma35d1-eadc";
>> +	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED;
> devm_iio_triggered_buffer_setup() sets the INDIO_BUFFER_TRIGGERED
> flag, so we don't need it here.
>
>> +	indio_dev->info = &ma35d1_adc_info;
>> +
>> +	ret = ma35d1_adc_parse_channels(indio_dev, dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ma35d1_adc_hw_init(adc);
>> +
>> +	ret = devm_add_action_or_reset(dev, ma35d1_adc_hw_disable, adc);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = ma35d1_adc_setup_trigger(indio_dev, dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	irq = platform_get_irq(pdev, 0);
>> +	if (irq < 0)
>> +		return irq;
>> +
>> +	ret = devm_request_irq(dev, irq, ma35d1_adc_isr, 0, dev_name(dev),
>> +			       indio_dev);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "failed to request IRQ %d\n", irq);
>> +
>> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
>> +					      iio_pollfunc_store_time,
>> +					      ma35d1_adc_trigger_handler,
>> +					      &ma35d1_adc_buffer_ops);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret,
>> +				     "failed to setup triggered buffer\n");
>> +
>> +	platform_set_drvdata(pdev, indio_dev);
>> +
>> +	ret = devm_iio_device_register(dev, indio_dev);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "failed to register IIO device\n");
>> +
>> +	return 0;
>> +}
>> +
>> +static int ma35d1_adc_suspend(struct device *dev)
>> +{
>> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
>> +
>> +	if (iio_buffer_enabled(indio_dev))
>> +		return -EBUSY;
>> +
>> +	ma35d1_adc_hw_disable(adc);
>> +	clk_disable_unprepare(adc->clk);
>> +
>> +	return 0;
>> +}
>> +
>> +static int ma35d1_adc_resume(struct device *dev)
>> +{
>> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	ret = clk_prepare_enable(adc->clk);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ma35d1_adc_hw_init(adc);
>> +
>> +	return 0;
>> +}
>> +
>> +static DEFINE_SIMPLE_DEV_PM_OPS(ma35d1_adc_pm_ops,
>> +				ma35d1_adc_suspend, ma35d1_adc_resume);
>> +
>> +static const struct of_device_id ma35d1_adc_of_match[] = {
>> +	{ .compatible = "nuvoton,ma35d1-eadc" },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, ma35d1_adc_of_match);
>> +
>> +static struct platform_driver ma35d1_adc_driver = {
>> +	.probe = ma35d1_adc_probe,
>> +	.driver = {
>> +		.name = "ma35d1-eadc",
>> +		.of_match_table = ma35d1_adc_of_match,
>> +		.pm = pm_sleep_ptr(&ma35d1_adc_pm_ops),
>> +	},
>> +};
>> +module_platform_driver(ma35d1_adc_driver);
>> +
>> +MODULE_AUTHOR("Chi-Wen Weng <cwweng@nuvoton.com>");
>> +MODULE_DESCRIPTION("Nuvoton MA35D1 EADC driver");
>> +MODULE_LICENSE("GPL");

Re: [PATCH 1/2] dt-bindings: iio: adc: Add Nuvoton MA35D1 EADC

[David Lechner]

On 6/29/26 2:11 AM, Chi-Wen Weng wrote:


>> Should there be a dmas property? Datasheet says it supports PDMA transfer.
> 
> The hardware does support PDMA, but DMA support is intentionally not
> included in this initial upstream version. The initial driver will only
> support interrupt-driven direct raw reads, and the MA35D1 PDMA provider
> is not upstream yet.
> 
> I would prefer to leave dmas/dma-names out of the initial binding and
> add them later together with DMA support. Please let me know if you
> would prefer optional DMA properties to be described now.

We always want the devicetree to be as complete as possible even
if the drier doesn't use all of the information.

So for trivial/well-known bindings like dmas, we should be able to
add it now.

> 
>> I assume 8 is for the internal batter voltage channel? Often, we don't
>> include fixed internal channels like this in the devicetree since they
>> are always the same and don't depend on external wiring.
> 
> Correct. Channels 0 to 7 are the external ADC input pins, while channel
> 8 is the internal VBAT input. I will limit the DT child channel nodes to
> external channels 0 to 7.
> 
> If VBAT support is added later, it can be exposed by the driver as a
> fixed internal channel rather than being described by devicetree.
> 
>> adc.yaml already specifies minItems and maxItems, so we don't need to
>> repeat it.
> 
> Since I plan to simplify v2 and drop differential channel support from
> the initial submission, I will remove diff-channels from the initial
> binding.

Same reasoning as above, we want the binding to be as complete as
possible, so we should not omit diff-channels since we know what
the bindings should look like already.

> 
> Differential input support can be added later once the fixed hardware
> pair constraints and signed output handling are implemented in the
> driver.
> 

Re: [PATCH 2/2] iio: adc: Add Nuvoton MA35D1 EADC driver

[David Lechner]

On 6/29/26 2:32 AM, Chi-Wen Weng wrote:
> Hi David,
> 
> Thanks for the detailed review.
> 
> After looking at your comments and the other review feedback, I plan to
> simplify v2 and limit the initial upstream driver to direct raw reads for
> the external single-ended ADC channels.
> 
> In v2, I will drop the triggered buffer support, the device trigger and
> the differential channel support for now. Buffered capture and
> differential inputs can be added later as follow-up patches once the
> scan sequencing, trigger model and differential pair constraints are
> handled properly.

This is a simple/small enough driver that it would be fine to still
keep all of those features in v2. It would still be fine to split
them into separate patches, but you could still send them all as
a single patch series.

> 
> This also means that the scan buffer layout comments will no longer
> apply to v2, since the triggered-buffer path will be removed from the
> initial submission.
> 
> I will address the other driver comments in v2:
> - drop the unused struct device pointer,
> - remove the triggered-buffer and trigger-related Kconfig selects,
> - switch the register access helpers to regmap,
> - avoid forcing the external reference path unconditionally,
> - add optional vref-supply handling,
> - add IIO_CHAN_INFO_SCALE based on the selected reference source,
> - use the internal reference when no vref-supply is provided,
> - add a named macro for the sample-time field,
> - drop the unused channel address field,
> - drop datasheet_name from the initial driver,
> - use device_for_each_child_node_scoped(),
> - use devm_mutex_init(),
> - keep only INDIO_DIRECT_MODE for the initial driver.
> 
> For the firmware-described channels, v2 will only accept the external
> ADC input channels 0 to 7. The internal VBAT channel and differential
> inputs will not be described or exposed by the initial driver.
> 
> Thanks,
> Chi-Wen
> 
> David Lechner 於 2026/6/28 上午 04:52 寫道:
>> On 6/25/26 6:06 AM, Chi-Wen Weng wrote:
>>> From: Chi-Wen Weng <cwweng@nuvoton.com>
>>>
>>> Add an IIO driver for the Nuvoton MA35D1 Enhanced ADC controller.
>>>
For future reference, please don't top-post, but rather put your
reply inline like this and trim any irrelevant context.