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

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

[Chi-Wen Weng]

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

Add devicetree binding and IIO driver support for the Enhanced
Analog-to-Digital Converter (EADC) found on Nuvoton MA35D1 SoCs.

The MA35D1 EADC is a 12-bit ADC controller with eight external analog
inputs. It supports single-ended conversion and four fixed differential
input pairs: 0-4, 1-5, 2-6 and 3-7.

The controller provides four interrupt outputs, ADINT0 through ADINT3.
ADINT0 through ADINT2 are associated with the corresponding conversion
interrupt flags. ADINT3 is shared by the ADIF3 conversion interrupt and
the ADCMPF0 through ADCMPF3 result-compare interrupt sources.

The initial driver uses sample module 0 and ADINT0 for conversion
completion. It supports direct reads and externally triggered buffered
capture. The driver supports an optional external reference supply and
uses the internal 1.6 V reference when no external reference is
provided.

The binding describes the optional DMA request and all four hardware
interrupt outputs, including the result-compare sources sharing ADINT3.
The initial driver does not use DMA or result-compare functionality.

Changes in v3:
- Document all four ADINT0 through ADINT3 hardware interrupt outputs and
  add interrupt-names.
- Document ADINT3 as the shared conversion and result-compare interrupt
  output.
- Remove the redundant minItems/maxItems constraints from the fixed
  interrupts items list.
- Rework the channel binding for the fixed differential pairs and use
  single-channel for single-ended inputs.
- Add #io-channel-cells so the EADC can be referenced by DT IIO
  consumers, and define the cell as the logical channel index.
- Add firmware-node channel translation so sparse or reordered channel
  child nodes are mapped by their logical channel index.
- Restrict unsupported generic ADC channel properties including bipolar,
  common-mode-channel, settling-time-us and oversampling-ratio.
- Explicitly configure INTPOS for end-of-conversion interrupt
  generation.
- Explicitly configure the ADINT0 sample-module source and clear unused
  sample-module routing.
- Reset the EADC control circuits during hardware initialization and
  clear inherited ADC interrupt enables, result-compare interrupt
  enables and sample-module trigger sources.
- Add timeout recovery using the EADC control-circuit reset, followed by
  IRQ synchronization and stale status/completion cleanup.
- Use iio_push_to_buffers_with_ts() for triggered buffered capture.
- Avoid mixing cleanup guards with goto paths in the trigger handler.
- Add ARCH_MA35 || COMPILE_TEST and HAS_IOMEM Kconfig dependencies.
- Drop redundant linux/bitops.h and linux/mod_devicetable.h includes.
- Use direct regmap helpers, explicit REGCACHE_NONE and
  regmap_assign_bits() where appropriate.
- Use fixed-width register offsets and kernel time/unit helpers.
- Use size_add() for channel array allocation.
- Request ADINT0 by interrupt name.
- Improve system suspend and resume handling.
- Address review comments from Jonathan Cameron, Rob Herring,
  Andy Shevchenko, Uwe Kleine-König and automated review/testing.

Changes in v2:
- Reworked the driver to use regmap.
- Added optional external reference supply support and IIO scale
  reporting.
- Added optional reset handling.
- Added externally triggered buffered capture.
- Added signed differential conversion handling.
- Added fixed differential-pair validation.
- Added system sleep support.
- Added optional DMA properties to the binding.

Patch 1 adds the devicetree binding.
Patch 2 adds the 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 | 175 ++++
 drivers/iio/adc/Kconfig                       |  14 +
 drivers/iio/adc/Makefile                      |   1 +
 drivers/iio/adc/ma35d1_eadc.c                 | 849 ++++++++++++++++++
 4 files changed, 1039 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml
 create mode 100644 drivers/iio/adc/ma35d1_eadc.c


base-commit: d89e1d186ec9271bf78e62771779e9fc03bf0fc3
-- 
2.25.1

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

[Chi-Wen Weng]

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

Add a devicetree binding for the Enhanced Analog-to-Digital Converter
(EADC) found on Nuvoton MA35D1 SoCs.

The controller provides eight external analog inputs which can be used
as single-ended channels or as four fixed differential pairs: 0-4,
1-5, 2-6 and 3-7.

Describe the four hardware interrupt outputs, ADINT0 through ADINT3.
ADINT0 through ADINT2 are associated with the corresponding conversion
interrupt flags, while ADINT3 is shared by the ADIF3 conversion
interrupt and the ADCMPF0 through ADCMPF3 result-compare interrupt
sources.

Also describe the functional clock, optional reset, optional external
reference supply and optional DMA request.

Expose the controller as an IIO channel provider through the
DT #io-channel-cells property. The cell identifies the logical EADC
channel selected by the reg value of the corresponding channel child
node.

Restrict the channel binding to features supported by the MA35D1 EADC
hardware and use the common single-channel representation for
single-ended inputs.

Signed-off-by: Chi-Wen Weng <cwweng@nuvoton.com>
---
 .../bindings/iio/adc/nuvoton,ma35d1-eadc.yaml | 175 ++++++++++++++++++
 1 file changed, 175 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..97144a8eeb5c
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml
@@ -0,0 +1,175 @@
+# 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 successive approximation converter with eight external analog
+  inputs and one internally connected VBAT input.
+
+  The external inputs can be used as eight single-ended channels or as
+  four fixed differential pairs: 0-4, 1-5, 2-6 and 3-7. The internally
+  connected VBAT input is not described by a channel child node.
+
+  The controller provides four interrupt outputs, ADINT0 through ADINT3.
+  ADINT0, ADINT1 and ADINT2 are generated by the corresponding ADIF0,
+  ADIF1 and ADIF2 conversion interrupt flags. ADINT3 is shared by the
+  ADIF3 conversion interrupt flag and the ADCMPF0 through ADCMPF3 result
+  compare flags.
+
+properties:
+  compatible:
+    const: nuvoton,ma35d1-eadc
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    items:
+      - description: ADINT0 conversion interrupt output generated by ADIF0
+      - description: ADINT1 conversion interrupt output generated by ADIF1
+      - description: ADINT2 conversion interrupt output generated by ADIF2
+      - description: >
+          ADINT3 shared interrupt output generated by ADIF3 or by the
+          ADCMPF0 through ADCMPF3 result compare flags
+
+  interrupt-names:
+    items:
+      - const: adint0
+      - const: adint1
+      - const: adint2
+      - const: adint3
+
+  clocks:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+  vref-supply:
+    description:
+      Optional external reference voltage connected to the VREF pin. If this
+      supply is absent, the controller uses its internal 1.6 V reference.
+
+  dmas:
+    maxItems: 1
+
+  dma-names:
+    const: rx
+
+  '#io-channel-cells':
+    const: 1
+    description:
+      The cell selects the logical EADC channel identified by the reg value
+      of the corresponding channel child node.
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+patternProperties:
+  '^channel@[0-7]$':
+    type: object
+    $ref: /schemas/iio/adc/adc.yaml#
+    unevaluatedProperties: false
+    description: |
+      External EADC channel description.
+
+      For a single-ended channel, single-channel specifies the external
+      analog input number (0 through 7), while reg provides the logical IIO
+      channel index.
+
+      For a differential channel, omit single-channel. The reg value selects
+      one of the four fixed hardware differential pairs: reg 0 selects 0-4,
+      reg 1 selects 1-5, reg 2 selects 2-6 and reg 3 selects 3-7.
+
+    properties:
+      reg:
+        maximum: 7
+
+      single-channel:
+        maximum: 7
+
+      # Differential pairs are fixed in hardware and are selected by reg.
+      diff-channels: false
+
+      bipolar: false
+
+      common-mode-channel: false
+
+      settling-time-us: false
+
+      oversampling-ratio: false
+
+    allOf:
+      - if:
+          not:
+            required:
+              - single-channel
+        then:
+          properties:
+            reg:
+              maximum: 3
+
+    required:
+      - reg
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - interrupt-names
+  - clocks
+  - '#io-channel-cells'
+  - '#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>
+    #include <dt-bindings/reset/nuvoton,ma35d1-reset.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>,
+                         <GIC_SPI 66 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>;
+            interrupt-names = "adint0", "adint1", "adint2", "adint3";
+            clocks = <&clk EADC_GATE>;
+            resets = <&reset MA35D1_RESET_EADC0>;
+
+            #io-channel-cells = <1>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            channel@0 {
+                reg = <0>;
+                single-channel = <0>;
+            };
+
+            channel@1 {
+                reg = <1>;
+            };
+        };
+    };
+
+...
-- 
2.25.1

