* [PATCH v4 0/2] iio: adc: Add Nuvoton MA35D1 EADC support @ 2026-11-04 9:11 Chi-Wen Weng 2026-11-04 9:11 ` [PATCH v4 1/2] dt-bindings: iio: adc: Add Nuvoton MA35D1 EADC Chi-Wen Weng 2026-11-04 9:11 ` [PATCH v4 2/2] iio: adc: Add Nuvoton MA35D1 EADC driver Chi-Wen Weng 0 siblings, 2 replies; 6+ messages in thread From: Chi-Wen Weng @ 2026-11-04 9:11 UTC (permalink / raw) To: jic23, robh, krzk+dt, conor+dt Cc: dlechner, nuno.sa, andy, linux-arm-kernel, linux-iio, devicetree, linux-kernel, cwweng, cwweng.linux From: Chi-Wen Weng <cwweng@nuvoton.com> This series adds IIO support for the Enhanced Analog-to-Digital Converter (EADC) found on Nuvoton MA35D1 SoCs. The EADC provides eight external analog inputs which may be used as single-ended inputs or as four fixed differential pairs: 0-4, 1-5, 2-6 and 3-7. The initial driver supports direct conversion and triggered buffered capture using sample module 0. Although the hardware provides multiple sample modules, this version of the driver uses only sample module 0 and therefore supports one active voltage channel per buffered scan. The series is based on Linux v7.4-rc1. Changes in v4: - Use the common ADC diff-channels property to describe differential inputs and keep reg as the logical channel index. - Restrict differential inputs to the four hardware-supported pairs. - Restrict channel child nodes to the ADC properties supported by the MA35D1 EADC. - Prevent single-channel and diff-channels from being specified together. - Document that each single-ended input and differential input pair may be described only once. - Remove interrupt-names and simplify the interrupt descriptions. - Update the binding example to demonstrate separation of the logical channel index from the physical ADC inputs. - Update channel parsing to match the revised binding semantics. - Store physical ADC inputs in the IIO channel/channel2 fields and use address for the logical DT channel index. - Reject duplicate physical single-ended inputs and differential input pairs. - Use the physical positive input when programming the sample-module channel selection and the logical index for firmware-node channel translation. - Use interrupt index 0 instead of looking up ADINT0 by name. - Add the standard one-hot scan-mask validation callback. - Use iio_for_each_active_channel() when selecting the buffered channel. - Use a fixed scan structure for the single-channel buffered layout. - Limit conversion results to the 12-bit ADC data width. - Always report -ETIMEDOUT when a conversion times out. - Make timeout recovery best effort so interrupt disable, control reset, IRQ synchronization and stale status cleanup are all attempted even if an earlier recovery step fails. - Use devm_regulator_get_enable_read_voltage() for the optional external reference supply. - Rework clock management to explicitly balance prepare/enable state across probe, suspend, resume, error paths and driver teardown. - Improve suspend and resume error-path handling. - Remove the redundant explicit REGCACHE_NONE setting. - Split interrupt status read failures from interrupt-source checks and handle failures when clearing the interrupt status. - Use symbolic definitions for the ADC resolution and differential scale width. - Use cleanup guards for mutex handling in buffered capture and keep trigger notification outside the locked section. - Use more specific error codes for channel capacity, duplicate channel definitions and out-of-range physical inputs. - Apply the requested regmap, control-flow and coding-style cleanups. 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 | 163 ++++ drivers/iio/adc/Kconfig | 14 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/ma35d1_eadc.c | 912 ++++++++++++++++++ 4 files changed, 1090 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml create mode 100644 drivers/iio/adc/ma35d1_eadc.c base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4 -- 2.25.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v4 1/2] dt-bindings: iio: adc: Add Nuvoton MA35D1 EADC 2026-11-04 9:11 [PATCH v4 0/2] iio: adc: Add Nuvoton MA35D1 EADC support Chi-Wen Weng @ 2026-11-04 9:11 ` Chi-Wen Weng 2026-11-04 9:18 ` sashiko-bot 2026-11-04 9:11 ` [PATCH v4 2/2] iio: adc: Add Nuvoton MA35D1 EADC driver Chi-Wen Weng 1 sibling, 1 reply; 6+ messages in thread From: Chi-Wen Weng @ 2026-11-04 9:11 UTC (permalink / raw) To: jic23, robh, krzk+dt, conor+dt Cc: dlechner, nuno.sa, andy, linux-arm-kernel, linux-iio, devicetree, linux-kernel, cwweng, cwweng.linux From: Chi-Wen Weng <cwweng@nuvoton.com> Add a binding for the Enhanced Analog-to-Digital Converter found on Nuvoton MA35D1 SoCs. The controller has eight external analog inputs. They can be used as single-ended inputs or as the four fixed differential pairs 0-4, 1-5, 2-6 and 3-7. Use the common ADC channel properties to describe the physical input independently of the logical channel index. 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..85b0feac0236 --- /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 (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. + +properties: + compatible: + const: nuvoton,ma35d1-eadc + + reg: + maxItems: 1 + + interrupts: + items: + - description: ADINT0 + - description: ADINT1 + - description: ADINT2 + - description: 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# + description: | + External EADC channel description. The reg property identifies the + logical IIO channel index. + + A single-ended channel may use single-channel to select an external + analog input independently of reg. If single-channel and diff-channels + are both absent, the external analog input is selected by reg. + + A differential channel uses diff-channels to select one of the fixed + hardware input pairs: 0-4, 1-5, 2-6 or 3-7. Each single-ended input + and each differential input pair may be described at most once. + + properties: + reg: + maximum: 7 + + single-channel: + maximum: 7 + + diff-channels: + oneOf: + - items: + - const: 0 + - const: 4 + - items: + - const: 1 + - const: 5 + - items: + - const: 2 + - const: 6 + - items: + - const: 3 + - const: 7 + + allOf: + - not: + required: + - single-channel + - diff-channels + + required: + - reg + + additionalProperties: false + +required: + - compatible + - reg + - interrupts + - 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>; + clocks = <&clk EADC_GATE>; + resets = <&sys MA35D1_RESET_EADC0>; + + #io-channel-cells = <1>; + #address-cells = <1>; + #size-cells = <0>; + + channel@0 { + reg = <0>; + single-channel = <2>; + }; + + channel@1 { + reg = <1>; + diff-channels = <0 4>; + }; + }; + }; + +... -- 2.25.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: iio: adc: Add Nuvoton MA35D1 EADC 2026-11-04 9:11 ` [PATCH v4 1/2] dt-bindings: iio: adc: Add Nuvoton MA35D1 EADC Chi-Wen Weng @ 2026-11-04 9:18 ` sashiko-bot 0 siblings, 0 replies; 6+ messages in thread From: sashiko-bot @ 2026-11-04 9:18 UTC (permalink / raw) To: Chi-Wen Weng; +Cc: devicetree, robh, conor+dt Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] dt-bindings: iio: adc: ma35d1-eadc lacks io-channel specifier for VBAT -- commit 10ca0224f7b8ba3a2de52a1dfd8d73f64b596541 Author: Chi-Wen Weng <cwweng@nuvoton.com> dt-bindings: iio: adc: Add Nuvoton MA35D1 EADC This commit adds the Device Tree binding schema for the Nuvoton MA35D1 Enhanced Analog-to-Digital Converter (EADC), describing its eight external analog inputs (usable as single-ended channels or four fixed differential pairs) and one internal VBAT input. > 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..85b0feac0236b > --- /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 (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. [ ... ] > + '#io-channel-cells': > + const: 1 > + description: > + The cell selects the logical EADC channel identified by the reg value > + of the corresponding channel child node. [Severity: Medium] As reported by Jonathan Cameron on v3 [1], how can an IIO consumer in Device Tree reference the internal VBAT input if #io-channel-cells in Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml only selects the logical EADC channel identified by the reg value of a channel@[0-7] child node, while description states that the internally connected VBAT input is not described by a channel child node? Also, should the top-level description block use > rather than | since it only needs to preserve paragraph breaks? [1] https://lore.kernel.org/r/20261101001934.23588aa2@jic23-hlaptop -- Sashiko AI review · https://sashiko.dev/#/patchset/20261104091136.151269-1-cwweng.linux@gmail.com?part=1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v4 2/2] iio: adc: Add Nuvoton MA35D1 EADC driver 2026-11-04 9:11 [PATCH v4 0/2] iio: adc: Add Nuvoton MA35D1 EADC support Chi-Wen Weng 2026-11-04 9:11 ` [PATCH v4 1/2] dt-bindings: iio: adc: Add Nuvoton MA35D1 EADC Chi-Wen Weng @ 2026-11-04 9:11 ` Chi-Wen Weng 2026-11-04 9:24 ` sashiko-bot 2026-11-04 11:50 ` Andy Shevchenko 1 sibling, 2 replies; 6+ messages in thread From: Chi-Wen Weng @ 2026-11-04 9:11 UTC (permalink / raw) To: jic23, robh, krzk+dt, conor+dt Cc: dlechner, nuno.sa, andy, linux-arm-kernel, linux-iio, devicetree, linux-kernel, cwweng, cwweng.linux From: Chi-Wen Weng <cwweng@nuvoton.com> Add an IIO driver for the Enhanced Analog-to-Digital Converter found on Nuvoton MA35D1 SoCs. Support single-ended and fixed differential input pairs, direct conversion, and triggered buffered capture using sample module 0. The hardware supports multiple sample modules, but this initial driver uses sample module 0 only and therefore supports one active voltage channel per buffered scan. 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 | 912 ++++++++++++++++++++++++++++++++++ 3 files changed, 927 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..b45ad3ef5264 --- /dev/null +++ b/drivers/iio/adc/ma35d1_eadc.c @@ -0,0 +1,912 @@ +// 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_RESOLUTION 12 +#define MA35D1_EADC_DIFF_SCALE_BITS (MA35D1_EADC_RESOLUTION - 1) + +#define MA35D1_EADC_DAT_RESULT_12BIT GENMASK(MA35D1_EADC_RESOLUTION - 1, 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_DIFF_PAIRS 4 +#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 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 clk_enabled; + bool external_vref; + 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 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->external_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, 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) +{ + struct ma35d1_adc *adc = data; + + if (adc->clk_enabled) + ma35d1_adc_hw_disable(adc); +} + +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->channel)); + if (ret) + return ret; + + return regmap_update_bits(map, MA35D1_EADC_INTSRC0, + MA35D1_EADC_INTSRC0_MASK, + MA35D1_EADC_INTSRC0_SPLIEN0); +} + +static void ma35d1_adc_abort_conversion(struct ma35d1_adc *adc) +{ + struct regmap *map = adc->regmap; + struct device *dev = regmap_get_device(map); + int ret; + + ret = ma35d1_adc_disable_irq(adc); + if (ret) + dev_err(dev, "failed to disable ADC interrupt: %d\n", ret); + + /* + * 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. + */ + ret = ma35d1_adc_reset_control(adc); + if (ret) + dev_err(dev, "failed to reset timed out conversion: %d\n", ret); + + /* Drain an interrupt which may already have been latched by the GIC. */ + synchronize_irq(adc->irq); + + ret = regmap_write(map, MA35D1_EADC_STATUS2, MA35D1_EADC_STATUS2_ADIF0); + if (ret) + dev_err(dev, "failed to clear ADC interrupt status: %d\n", 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) { + ma35d1_adc_abort_conversion(adc); + return -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_12BIT, 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); + struct regmap *map = adc->regmap; + unsigned int status; + int ret; + + ret = regmap_read(map, MA35D1_EADC_STATUS2, &status); + if (ret) + return IRQ_NONE; + + if (!(status & MA35D1_EADC_STATUS2_ADIF0)) + return IRQ_NONE; + + ret = regmap_write(map, MA35D1_EADC_STATUS2, MA35D1_EADC_STATUS2_ADIF0); + if (ret) + dev_err_ratelimited(regmap_get_device(map), + "failed to clear ADC interrupt status: %d\n", ret); + + complete(&adc->completion); + + return IRQ_HANDLED; +} + +static int ma35d1_adc_sign_extend(u16 raw) +{ + return sign_extend32(raw, MA35D1_EADC_DIFF_SCALE_BITS); +} + +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; + + 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 ? MA35D1_EADC_DIFF_SCALE_BITS : + MA35D1_EADC_RESOLUTION; + 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); + int bit; + + adc->scan_chan = NULL; + iio_for_each_active_channel(indio_dev, bit) { + const struct iio_chan_spec *chan = &indio_dev->channels[bit]; + + if (chan->type != IIO_VOLTAGE) + return -EINVAL; + + adc->scan_chan = chan; + } + + if (!adc->scan_chan) + return -EINVAL; + + 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; + + if (!adc->scan_chan) + return -EINVAL; + + return 0; +} + +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 void ma35d1_adc_trigger_capture(struct iio_poll_func *pf) +{ + struct iio_dev *indio_dev = pf->indio_dev; + struct ma35d1_adc *adc = iio_priv(indio_dev); + struct { + u16 channel; + aligned_s64 timestamp; + } scan = { }; + u16 raw; + int ret; + + guard(mutex)(&adc->lock); + + if (adc->suspended || !adc->scan_chan) + return; + + ret = ma35d1_adc_read_conversion(adc, adc->scan_chan, &raw); + if (ret) + return; + + scan.channel = raw; + iio_push_to_buffers_with_ts(indio_dev, &scan, sizeof(scan), pf->timestamp); +} + +static irqreturn_t ma35d1_adc_trigger_handler(int irq, void *p) +{ + struct iio_poll_func *pf = p; + + ma35d1_adc_trigger_capture(pf); + iio_trigger_notify_done(pf->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, + .validate_scan_mask = iio_validate_scan_mask_onehot, +}; + +static int ma35d1_adc_fwnode_xlate(struct iio_dev *indio_dev, + const struct fwnode_reference_args *iiospec) +{ + if (iiospec->nargs != 1) + return -EINVAL; + + for (unsigned int i = 0; i < indio_dev->num_channels; i++) { + const struct iio_chan_spec *chan = &indio_dev->channels[i]; + + if (chan->type == IIO_VOLTAGE && + chan->address == 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 int ma35d1_adc_parse_channels(struct iio_dev *indio_dev, + struct device *dev) +{ + DECLARE_BITMAP(used_single_inputs, MA35D1_EADC_MAX_EXT_CHANNELS); + DECLARE_BITMAP(used_diff_pairs, MA35D1_EADC_NUM_DIFF_PAIRS); + 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_single_inputs, MA35D1_EADC_MAX_EXT_CHANNELS); + bitmap_zero(used_diff_pairs, MA35D1_EADC_NUM_DIFF_PAIRS); + 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, -ENOSPC, "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 has_diff, has_single; + u32 reg, vinp, vinn; + + ret = fwnode_property_read_u32(child, "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, -ENOSPC, + "invalid ADC channel index %u\n", reg); + + if (test_and_set_bit(reg, used_regs)) + return dev_err_probe(dev, -EEXIST, + "duplicate ADC channel index %u\n", reg); + + has_single = fwnode_property_present(child, "single-channel"); + has_diff = fwnode_property_present(child, "diff-channels"); + if (has_single && has_diff) + return dev_err_probe(dev, -EINVAL, + "channel %u has conflicting input properties\n", + reg); + + if (has_diff) { + u32 inputs[2]; + + ret = fwnode_property_read_u32_array(child, "diff-channels", + inputs, ARRAY_SIZE(inputs)); + if (ret) + return dev_err_probe(dev, ret, + "invalid diff-channels for index %u\n", + reg); + + vinp = inputs[0]; + vinn = inputs[1]; + if (vinp >= 4 || vinn != vinp + 4) + return dev_err_probe(dev, -ECHRNG, + "invalid differential inputs %u-%u\n", + vinp, vinn); + + if (test_and_set_bit(vinp, used_diff_pairs)) + return dev_err_probe(dev, -EEXIST, + "duplicate differential inputs %u-%u\n", + vinp, vinn); + + channels[scan_index] = (struct iio_chan_spec) { + .type = IIO_VOLTAGE, + .indexed = 1, + .differential = 1, + .channel = vinp, + .channel2 = vinn, + .address = reg, + .scan_index = scan_index, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE), + .scan_type = { + .format = IIO_SCAN_FORMAT_SIGNED_INT, + .realbits = MA35D1_EADC_RESOLUTION, + .storagebits = 16, + .endianness = IIO_CPU, + }, + }; + } else { + if (has_single) { + 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); + } else { + vinp = reg; + } + + if (vinp >= MA35D1_EADC_MAX_EXT_CHANNELS) + return dev_err_probe(dev, -ECHRNG, + "invalid ADC input %u\n", vinp); + + if (test_and_set_bit(vinp, used_single_inputs)) + return dev_err_probe(dev, -EEXIST, + "duplicate ADC input %u\n", vinp); + + channels[scan_index] = (struct iio_chan_spec) { + .type = IIO_VOLTAGE, + .indexed = 1, + .channel = vinp, + .address = reg, + .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 = MA35D1_EADC_RESOLUTION, + .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; + + ret = devm_regulator_get_enable_read_voltage(dev, "vref"); + if (ret == -ENODEV) { + adc->vref_mV = MA35D1_EADC_INTERNAL_VREF_mV; + return 0; + } + if (ret < 0) + return dev_err_probe(dev, ret, "failed to get VREF supply\n"); + + if (!ret) + return dev_err_probe(dev, -EINVAL, "invalid VREF voltage\n"); + + adc->external_vref = true; + adc->vref_mV = ret / (MICRO / MILLI); + + return 0; +} + +static void ma35d1_adc_clk_disable_action(void *data) +{ + struct ma35d1_adc *adc = data; + + if (!adc->clk_enabled) + return; + + clk_disable_unprepare(adc->clk); + adc->clk_enabled = false; +} + +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(dev, NULL); + if (IS_ERR(adc->clk)) + return dev_err_probe(dev, PTR_ERR(adc->clk), + "failed to get ADC clock\n"); + + ret = clk_prepare_enable(adc->clk); + if (ret) + return dev_err_probe(dev, ret, "failed to enable ADC clock\n"); + + adc->clk_enabled = true; + ret = devm_add_action_or_reset(dev, ma35d1_adc_clk_disable_action, adc); + if (ret) + return ret; + + 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(pdev, 0); + 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); + struct regmap *map = adc->regmap; + 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(map, MA35D1_EADC_CTL, MA35D1_EADC_CTL_ADCEN); + if (ret) { + /* + * ADCIEN0 is enabled only while a conversion is active. Holding + * adc->lock here guarantees that no conversion is in progress, so + * leave it disabled. The next conversion enables it again. + */ + goto resume_triggering; + } + + adc->suspended = true; + clk_disable_unprepare(adc->clk); + adc->clk_enabled = false; + 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; + + adc->clk_enabled = true; + ret = ma35d1_adc_hw_init(adc); + if (ret) { + clk_disable_unprepare(adc->clk); + adc->clk_enabled = false; + 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v4 2/2] iio: adc: Add Nuvoton MA35D1 EADC driver 2026-11-04 9:11 ` [PATCH v4 2/2] iio: adc: Add Nuvoton MA35D1 EADC driver Chi-Wen Weng @ 2026-11-04 9:24 ` sashiko-bot 2026-11-04 11:50 ` Andy Shevchenko 1 sibling, 0 replies; 6+ messages in thread From: sashiko-bot @ 2026-11-04 9:24 UTC (permalink / raw) To: Chi-Wen Weng; +Cc: devicetree, robh, conor+dt Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] iio: adc: ma35d1: unclocked MMIO access when disabling buffer or suspending -- commit 42a7961767520f06d7be9ff697854a2064ddd67f Author: Chi-Wen Weng <cwweng@nuvoton.com> iio: adc: Add Nuvoton MA35D1 EADC driver This commit adds an IIO ADC driver for the Enhanced Analog-to-Digital Converter on Nuvoton MA35D1 SoCs, supporting single-ended and differential inputs, direct conversions, and triggered buffered capture using sample module 0. > diff --git a/drivers/iio/adc/ma35d1_eadc.c b/drivers/iio/adc/ma35d1_eadc.c > new file mode 100644 > index 0000000000000..b45ad3ef52645 > --- /dev/null > +++ b/drivers/iio/adc/ma35d1_eadc.c > @@ -0,0 +1,912 @@ [ ... ] > +static void ma35d1_adc_hw_disable_action(void *data) > +{ > + struct ma35d1_adc *adc = data; > + > + if (adc->clk_enabled) > + ma35d1_adc_hw_disable(adc); > +} [ ... ] > +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 int ma35d1_adc_suspend(struct device *dev) > +{ > + struct iio_dev *indio_dev = dev_get_drvdata(dev); > + struct ma35d1_adc *adc = iio_priv(indio_dev); > + struct regmap *map = adc->regmap; > + 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(map, MA35D1_EADC_CTL, MA35D1_EADC_CTL_ADCEN); > + if (ret) { > + /* > + * ADCIEN0 is enabled only while a conversion is active. Holding > + * adc->lock here guarantees that no conversion is in progress, so > + * leave it disabled. The next conversion enables it again. > + */ > + goto resume_triggering; > + } > + > + adc->suspended = true; > + clk_disable_unprepare(adc->clk); > + adc->clk_enabled = false; > + 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; > + > + adc->clk_enabled = true; > + ret = ma35d1_adc_hw_init(adc); > + if (ret) { > + clk_disable_unprepare(adc->clk); > + adc->clk_enabled = false; > + goto unlock; > + } > + > + adc->suspended = false; > + mutex_unlock(&adc->lock); > + > + return iio_device_resume_triggering(indio_dev); > + > +unlock: > + mutex_unlock(&adc->lock); > + > + return ret; > +} [Severity: High] Can ma35d1_adc_buffer_predisable() and ma35d1_adc_suspend() access EADC MMIO registers while adc->clk is disabled? During system suspend, ma35d1_adc_suspend() sets adc->suspended = true, calls clk_disable_unprepare(adc->clk), and sets adc->clk_enabled = false. If a subsequent ma35d1_adc_resume() fails in clk_prepare_enable() or ma35d1_adc_hw_init(), its error path leaves adc->clk_enabled = false and adc->suspended = true while any previously enabled triggered buffer remains active. When userspace later disables the buffer or unbinds the driver (or if the driver is unbound while suspended, since devm_iio_device_register() teardown runs iio_device_unregister() -> iio_disable_all_buffers() -> ma35d1_adc_buffer_predisable() before ma35d1_adc_hw_disable_action()), ma35d1_adc_buffer_predisable() unconditionally calls ma35d1_adc_disable_irq() and regmap_write(adc->regmap, MA35D1_EADC_STATUS2, MA35D1_EADC_STATUS2_ADIF0) without checking adc->suspended or adc->clk_enabled, accessing MMIO registers with the peripheral clock gated. Similarly, if another suspend cycle runs after a failed resume, ma35d1_adc_suspend() does not check whether the device is already suspended before accessing registers, calling clk_disable_unprepare(adc->clk) a second time, and calling iio_device_suspend_triggering(indio_dev) again. Should ma35d1_adc_buffer_predisable() and ma35d1_adc_suspend() check adc->suspended (or adc->clk_enabled, as ma35d1_adc_hw_disable_action() does) before touching the hardware registers and disabling the clock? -- Sashiko AI review · https://sashiko.dev/#/patchset/20261104091136.151269-1-cwweng.linux@gmail.com?part=2 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 2/2] iio: adc: Add Nuvoton MA35D1 EADC driver 2026-11-04 9:11 ` [PATCH v4 2/2] iio: adc: Add Nuvoton MA35D1 EADC driver Chi-Wen Weng 2026-11-04 9:24 ` sashiko-bot @ 2026-11-04 11:50 ` Andy Shevchenko 1 sibling, 0 replies; 6+ messages in thread From: Andy Shevchenko @ 2026-11-04 11:50 UTC (permalink / raw) To: Chi-Wen Weng Cc: jic23, robh, krzk+dt, conor+dt, dlechner, nuno.sa, andy, linux-arm-kernel, linux-iio, devicetree, linux-kernel, cwweng On Wed, Nov 04, 2026 at 05:11:36PM +0800, Chi-Wen Weng wrote: > Add an IIO driver for the Enhanced Analog-to-Digital Converter found on > Nuvoton MA35D1 SoCs. > > Support single-ended and fixed differential input pairs, direct > conversion, and triggered buffered capture using sample module 0. > > The hardware supports multiple sample modules, but this initial driver > uses sample module 0 only and therefore supports one active voltage > channel per buffered scan. ... > +struct ma35d1_adc { > + struct regmap *regmap; > + struct clk *clk; > + struct reset_control *rst; > + 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 clk_enabled; > + bool external_vref; > + bool suspended; > +}; Now when it has optimal layout it's better to regroup the fields with some ties. struct ma35d1_adc { struct regmap *regmap; struct clk *clk; struct reset_control *rst; struct completion completion; /* Protect conversion state and register accesses from PM transitions. */ struct mutex lock; const struct iio_chan_spec *scan_chan; int irq; unsigned int vref_mV; bool external_vref; bool clk_enabled; 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, Add a comment why no cache is enabled. > +}; ... > +static int ma35d1_adc_hw_init(struct ma35d1_adc *adc) > +{ > + struct regmap *map = adc->regmap; > + unsigned int i; Not used outside the loops, just declare inside respective for:s. > +} ... > +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; Use room on the previous line(s). > + ret = regmap_update_bits(map, MA35D1_EADC_SCTL(0), mask, > + FIELD_PREP(MA35D1_EADC_SCTL_CHSEL_MASK, > + chan->channel)); > + if (ret) > + return ret; > + > + return regmap_update_bits(map, MA35D1_EADC_INTSRC0, > + MA35D1_EADC_INTSRC0_MASK, > + MA35D1_EADC_INTSRC0_SPLIEN0); > +} ... > +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); Make it a single (yes, a bit longer) line. > + if (!timeout) { > + ma35d1_adc_abort_conversion(adc); > + return -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_12BIT, 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); > + struct regmap *map = adc->regmap; > + unsigned int status; > + int ret; > + > + ret = regmap_read(map, MA35D1_EADC_STATUS2, &status); > + if (ret) > + return IRQ_NONE; > + > + if (!(status & MA35D1_EADC_STATUS2_ADIF0)) > + return IRQ_NONE; regmap_test_bits() ? It will hide status OTOH. Up to you. > + ret = regmap_write(map, MA35D1_EADC_STATUS2, MA35D1_EADC_STATUS2_ADIF0); > + if (ret) > + dev_err_ratelimited(regmap_get_device(map), > + "failed to clear ADC interrupt status: %d\n", ret); > + > + complete(&adc->completion); > + > + return IRQ_HANDLED; > +} ... > +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; > + > + 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 ? MA35D1_EADC_DIFF_SCALE_BITS : > + MA35D1_EADC_RESOLUTION; Broken indentation. > + 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); > + int bit; > + adc->scan_chan = NULL; Should we update this even in case of wrong channel type? > + iio_for_each_active_channel(indio_dev, bit) { > + const struct iio_chan_spec *chan = &indio_dev->channels[bit]; > + > + if (chan->type != IIO_VOLTAGE) > + return -EINVAL; > + > + adc->scan_chan = chan; > + } > + > + if (!adc->scan_chan) > + return -EINVAL; > + > + return 0; > +} ... > + return regmap_write(adc->regmap, MA35D1_EADC_STATUS2, > + MA35D1_EADC_STATUS2_ADIF0); struct regmap *map = adc->regmap; ... return regmap_write(map, MA35D1_EADC_STATUS2, MA35D1_EADC_STATUS2_ADIF0); *yes, it's 81 characters long. ... > +static int ma35d1_adc_parse_channels(struct iio_dev *indio_dev, > + struct device *dev) > +{ > + DECLARE_BITMAP(used_single_inputs, MA35D1_EADC_MAX_EXT_CHANNELS); > + DECLARE_BITMAP(used_diff_pairs, MA35D1_EADC_NUM_DIFF_PAIRS); > + 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_single_inputs, MA35D1_EADC_MAX_EXT_CHANNELS); > + bitmap_zero(used_diff_pairs, MA35D1_EADC_NUM_DIFF_PAIRS); > + 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, -ENOSPC, "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 has_diff, has_single; > + u32 reg, vinp, vinn; > + > + ret = fwnode_property_read_u32(child, "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, -ENOSPC, > + "invalid ADC channel index %u\n", reg); > + > + if (test_and_set_bit(reg, used_regs)) > + return dev_err_probe(dev, -EEXIST, > + "duplicate ADC channel index %u\n", reg); > + > + has_single = fwnode_property_present(child, "single-channel"); > + has_diff = fwnode_property_present(child, "diff-channels"); > + if (has_single && has_diff) > + return dev_err_probe(dev, -EINVAL, > + "channel %u has conflicting input properties\n", > + reg); > + > + if (has_diff) { > + u32 inputs[2]; No need, move this above instead of vinn and vinp. Or use custom data: struct { u32 p; u32 n; } vin; if you want to have distinct names. > + ret = fwnode_property_read_u32_array(child, "diff-channels", > + inputs, ARRAY_SIZE(inputs)); > + if (ret) > + return dev_err_probe(dev, ret, > + "invalid diff-channels for index %u\n", > + reg); > + > + vinp = inputs[0]; > + vinn = inputs[1]; > + if (vinp >= 4 || vinn != vinp + 4) > + return dev_err_probe(dev, -ECHRNG, > + "invalid differential inputs %u-%u\n", > + vinp, vinn); > + > + if (test_and_set_bit(vinp, used_diff_pairs)) > + return dev_err_probe(dev, -EEXIST, > + "duplicate differential inputs %u-%u\n", > + vinp, vinn); > + > + channels[scan_index] = (struct iio_chan_spec) { > + .type = IIO_VOLTAGE, > + .indexed = 1, > + .differential = 1, > + .channel = vinp, > + .channel2 = vinn, > + .address = reg, > + .scan_index = scan_index, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > + BIT(IIO_CHAN_INFO_SCALE), > + .scan_type = { > + .format = IIO_SCAN_FORMAT_SIGNED_INT, > + .realbits = MA35D1_EADC_RESOLUTION, > + .storagebits = 16, > + .endianness = IIO_CPU, > + }, > + }; > + } else { > + if (has_single) { > + 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); > + } else { > + vinp = reg; > + } > + > + if (vinp >= MA35D1_EADC_MAX_EXT_CHANNELS) > + return dev_err_probe(dev, -ECHRNG, > + "invalid ADC input %u\n", vinp); > + > + if (test_and_set_bit(vinp, used_single_inputs)) > + return dev_err_probe(dev, -EEXIST, > + "duplicate ADC input %u\n", vinp); > + > + channels[scan_index] = (struct iio_chan_spec) { > + .type = IIO_VOLTAGE, > + .indexed = 1, > + .channel = vinp, > + .address = reg, > + .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 = MA35D1_EADC_RESOLUTION, > + .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; > + > + ret = devm_regulator_get_enable_read_voltage(dev, "vref"); > + if (ret == -ENODEV) { > + adc->vref_mV = MA35D1_EADC_INTERNAL_VREF_mV; > + return 0; Can be deduplicated. See below how. > + } > + if (ret < 0) > + return dev_err_probe(dev, ret, "failed to get VREF supply\n"); > + > + if (!ret) > + return dev_err_probe(dev, -EINVAL, "invalid VREF voltage\n"); > + > + adc->external_vref = true; > + adc->vref_mV = ret / (MICRO / MILLI); if (ret == -ENODEV) { adc->vref_mV = MA35D1_EADC_INTERNAL_VREF_mV; } else if (ret < 0) { return dev_err_probe(dev, ret, "failed to get VREF supply\n"); } else { adc->external_vref = true; adc->vref_mV = ret / (MICRO / MILLI); } > + return 0; > +} ... > + adc->clk = devm_clk_get(dev, NULL); > + if (IS_ERR(adc->clk)) > + return dev_err_probe(dev, PTR_ERR(adc->clk), > + "failed to get ADC clock\n"); > + > + ret = clk_prepare_enable(adc->clk); > + if (ret) > + return dev_err_probe(dev, ret, "failed to enable ADC clock\n"); > + adc->clk_enabled = true; Repeats what __clk_is_enabled() provides. > + ret = devm_add_action_or_reset(dev, ma35d1_adc_clk_disable_action, adc); > + if (ret) > + 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); What's wrong with guard()()? Yes, for resume triggering support this needs to be wrapped. Same Q to other explicit mutex_lock() use cases. > + ret = clk_prepare_enable(adc->clk); > + if (ret) > + goto unlock; > + > + adc->clk_enabled = true; > + ret = ma35d1_adc_hw_init(adc); > + if (ret) { > + clk_disable_unprepare(adc->clk); > + adc->clk_enabled = false; > + 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); -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-11-04 11:50 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-11-04 9:11 [PATCH v4 0/2] iio: adc: Add Nuvoton MA35D1 EADC support Chi-Wen Weng 2026-11-04 9:11 ` [PATCH v4 1/2] dt-bindings: iio: adc: Add Nuvoton MA35D1 EADC Chi-Wen Weng 2026-11-04 9:18 ` sashiko-bot 2026-11-04 9:11 ` [PATCH v4 2/2] iio: adc: Add Nuvoton MA35D1 EADC driver Chi-Wen Weng 2026-11-04 9:24 ` sashiko-bot 2026-11-04 11:50 ` Andy Shevchenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox