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

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

[Chi-Wen Weng]

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

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

The MA35D1 EADC controller supports external analog input channels,
fixed differential input pairs, an optional external reference voltage,
up to four end-of-conversion interrupts, an optional reset line, and an
ADC-to-memory DMA request.

This initial driver supports interrupt-driven direct raw reads and
triggered buffered capture. Buffered capture is paced by an external IIO
trigger and is limited to a single enabled voltage channel per scan in
this version. Multi-channel buffered scans and DMA support can be added
later.

The driver supports both single-ended external channels and the fixed
hardware differential input pairs. Differential conversions are reported
as signed two's complement values. The ADC scale is reported using either
the optional external vref-supply or the internal 1.6 V reference.

Changes in v2:
- Update the binding to allow up to four interrupt entries for ADINT0 to
  ADINT3.
- Add optional vref-supply, dmas, dma-names and resets properties to the
  binding.
- Keep diff-channels in the binding and restrict differential inputs to
  the fixed hardware pairs.
- Limit DT child channel nodes to external channels 0 to 7.
- Use regmap for register access.
- Add optional reset handling.
- Add optional external reference handling and IIO scale reporting.
- Use the internal 1.6 V reference when no vref-supply is present.
- Handle differential conversions as signed two's complement values.
- Use an external IIO trigger for buffered capture instead of a
  self-retriggering EOC loop.
- Limit buffered capture to a single enabled voltage channel per scan.
- Rework suspend and resume to save and restore the buffered capture
  state.
- Use devm_mutex_init() and scoped child-node iteration.
- Use the new scan_type.format field for buffered scan type metadata.
- Drop datasheet_name from channel setup.
- Address header, helper naming and cleanup comments from review.

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 | 163 ++++
 drivers/iio/adc/Kconfig                       |  12 +
 drivers/iio/adc/Makefile                      |   1 +
 drivers/iio/adc/ma35d1_eadc.c                 | 740 ++++++++++++++++++
 4 files changed, 916 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml
 create mode 100644 drivers/iio/adc/ma35d1_eadc.c

-- 
2.25.1

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

[Chi-Wen Weng]

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

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

The controller has one register region, up to four end-of-conversion
interrupts, one functional clock, and an optional reset line. The
binding also describes the optional external reference supply and the
optional DMA request supported by the hardware.

ADC input pins are described using standard channel child nodes. The
external ADC input channels are 0 to 7. Differential inputs are described
with diff-channels and are limited to the fixed hardware pairs.

Signed-off-by: Chi-Wen Weng <cwweng@nuvoton.com>
---
 .../bindings/iio/adc/nuvoton,ma35d1-eadc.yaml | 163 ++++++++++++++++++
 1 file changed, 163 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..1be5470643d1
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml
@@ -0,0 +1,163 @@
+# 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 is a 12-bit
+  ADC controller with 8 external input channels, 4 fixed differential
+  input pairs and 1 internal VBAT input. Board wired ADC inputs are
+  described using channel child nodes.
+
+properties:
+  compatible:
+    const: nuvoton,ma35d1-eadc
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    minItems: 1
+    maxItems: 4
+    items:
+      - description: ADINT0 end-of-conversion interrupt
+      - description: ADINT1 end-of-conversion interrupt
+      - description: ADINT2 end-of-conversion interrupt
+      - description: ADINT3 end-of-conversion interrupt
+
+  clocks:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+  vref-supply:
+    description:
+      External reference voltage supply connected to the VREF pin. If this
+      property is omitted, the internal 1.6 V reference is used.
+
+  dmas:
+    maxItems: 1
+
+  dma-names:
+    const: rx
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+patternProperties:
+  '^channel@[0-7]$':
+    type: object
+    $ref: adc.yaml
+    unevaluatedProperties: false
+
+    properties:
+      reg:
+        maximum: 7
+
+      diff-channels: true
+
+    allOf:
+      - if:
+          properties:
+            reg:
+              const: 0
+        then:
+          properties:
+            diff-channels:
+              items:
+                - const: 0
+                - const: 4
+      - if:
+          properties:
+            reg:
+              const: 1
+        then:
+          properties:
+            diff-channels:
+              items:
+                - const: 1
+                - const: 5
+      - if:
+          properties:
+            reg:
+              const: 2
+        then:
+          properties:
+            diff-channels:
+              items:
+                - const: 2
+                - const: 6
+      - if:
+          properties:
+            reg:
+              const: 3
+        then:
+          properties:
+            diff-channels:
+              items:
+                - const: 3
+                - const: 7
+      - if:
+          properties:
+            reg:
+              enum: [4, 5, 6, 7]
+        then:
+          not:
+            required:
+              - diff-channels
+
+    required:
+      - reg
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - '#address-cells'
+  - '#size-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/nuvoton,ma35d1-clk.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #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>;
+            clocks = <&clk EADC_GATE>;
+            resets = <&reset MA35D1_RESET_EADC0>;
+
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            channel@0 {
+                reg = <0>;
+            };
+
+            channel@2 {
+                reg = <2>;
+                diff-channels = <2 6>;
+            };
+        };
+    };
+...
-- 
2.25.1

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

[Rob Herring (Arm)]