[PATCH v3 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 Enhanced Analog-to-Digital Converter (EADC)
found on Nuvoton MA35D1 SoCs.

The driver supports direct conversion and externally triggered buffered
capture using sample module 0. Both single-ended inputs and the four
fixed differential input pairs are supported.

Use the internal 1.6 V reference when no external reference supply is
provided, and report the reference voltage through the IIO scale
interface.

Use ADINT0 for conversion completion. Configure the interrupt source
and end-of-conversion position explicitly, and recover from conversion
timeouts by resetting the EADC control circuits, synchronizing the IRQ
and clearing stale conversion state before another conversion is
started.

Reset the EADC control circuits during hardware initialization and
sanitize interrupt, compare and sample-module trigger configuration so
the driver does not depend on state left by firmware when the external
reset control is unavailable.

Provide firmware-node channel translation so DT IIO consumers select
channels by the logical channel index described by the channel node's
reg property rather than by the position of the channel in the driver's
channel array.

Add clock, optional reset and system sleep handling, and restore the
controller configuration after resume.

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

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index a802d8ee4dd9..6868b67d852f 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1032,6 +1032,20 @@ config LTC2497
 	  To compile this driver as a module, choose M here: the module will be
 	  called ltc2497.
 
+config MA35D1_EADC
+	tristate "Nuvoton MA35D1 EADC driver"
+	depends on ARCH_MA35 || COMPILE_TEST
+	depends on HAS_IOMEM
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	select REGMAP_MMIO
+	help
+	  Say yes here to build support for the Nuvoton MA35D1
+	  Enhanced Analog to Digital Converter.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called ma35d1_eadc.
+
 config MAX1027
 	tristate "Maxim max1027 ADC driver"
 	depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 9e82c21dbd37..a478fef5d0b7 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -88,6 +88,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..9b6c38a66a66
--- /dev/null
+++ b/drivers/iio/adc/ma35d1_eadc.c
@@ -0,0 +1,849 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Nuvoton MA35D1 EADC driver
+ *
+ * Copyright (c) 2026 Nuvoton Technology Corp.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bitmap.h>
+#include <linux/cleanup.h>
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/jiffies.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/overflow.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/reset.h>
+#include <linux/time.h>
+#include <linux/types.h>
+#include <linux/units.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)		(0x000 + (n) * 0x004)
+#define MA35D1_EADC_CTL			0x050
+#define MA35D1_EADC_SWTRG		0x054
+#define MA35D1_EADC_SCTL(n)		(0x080 + (n) * 0x004)
+#define MA35D1_EADC_INTSRC0		0x0d0
+#define MA35D1_EADC_CMP(n)		(0x0e0 + (n) * 0x004)
+#define MA35D1_EADC_STATUS2		0x0f8
+#define MA35D1_EADC_SELSMP0		0x140
+#define MA35D1_EADC_REFADJCTL		0x150
+
+#define MA35D1_EADC_CTL_ADCEN		BIT(0)
+#define MA35D1_EADC_CTL_EADCRST		BIT(1)
+#define MA35D1_EADC_CTL_ADCIEN0		BIT(2)
+#define MA35D1_EADC_CTL_ADCIEN_MASK	GENMASK(5, 2)
+#define MA35D1_EADC_CTL_DIFFEN		BIT(8)
+#define MA35D1_EADC_CTL_DMOF		BIT(9)
+#define MA35D1_EADC_CTL_VREFSEL_MASK	GENMASK(11, 10)
+#define MA35D1_EADC_CTL_VREFSEL_1V6	FIELD_PREP(MA35D1_EADC_CTL_VREFSEL_MASK, 0)
+
+#define MA35D1_EADC_SCTL_CHSEL_MASK	GENMASK(3, 0)
+#define MA35D1_EADC_SCTL_TRGSEL_MASK	GENMASK(21, 16)
+#define MA35D1_EADC_SCTL_INTPOS		BIT(22)
+
+#define MA35D1_EADC_DAT_RESULT_MASK	GENMASK(15, 0)
+#define MA35D1_EADC_DAT_RESULT_12BIT	GENMASK(11, 0)
+#define MA35D1_EADC_DAT_OV		BIT(16)
+#define MA35D1_EADC_DAT_VALID		BIT(17)
+
+#define MA35D1_EADC_INTSRC0_MASK		GENMASK(8, 0)
+#define MA35D1_EADC_INTSRC0_SPLIEN0	BIT(0)
+#define MA35D1_EADC_STATUS2_ADIF0	BIT(0)
+#define MA35D1_EADC_CMP_EADCMPIE	BIT(1)
+#define MA35D1_EADC_REFADJCTL_PDREF	BIT(0)
+#define MA35D1_EADC_SELSMP0_SMPT0_MASK	GENMASK(1, 0)
+#define MA35D1_EADC_SELSMP_LONG		FIELD_PREP(MA35D1_EADC_SELSMP0_SMPT0_MASK, 3)
+
+#define MA35D1_EADC_MAX_EXT_CHANNELS	8
+#define MA35D1_EADC_NUM_SAMPLE_MODULES	9
+#define MA35D1_EADC_NUM_CMP		4
+#define MA35D1_EADC_INTERNAL_VREF_mV	1600
+#define MA35D1_EADC_TIMEOUT		msecs_to_jiffies(1000)
+#define MA35D1_EADC_RESET_DELAY_US	10
+#define MA35D1_EADC_REF_STABLE_US	(1 * USEC_PER_MSEC)
+
+struct ma35d1_adc {
+	struct regmap *regmap;
+	struct clk *clk;
+	struct reset_control *rst;
+	struct regulator *vref;
+	struct completion completion;
+	/* Protect conversion state and register accesses from PM transitions. */
+	struct mutex lock;
+	const struct iio_chan_spec *scan_chan;
+	unsigned int vref_mV;
+	int irq;
+	bool suspended;
+};
+
+static const struct regmap_config ma35d1_adc_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.max_register = MA35D1_EADC_REFADJCTL,
+	.cache_type = REGCACHE_NONE,
+};
+
+static int ma35d1_adc_disable_irq(struct ma35d1_adc *adc)
+{
+	return regmap_clear_bits(adc->regmap, MA35D1_EADC_CTL, MA35D1_EADC_CTL_ADCIEN0);
+}
+
+static int ma35d1_adc_setup_reference(struct ma35d1_adc *adc)
+{
+	struct regmap *map = adc->regmap;
+	int ret;
+
+	/*
+	 * MA35D1 TRM EADC reference control: PDREF selects the external VREF
+	 * pin. With the internal reference selected, VREFSEL = 0 selects 1.6 V.
+	 */
+	if (adc->vref) {
+		ret = regmap_set_bits(map, MA35D1_EADC_REFADJCTL,
+				      MA35D1_EADC_REFADJCTL_PDREF);
+		if (ret)
+			return ret;
+	} else {
+		ret = regmap_clear_bits(map, MA35D1_EADC_REFADJCTL,
+					MA35D1_EADC_REFADJCTL_PDREF);
+		if (ret)
+			return ret;
+
+		ret = regmap_update_bits(map, MA35D1_EADC_CTL,
+					 MA35D1_EADC_CTL_VREFSEL_MASK,
+					 MA35D1_EADC_CTL_VREFSEL_1V6);
+		if (ret)
+			return ret;
+	}
+
+	fsleep(MA35D1_EADC_REF_STABLE_US);
+
+	return 0;
+}
+
+static int ma35d1_adc_reset_control(struct ma35d1_adc *adc)
+{
+	struct regmap *map = adc->regmap;
+	unsigned int val;
+	int ret;
+
+	ret = regmap_set_bits(map, MA35D1_EADC_CTL, MA35D1_EADC_CTL_EADCRST);
+	if (ret)
+		return ret;
+
+	return regmap_read_poll_timeout(map, MA35D1_EADC_CTL, val,
+					!(val & MA35D1_EADC_CTL_EADCRST),
+					1, USEC_PER_MSEC);
+}
+
+static int ma35d1_adc_hw_init(struct ma35d1_adc *adc)
+{
+	struct regmap *map = adc->regmap;
+	unsigned int i;
+	int ret;
+
+	/*
+	 * EADCRST resets the control circuits but preserves register values.
+	 * Quiesce interrupt sources first so firmware configuration cannot
+	 * generate an interrupt while Linux takes ownership of the block.
+	 */
+	ret = regmap_clear_bits(map, MA35D1_EADC_CTL,
+				MA35D1_EADC_CTL_ADCIEN_MASK);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < MA35D1_EADC_NUM_CMP; i++) {
+		ret = regmap_clear_bits(map, MA35D1_EADC_CMP(i),
+					MA35D1_EADC_CMP_EADCMPIE);
+		if (ret)
+			return ret;
+	}
+
+	ret = ma35d1_adc_reset_control(adc);
+	if (ret)
+		return ret;
+
+	/* Drop trigger and interrupt routing inherited from firmware. */
+	ret = regmap_update_bits(map, MA35D1_EADC_INTSRC0,
+				 MA35D1_EADC_INTSRC0_MASK, 0);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < MA35D1_EADC_NUM_SAMPLE_MODULES; i++) {
+		ret = regmap_clear_bits(map, MA35D1_EADC_SCTL(i),
+					MA35D1_EADC_SCTL_TRGSEL_MASK);
+		if (ret)
+			return ret;
+	}
+
+	ret = ma35d1_adc_setup_reference(adc);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(map, MA35D1_EADC_SELSMP0,
+				 MA35D1_EADC_SELSMP0_SMPT0_MASK,
+				 MA35D1_EADC_SELSMP_LONG);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(map, MA35D1_EADC_STATUS2, MA35D1_EADC_STATUS2_ADIF0);
+	if (ret)
+		return ret;
+
+	return regmap_set_bits(map, MA35D1_EADC_CTL, MA35D1_EADC_CTL_ADCEN);
+}
+
+static int ma35d1_adc_hw_disable(struct ma35d1_adc *adc)
+{
+	struct regmap *map = adc->regmap;
+	int ret;
+
+	ret = ma35d1_adc_disable_irq(adc);
+	if (ret)
+		return ret;
+
+	return regmap_clear_bits(map, MA35D1_EADC_CTL, MA35D1_EADC_CTL_ADCEN);
+}
+
+static void ma35d1_adc_hw_disable_action(void *data)
+{
+	ma35d1_adc_hw_disable(data);
+}
+
+static void ma35d1_adc_vref_disable(void *data)
+{
+	regulator_disable(data);
+}
+
+static int ma35d1_adc_reset(struct ma35d1_adc *adc)
+{
+	int ret;
+
+	if (!adc->rst)
+		return 0;
+
+	ret = reset_control_assert(adc->rst);
+	if (ret)
+		return ret;
+
+	fsleep(MA35D1_EADC_RESET_DELAY_US);
+
+	return reset_control_deassert(adc->rst);
+}
+
+static int ma35d1_adc_config_channel(struct ma35d1_adc *adc,
+				     const struct iio_chan_spec *chan)
+{
+	struct regmap *map = adc->regmap;
+	unsigned int mask;
+	int ret;
+
+	mask = MA35D1_EADC_CTL_DIFFEN | MA35D1_EADC_CTL_DMOF;
+	ret = regmap_assign_bits(map, MA35D1_EADC_CTL, mask, chan->differential);
+	if (ret)
+		return ret;
+
+	/*
+	 * INTPOS = 0 generates ADIF0 at end of conversion. TRGSEL = 0 disables
+	 * the hardware trigger; conversions are started through SWTRG0.
+	 */
+	mask = MA35D1_EADC_SCTL_CHSEL_MASK |
+	       MA35D1_EADC_SCTL_TRGSEL_MASK |
+	       MA35D1_EADC_SCTL_INTPOS;
+	ret = regmap_update_bits(map, MA35D1_EADC_SCTL(0), mask,
+				 FIELD_PREP(MA35D1_EADC_SCTL_CHSEL_MASK,
+					    chan->address));
+	if (ret)
+		return ret;
+
+	return regmap_update_bits(map, MA35D1_EADC_INTSRC0,
+				  MA35D1_EADC_INTSRC0_MASK,
+				  MA35D1_EADC_INTSRC0_SPLIEN0);
+}
+
+static int ma35d1_adc_abort_conversion(struct ma35d1_adc *adc)
+{
+	struct regmap *map = adc->regmap;
+	int ret;
+	int err;
+
+	ret = ma35d1_adc_disable_irq(adc);
+
+	/*
+	 * Reset the EADC control circuits to terminate a conversion which may
+	 * still be in progress. EADCRST is cleared automatically by hardware
+	 * when the reset completes and the EADC register values are preserved.
+	 */
+	err = ma35d1_adc_reset_control(adc);
+	if (!ret)
+		ret = err;
+
+	/* Drain an interrupt which may already have been latched by the GIC. */
+	synchronize_irq(adc->irq);
+
+	err = regmap_write(map, MA35D1_EADC_STATUS2,
+			   MA35D1_EADC_STATUS2_ADIF0);
+	if (!ret)
+		ret = err;
+
+	reinit_completion(&adc->completion);
+
+	return ret;
+}
+
+static int ma35d1_adc_read_conversion(struct ma35d1_adc *adc,
+				      const struct iio_chan_spec *chan, u16 *raw)
+{
+	struct regmap *map = adc->regmap;
+	unsigned int val;
+	long timeout;
+	int ret;
+
+	reinit_completion(&adc->completion);
+
+	ret = regmap_write(map, MA35D1_EADC_STATUS2, MA35D1_EADC_STATUS2_ADIF0);
+	if (ret)
+		return ret;
+
+	ret = ma35d1_adc_config_channel(adc, chan);
+	if (ret)
+		return ret;
+
+	ret = regmap_set_bits(map, MA35D1_EADC_CTL, MA35D1_EADC_CTL_ADCIEN0);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(map, MA35D1_EADC_SWTRG, BIT(0));
+	if (ret)
+		goto disable_irq;
+
+	timeout = wait_for_completion_timeout(&adc->completion,
+					      MA35D1_EADC_TIMEOUT);
+	if (!timeout) {
+		ret = ma35d1_adc_abort_conversion(adc);
+		return ret ?: -ETIMEDOUT;
+	}
+
+	ret = ma35d1_adc_disable_irq(adc);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(map, MA35D1_EADC_DAT(0), &val);
+	if (ret)
+		return ret;
+
+	if (!(val & MA35D1_EADC_DAT_VALID))
+		return -EIO;
+
+	if (val & MA35D1_EADC_DAT_OV)
+		return -EOVERFLOW;
+
+	*raw = FIELD_GET(MA35D1_EADC_DAT_RESULT_MASK, val);
+
+	return 0;
+
+disable_irq:
+	ma35d1_adc_disable_irq(adc);
+
+	return ret;
+}
+
+static irqreturn_t ma35d1_adc_isr(int irq, void *data)
+{
+	struct iio_dev *indio_dev = data;
+	struct ma35d1_adc *adc = iio_priv(indio_dev);
+	unsigned int status;
+	int ret;
+
+	ret = regmap_read(adc->regmap, MA35D1_EADC_STATUS2, &status);
+	if (ret || !(status & MA35D1_EADC_STATUS2_ADIF0))
+		return IRQ_NONE;
+
+	regmap_write(adc->regmap, MA35D1_EADC_STATUS2,
+		     MA35D1_EADC_STATUS2_ADIF0);
+	complete(&adc->completion);
+
+	return IRQ_HANDLED;
+}
+
+static int ma35d1_adc_sign_extend(u16 raw)
+{
+	return sign_extend32(raw & MA35D1_EADC_DAT_RESULT_12BIT, 11);
+}
+
+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);
+	u16 raw;
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW: {
+		IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
+
+		if (IIO_DEV_ACQUIRE_FAILED(claim))
+			return -EBUSY;
+
+		guard(mutex)(&adc->lock);
+		if (adc->suspended)
+			return -EBUSY;
+
+		ret = ma35d1_adc_read_conversion(adc, chan, &raw);
+		if (ret)
+			return ret;
+
+		if (chan->differential)
+			*val = ma35d1_adc_sign_extend(raw);
+		else
+			*val = raw & MA35D1_EADC_DAT_RESULT_12BIT;
+
+		return IIO_VAL_INT;
+	}
+	case IIO_CHAN_INFO_SCALE:
+		*val = adc->vref_mV;
+		/* Differential mode has a VREF / 2048 LSB; single-ended uses /4096. */
+		*val2 = chan->differential ? 11 : 12;
+		return IIO_VAL_FRACTIONAL_LOG2;
+	default:
+		return -EINVAL;
+	}
+}
+
+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 *scan_chan = NULL;
+	unsigned long bit;
+
+	for_each_set_bit(bit, scan_mask, iio_get_masklength(indio_dev)) {
+		const struct iio_chan_spec *chan = &indio_dev->channels[bit];
+
+		if (chan->type == IIO_TIMESTAMP)
+			continue;
+
+		if (chan->type != IIO_VOLTAGE || scan_chan)
+			return -EINVAL;
+
+		scan_chan = chan;
+	}
+
+	if (!scan_chan)
+		return -EINVAL;
+
+	adc->scan_chan = scan_chan;
+
+	return 0;
+}
+
+static int ma35d1_adc_buffer_postenable(struct iio_dev *indio_dev)
+{
+	struct ma35d1_adc *adc = iio_priv(indio_dev);
+
+	guard(mutex)(&adc->lock);
+
+	if (adc->suspended)
+		return -EBUSY;
+
+	return adc->scan_chan ? 0 : -EINVAL;
+}
+
+static int ma35d1_adc_buffer_predisable(struct iio_dev *indio_dev)
+{
+	struct ma35d1_adc *adc = iio_priv(indio_dev);
+	int ret;
+
+	guard(mutex)(&adc->lock);
+
+	ret = ma35d1_adc_disable_irq(adc);
+	if (ret)
+		return ret;
+
+	synchronize_irq(adc->irq);
+
+	return regmap_write(adc->regmap, MA35D1_EADC_STATUS2,
+			    MA35D1_EADC_STATUS2_ADIF0);
+}
+
+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);
+	IIO_DECLARE_BUFFER_WITH_TS(u16, scan, 1) = { };
+	u16 raw;
+	int ret;
+
+	mutex_lock(&adc->lock);
+
+	if (!adc->suspended && adc->scan_chan) {
+		ret = ma35d1_adc_read_conversion(adc, adc->scan_chan, &raw);
+		if (!ret) {
+			scan[0] = raw & MA35D1_EADC_DAT_RESULT_12BIT;
+			iio_push_to_buffers_with_ts(indio_dev, scan, sizeof(scan),
+						    pf->timestamp);
+		}
+	}
+
+	mutex_unlock(&adc->lock);
+
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static const struct iio_buffer_setup_ops ma35d1_adc_buffer_ops = {
+	.postenable = ma35d1_adc_buffer_postenable,
+	.predisable = ma35d1_adc_buffer_predisable,
+};
+
+static int ma35d1_adc_fwnode_xlate(struct iio_dev *indio_dev,
+				   const struct fwnode_reference_args *iiospec)
+{
+	unsigned int i;
+
+	if (iiospec->nargs != 1)
+		return -EINVAL;
+
+	for (i = 0; i < indio_dev->num_channels; i++) {
+		const struct iio_chan_spec *chan = &indio_dev->channels[i];
+
+		if (chan->type == IIO_VOLTAGE &&
+		    chan->channel == iiospec->args[0])
+			return i;
+	}
+
+	return -EINVAL;
+}
+
+static const struct iio_info ma35d1_adc_info = {
+	.read_raw = ma35d1_adc_read_raw,
+	.update_scan_mode = ma35d1_adc_update_scan_mode,
+	.fwnode_xlate = ma35d1_adc_fwnode_xlate,
+};
+
+static void ma35d1_adc_init_channel(struct iio_chan_spec *chan, u32 index,
+				    u32 vinp, u32 vinn,
+				    unsigned int scan_index,
+				    bool differential)
+{
+	chan->type = IIO_VOLTAGE;
+	chan->indexed = 1;
+	chan->channel = index;
+	chan->address = vinp;
+	chan->scan_index = scan_index;
+	chan->info_mask_separate =
+		BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE);
+	chan->scan_type.format = differential ? IIO_SCAN_FORMAT_SIGNED_INT :
+					      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;
+	}
+}
+
+static int ma35d1_adc_parse_channels(struct iio_dev *indio_dev,
+				     struct device *dev)
+{
+	DECLARE_BITMAP(used_regs, MA35D1_EADC_MAX_EXT_CHANNELS);
+	struct iio_chan_spec *channels;
+	unsigned int scan_index;
+	int num_channels;
+	int ret;
+
+	bitmap_zero(used_regs, MA35D1_EADC_MAX_EXT_CHANNELS);
+
+	num_channels = device_get_child_node_count(dev);
+	if (!num_channels)
+		return dev_err_probe(dev, -ENOENT, "no ADC channels configured\n");
+
+	if (num_channels > MA35D1_EADC_MAX_EXT_CHANNELS)
+		return dev_err_probe(dev, -EINVAL, "too many ADC channels\n");
+
+	channels = devm_kcalloc(dev, size_add(num_channels, 1),
+				sizeof(*channels), GFP_KERNEL);
+	if (!channels)
+		return -ENOMEM;
+
+	scan_index = 0;
+	device_for_each_child_node_scoped(dev, child) {
+		bool differential;
+		u32 reg;
+		u32 vinp;
+		u32 vinn;
+
+		ret = fwnode_property_read_u32(child, "reg", &reg);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "missing channel reg property\n");
+
+		if (reg >= MA35D1_EADC_MAX_EXT_CHANNELS)
+			return dev_err_probe(dev, -EINVAL,
+					     "invalid ADC channel index %u\n", reg);
+
+		if (test_and_set_bit(reg, used_regs))
+			return dev_err_probe(dev, -EINVAL,
+					     "duplicate ADC channel index %u\n", reg);
+
+		if (fwnode_property_present(child, "single-channel")) {
+			ret = fwnode_property_read_u32(child, "single-channel",
+						       &vinp);
+			if (ret)
+				return dev_err_probe(dev, ret,
+						     "invalid single-channel for index %u\n",
+						     reg);
+
+			if (vinp >= MA35D1_EADC_MAX_EXT_CHANNELS)
+				return dev_err_probe(dev, -EINVAL,
+						     "invalid ADC input %u\n", vinp);
+
+			vinn = 0;
+			differential = false;
+		} else {
+			if (reg >= 4)
+				return dev_err_probe(dev, -EINVAL,
+						     "invalid differential pair %u\n", reg);
+
+			vinp = reg;
+			vinn = reg + 4;
+			differential = true;
+		}
+
+		ma35d1_adc_init_channel(&channels[scan_index], reg, vinp, vinn,
+					scan_index, differential);
+		scan_index++;
+	}
+
+	channels[scan_index] = IIO_CHAN_SOFT_TIMESTAMP(scan_index);
+
+	indio_dev->channels = channels;
+	indio_dev->num_channels = scan_index + 1;
+
+	return 0;
+}
+
+static int ma35d1_adc_init_vref(struct ma35d1_adc *adc, struct device *dev)
+{
+	int ret;
+
+	adc->vref = devm_regulator_get_optional(dev, "vref");
+	ret = PTR_ERR_OR_ZERO(adc->vref);
+	if (ret == -ENODEV) {
+		adc->vref = NULL;
+		adc->vref_mV = MA35D1_EADC_INTERNAL_VREF_mV;
+		return 0;
+	}
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to get VREF supply\n");
+
+	ret = regulator_enable(adc->vref);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to enable VREF supply\n");
+
+	ret = devm_add_action_or_reset(dev, ma35d1_adc_vref_disable, adc->vref);
+	if (ret)
+		return ret;
+
+	ret = regulator_get_voltage(adc->vref);
+	if (ret <= 0)
+		return dev_err_probe(dev, ret ?: -EINVAL,
+				     "failed to get VREF voltage\n");
+
+	adc->vref_mV = ret / (MICRO / MILLI);
+
+	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;
+	void __iomem *regs;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*adc));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	adc = iio_priv(indio_dev);
+	init_completion(&adc->completion);
+
+	ret = devm_mutex_init(dev, &adc->lock);
+	if (ret)
+		return ret;
+
+	regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(regs))
+		return dev_err_probe(dev, PTR_ERR(regs),
+				     "failed to map registers\n");
+
+	adc->regmap = devm_regmap_init_mmio(dev, regs, &ma35d1_adc_regmap_config);
+	if (IS_ERR(adc->regmap))
+		return dev_err_probe(dev, PTR_ERR(adc->regmap),
+				     "failed to initialize regmap\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");
+
+	adc->rst = devm_reset_control_get_optional_exclusive(dev, NULL);
+	if (IS_ERR(adc->rst))
+		return dev_err_probe(dev, PTR_ERR(adc->rst),
+				     "failed to get reset control\n");
+
+	ret = ma35d1_adc_reset(adc);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to reset ADC\n");
+
+	ret = ma35d1_adc_init_vref(adc, dev);
+	if (ret)
+		return ret;
+
+	indio_dev->name = "ma35d1-eadc";
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &ma35d1_adc_info;
+
+	ret = ma35d1_adc_parse_channels(indio_dev, dev);
+	if (ret)
+		return ret;
+
+	ret = ma35d1_adc_hw_init(adc);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to initialize ADC\n");
+
+	ret = devm_add_action_or_reset(dev, ma35d1_adc_hw_disable_action, adc);
+	if (ret)
+		return ret;
+
+	adc->irq = platform_get_irq_byname(pdev, "adint0");
+	if (adc->irq < 0)
+		return adc->irq;
+
+	ret = devm_request_irq(dev, adc->irq, ma35d1_adc_isr, 0,
+			       dev_name(dev), indio_dev);
+	if (ret)
+		return ret;
+
+	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
+					      iio_pollfunc_store_time,
+					      ma35d1_adc_trigger_handler,
+					      &ma35d1_adc_buffer_ops);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, indio_dev);
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+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);
+	int ret;
+
+	ret = iio_device_suspend_triggering(indio_dev);
+	if (ret)
+		return ret;
+
+	mutex_lock(&adc->lock);
+
+	ret = ma35d1_adc_disable_irq(adc);
+	if (ret)
+		goto resume_triggering;
+
+	synchronize_irq(adc->irq);
+
+	ret = regmap_clear_bits(adc->regmap, MA35D1_EADC_CTL,
+				MA35D1_EADC_CTL_ADCEN);
+	if (ret)
+		goto resume_triggering;
+
+	adc->suspended = true;
+	clk_disable_unprepare(adc->clk);
+	mutex_unlock(&adc->lock);
+
+	return 0;
+
+resume_triggering:
+	mutex_unlock(&adc->lock);
+	iio_device_resume_triggering(indio_dev);
+
+	return ret;
+}
+
+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;
+
+	mutex_lock(&adc->lock);
+
+	ret = clk_prepare_enable(adc->clk);
+	if (ret)
+		goto unlock;
+
+	ret = ma35d1_adc_hw_init(adc);
+	if (ret) {
+		clk_disable_unprepare(adc->clk);
+		goto unlock;
+	}
+
+	adc->suspended = false;
+	mutex_unlock(&adc->lock);
+
+	return iio_device_resume_triggering(indio_dev);
+
+unlock:
+	mutex_unlock(&adc->lock);
+
+	return ret;
+}
+
+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 v3 1/2] dt-bindings: iio: adc: Add Nuvoton MA35D1 EADC

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] dt-bindings: iio: adc: channel schema inverts adc.yaml differential semantics
- [Medium] dt-bindings: iio: adc: incomplete channel property restriction with adc.yaml
--

commit f0823504c40ee953b211fa5196cd4cbe1be6611c
Author: Chi-Wen Weng <cwweng@nuvoton.com>

dt-bindings: iio: adc: Add Nuvoton MA35D1 EADC

This commit adds the devicetree binding schema for the Nuvoton MA35D1
Enhanced Analog-to-Digital Converter (EADC), describing its interrupts,
clocks, resets, reference supply, DMA support, and channel child nodes.

> 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 0000000000000..97144a8eeb5c1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml

[ ... ]

> +patternProperties:
> +  '^channel@[0-7]$':
> +    type: object
> +    $ref: /schemas/iio/adc/adc.yaml#
> +    unevaluatedProperties: false
> +    description: |
> +      External EADC channel description.
> +
> +      For a single-ended channel, single-channel specifies the external
> +      analog input number (0 through 7), while reg provides the logical IIO
> +      channel index.
> +
> +      For a differential channel, omit single-channel. The reg value selects
> +      one of the four fixed hardware differential pairs: reg 0 selects 0-4,
> +      reg 1 selects 1-5, reg 2 selects 2-6 and reg 3 selects 3-7.
> +
> +    properties:
> +      reg:
> +        maximum: 7
> +
> +      single-channel:
> +        maximum: 7
> +
> +      # Differential pairs are fixed in hardware and are selected by reg.
> +      diff-channels: false
> +
> +      bipolar: false
> +
> +      common-mode-channel: false
> +
> +      settling-time-us: false
> +
> +      oversampling-ratio: false