On Mon, 13 Jul 2026 16:11:26 +0800, Chi-Wen Weng wrote:
> From: Chi-Wen Weng <cwweng@nuvoton.com>
> 
> Add devicetree binding for the Enhanced ADC controller found on
> Nuvoton MA35D1 SoCs.
> 
> The controller has one register region, up to four end-of-conversion
> interrupts, one functional clock, and an optional reset line. The
> binding also describes the optional external reference supply and the
> optional DMA request supported by the hardware.
> 
> ADC input pins are described using standard channel child nodes. The
> external ADC input channels are 0 to 7. Differential inputs are described
> with diff-channels and are limited to the fixed hardware pairs.
> 
> Signed-off-by: Chi-Wen Weng <cwweng@nuvoton.com>
> ---
>  .../bindings/iio/adc/nuvoton,ma35d1-eadc.yaml | 163 ++++++++++++++++++
>  1 file changed, 163 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml: properties:interrupts: {'minItems': 1, 'maxItems': 4, 'items': [{'description': 'ADINT0 end-of-conversion interrupt'}, {'description': 'ADINT1 end-of-conversion interrupt'}, {'description': 'ADINT2 end-of-conversion interrupt'}, {'description': 'ADINT3 end-of-conversion interrupt'}]} should not be valid under {'required': ['maxItems']}
	hint: "maxItems" is not needed with an "items" list
	from schema $id: http://devicetree.org/meta-schemas/items.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.kernel.org/project/devicetree/patch/20260713081127.115197-2-cwweng.linux@gmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

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

[Chi-Wen Weng]

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

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

The driver supports interrupt-driven direct raw reads and triggered
buffered capture paced by an external IIO trigger. Buffered capture is
limited to a single enabled voltage channel per scan in this initial
implementation.

Channels are described by firmware child nodes. Single-ended external
channels and the fixed hardware differential input pairs are supported.
The driver reports IIO scale from either an optional external reference
supply or the internal 1.6 V reference.

The driver also handles the functional clock and optional reset line.
DMA support is not used by this initial driver.

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

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 1c663c98c6c9..509118dc5643 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -981,6 +981,18 @@ 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"
+	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 707dd708912f..7b9b38688223 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -85,6 +85,7 @@ obj-$(CONFIG_LTC2471) += ltc2471.o
 obj-$(CONFIG_LTC2485) += ltc2485.o
 obj-$(CONFIG_LTC2496) += ltc2496.o ltc2497-core.o
 obj-$(CONFIG_LTC2497) += ltc2497.o ltc2497-core.o
+obj-$(CONFIG_MA35D1_EADC) += ma35d1_eadc.o
 obj-$(CONFIG_MAX1027) += max1027.o
 obj-$(CONFIG_MAX11100) += max11100.o
 obj-$(CONFIG_MAX1118) += max1118.o