[Severity: Medium]
Does using unevaluatedProperties: false here leave other unsupported
properties from Documentation/devicetree/bindings/iio/adc/adc.yaml permitted
in channel@[0-7] child nodes?

Because adc.yaml is referenced with unevaluatedProperties: false and only
five properties are explicitly set to false above, the remaining properties
defined in adc.yaml (reference-sources, input-chopping, excitation-channels,
excitation-current-nanoamp, and burn-out-current-nanoamp) will still pass
schema validation.

Should patternProperties.'^channel@[0-7]$' use additionalProperties: false
instead and list only the supported properties from adc.yaml?

> +
> +    allOf:
> +      - if:
> +          not:
> +            required:
> +              - single-channel
> +        then:
> +          properties:
> +            reg:
> +              maximum: 3

[Severity: Medium]
Doesn't forbidding diff-channels and using the omission of single-channel to
select differential pairs invert the standard semantics in
Documentation/devicetree/bindings/iio/adc/adc.yaml?

In adc.yaml, properties.single-channel specifies:

  single-channel:
    $ref: /schemas/types.yaml#/definitions/uint32
    description:
      When devices combine single-ended and differential channels, allow the
      channel for a single element to be specified, independent of reg (as for
      differential channels). If this and diff-channels are not present reg
      shall be used instead.

Under adc.yaml, a child node with neither single-channel nor diff-channels
(such as channel@1 with only reg = <1> in the example) represents a
single-ended channel on input reg, whereas this schema treats it as
differential pair 1-5.

Additionally, for single-ended channels, reg (0 through 7) acts as a logical
channel index independent of the input pin selected by single-channel, while
for differential channels reg (0 through 3) is forced to act as both the
child node unit address and the hardware differential pair selector. Because
sibling devicetree nodes cannot share the same reg value, a board that uses
reg = <0> for a single-ended channel on another input (for instance,
channel@0 with reg = <0> and single-channel = <2>) cannot also describe
hardware differential pair 0-4, which requires reg = <0>.

Could differential channels use diff-channels constrained to the four valid
hardware pairs (<0 4>, <1 5>, <2 6>, and <3 7>) so that reg remains purely
the logical channel index?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030031501.130290-1-cwweng.linux@gmail.com?part=1

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

[Jonathan Cameron]

On Fri, 30 Oct 2026 11:15:00 +0800
Chi-Wen Weng <cwweng.linux@gmail.com> wrote:

> From: Chi-Wen Weng <cwweng@nuvoton.com>
> 
> Add a devicetree binding for the Enhanced Analog-to-Digital Converter
> (EADC) found on Nuvoton MA35D1 SoCs.
> 
> The controller provides eight external analog inputs which can be used
> as single-ended channels or as four fixed differential pairs: 0-4,
> 1-5, 2-6 and 3-7.
> 
> Describe the four hardware interrupt outputs, ADINT0 through ADINT3.
> ADINT0 through ADINT2 are associated with the corresponding conversion
> interrupt flags, while ADINT3 is shared by the ADIF3 conversion
> interrupt and the ADCMPF0 through ADCMPF3 result-compare interrupt
> sources.
> 
> Also describe the functional clock, optional reset, optional external
> reference supply and optional DMA request.
> 
> Expose the controller as an IIO channel provider through the
> DT #io-channel-cells property. The cell identifies the logical EADC
> channel selected by the reg value of the corresponding channel child
> node.
> 
> Restrict the channel binding to features supported by the MA35D1 EADC
> hardware and use the common single-channel representation for
> single-ended inputs.
This description seems a little verbose.  Maybe only talk about the
stuff we can't tell from the patch. 

1. Why it needs a new binding and isn't similar enough to existing
   ones for reuse.
2. Any unusual aspects.  Calling out the stuff that I note below on
   the per channel binding oddity more clearly would have been helpful.

For the other stuff the reader can just look at the patch,

Thanks,

Jonathan

> 
> Signed-off-by: Chi-Wen Weng <cwweng@nuvoton.com>
> ---
>  .../bindings/iio/adc/nuvoton,ma35d1-eadc.yaml | 175 ++++++++++++++++++
>  1 file changed, 175 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..97144a8eeb5c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml
> @@ -0,0 +1,175 @@
> +# 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: |

I think you only want to keep paragraphs. In which case > rather than |

> +  The Nuvoton MA35D1 Enhanced Analog-to-Digital Converter (EADC) is a
> +  12-bit successive approximation converter with eight external analog
> +  inputs and one internally connected VBAT input.
> +
> +  The external inputs can be used as eight single-ended channels or as
> +  four fixed differential pairs: 0-4, 1-5, 2-6 and 3-7. The internally
> +  connected VBAT input is not described by a channel child node.

How would you have a consumer of VBAT given the io-channel-cells
comment about using the channel reg values for xlate?


> +
> +  The controller provides four interrupt outputs, ADINT0 through ADINT3.
> +  ADINT0, ADINT1 and ADINT2 are generated by the corresponding ADIF0,
> +  ADIF1 and ADIF2 conversion interrupt flags. ADINT3 is shared by the
> +  ADIF3 conversion interrupt flag and the ADCMPF0 through ADCMPF3 result
> +  compare flags.

> +
> +patternProperties:
> +  '^channel@[0-7]$':
> +    type: object
> +    $ref: /schemas/iio/adc/adc.yaml#
> +    unevaluatedProperties: false
> +    description: |
> +      External EADC channel description.
> +
> +      For a single-ended channel, single-channel specifies the external
> +      analog input number (0 through 7), while reg provides the logical IIO
> +      channel index.
> +
> +      For a differential channel, omit single-channel. The reg value selects
> +      one of the four fixed hardware differential pairs: reg 0 selects 0-4,
> +      reg 1 selects 1-5, reg 2 selects 2-6 and reg 3 selects 3-7.

Sorry, I wasn't paying attention on earlier versions.  This is a very
unintuitive binding particularly as we may want to have a single channel
before a differential one.  Better I think to use the differential
channel bindings and restrict which ones are allowed to this fixed set.


> +
> +    properties:
> +      reg:
> +        maximum: 7
> +
> +      single-channel:
> +        maximum: 7
> +
> +      # Differential pairs are fixed in hardware and are selected by reg.
> +      diff-channels: false
> +
> +      bipolar: false
> +
> +      common-mode-channel: false
> +
> +      settling-time-us: false
> +
> +      oversampling-ratio: false
> +
> +    allOf:
> +      - if:
> +          not:
> +            required:
> +              - single-channel
> +        then:
> +          properties:
> +            reg:
> +              maximum: 3
> +
> +    required:
> +      - reg
> +

> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/nuvoton,ma35d1-clk.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/reset/nuvoton,ma35d1-reset.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>,
> +                         <GIC_SPI 66 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>;
> +            interrupt-names = "adint0", "adint1", "adint2", "adint3";
> +            clocks = <&clk EADC_GATE>;
> +            resets = <&reset MA35D1_RESET_EADC0>;
> +
> +            #io-channel-cells = <1>;
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            channel@0 {
> +                reg = <0>;
> +                single-channel = <0>;
> +            };
> +
> +            channel@1 {
> +                reg = <1>;

This looked very odd and is why I had a closer read of the docs
above.  To make this a reasonable binding to read this
needs to do the full differential channel binding from adc.yaml
with reg just being an index and not specifying channels at all
(same as for the single channel case).

> +            };
> +        };
> +    };
> +
> +...

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

[Jonathan Cameron]

On Fri, 30 Oct 2026 11:15:01 +0800
Chi-Wen Weng <cwweng.linux@gmail.com> wrote:

> From: Chi-Wen Weng <cwweng@nuvoton.com>
> 
> Add an IIO driver for the Enhanced Analog-to-Digital Converter (EADC)
> found on Nuvoton MA35D1 SoCs.
> 
> The driver supports direct conversion and externally triggered buffered
> capture using sample module 0. Both single-ended inputs and the four
> fixed differential input pairs are supported.
> 
> Use the internal 1.6 V reference when no external reference supply is
> provided, and report the reference voltage through the IIO scale
> interface.
> 
> Use ADINT0 for conversion completion. Configure the interrupt source
> and end-of-conversion position explicitly, and recover from conversion
> timeouts by resetting the EADC control circuits, synchronizing the IRQ
> and clearing stale conversion state before another conversion is
> started.
> 
> Reset the EADC control circuits during hardware initialization and
> sanitize interrupt, compare and sample-module trigger configuration so
> the driver does not depend on state left by firmware when the external
> reset control is unavailable.
> 
> Provide firmware-node channel translation so DT IIO consumers select
> channels by the logical channel index described by the channel node's
> reg property rather than by the position of the channel in the driver's
> channel array.
> 
> Add clock, optional reset and system sleep handling, and restore the
> controller configuration after resume.
> 
> Signed-off-by: Chi-Wen Weng <cwweng@nuvoton.com>
Hi Chi-Wen

Various comments inline.

Thanks,

Jonathan

> diff --git a/drivers/iio/adc/ma35d1_eadc.c b/drivers/iio/adc/ma35d1_eadc.c
> new file mode 100644
> index 000000000000..9b6c38a66a66
> --- /dev/null
> +++ b/drivers/iio/adc/ma35d1_eadc.c


> +
> +static int ma35d1_adc_disable_irq(struct ma35d1_adc *adc)
> +{
> +	return regmap_clear_bits(adc->regmap, MA35D1_EADC_CTL, MA35D1_EADC_CTL_ADCIEN0);
> +}
> +
> +static int ma35d1_adc_setup_reference(struct ma35d1_adc *adc)
> +{
> +	struct regmap *map = adc->regmap;
> +	int ret;
> +
> +	/*
> +	 * MA35D1 TRM EADC reference control: PDREF selects the external VREF
> +	 * pin. With the internal reference selected, VREFSEL = 0 selects 1.6 V.
> +	 */
> +	if (adc->vref) {

With change below to use the read_voltage function this would then be
using a flag set there.

> +		ret = regmap_set_bits(map, MA35D1_EADC_REFADJCTL,
> +				      MA35D1_EADC_REFADJCTL_PDREF);
> +		if (ret)
> +			return ret;
> +	} else {
> +		ret = regmap_clear_bits(map, MA35D1_EADC_REFADJCTL,
> +					MA35D1_EADC_REFADJCTL_PDREF);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_update_bits(map, MA35D1_EADC_CTL,
> +					 MA35D1_EADC_CTL_VREFSEL_MASK,
> +					 MA35D1_EADC_CTL_VREFSEL_1V6);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	fsleep(MA35D1_EADC_REF_STABLE_US);
> +
> +	return 0;
> +}


> +static int ma35d1_adc_abort_conversion(struct ma35d1_adc *adc)
> +{
> +	struct regmap *map = adc->regmap;
> +	int ret;
> +	int err;
> +
> +	ret = ma35d1_adc_disable_irq(adc);
> +
> +	/*
> +	 * Reset the EADC control circuits to terminate a conversion which may
> +	 * still be in progress. EADCRST is cleared automatically by hardware
> +	 * when the reset completes and the EADC register values are preserved.
> +	 */
> +	err = ma35d1_adc_reset_control(adc);
> +	if (!ret)
> +		ret = err;
Why not just set ret directly for this one?
> +
> +	/* Drain an interrupt which may already have been latched by the GIC. */
> +	synchronize_irq(adc->irq);
> +
> +	err = regmap_write(map, MA35D1_EADC_STATUS2,
> +			   MA35D1_EADC_STATUS2_ADIF0);
I don't mind going slightly over 80 chars. I would do so for the line above.

> +	if (!ret)
> +		ret = err;
> +
> +	reinit_completion(&adc->completion);
Why?  It is reinit at start of read conversion - why else do we need to 
do it?  Add a comment

> +
> +	return ret;
> +}
> +
> +static int ma35d1_adc_read_conversion(struct ma35d1_adc *adc,
> +				      const struct iio_chan_spec *chan, u16 *raw)
> +{
> +	struct regmap *map = adc->regmap;
> +	unsigned int val;
> +	long timeout;
> +	int ret;
> +
> +	reinit_completion(&adc->completion);
> +
> +	ret = regmap_write(map, MA35D1_EADC_STATUS2, MA35D1_EADC_STATUS2_ADIF0);
> +	if (ret)
> +		return ret;
> +
> +	ret = ma35d1_adc_config_channel(adc, chan);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_set_bits(map, MA35D1_EADC_CTL, MA35D1_EADC_CTL_ADCIEN0);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(map, MA35D1_EADC_SWTRG, BIT(0));
> +	if (ret)
> +		goto disable_irq;
> +
> +	timeout = wait_for_completion_timeout(&adc->completion,
> +					      MA35D1_EADC_TIMEOUT);
> +	if (!timeout) {
> +		ret = ma35d1_adc_abort_conversion(adc);
> +		return ret ?: -ETIMEDOUT;

Timed out first, so I think that should be the return value
even if the abort then failed.  Maybe an error print is
more appropriate for a failed abort so we don't loose
that extra info.

> +	}
> +
> +	ret = ma35d1_adc_disable_irq(adc);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(map, MA35D1_EADC_DAT(0), &val);
> +	if (ret)
> +		return ret;
> +
> +	if (!(val & MA35D1_EADC_DAT_VALID))
> +		return -EIO;
> +
> +	if (val & MA35D1_EADC_DAT_OV)
> +		return -EOVERFLOW;
> +
> +	*raw = FIELD_GET(MA35D1_EADC_DAT_RESULT_MASK, val);

As below - I don't follow why we'd get 16 bits when I think
all paths throw away everything above the 12th.

> +
> +	return 0;
> +
> +disable_irq:
> +	ma35d1_adc_disable_irq(adc);
> +
> +	return ret;
> +}
> +
> +static irqreturn_t ma35d1_adc_isr(int irq, void *data)
> +{
> +	struct iio_dev *indio_dev = data;
> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
> +	unsigned int status;
> +	int ret;
> +
> +	ret = regmap_read(adc->regmap, MA35D1_EADC_STATUS2, &status);
> +	if (ret || !(status & MA35D1_EADC_STATUS2_ADIF0))
> +		return IRQ_NONE;
> +
> +	regmap_write(adc->regmap, MA35D1_EADC_STATUS2,
> +		     MA35D1_EADC_STATUS2_ADIF0);
> +	complete(&adc->completion);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int ma35d1_adc_sign_extend(u16 raw)
> +{
> +	return sign_extend32(raw & MA35D1_EADC_DAT_RESULT_12BIT, 11);

If the result is always 12 bits, why is read_conversion returning 16?

> +}
> +
> +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);
> +	u16 raw;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW: {
> +		IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
> +

No blank line here.  We want the acquire and the error check tightly
coupled visually.

> +		if (IIO_DEV_ACQUIRE_FAILED(claim))
> +			return -EBUSY;
> +
> +		guard(mutex)(&adc->lock);
> +		if (adc->suspended)
> +			return -EBUSY;
> +
> +		ret = ma35d1_adc_read_conversion(adc, chan, &raw);
> +		if (ret)
> +			return ret;
> +
> +		if (chan->differential)
> +			*val = ma35d1_adc_sign_extend(raw);
> +		else
> +			*val = raw & MA35D1_EADC_DAT_RESULT_12BIT;
> +
> +		return IIO_VAL_INT;
> +	}
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = adc->vref_mV;
> +		/* Differential mode has a VREF / 2048 LSB; single-ended uses /4096. */
> +		*val2 = chan->differential ? 11 : 12;
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +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 *scan_chan = NULL;
> +	unsigned long bit;
> +
> +	for_each_set_bit(bit, scan_mask, iio_get_masklength(indio_dev)) {
	iio_for_each_active_channel()

> +		const struct iio_chan_spec *chan = &indio_dev->channels[bit];
> +
> +		if (chan->type == IIO_TIMESTAMP)
> +			continue;
> +
> +		if (chan->type != IIO_VOLTAGE || scan_chan)
> +			return -EINVAL;
> +
> +		scan_chan = chan;

This only makes sense if there is only one channel enabled.
The driver needs to ensure that is the case (there is a standard
validation callback for this).

> +	}
> +
> +	if (!scan_chan)
> +		return -EINVAL;
> +
> +	adc->scan_chan = scan_chan;
> +
> +	return 0;
> +}

> +
> +static irqreturn_t ma35d1_adc_trigger_handler(int irq, void *p)
This looks like the device only supports onehot channels (one at a time)
That's fine but then you should have the callbacks or available_channels
set to indicate that.

> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
> +	IIO_DECLARE_BUFFER_WITH_TS(u16, scan, 1) = { };
Where the timestamp location is fixed (if present) I prefer 
	struct {
		u16 channel;
		aligned_s64 timestamp;
	} scan;

as it makes it easier to understand the layout. The macro
is for the case where different numbers of channels can be enabled
and there are enough of them to have the timestamp moving around
depending on what is enabled.

> +	u16 raw;
> +	int ret;
> +
> +	mutex_lock(&adc->lock);
> +
> +	if (!adc->suspended && adc->scan_chan) {
> +		ret = ma35d1_adc_read_conversion(adc, adc->scan_chan, &raw);
> +		if (!ret) {
> +			scan[0] = raw & MA35D1_EADC_DAT_RESULT_12BIT;
> +			iio_push_to_buffers_with_ts(indio_dev, scan, sizeof(scan),
> +						    pf->timestamp);
> +		}
> +	}
> +
> +	mutex_unlock(&adc->lock);
> +
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}

> +
> +static int ma35d1_adc_fwnode_xlate(struct iio_dev *indio_dev,
> +				   const struct fwnode_reference_args *iiospec)
> +{
> +	unsigned int i;
> +
> +	if (iiospec->nargs != 1)
> +		return -EINVAL;
> +
> +	for (i = 0; i < indio_dev->num_channels; i++) {

	for (unsigned int i = 0; ..

> +		const struct iio_chan_spec *chan = &indio_dev->channels[i];
> +
> +		if (chan->type == IIO_VOLTAGE &&
> +		    chan->channel == iiospec->args[0])
> +			return i;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static const struct iio_info ma35d1_adc_info = {
> +	.read_raw = ma35d1_adc_read_raw,
> +	.update_scan_mode = ma35d1_adc_update_scan_mode,
> +	.fwnode_xlate = ma35d1_adc_fwnode_xlate,
> +};
> +
> +static void ma35d1_adc_init_channel(struct iio_chan_spec *chan, u32 index,

As below.  Given C provides nice ways to set lots of structure elements
helpers use those, rather than a fill the values in helper.

> +				    u32 vinp, u32 vinn,
> +				    unsigned int scan_index,
> +				    bool differential)
> +{
> +	chan->type = IIO_VOLTAGE;
> +	chan->indexed = 1;
> +	chan->channel = index;
> +	chan->address = vinp;
> +	chan->scan_index = scan_index;
> +	chan->info_mask_separate =
> +		BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE);
> +	chan->scan_type.format = differential ? IIO_SCAN_FORMAT_SIGNED_INT :
> +					      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;
> +	}
> +}
> +
> +static int ma35d1_adc_parse_channels(struct iio_dev *indio_dev,
> +				     struct device *dev)
> +{
> +	DECLARE_BITMAP(used_regs, MA35D1_EADC_MAX_EXT_CHANNELS);
> +	struct iio_chan_spec *channels;
> +	unsigned int scan_index;
> +	int num_channels;
> +	int ret;
> +
> +	bitmap_zero(used_regs, MA35D1_EADC_MAX_EXT_CHANNELS);
> +
> +	num_channels = device_get_child_node_count(dev);
> +	if (!num_channels)
> +		return dev_err_probe(dev, -ENOENT, "no ADC channels configured\n");
> +
> +	if (num_channels > MA35D1_EADC_MAX_EXT_CHANNELS)
> +		return dev_err_probe(dev, -EINVAL, "too many ADC channels\n");
> +
> +	channels = devm_kcalloc(dev, size_add(num_channels, 1),
> +				sizeof(*channels), GFP_KERNEL);
> +	if (!channels)
> +		return -ENOMEM;
> +
> +	scan_index = 0;
> +	device_for_each_child_node_scoped(dev, child) {
> +		bool differential;
> +		u32 reg;
> +		u32 vinp;
> +		u32 vinn;

Can combine these related u32s on oneline.

> +
> +		ret = fwnode_property_read_u32(child, "reg", &reg);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "missing channel reg property\n");
> +
> +		if (reg >= MA35D1_EADC_MAX_EXT_CHANNELS)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "invalid ADC channel index %u\n", reg);
> +
> +		if (test_and_set_bit(reg, used_regs))
> +			return dev_err_probe(dev, -EINVAL,
> +					     "duplicate ADC channel index %u\n", reg);
> +
> +		if (fwnode_property_present(child, "single-channel")) {
> +			ret = fwnode_property_read_u32(child, "single-channel",
> +						       &vinp);
> +			if (ret)
> +				return dev_err_probe(dev, ret,
> +						     "invalid single-channel for index %u\n",
> +						     reg);
> +
> +			if (vinp >= MA35D1_EADC_MAX_EXT_CHANNELS)
> +				return dev_err_probe(dev, -EINVAL,
> +						     "invalid ADC input %u\n", vinp);
> +
> +			vinn = 0;
> +			differential = false;
> +		} else {

This will change to match my requests for the binding so I won't look too closely
at it in this revision.

> +			if (reg >= 4)
> +				return dev_err_probe(dev, -EINVAL,
> +						     "invalid differential pair %u\n", reg);
> +
> +			vinp = reg;
> +			vinn = reg + 4;
> +			differential = true;
> +		}
> +
> +		ma35d1_adc_init_channel(&channels[scan_index], reg, vinp, vinn,
> +					scan_index, differential);
get rid of this and fill the structure in directly in the two different
paths above - so have a single ended copy of this and the slightly
different one for the differential channels. Bit of duplication
but simpler code by putting it all in one place.

			channels[scan_index] = (struct iio_chan_spec) {
				.type = IIO_VOLTAGE,
				.indexed = 1,
				.channel = index,
				.address = vinp,
				.scan_index = scan_index,
				.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
						      BIT(IIO_CHAN_INFO_SCALE);
				.scan_type = {
					.format = IIO_SCAN_FORMAT_UNSIGNED_INT,
					.realbits = 12,
					.storagebits = 16,
					.endianness = IIO_CPU,;
			};
		
> +		scan_index++;

> +	}
> +
> +	channels[scan_index] = IIO_CHAN_SOFT_TIMESTAMP(scan_index);

> +
> +	indio_dev->channels = channels;
> +	indio_dev->num_channels = scan_index + 1;
> +
> +	return 0;
> +}
> +
> +static int ma35d1_adc_init_vref(struct ma35d1_adc *adc, struct device *dev)
> +{
> +	int ret;
> +
> +	adc->vref = devm_regulator_get_optional(dev, "vref");
> +	ret = PTR_ERR_OR_ZERO(adc->vref);
> +	if (ret == -ENODEV) {
> +		adc->vref = NULL;
> +		adc->vref_mV = MA35D1_EADC_INTERNAL_VREF_mV;
> +		return 0;
> +	}
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to get VREF supply\n");
> +
> +	ret = regulator_enable(adc->vref);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to enable VREF supply\n");
> +
> +	ret = devm_add_action_or_reset(dev, ma35d1_adc_vref_disable, adc->vref);
> +	if (ret)
> +		return ret;
> +
> +	ret = regulator_get_voltage(adc->vref);

There are plenty of examples of how to use devm_regulator_get_enable_read_voltage()
with optional regulators in tree.  Follow one of those and this code will be a lot
shorter.

> +	if (ret <= 0)
> +		return dev_err_probe(dev, ret ?: -EINVAL,
> +				     "failed to get VREF voltage\n");
> +
> +	adc->vref_mV = ret / (MICRO / MILLI);
> +
> +	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);
> +	int ret;
> +
> +	ret = iio_device_suspend_triggering(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&adc->lock);
> +
> +	ret = ma35d1_adc_disable_irq(adc);
> +	if (ret)
> +		goto resume_triggering;
> +
> +	synchronize_irq(adc->irq);
> +
> +	ret = regmap_clear_bits(adc->regmap, MA35D1_EADC_CTL,
> +				MA35D1_EADC_CTL_ADCEN);
> +	if (ret)