diff --git a/drivers/iio/adc/ma35d1_eadc.c b/drivers/iio/adc/ma35d1_eadc.c
new file mode 100644
index 000000000000..b2733e816c49
--- /dev/null
+++ b/drivers/iio/adc/ma35d1_eadc.c
@@ -0,0 +1,740 @@
+// 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/bitops.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/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.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/types.h>
+
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+#define MA35D1_EADC_DAT(n)		(0x00 + (n) * 0x04)
+#define MA35D1_EADC_CTL			0x50
+#define MA35D1_EADC_SWTRG		0x54
+#define MA35D1_EADC_SCTL(n)		(0x80 + (n) * 0x04)
+#define MA35D1_EADC_INTSRC0		0xd0
+#define MA35D1_EADC_STATUS2		0xf8
+#define MA35D1_EADC_SELSMP0		0x140
+#define MA35D1_EADC_REFADJCTL		0x150
+
+#define MA35D1_EADC_CTL_ADCEN		BIT(0)
+#define MA35D1_EADC_CTL_ADCIEN0		BIT(2)
+#define MA35D1_EADC_CTL_DIFFEN		BIT(8)
+#define MA35D1_EADC_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_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_SPLIEN(n)	BIT(n)
+#define MA35D1_EADC_STATUS2_ADIF0	BIT(0)
+#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_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	1000
+
+struct ma35d1_adc {
+	struct regmap *regmap;
+	struct clk *clk;
+	struct reset_control *rst;
+	struct regulator *vref;
+	struct completion completion;
+	/* Protects 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,
+};
+
+static bool ma35d1_adc_valid_diff_pair(unsigned int vinp, unsigned int vinn)
+{
+	return (vinp == 0 && vinn == 4) ||
+	       (vinp == 1 && vinn == 5) ||
+	       (vinp == 2 && vinn == 6) ||
+	       (vinp == 3 && vinn == 7);
+}
+
+static int ma35d1_adc_set_bits(struct ma35d1_adc *adc, unsigned int reg,
+			       unsigned int bits)
+{
+	return regmap_set_bits(adc->regmap, reg, bits);
+}
+
+static int ma35d1_adc_clear_bits(struct ma35d1_adc *adc, unsigned int reg,
+				 unsigned int bits)
+{
+	return regmap_clear_bits(adc->regmap, reg, bits);
+}
+
+static int ma35d1_adc_disable_irq(struct ma35d1_adc *adc)
+{
+	return ma35d1_adc_clear_bits(adc, MA35D1_EADC_CTL,
+				     MA35D1_EADC_CTL_ADCIEN0);
+}
+
+static int ma35d1_adc_setup_reference(struct ma35d1_adc *adc)
+{
+	int ret;
+
+	if (adc->vref) {
+		ret = regmap_set_bits(adc->regmap, MA35D1_EADC_REFADJCTL,
+				      MA35D1_EADC_REFADJCTL_PDREF);
+		if (ret)
+			return ret;
+	} else {
+		ret = regmap_clear_bits(adc->regmap, MA35D1_EADC_REFADJCTL,
+					MA35D1_EADC_REFADJCTL_PDREF);
+		if (ret)
+			return ret;
+
+		ret = regmap_update_bits(adc->regmap, 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_hw_init(struct ma35d1_adc *adc)
+{
+	int ret;
+
+	ret = ma35d1_adc_disable_irq(adc);
+	if (ret)
+		return ret;
+
+	ret = ma35d1_adc_setup_reference(adc);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(adc->regmap, MA35D1_EADC_SELSMP0,
+				 MA35D1_EADC_SELSMP0_SMPT0_MASK,
+				 MA35D1_EADC_SELSMP_LONG);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(adc->regmap, MA35D1_EADC_STATUS2,
+			   MA35D1_EADC_STATUS2_ADIF0);
+	if (ret)
+		return ret;
+
+	return ma35d1_adc_set_bits(adc, MA35D1_EADC_CTL, MA35D1_EADC_CTL_ADCEN);
+}
+
+static int ma35d1_adc_hw_disable(struct ma35d1_adc *adc)
+{
+	int ret;
+
+	ret = ma35d1_adc_disable_irq(adc);
+	if (ret)
+		return ret;
+
+	return ma35d1_adc_clear_bits(adc, 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)
+{
+	unsigned int ctl;
+	int ret;
+
+	ctl = chan->differential ? MA35D1_EADC_CTL_DIFFEN |
+					  MA35D1_EADC_CTL_DMOF : 0;
+
+	ret = regmap_update_bits(adc->regmap, MA35D1_EADC_CTL,
+				 MA35D1_EADC_CTL_DIFFEN |
+				 MA35D1_EADC_CTL_DMOF, ctl);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(adc->regmap, MA35D1_EADC_SCTL(0),
+				 MA35D1_EADC_SCTL_CHSEL_MASK |
+				 MA35D1_EADC_SCTL_TRGSEL_MASK,
+				 FIELD_PREP(MA35D1_EADC_SCTL_CHSEL_MASK,
+					    chan->channel));
+	if (ret)
+		return ret;
+
+	return regmap_update_bits(adc->regmap, MA35D1_EADC_INTSRC0,
+				  MA35D1_EADC_INTSRC0_SPLIEN(0),
+				  MA35D1_EADC_INTSRC0_SPLIEN(0));
+}
+
+static int ma35d1_adc_read_conversion(struct ma35d1_adc *adc,
+				      const struct iio_chan_spec *chan, u16 *raw)
+{
+	unsigned int val;
+	long timeout;
+	int ret;
+
+	reinit_completion(&adc->completion);
+
+	ret = regmap_write(adc->regmap, MA35D1_EADC_STATUS2,
+			   MA35D1_EADC_STATUS2_ADIF0);
+	if (ret)
+		return ret;
+
+	ret = ma35d1_adc_config_channel(adc, chan);
+	if (ret)
+		return ret;
+
+	ret = ma35d1_adc_set_bits(adc, MA35D1_EADC_CTL, MA35D1_EADC_CTL_ADCIEN0);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(adc->regmap, MA35D1_EADC_SWTRG, BIT(0));
+	if (ret)
+		goto disable_irq;
+
+	timeout = wait_for_completion_timeout(&adc->completion,
+					      MA35D1_EADC_TIMEOUT);
+
+	ret = ma35d1_adc_disable_irq(adc);
+	if (ret)
+		return ret;
+
+	if (!timeout)
+		return -ETIMEDOUT;
+
+	ret = regmap_read(adc->regmap, 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;
+		*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;
+	unsigned int count;
+
+	count = 0;
+	for_each_set_bit(bit, scan_mask, indio_dev->masklength) {
+		scan_chan = &indio_dev->channels[bit];
+		count++;
+	}
+
+	if (count != 1 || scan_chan->type != IIO_VOLTAGE)
+		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;
+
+	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;
+
+	guard(mutex)(&adc->lock);
+	if (adc->suspended || !adc->scan_chan)
+		goto done;
+
+	ret = ma35d1_adc_read_conversion(adc, adc->scan_chan, &raw);
+	if (ret)
+		goto done;
+
+	scan[0] = raw & MA35D1_EADC_DAT_RESULT_12BIT;
+	iio_push_to_buffers_with_timestamp(indio_dev, scan, pf->timestamp);
+
+done:
+	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 const struct iio_info ma35d1_adc_info = {
+	.read_raw = ma35d1_adc_read_raw,
+	.update_scan_mode = ma35d1_adc_update_scan_mode,
+};
+
+static void ma35d1_adc_init_channel(struct iio_chan_spec *chan, u32 vinp,
+				    u32 vinn, unsigned int scan_index,
+				    bool differential)
+{
+	chan->type = IIO_VOLTAGE;
+	chan->indexed = 1;
+	chan->channel = 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 struct iio_chan_spec
+ma35d1_adc_timestamp_channel(unsigned int scan_index)
+{
+	struct iio_chan_spec chan = IIO_CHAN_SOFT_TIMESTAMP(scan_index);
+
+	return chan;
+}
+
+static int ma35d1_adc_parse_channels(struct iio_dev *indio_dev,
+				     struct device *dev)
+{
+	DECLARE_BITMAP(used_channels, MA35D1_EADC_MAX_EXT_CHANNELS);
+	struct iio_chan_spec *channels;
+	unsigned int scan_index;
+	int num_channels;
+	int ret;
+
+	bitmap_zero(used_channels, MA35D1_EADC_MAX_EXT_CHANNELS);
+
+	num_channels = device_get_child_node_count(dev);
+	if (!num_channels)
+		return dev_err_probe(dev, -ENODATA,
+				     "no ADC channels configured\n");
+
+	if (num_channels > MA35D1_EADC_MAX_EXT_CHANNELS)
+		return dev_err_probe(dev, -EINVAL, "too many ADC channels\n");
+
+	channels = devm_kcalloc(dev, num_channels + 1, sizeof(*channels), GFP_KERNEL);
+	if (!channels)
+		return -ENOMEM;
+
+	scan_index = 0;
+	device_for_each_child_node_scoped(dev, child) {
+		u32 diff[2];
+		u32 reg;
+		u32 vinn;
+		bool differential;
+
+		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 %u\n", reg);
+
+		if (test_and_set_bit(reg, used_channels))
+			return dev_err_probe(dev, -EINVAL,
+					     "duplicate ADC channel %u\n", reg);
+
+		differential = false;
+		vinn = 0;
+		if (fwnode_property_present(child, "diff-channels")) {
+			ret = fwnode_property_read_u32_array(child, "diff-channels", diff,
+							     ARRAY_SIZE(diff));
+			if (ret)
+				return dev_err_probe(dev, ret,
+						     "invalid diff-channels for channel %u\n",
+						     reg);
+
+			if (diff[0] != reg ||
+			    !ma35d1_adc_valid_diff_pair(diff[0], diff[1]))
+				return dev_err_probe(dev, -EINVAL,
+						     "invalid differential ADC channel %u-%u\n",
+						     diff[0], diff[1]);
+
+			if (test_and_set_bit(diff[1], used_channels))
+				return dev_err_probe(dev, -EINVAL,
+						     "ADC channel %u already used\n",
+						     diff[1]);
+
+			vinn = diff[1];
+			differential = true;
+		}
+
+		ma35d1_adc_init_channel(&channels[scan_index], reg, vinn,
+					scan_index, differential);
+		scan_index++;
+	}
+
+	channels[scan_index] = ma35d1_adc_timestamp_channel(scan_index);
+
+	indio_dev->channels = channels;
+	indio_dev->num_channels = scan_index + 1;
+	indio_dev->masklength = indio_dev->num_channels;
+
+	return 0;
+}
+
+static int ma35d1_adc_init_vref(struct ma35d1_adc *adc, struct device *dev)
+{
+	int ret;
+
+	adc->vref = devm_regulator_get_optional(dev, "vref");
+	if (IS_ERR(adc->vref)) {
+		if (PTR_ERR(adc->vref) != -ENODEV)
+			return dev_err_probe(dev, PTR_ERR(adc->vref),
+					     "failed to get VREF supply\n");
+
+		adc->vref = NULL;
+		adc->vref_mv = MA35D1_EADC_INTERNAL_VREF_MV;
+
+		return 0;
+	}
+
+	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 / 1000;
+
+	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 irq;
+	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;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	adc->irq = irq;
+	ret = devm_request_irq(dev, 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;
+
+	guard(mutex)(&adc->lock);
+
+	ret = ma35d1_adc_disable_irq(adc);
+	if (ret)
+		return ret;
+
+	synchronize_irq(adc->irq);
+
+	ret = ma35d1_adc_hw_disable(adc);
+	if (ret)
+		return ret;
+
+	clk_disable_unprepare(adc->clk);
+	adc->suspended = true;
+
+	return 0;
+}
+
+static int ma35d1_adc_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct ma35d1_adc *adc = iio_priv(indio_dev);
+	int ret;
+
+	guard(mutex)(&adc->lock);
+
+	ret = clk_prepare_enable(adc->clk);
+	if (ret)
+		return ret;
+
+	ret = ma35d1_adc_hw_init(adc);
+	if (ret) {
+		clk_disable_unprepare(adc->clk);
+		return ret;
+	}
+
+	adc->suspended = false;
+
+	return 0;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(ma35d1_adc_pm_ops,
+				ma35d1_adc_suspend, ma35d1_adc_resume);
+
+static const struct of_device_id ma35d1_adc_of_match[] = {
+	{ .compatible = "nuvoton,ma35d1-eadc" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ma35d1_adc_of_match);
+
+static struct platform_driver ma35d1_adc_driver = {
+	.probe = ma35d1_adc_probe,
+	.driver = {
+		.name = "ma35d1-eadc",
+		.of_match_table = ma35d1_adc_of_match,
+		.pm = pm_sleep_ptr(&ma35d1_adc_pm_ops),
+	},
+};
+module_platform_driver(ma35d1_adc_driver);
+
+MODULE_AUTHOR("Chi-Wen Weng <cwweng@nuvoton.com>");
+MODULE_DESCRIPTION("Nuvoton MA35D1 EADC driver");
+MODULE_LICENSE("GPL");
-- 
2.25.1

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] `ma35d1_adc_update_scan_mode` incorrectly rejects valid scan masks that include the timestamp channel.
- [High] A timed-out conversion can leave a pending ISR that spuriously completes the next conversion, leading to stale data.
- [Low] The driver uses non-existent macros `IIO_DEV_ACQUIRE_DIRECT_MODE` and `IIO_DEV_ACQUIRE_FAILED`.
- [Low] The driver attempts to assign to a non-existent `format` field in `struct iio_scan_type`.
- [Low] The `MA35D1_EADC` Kconfig entry lacks an architecture or `COMPILE_TEST` dependency.
- [Low] Mixing `guard()` with `goto` for error handling violates `cleanup.h` rules.
--

--- Patch [2]: [PATCH v2 2/2] iio: adc: Add Nuvoton MA35D1 EADC driver ---
commit e4312453e3f2955d13e6052b2ffd1d6a2a5739a1
Author: Chi-Wen Weng <cwweng@nuvoton.com>

iio: adc: Add Nuvoton MA35D1 EADC driver
    
This commit adds an IIO driver for the Nuvoton MA35D1 Enhanced ADC
controller. It supports direct raw reads and triggered buffered capture
using an external IIO trigger.

> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index ea3ba13973928..c657b3627a11a 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
[ ... ]
> @@ -844,6 +844,18 @@ 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"

[Severity: Low]
Is this missing a dependency on the architecture or COMPILE_TEST?
Without something like depends on ARCH_MA35 || COMPILE_TEST, this will
prompt users during kernel configuration on all architectures.

> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
> +	select REGMAP_MMIO
[ ... ]
> diff --git a/drivers/iio/adc/ma35d1_eadc.c b/drivers/iio/adc/ma35d1_eadc.c
> new file mode 100644
> index 0000000000000..b2733e816c49d
> --- /dev/null
> +++ b/drivers/iio/adc/ma35d1_eadc.c
[ ... ]
> +static int ma35d1_adc_read_conversion(struct ma35d1_adc *adc,
> +				      const struct iio_chan_spec *chan, u16 *raw)
> +{
[ ... ]
> +	timeout = wait_for_completion_timeout(&adc->completion,
> +					      MA35D1_EADC_TIMEOUT);
> +
> +	ret = ma35d1_adc_disable_irq(adc);
> +	if (ret)
> +		return ret;
> +
> +	if (!timeout)
> +		return -ETIMEDOUT;

[Severity: High]
Does this timeout path need a call to synchronize_irq()?

If the hardware interrupt fires right as the timeout expires, the completion
could be signalled by the delayed ISR after we return -ETIMEDOUT. The next
conversion would call reinit_completion(), but the pending ISR might
immediately complete it, returning stale data or failing.

> +
> +	ret = regmap_read(adc->regmap, MA35D1_EADC_DAT(0), &val);
[ ... ]
> +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;

[Severity: Low]
Will this compile? The IIO_DEV_ACQUIRE_DIRECT_MODE and IIO_DEV_ACQUIRE_FAILED
macros do not appear to exist in the IIO subsystem. 

Should this use iio_device_claim_direct_mode() or 
iio_device_claim_direct_scoped() instead?

> +
> +		guard(mutex)(&adc->lock);
> +		if (adc->suspended)
[ ... ]
> +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;
> +	unsigned int count;
> +
> +	count = 0;
> +	for_each_set_bit(bit, scan_mask, indio_dev->masklength) {
> +		scan_chan = &indio_dev->channels[bit];
> +		count++;
> +	}
> +
> +	if (count != 1 || scan_chan->type != IIO_VOLTAGE)
> +		return -EINVAL;

[Severity: High]
Does this logic unintentionally reject valid scan masks that include the
timestamp channel?

If a userspace application enables one voltage channel and the timestamp
channel, the mask will have 2 bits set. The loop will increment count
for both, causing count != 1 to trigger and reject the configuration.

> +
> +	adc->scan_chan = scan_chan;
[ ... ]
> +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;
> +
> +	guard(mutex)(&adc->lock);
> +	if (adc->suspended || !adc->scan_chan)
> +		goto done;

[Severity: Low]
This isn't a runtime bug, but does this violate the cleanup.h guidelines?

The documentation for cleanup helpers states that we should never mix
goto-based error handling with guard() declarations in the same function.

> +
> +	ret = ma35d1_adc_read_conversion(adc, adc->scan_chan, &raw);
[ ... ]
> +static void ma35d1_adc_init_channel(struct iio_chan_spec *chan, u32 vinp,
> +				    u32 vinn, unsigned int scan_index,
> +				    bool differential)
> +{
> +	chan->type = IIO_VOLTAGE;
> +	chan->indexed = 1;
> +	chan->channel = 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;

[Severity: Low]
Does struct iio_scan_type have a format field? 

It looks like the IIO API uses a char sign field instead, and
IIO_SCAN_FORMAT_SIGNED_INT does not exist. This will likely cause a build
failure.

> +	chan->scan_type.realbits = 12;
> +	chan->scan_type.storagebits = 16;
> +	chan->scan_type.endianness = IIO_CPU;
> +

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

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

[Andy Shevchenko]

On Mon, Jul 13, 2026 at 04:11:27PM +0800, Chi-Wen Weng wrote:

> Add an IIO driver for the Nuvoton MA35D1 Enhanced ADC controller.
> 
> The driver supports interrupt-driven direct raw reads and triggered
> buffered capture paced by an external IIO trigger. Buffered capture is
> limited to a single enabled voltage channel per scan in this initial
> implementation.
> 
> Channels are described by firmware child nodes. Single-ended external
> channels and the fixed hardware differential input pairs are supported.
> The driver reports IIO scale from either an optional external reference
> supply or the internal 1.6 V reference.
> 
> The driver also handles the functional clock and optional reset line.
> DMA support is not used by this initial driver.

...

> +#include <linux/bitfield.h>
> +#include <linux/bitmap.h>

bitmap.h implies bitops.h...

> +#include <linux/bitops.h>

...so you may drop this one.

> +#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/mod_devicetable.h>

Uwe made a series to get rid of this header. The platform_device.h seems
the one which will provide the thing.

> +#include <linux/module.h>
> +#include <linux/mutex.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/types.h>

...

> +#define MA35D1_EADC_DAT(n)		(0x00 + (n) * 0x04)
> +#define MA35D1_EADC_CTL			0x50
> +#define MA35D1_EADC_SWTRG		0x54
> +#define MA35D1_EADC_SCTL(n)		(0x80 + (n) * 0x04)
> +#define MA35D1_EADC_INTSRC0		0xd0
> +#define MA35D1_EADC_STATUS2		0xf8
> +#define MA35D1_EADC_SELSMP0		0x140
> +#define MA35D1_EADC_REFADJCTL		0x150

Please, make all register offsets fixed width, exempli gratia

define MA35D1_EADC_STATUS2		0x0f8

...

> +#define MA35D1_EADC_INTSRC0_SPLIEN(n)	BIT(n)

Useless? Can't BIT() be used directly? (Note, I don't know
if it's good or bad suggestion, wanting to understand a bit more.)

...

> +#define MA35D1_EADC_MAX_EXT_CHANNELS	8
> +#define MA35D1_EADC_INTERNAL_VREF_MV	1600

_mV (yes, as per SI).

> +#define MA35D1_EADC_TIMEOUT		msecs_to_jiffies(1000)
> +#define MA35D1_EADC_RESET_DELAY_US	10
> +#define MA35D1_EADC_REF_STABLE_US	1000

(1 * USEC_PER_MSEC) ?
If go this way, include time.h for the multiplier definition.

...

> +struct ma35d1_adc {
> +	struct regmap *regmap;
> +	struct clk *clk;
> +	struct reset_control *rst;
> +	struct regulator *vref;
> +	struct completion completion;
> +	/* Protects conversion state and register accesses from PM transitions. */
> +	struct mutex lock;
> +	const struct iio_chan_spec *scan_chan;
> +	unsigned int vref_mv;

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

No cache?

> +};