If this fails, for consistency I think you should probably
reenable the irq.  If not add a comment on your thinking for
why.

 
> +		goto resume_triggering;
> +
> +	adc->suspended = true;
> +	clk_disable_unprepare(adc->clk);
> +	mutex_unlock(&adc->lock);
> +
> +	return 0;
> +
> +resume_triggering:
> +	mutex_unlock(&adc->lock);
> +	iio_device_resume_triggering(indio_dev);
> +
> +	return ret;
> +}
> +

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

[Chi-Wen Weng]

Jonathan Cameron 於 2026/11/1 上午 08:19 寫道:
> On Fri, 30 Oct 2026 11:15:00 +0800
> Chi-Wen Weng <cwweng.linux@gmail.com> wrote:
> 
>> From: Chi-Wen Weng <cwweng@nuvoton.com>
>>
>> Add a devicetree binding for the Enhanced Analog-to-Digital Converter
>> (EADC) found on Nuvoton MA35D1 SoCs.
>>
>> The controller provides eight external analog inputs which can be used
>> as single-ended channels or as four fixed differential pairs: 0-4,
>> 1-5, 2-6 and 3-7.
>>
>> Describe the four hardware interrupt outputs, ADINT0 through ADINT3.
>> ADINT0 through ADINT2 are associated with the corresponding conversion
>> interrupt flags, while ADINT3 is shared by the ADIF3 conversion
>> interrupt and the ADCMPF0 through ADCMPF3 result-compare interrupt
>> sources.
>>
>> Also describe the functional clock, optional reset, optional external
>> reference supply and optional DMA request.
>>
>> Expose the controller as an IIO channel provider through the
>> DT #io-channel-cells property. The cell identifies the logical EADC
>> channel selected by the reg value of the corresponding channel child
>> node.
>>
>> Restrict the channel binding to features supported by the MA35D1 EADC
>> hardware and use the common single-channel representation for
>> single-ended inputs.
> This description seems a little verbose.  Maybe only talk about the
> stuff we can't tell from the patch.
> 
> 1. Why it needs a new binding and isn't similar enough to existing
>     ones for reuse.
> 2. Any unusual aspects.  Calling out the stuff that I note below on
>     the per channel binding oddity more clearly would have been helpful.
> 
> For the other stuff the reader can just look at the patch,
> 
> Thanks,
> 
> Jonathan
> 
>>
>> Signed-off-by: Chi-Wen Weng <cwweng@nuvoton.com>
>> ---
>>   .../bindings/iio/adc/nuvoton,ma35d1-eadc.yaml | 175 ++++++++++++++++++
>>   1 file changed, 175 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..97144a8eeb5c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml
>> @@ -0,0 +1,175 @@
>> +# 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: |
> 
> I think you only want to keep paragraphs. In which case > rather than |
> 
>> +  The Nuvoton MA35D1 Enhanced Analog-to-Digital Converter (EADC) is a
>> +  12-bit successive approximation converter with eight external analog
>> +  inputs and one internally connected VBAT input.
>> +
>> +  The external inputs can be used as eight single-ended channels or as
>> +  four fixed differential pairs: 0-4, 1-5, 2-6 and 3-7. The internally
>> +  connected VBAT input is not described by a channel child node.
> 
> How would you have a consumer of VBAT given the io-channel-cells
> comment about using the channel reg values for xlate?
> 
> 
>> +
>> +  The controller provides four interrupt outputs, ADINT0 through ADINT3.
>> +  ADINT0, ADINT1 and ADINT2 are generated by the corresponding ADIF0,
>> +  ADIF1 and ADIF2 conversion interrupt flags. ADINT3 is shared by the
>> +  ADIF3 conversion interrupt flag and the ADCMPF0 through ADCMPF3 result
>> +  compare flags.
> 
>> +
>> +patternProperties:
>> +  '^channel@[0-7]$':
>> +    type: object
>> +    $ref: /schemas/iio/adc/adc.yaml#
>> +    unevaluatedProperties: false
>> +    description: |
>> +      External EADC channel description.
>> +
>> +      For a single-ended channel, single-channel specifies the external
>> +      analog input number (0 through 7), while reg provides the logical IIO
>> +      channel index.
>> +
>> +      For a differential channel, omit single-channel. The reg value selects
>> +      one of the four fixed hardware differential pairs: reg 0 selects 0-4,
>> +      reg 1 selects 1-5, reg 2 selects 2-6 and reg 3 selects 3-7.
> 
> Sorry, I wasn't paying attention on earlier versions.  This is a very
> unintuitive binding particularly as we may want to have a single channel
> before a differential one.  Better I think to use the differential
> channel bindings and restrict which ones are allowed to this fixed set.
> 
> 
>> +
>> +    properties:
>> +      reg:
>> +        maximum: 7
>> +
>> +      single-channel:
>> +        maximum: 7
>> +
>> +      # Differential pairs are fixed in hardware and are selected by reg.
>> +      diff-channels: false
>> +
>> +      bipolar: false
>> +
>> +      common-mode-channel: false
>> +
>> +      settling-time-us: false
>> +
>> +      oversampling-ratio: false
>> +
>> +    allOf:
>> +      - if:
>> +          not:
>> +            required:
>> +              - single-channel
>> +        then:
>> +          properties:
>> +            reg:
>> +              maximum: 3
>> +
>> +    required:
>> +      - reg
>> +
> 
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/nuvoton,ma35d1-clk.h>
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +    #include <dt-bindings/reset/nuvoton,ma35d1-reset.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>,
>> +                         <GIC_SPI 66 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>;
>> +            interrupt-names = "adint0", "adint1", "adint2", "adint3";
>> +            clocks = <&clk EADC_GATE>;
>> +            resets = <&reset MA35D1_RESET_EADC0>;
>> +
>> +            #io-channel-cells = <1>;
>> +            #address-cells = <1>;
>> +            #size-cells = <0>;
>> +
>> +            channel@0 {
>> +                reg = <0>;
>> +                single-channel = <0>;
>> +            };
>> +
>> +            channel@1 {
>> +                reg = <1>;
> 
> This looked very odd and is why I had a closer read of the docs
> above.  To make this a reasonable binding to read this
> needs to do the full differential channel binding from adc.yaml
> with reg just being an index and not specifying channels at all
> (same as for the single channel case).
> 
>> +            };
>> +        };
>> +    };
>> +
>> +...
> 

Hi Jonathan,

Thank you for the detailed review.

I agree that the current channel representation is unnecessarily
unusual and does not make good use of the common ADC channel binding.

For v4 I will rework it so that reg is only the logical channel index,
while the physical inputs are described explicitly:

- single-channel for a single-ended input
- diff-channels for a differential input

The differential channels will be restricted to the four combinations
supported by the MA35D1 hardware:

   <0 4>
   <1 5>
   <2 6>
   <3 7>

I will also require each external channel node to explicitly describe
either single-channel or diff-channels, instead of using the absence of
single-channel to imply differential mode.

This should also make the example clearer, for example:

   channel@0 {
       reg = <0>;
       single-channel = <2>;
   };

   channel@1 {
       reg = <1>;
       diff-channels = <0 4>;
   };

so that reg is clearly independent of the physical ADC input selection.

I will also switch the channel schema to additionalProperties: false
and only expose the common ADC properties supported by this hardware,
rather than inheriting unsupported properties from adc.yaml.

For the top-level description I will switch from the literal block
style to the folded paragraph style and shorten the commit message to
focus on the unusual hardware aspects rather than repeating information
visible directly in the schema.

Regarding VBAT, thank you for pointing out the inconsistency with the
#io-channel-cells description. The internal VBAT input is a fixed
hardware connection rather than a board-routed input, so it should not
need a channel child node. I will rework the provider/channel numbering
description in v4 so that the internal VBAT channel, if exposed to an
IIO consumer, has an unambiguous channel specifier without requiring a
DT child node.

Thanks,
Chi-Wen

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

[Chi-Wen Weng]