...

> +static bool ma35d1_adc_valid_diff_pair(unsigned int vinp, unsigned int vinn)
> +{
> +	return (vinp == 0 && vinn == 4) ||
> +	       (vinp == 1 && vinn == 5) ||
> +	       (vinp == 2 && vinn == 6) ||
> +	       (vinp == 3 && vinn == 7);

Wondering if this is just

	return (vinp >= 0 && vinp < 4 && (vinn == vinp + 4));

But I'm fine with the original, perhaps compiler may optimise that.

> +}

...

> +static int ma35d1_adc_set_bits(struct ma35d1_adc *adc, unsigned int reg,
> +			       unsigned int bits)
> +{
> +	return regmap_set_bits(adc->regmap, reg, bits);
> +}
> +
> +static int ma35d1_adc_clear_bits(struct ma35d1_adc *adc, unsigned int reg,
> +				 unsigned int bits)
> +{
> +	return regmap_clear_bits(adc->regmap, reg, bits);
> +}

What's the point in these wrappers?

...

> +static int ma35d1_adc_disable_irq(struct ma35d1_adc *adc)
> +{
> +	return ma35d1_adc_clear_bits(adc, MA35D1_EADC_CTL,
> +				     MA35D1_EADC_CTL_ADCIEN0);

I would make it one line (*yes, 84 characters).

> +}

...

> +static int ma35d1_adc_setup_reference(struct ma35d1_adc *adc)
> +{
> +	int ret;
> +
> +	if (adc->vref) {
> +		ret = regmap_set_bits(adc->regmap, MA35D1_EADC_REFADJCTL,
> +				      MA35D1_EADC_REFADJCTL_PDREF);
> +		if (ret)
> +			return ret;
> +	} else {
> +		ret = regmap_clear_bits(adc->regmap, MA35D1_EADC_REFADJCTL,
> +					MA35D1_EADC_REFADJCTL_PDREF);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_update_bits(adc->regmap, MA35D1_EADC_CTL,
> +					 MA35D1_EADC_CTL_VREFSEL_MASK,
> +					 MA35D1_EADC_CTL_VREFSEL_1V6);
> +		if (ret)
> +			return ret;
> +	}

Add a short comment with reference to a datasheet table/section/et cetera.

> +	fsleep(MA35D1_EADC_REF_STABLE_US);
> +
> +	return 0;
> +}

...

> +static int ma35d1_adc_hw_init(struct ma35d1_adc *adc)
> +{
> +	int ret;
> +
> +	ret = ma35d1_adc_disable_irq(adc);
> +	if (ret)
> +		return ret;
> +
> +	ret = ma35d1_adc_setup_reference(adc);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(adc->regmap, MA35D1_EADC_SELSMP0,
> +				 MA35D1_EADC_SELSMP0_SMPT0_MASK,
> +				 MA35D1_EADC_SELSMP_LONG);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(adc->regmap, MA35D1_EADC_STATUS2,
> +			   MA35D1_EADC_STATUS2_ADIF0);

With

	static regmap *map = adc->regmap;

this becomes

	ret = regmap_write(map, MA35D1_EADC_STATUS2, MA35D1_EADC_STATUS2_ADIF0);

exactly one line. Apply this trick everywhere and it might help reduce
amount of LoC.

> +	if (ret)
> +		return ret;
> +
> +	return ma35d1_adc_set_bits(adc, MA35D1_EADC_CTL, MA35D1_EADC_CTL_ADCEN);
> +}