Jonathan Cameron 於 2026/11/1 上午 08:53 寫道:
> On Fri, 30 Oct 2026 11:15:01 +0800
> Chi-Wen Weng <cwweng.linux@gmail.com> wrote:
> 
>> From: Chi-Wen Weng <cwweng@nuvoton.com>
>>
>> Add an IIO driver for the Enhanced Analog-to-Digital Converter (EADC)
>> found on Nuvoton MA35D1 SoCs.
>>
>> The driver supports direct conversion and externally triggered buffered
>> capture using sample module 0. Both single-ended inputs and the four
>> fixed differential input pairs are supported.
>>
>> Use the internal 1.6 V reference when no external reference supply is
>> provided, and report the reference voltage through the IIO scale
>> interface.
>>
>> Use ADINT0 for conversion completion. Configure the interrupt source
>> and end-of-conversion position explicitly, and recover from conversion
>> timeouts by resetting the EADC control circuits, synchronizing the IRQ
>> and clearing stale conversion state before another conversion is
>> started.
>>
>> Reset the EADC control circuits during hardware initialization and
>> sanitize interrupt, compare and sample-module trigger configuration so
>> the driver does not depend on state left by firmware when the external
>> reset control is unavailable.
>>
>> Provide firmware-node channel translation so DT IIO consumers select
>> channels by the logical channel index described by the channel node's
>> reg property rather than by the position of the channel in the driver's
>> channel array.
>>
>> Add clock, optional reset and system sleep handling, and restore the
>> controller configuration after resume.
>>
>> Signed-off-by: Chi-Wen Weng <cwweng@nuvoton.com>
> Hi Chi-Wen
> 
> Various comments inline.
> 
> Thanks,
> 
> Jonathan
> 
>> diff --git a/drivers/iio/adc/ma35d1_eadc.c b/drivers/iio/adc/ma35d1_eadc.c
>> new file mode 100644
>> index 000000000000..9b6c38a66a66
>> --- /dev/null
>> +++ b/drivers/iio/adc/ma35d1_eadc.c
> 
> 
>> +
>> +static int ma35d1_adc_disable_irq(struct ma35d1_adc *adc)
>> +{
>> +	return regmap_clear_bits(adc->regmap, MA35D1_EADC_CTL, MA35D1_EADC_CTL_ADCIEN0);
>> +}
>> +
>> +static int ma35d1_adc_setup_reference(struct ma35d1_adc *adc)
>> +{
>> +	struct regmap *map = adc->regmap;
>> +	int ret;
>> +
>> +	/*
>> +	 * MA35D1 TRM EADC reference control: PDREF selects the external VREF
>> +	 * pin. With the internal reference selected, VREFSEL = 0 selects 1.6 V.
>> +	 */
>> +	if (adc->vref) {
> 
> With change below to use the read_voltage function this would then be
> using a flag set there.
> 
>> +		ret = regmap_set_bits(map, MA35D1_EADC_REFADJCTL,
>> +				      MA35D1_EADC_REFADJCTL_PDREF);
>> +		if (ret)
>> +			return ret;
>> +	} else {
>> +		ret = regmap_clear_bits(map, MA35D1_EADC_REFADJCTL,
>> +					MA35D1_EADC_REFADJCTL_PDREF);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = regmap_update_bits(map, MA35D1_EADC_CTL,
>> +					 MA35D1_EADC_CTL_VREFSEL_MASK,
>> +					 MA35D1_EADC_CTL_VREFSEL_1V6);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	fsleep(MA35D1_EADC_REF_STABLE_US);
>> +
>> +	return 0;
>> +}
> 
> 
>> +static int ma35d1_adc_abort_conversion(struct ma35d1_adc *adc)
>> +{
>> +	struct regmap *map = adc->regmap;
>> +	int ret;
>> +	int err;
>> +
>> +	ret = ma35d1_adc_disable_irq(adc);
>> +
>> +	/*
>> +	 * Reset the EADC control circuits to terminate a conversion which may
>> +	 * still be in progress. EADCRST is cleared automatically by hardware
>> +	 * when the reset completes and the EADC register values are preserved.
>> +	 */
>> +	err = ma35d1_adc_reset_control(adc);
>> +	if (!ret)
>> +		ret = err;
> Why not just set ret directly for this one?
>> +
>> +	/* Drain an interrupt which may already have been latched by the GIC. */
>> +	synchronize_irq(adc->irq);
>> +
>> +	err = regmap_write(map, MA35D1_EADC_STATUS2,
>> +			   MA35D1_EADC_STATUS2_ADIF0);
> I don't mind going slightly over 80 chars. I would do so for the line above.
> 
>> +	if (!ret)
>> +		ret = err;
>> +
>> +	reinit_completion(&adc->completion);
> Why?  It is reinit at start of read conversion - why else do we need to
> do it?  Add a comment
> 
>> +
>> +	return ret;
>> +}
>> +
>> +static int ma35d1_adc_read_conversion(struct ma35d1_adc *adc,
>> +				      const struct iio_chan_spec *chan, u16 *raw)
>> +{
>> +	struct regmap *map = adc->regmap;
>> +	unsigned int val;
>> +	long timeout;
>> +	int ret;
>> +
>> +	reinit_completion(&adc->completion);
>> +
>> +	ret = regmap_write(map, MA35D1_EADC_STATUS2, MA35D1_EADC_STATUS2_ADIF0);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = ma35d1_adc_config_channel(adc, chan);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regmap_set_bits(map, MA35D1_EADC_CTL, MA35D1_EADC_CTL_ADCIEN0);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regmap_write(map, MA35D1_EADC_SWTRG, BIT(0));
>> +	if (ret)
>> +		goto disable_irq;
>> +
>> +	timeout = wait_for_completion_timeout(&adc->completion,
>> +					      MA35D1_EADC_TIMEOUT);
>> +	if (!timeout) {
>> +		ret = ma35d1_adc_abort_conversion(adc);
>> +		return ret ?: -ETIMEDOUT;
> 
> Timed out first, so I think that should be the return value
> even if the abort then failed.  Maybe an error print is
> more appropriate for a failed abort so we don't loose
> that extra info.
> 
>> +	}
>> +
>> +	ret = ma35d1_adc_disable_irq(adc);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regmap_read(map, MA35D1_EADC_DAT(0), &val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (!(val & MA35D1_EADC_DAT_VALID))
>> +		return -EIO;
>> +
>> +	if (val & MA35D1_EADC_DAT_OV)
>> +		return -EOVERFLOW;
>> +
>> +	*raw = FIELD_GET(MA35D1_EADC_DAT_RESULT_MASK, val);
> 
> As below - I don't follow why we'd get 16 bits when I think
> all paths throw away everything above the 12th.
> 
>> +
>> +	return 0;
>> +
>> +disable_irq:
>> +	ma35d1_adc_disable_irq(adc);
>> +
>> +	return ret;
>> +}
>> +
>> +static irqreturn_t ma35d1_adc_isr(int irq, void *data)
>> +{
>> +	struct iio_dev *indio_dev = data;
>> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
>> +	unsigned int status;
>> +	int ret;
>> +
>> +	ret = regmap_read(adc->regmap, MA35D1_EADC_STATUS2, &status);
>> +	if (ret || !(status & MA35D1_EADC_STATUS2_ADIF0))
>> +		return IRQ_NONE;
>> +
>> +	regmap_write(adc->regmap, MA35D1_EADC_STATUS2,
>> +		     MA35D1_EADC_STATUS2_ADIF0);
>> +	complete(&adc->completion);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int ma35d1_adc_sign_extend(u16 raw)
>> +{
>> +	return sign_extend32(raw & MA35D1_EADC_DAT_RESULT_12BIT, 11);
> 
> If the result is always 12 bits, why is read_conversion returning 16?
> 
>> +}
>> +
>> +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);
>> +	u16 raw;
>> +	int ret;
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW: {
>> +		IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
>> +
> 
> No blank line here.  We want the acquire and the error check tightly
> coupled visually.
> 
>> +		if (IIO_DEV_ACQUIRE_FAILED(claim))
>> +			return -EBUSY;
>> +
>> +		guard(mutex)(&adc->lock);
>> +		if (adc->suspended)
>> +			return -EBUSY;
>> +
>> +		ret = ma35d1_adc_read_conversion(adc, chan, &raw);
>> +		if (ret)
>> +			return ret;
>> +
>> +		if (chan->differential)
>> +			*val = ma35d1_adc_sign_extend(raw);
>> +		else
>> +			*val = raw & MA35D1_EADC_DAT_RESULT_12BIT;
>> +
>> +		return IIO_VAL_INT;
>> +	}
>> +	case IIO_CHAN_INFO_SCALE:
>> +		*val = adc->vref_mV;
>> +		/* Differential mode has a VREF / 2048 LSB; single-ended uses /4096. */
>> +		*val2 = chan->differential ? 11 : 12;
>> +		return IIO_VAL_FRACTIONAL_LOG2;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +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 *scan_chan = NULL;
>> +	unsigned long bit;
>> +
>> +	for_each_set_bit(bit, scan_mask, iio_get_masklength(indio_dev)) {
> 	iio_for_each_active_channel()
> 
>> +		const struct iio_chan_spec *chan = &indio_dev->channels[bit];
>> +
>> +		if (chan->type == IIO_TIMESTAMP)
>> +			continue;
>> +
>> +		if (chan->type != IIO_VOLTAGE || scan_chan)
>> +			return -EINVAL;
>> +
>> +		scan_chan = chan;
> 
> This only makes sense if there is only one channel enabled.
> The driver needs to ensure that is the case (there is a standard
> validation callback for this).
> 
>> +	}
>> +
>> +	if (!scan_chan)
>> +		return -EINVAL;
>> +
>> +	adc->scan_chan = scan_chan;
>> +
>> +	return 0;
>> +}
> 
>> +
>> +static irqreturn_t ma35d1_adc_trigger_handler(int irq, void *p)
> This looks like the device only supports onehot channels (one at a time)
> That's fine but then you should have the callbacks or available_channels
> set to indicate that.
> 
>> +{
>> +	struct iio_poll_func *pf = p;
>> +	struct iio_dev *indio_dev = pf->indio_dev;
>> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
>> +	IIO_DECLARE_BUFFER_WITH_TS(u16, scan, 1) = { };
> Where the timestamp location is fixed (if present) I prefer
> 	struct {
> 		u16 channel;
> 		aligned_s64 timestamp;
> 	} scan;
> 
> as it makes it easier to understand the layout. The macro
> is for the case where different numbers of channels can be enabled
> and there are enough of them to have the timestamp moving around
> depending on what is enabled.
> 
>> +	u16 raw;
>> +	int ret;
>> +
>> +	mutex_lock(&adc->lock);
>> +
>> +	if (!adc->suspended && adc->scan_chan) {
>> +		ret = ma35d1_adc_read_conversion(adc, adc->scan_chan, &raw);
>> +		if (!ret) {
>> +			scan[0] = raw & MA35D1_EADC_DAT_RESULT_12BIT;
>> +			iio_push_to_buffers_with_ts(indio_dev, scan, sizeof(scan),
>> +						    pf->timestamp);
>> +		}
>> +	}
>> +
>> +	mutex_unlock(&adc->lock);
>> +
>> +	iio_trigger_notify_done(indio_dev->trig);
>> +
>> +	return IRQ_HANDLED;
>> +}
> 
>> +
>> +static int ma35d1_adc_fwnode_xlate(struct iio_dev *indio_dev,
>> +				   const struct fwnode_reference_args *iiospec)
>> +{
>> +	unsigned int i;
>> +
>> +	if (iiospec->nargs != 1)
>> +		return -EINVAL;
>> +
>> +	for (i = 0; i < indio_dev->num_channels; i++) {
> 
> 	for (unsigned int i = 0; ..
> 
>> +		const struct iio_chan_spec *chan = &indio_dev->channels[i];
>> +
>> +		if (chan->type == IIO_VOLTAGE &&
>> +		    chan->channel == iiospec->args[0])
>> +			return i;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static const struct iio_info ma35d1_adc_info = {
>> +	.read_raw = ma35d1_adc_read_raw,
>> +	.update_scan_mode = ma35d1_adc_update_scan_mode,
>> +	.fwnode_xlate = ma35d1_adc_fwnode_xlate,
>> +};
>> +
>> +static void ma35d1_adc_init_channel(struct iio_chan_spec *chan, u32 index,
> 
> As below.  Given C provides nice ways to set lots of structure elements
> helpers use those, rather than a fill the values in helper.
> 
>> +				    u32 vinp, u32 vinn,
>> +				    unsigned int scan_index,
>> +				    bool differential)
>> +{
>> +	chan->type = IIO_VOLTAGE;
>> +	chan->indexed = 1;
>> +	chan->channel = index;
>> +	chan->address = vinp;
>> +	chan->scan_index = scan_index;
>> +	chan->info_mask_separate =
>> +		BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE);
>> +	chan->scan_type.format = differential ? IIO_SCAN_FORMAT_SIGNED_INT :
>> +					      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;
>> +	}
>> +}
>> +
>> +static int ma35d1_adc_parse_channels(struct iio_dev *indio_dev,
>> +				     struct device *dev)
>> +{
>> +	DECLARE_BITMAP(used_regs, MA35D1_EADC_MAX_EXT_CHANNELS);
>> +	struct iio_chan_spec *channels;
>> +	unsigned int scan_index;
>> +	int num_channels;
>> +	int ret;
>> +
>> +	bitmap_zero(used_regs, MA35D1_EADC_MAX_EXT_CHANNELS);
>> +
>> +	num_channels = device_get_child_node_count(dev);
>> +	if (!num_channels)
>> +		return dev_err_probe(dev, -ENOENT, "no ADC channels configured\n");
>> +
>> +	if (num_channels > MA35D1_EADC_MAX_EXT_CHANNELS)
>> +		return dev_err_probe(dev, -EINVAL, "too many ADC channels\n");
>> +
>> +	channels = devm_kcalloc(dev, size_add(num_channels, 1),
>> +				sizeof(*channels), GFP_KERNEL);
>> +	if (!channels)
>> +		return -ENOMEM;
>> +
>> +	scan_index = 0;
>> +	device_for_each_child_node_scoped(dev, child) {
>> +		bool differential;
>> +		u32 reg;
>> +		u32 vinp;
>> +		u32 vinn;
> 
> Can combine these related u32s on oneline.
> 
>> +
>> +		ret = fwnode_property_read_u32(child, "reg", &reg);
>> +		if (ret)
>> +			return dev_err_probe(dev, ret,
>> +					     "missing channel reg property\n");
>> +
>> +		if (reg >= MA35D1_EADC_MAX_EXT_CHANNELS)
>> +			return dev_err_probe(dev, -EINVAL,
>> +					     "invalid ADC channel index %u\n", reg);
>> +
>> +		if (test_and_set_bit(reg, used_regs))
>> +			return dev_err_probe(dev, -EINVAL,
>> +					     "duplicate ADC channel index %u\n", reg);
>> +
>> +		if (fwnode_property_present(child, "single-channel")) {
>> +			ret = fwnode_property_read_u32(child, "single-channel",
>> +						       &vinp);
>> +			if (ret)
>> +				return dev_err_probe(dev, ret,
>> +						     "invalid single-channel for index %u\n",
>> +						     reg);
>> +
>> +			if (vinp >= MA35D1_EADC_MAX_EXT_CHANNELS)
>> +				return dev_err_probe(dev, -EINVAL,
>> +						     "invalid ADC input %u\n", vinp);
>> +
>> +			vinn = 0;
>> +			differential = false;
>> +		} else {
> 
> This will change to match my requests for the binding so I won't look too closely
> at it in this revision.
> 
>> +			if (reg >= 4)
>> +				return dev_err_probe(dev, -EINVAL,
>> +						     "invalid differential pair %u\n", reg);
>> +
>> +			vinp = reg;
>> +			vinn = reg + 4;
>> +			differential = true;
>> +		}
>> +
>> +		ma35d1_adc_init_channel(&channels[scan_index], reg, vinp, vinn,
>> +					scan_index, differential);
> get rid of this and fill the structure in directly in the two different
> paths above - so have a single ended copy of this and the slightly
> different one for the differential channels. Bit of duplication
> but simpler code by putting it all in one place.
> 
> 			channels[scan_index] = (struct iio_chan_spec) {
> 				.type = IIO_VOLTAGE,
> 				.indexed = 1,
> 				.channel = index,
> 				.address = vinp,
> 				.scan_index = scan_index,
> 				.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> 						      BIT(IIO_CHAN_INFO_SCALE);
> 				.scan_type = {
> 					.format = IIO_SCAN_FORMAT_UNSIGNED_INT,
> 					.realbits = 12,
> 					.storagebits = 16,
> 					.endianness = IIO_CPU,;
> 			};
> 		
>> +		scan_index++;
> 
>> +	}
>> +
>> +	channels[scan_index] = IIO_CHAN_SOFT_TIMESTAMP(scan_index);
> 
>> +
>> +	indio_dev->channels = channels;
>> +	indio_dev->num_channels = scan_index + 1;
>> +
>> +	return 0;
>> +}
>> +
>> +static int ma35d1_adc_init_vref(struct ma35d1_adc *adc, struct device *dev)
>> +{
>> +	int ret;
>> +
>> +	adc->vref = devm_regulator_get_optional(dev, "vref");
>> +	ret = PTR_ERR_OR_ZERO(adc->vref);
>> +	if (ret == -ENODEV) {
>> +		adc->vref = NULL;
>> +		adc->vref_mV = MA35D1_EADC_INTERNAL_VREF_mV;
>> +		return 0;
>> +	}
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "failed to get VREF supply\n");
>> +
>> +	ret = regulator_enable(adc->vref);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "failed to enable VREF supply\n");
>> +
>> +	ret = devm_add_action_or_reset(dev, ma35d1_adc_vref_disable, adc->vref);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regulator_get_voltage(adc->vref);
> 
> There are plenty of examples of how to use devm_regulator_get_enable_read_voltage()
> with optional regulators in tree.  Follow one of those and this code will be a lot
> shorter.
> 
>> +	if (ret <= 0)
>> +		return dev_err_probe(dev, ret ?: -EINVAL,
>> +				     "failed to get VREF voltage\n");
>> +
>> +	adc->vref_mV = ret / (MICRO / MILLI);
>> +
>> +	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);
>> +	int ret;
>> +
>> +	ret = iio_device_suspend_triggering(indio_dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	mutex_lock(&adc->lock);
>> +
>> +	ret = ma35d1_adc_disable_irq(adc);
>> +	if (ret)
>> +		goto resume_triggering;
>> +
>> +	synchronize_irq(adc->irq);
>> +
>> +	ret = regmap_clear_bits(adc->regmap, MA35D1_EADC_CTL,
>> +				MA35D1_EADC_CTL_ADCEN);
>> +	if (ret)
> 
> If this fails, for consistency I think you should probably
> reenable the irq.  If not add a comment on your thinking for
> why.
> 
>   
>> +		goto resume_triggering;
>> +
>> +	adc->suspended = true;
>> +	clk_disable_unprepare(adc->clk);
>> +	mutex_unlock(&adc->lock);
>> +
>> +	return 0;
>> +
>> +resume_triggering:
>> +	mutex_unlock(&adc->lock);
>> +	iio_device_resume_triggering(indio_dev);
>> +
>> +	return ret;
>> +}
>> +
> 

Hi Jonathan,

Thank you for the detailed review.

I will address these comments in v4.

The channel parsing will be reworked together with the binding changes.
reg will remain the logical channel index, while single-channel and
diff-channels will describe the physical inputs. Accordingly, the IIO
channel specification will use channel/channel2 for the physical input
or differential pair, while address will keep the logical DT channel
index used for firmware-node translation.

I will also remove ma35d1_adc_init_channel() and initialize the
iio_chan_spec structures directly in the single-ended and differential
parsing paths.

Regarding the one-hot scan-mask handling, the MA35D1 hardware itself is
not limited to one sample module at a time. The EADC has one SAR
converter shared by multiple sample modules. Multiple sample modules can
be triggered together, after which the hardware priority arbitrator
performs their conversions sequentially.

The initial driver, however, currently uses only sample module 0 and
performs one voltage conversion for each buffered trigger. Therefore
the buffered interface exposed by this version of the driver supports
one active voltage channel per scan. I will make this limitation
explicit using the standard one-hot scan-mask validation callback and
use iio_for_each_active_channel() when selecting the active channel.
Support for using multiple sample modules for multi-channel buffered
scans can be added separately in the future.

Since the scan layout is therefore fixed to one u16 sample plus an
optional timestamp, I will replace IIO_DECLARE_BUFFER_WITH_TS() with a
fixed scan structure containing one u16 sample and an aligned timestamp.

For the conversion path I will make the returned ADC sample explicitly
12 bits. Differential values will then be sign-extended directly from
that 12-bit value, rather than carrying a 16-bit intermediate result
whose upper bits are discarded later.

For timeout recovery I will keep -ETIMEDOUT as the primary error. If
resetting or aborting the timed-out conversion fails, I will report that
separately instead of replacing the timeout return value. I will also
remove the redundant completion reinitialization from the abort path,
since the completion is reinitialized before each new conversion and
the abort path already synchronizes the IRQ.

I will simplify the optional VREF handling using
devm_regulator_get_enable_read_voltage() and keep a flag indicating
whether the external reference is in use.

Regarding the suspend error path, ADCIEN0 is only enabled while a
conversion is in progress and is normally disabled while the device is
idle. The suspend path holds the device lock after triggering has been
suspended, so there is no active conversion at that point. If disabling
ADCEN fails, leaving ADCIEN0 disabled therefore matches the normal idle
state, and the next conversion will enable it again. I will add a
comment to make this state transition clear rather than unconditionally
re-enabling the interrupt.

I will also take care of the smaller style points, including keeping the
direct-mode acquire and error check together, using a loop-local
variable in fwnode_xlate(), combining the related u32 declarations, and
avoiding unnecessary line wrapping.

Thanks,
Chi-Wen

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

[Krzysztof Kozlowski]

On Fri, Oct 30, 2026 at 11:15:00AM +0800, Chi-Wen Weng wrote:
> From: Chi-Wen Weng <cwweng@nuvoton.com>
> 
> Add a devicetree binding for the Enhanced Analog-to-Digital Converter
> (EADC) found on Nuvoton MA35D1 SoCs.
> 
> The controller provides eight external analog inputs which can be used
> as single-ended channels or as four fixed differential pairs: 0-4,
> 1-5, 2-6 and 3-7.
> 
> Describe the four hardware interrupt outputs, ADINT0 through ADINT3.
> ADINT0 through ADINT2 are associated with the corresponding conversion
> interrupt flags, while ADINT3 is shared by the ADIF3 conversion
> interrupt and the ADCMPF0 through ADCMPF3 result-compare interrupt
> sources.
> 
> Also describe the functional clock, optional reset, optional external
> reference supply and optional DMA request.

Don't repeat the patch contents.

> 
> Expose the controller as an IIO channel provider through the
> DT #io-channel-cells property. The cell identifies the logical EADC
> channel selected by the reg value of the corresponding channel child
> node.
> 
> Restrict the channel binding to features supported by the MA35D1 EADC
> hardware and use the common single-channel representation for
> single-ended inputs.

And here even more.

> 
> Signed-off-by: Chi-Wen Weng <cwweng@nuvoton.com>
> ---
>  .../bindings/iio/adc/nuvoton,ma35d1-eadc.yaml | 175 ++++++++++++++++++
>  1 file changed, 175 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..97144a8eeb5c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml
> @@ -0,0 +1,175 @@
> +# 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 successive approximation converter with eight external analog
> +  inputs and one internally connected VBAT input.
> +
> +  The external inputs can be used as eight single-ended channels or as
> +  four fixed differential pairs: 0-4, 1-5, 2-6 and 3-7. The internally
> +  connected VBAT input is not described by a channel child node.
> +
> +  The controller provides four interrupt outputs, ADINT0 through ADINT3.
> +  ADINT0, ADINT1 and ADINT2 are generated by the corresponding ADIF0,
> +  ADIF1 and ADIF2 conversion interrupt flags. ADINT3 is shared by the
> +  ADIF3 conversion interrupt flag and the ADCMPF0 through ADCMPF3 result
> +  compare flags.
> +
> +properties:
> +  compatible:
> +    const: nuvoton,ma35d1-eadc
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    items:
> +      - description: ADINT0 conversion interrupt output generated by ADIF0
> +      - description: ADINT1 conversion interrupt output generated by ADIF1
> +      - description: ADINT2 conversion interrupt output generated by ADIF2
> +      - description: >

Drop >

> +          ADINT3 shared interrupt output generated by ADIF3 or by the
> +          ADCMPF0 through ADCMPF3 result compare flags
> +
> +  interrupt-names:
> +    items:
> +      - const: adint0
> +      - const: adint1
> +      - const: adint2
> +      - const: adint3

"int" is redundant, so honestly names should rather go away completely.

> +
> +  clocks:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 1

Best regards,
Krzysztof