...

> +static int ma35d1_adc_config_channel(struct ma35d1_adc *adc,
> +				     const struct iio_chan_spec *chan)
> +{
> +	unsigned int ctl;
> +	int ret;
> +
> +	ctl = chan->differential ? MA35D1_EADC_CTL_DIFFEN |
> +					  MA35D1_EADC_CTL_DMOF : 0;
> +
> +	ret = regmap_update_bits(adc->regmap, MA35D1_EADC_CTL,
> +				 MA35D1_EADC_CTL_DIFFEN |
> +				 MA35D1_EADC_CTL_DMOF, ctl);
> +	if (ret)
> +		return ret;

regmap_assign_bits()

> +	ret = regmap_update_bits(adc->regmap, MA35D1_EADC_SCTL(0),
> +				 MA35D1_EADC_SCTL_CHSEL_MASK |
> +				 MA35D1_EADC_SCTL_TRGSEL_MASK,
> +				 FIELD_PREP(MA35D1_EADC_SCTL_CHSEL_MASK,
> +					    chan->channel));
> +	if (ret)
> +		return ret;
> +
> +	return regmap_update_bits(adc->regmap, MA35D1_EADC_INTSRC0,
> +				  MA35D1_EADC_INTSRC0_SPLIEN(0),
> +				  MA35D1_EADC_INTSRC0_SPLIEN(0));
> +}

...

> +static int ma35d1_adc_update_scan_mode(struct iio_dev *indio_dev,
> +				       const unsigned long *scan_mask)
> +{
> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
> +	const struct iio_chan_spec *scan_chan = NULL;
> +	unsigned long bit;
> +	unsigned int count;
> +
> +	count = 0;
> +	for_each_set_bit(bit, scan_mask, indio_dev->masklength) {
> +		scan_chan = &indio_dev->channels[bit];
> +		count++;
> +	}
> +
> +	if (count != 1 || scan_chan->type != IIO_VOLTAGE)
> +		return -EINVAL;


This is interesting check. First of all, it checks if the only a single bit is
set in the whole mask, then it checks the type of the *last* listed channel.
The latter seems fragile to me, as it depends on ordering (yes, the limitation
to a single enabled channel helps).

With that being said, the both checks can be unrolled to the find_next_bit().

	/* Find the first enabled channel to scan */
	bit = find_next_bit(scan_mask, indio_dev->masklength, 0);
	if (bit >= indio_dev->masklength)
		return -EINVAL;

	/* Check that the type of the channel is what we are looking for */
	scan_chan = &indio_dev->channels[bit];
	if (scan_chan->type != IIO_VOLTAGE)
		return -EINVAL;

	/* Check that this is the only enabled channel */
	bit = find_next_bit(scan_mask, indio_dev->masklength, bit);
	if (bit < indio_dev->masklength)
		return -EINVAL;

Not sure that this will generate less code, though. So up to you.
One also might argue that the original code is easier to understand.

> +	adc->scan_chan = scan_chan;
> +
> +	return 0;
> +}

...

> +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;
> +
> +	guard(mutex)(&adc->lock);
> +	if (adc->suspended || !adc->scan_chan)
> +		goto done;

This is usually leads to a mess in the compiler. This is written in the top of
cleanup.h that goto should be avoided while using guard()() or similar macros.

> +	ret = ma35d1_adc_read_conversion(adc, adc->scan_chan, &raw);
> +	if (ret)
> +		goto done;
> +
> +	scan[0] = raw & MA35D1_EADC_DAT_RESULT_12BIT;
> +	iio_push_to_buffers_with_timestamp(indio_dev, scan, pf->timestamp);
> +
> +done:
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}

...

> +static void ma35d1_adc_init_channel(struct iio_chan_spec *chan, u32 vinp,
> +				    u32 vinn, unsigned int scan_index,
> +				    bool differential)
> +{
> +	chan->type = IIO_VOLTAGE;
> +	chan->indexed = 1;
> +	chan->channel = vinp;
> +	chan->scan_index = scan_index;
> +	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +					 BIT(IIO_CHAN_INFO_SCALE);

Broken indentation, better to have

	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE);

OR

	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 struct iio_chan_spec
> +ma35d1_adc_timestamp_channel(unsigned int scan_index)
> +{
> +	struct iio_chan_spec chan = IIO_CHAN_SOFT_TIMESTAMP(scan_index);
> +
> +	return chan;
> +}

Useless wrapper. Just put _SOFT_TIMESTAMP() in place.

...

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

I would return -ENOENT, as it's usual error code for 0 count in counting APIs
in the kernel. Also you can put it on a single line.

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

size_add() ?

> +	if (!channels)
> +		return -ENOMEM;
> +
> +	scan_index = 0;
> +	device_for_each_child_node_scoped(dev, child) {
> +		u32 diff[2];
> +		u32 reg;
> +		u32 vinn;
> +		bool differential;
> +
> +		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 %u\n", reg);
> +
> +		if (test_and_set_bit(reg, used_channels))
> +			return dev_err_probe(dev, -EINVAL,
> +					     "duplicate ADC channel %u\n", reg);

> +		differential = false;
> +		vinn = 0;

Make it an 'else' branch.

> +		if (fwnode_property_present(child, "diff-channels")) {
> +			ret = fwnode_property_read_u32_array(child, "diff-channels", diff,
> +							     ARRAY_SIZE(diff));
> +			if (ret)
> +				return dev_err_probe(dev, ret,
> +						     "invalid diff-channels for channel %u\n",
> +						     reg);
> +
> +			if (diff[0] != reg ||
> +			    !ma35d1_adc_valid_diff_pair(diff[0], diff[1]))
> +				return dev_err_probe(dev, -EINVAL,
> +						     "invalid differential ADC channel %u-%u\n",
> +						     diff[0], diff[1]);
> +
> +			if (test_and_set_bit(diff[1], used_channels))
> +				return dev_err_probe(dev, -EINVAL,
> +						     "ADC channel %u already used\n",
> +						     diff[1]);
> +
> +			vinn = diff[1];
> +			differential = true;
> +		}
> +
> +		ma35d1_adc_init_channel(&channels[scan_index], reg, vinn,
> +					scan_index, differential);
> +		scan_index++;
> +	}
> +
> +	channels[scan_index] = ma35d1_adc_timestamp_channel(scan_index);
> +
> +	indio_dev->channels = channels;
> +	indio_dev->num_channels = scan_index + 1;
> +	indio_dev->masklength = indio_dev->num_channels;
> +
> +	return 0;
> +}

...

> +static int ma35d1_adc_init_vref(struct ma35d1_adc *adc, struct device *dev)
> +{
> +	int ret;

> +	adc->vref = devm_regulator_get_optional(dev, "vref");
> +	if (IS_ERR(adc->vref)) {
> +		if (PTR_ERR(adc->vref) != -ENODEV)
> +			return dev_err_probe(dev, PTR_ERR(adc->vref),
> +					     "failed to get VREF supply\n");
> +
> +		adc->vref = NULL;
> +		adc->vref_mv = MA35D1_EADC_INTERNAL_VREF_MV;
> +
> +		return 0;
> +	}

It can be unnested:


	if (PTR_ERR(adc->vref) == -ENODEV) {
		adc->vref = NULL;
		adc->vref_mV = MA35D1_EADC_INTERNAL_VREF_mV;
		return 0;
	}
	if (IS_ERR(adc->vref))
		return dev_err_probe(dev, PTR_ERR(adc->vref),
				     "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 / 1000;

(MICRO / MILLI)

instead of plain 1000.

> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko

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

[Chi-Wen Weng]

Hi Andy,

Thank you for the detailed review.

 > bitmap.h implies bitops.h...
 >
 >> +#include <linux/bitops.h>
 >
 > ...so you may drop this one.

Will do. I will drop linux/bitops.h. I will also drop
linux/mod_devicetable.h since platform_device.h is enough here.

 > Please, make all register offsets fixed width, exempli gratia
 >
 > define MA35D1_EADC_STATUS2        0x0f8

Will fix all register offsets to use fixed-width hex values.

 >> +#define MA35D1_EADC_INTSRC0_SPLIEN(n)    BIT(n)
 >
 > Useless? Can't BIT() be used directly?

The field selects which sample module raises ADINT0. This initial driver
only uses sample module 0, so I will replace this helper with a fixed
named bit for SPLIEN0.

 >> +#define MA35D1_EADC_INTERNAL_VREF_MV    1600
 >
 > _mV (yes, as per SI).

Will rename the macro and field to use _mV.

 >> +#define MA35D1_EADC_REF_STABLE_US    1000
 >
 > (1 * USEC_PER_MSEC) ?
 > If go this way, include time.h for the multiplier definition.

Will change this to use USEC_PER_MSEC and include linux/time.h.

 >> +static const struct regmap_config ma35d1_adc_regmap_config = {
 >> +    .reg_bits = 32,
 >> +    .val_bits = 32,
 >> +    .reg_stride = 4,
 >> +    .max_register = MA35D1_EADC_REFADJCTL,
 >
 > No cache?

No register cache is intended for this MMIO ADC block. I will make that
explicit by setting .cache_type = REGCACHE_NONE.

 >> +static bool ma35d1_adc_valid_diff_pair(unsigned int vinp, unsigned 
int vinn)
 >
 > Wondering if this is just
 >
 >    return (vinp >= 0 && vinp < 4 && (vinn == vinp + 4));

Yes, the valid differential pairs are 0-4, 1-5, 2-6 and 3-7. Since vinp
is unsigned, I will simplify this to:

     return vinp < 4 && vinn == vinp + 4;

 >> +static int ma35d1_adc_set_bits(...)
 >> +static int ma35d1_adc_clear_bits(...)
 >
 > What's the point in these wrappers?

There is no strong reason to keep them. I will remove the wrappers and
use regmap_set_bits() / regmap_clear_bits() directly.

 >> +static int ma35d1_adc_setup_reference(struct ma35d1_adc *adc)
 >
 > Add a short comment with reference to a datasheet table/section/et 
cetera.

Will add a short comment describing the REFADJCTL/PDREF and VREFSEL
settings according to the MA35D1 EADC reference control description.

 > With
 >
 >    static regmap *map = adc->regmap;
 >
 > this becomes ...

Will use a local regmap pointer in functions that access several
registers. That should make the code shorter and easier to read.

 >> +    ret = regmap_update_bits(adc->regmap, MA35D1_EADC_CTL,
 >> +                 MA35D1_EADC_CTL_DIFFEN |
 >> +                 MA35D1_EADC_CTL_DMOF, ctl);
 >
 > regmap_assign_bits()

Will use regmap_assign_bits() for the differential mode bits.

 >> +static int ma35d1_adc_update_scan_mode(...)
 >
 > This is interesting check...

I will rework this to explicitly accept exactly one IIO_VOLTAGE scan
channel. This avoids depending on the last bit found in the scan mask.

 >> +    guard(mutex)(&adc->lock);
 >> +    if (adc->suspended || !adc->scan_chan)
 >> +        goto done;
 >
 > This is usually leads to a mess in the compiler...

Will fix this by removing the goto from the trigger handler and using a
scoped_guard() block instead.

 >> +    chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
 >> +                     BIT(IIO_CHAN_INFO_SCALE);
 >
 > Broken indentation...

Will fix the indentation.

 >> +static struct iio_chan_spec
 >> +ma35d1_adc_timestamp_channel(unsigned int scan_index)
 >
 > Useless wrapper. Just put _SOFT_TIMESTAMP() in place.

Will remove the helper and assign the timestamp channel directly using a
compound literal.

 >> +    if (!num_channels)
 >> +        return dev_err_probe(dev, -ENODATA,
 >> +                     "no ADC channels configured\n");
 >
 > I would return -ENOENT...

Will change this to -ENOENT.

 >> +    channels = devm_kcalloc(dev, num_channels + 1, 
sizeof(*channels), GFP_KERNEL);
 >
 > size_add() ?

Will use size_add(num_channels, 1).

 >> +        differential = false;
 >> +        vinn = 0;
 >
 > Make it an 'else' branch.

Will do.

 >> +    adc->vref = devm_regulator_get_optional(dev, "vref");
 >
 > It can be unnested:

Will unnest the optional regulator handling in v3.

 >> +    adc->vref_mv = ret / 1000;
 >
 > (MICRO / MILLI)
 >
 > instead of plain 1000.

Will fix this as well. The regulator API returns the voltage in
microvolts, while the driver stores the reference in millivolts for
IIO scale reporting. I will rename the field to vref_mV and use:

     adc->vref_mV = ret / (MICRO / MILLI);

with linux/units.h included, instead of using the plain 1000 constant.

Thanks again for the review. I will address these comments in the next
revision.

Best regards,
Chi-Wen