iio: adc: Add Nuvoton MA35D1 EADC support
Subsystems (3)
v3 cover iio: adc: Add Nuvoton MA35D1 EADC support
From: Chi-Wen Weng <[email protected]>
Add devicetree binding and IIO driver support for the Enhanced Analog-to-Digital Converter (EADC) found on Nuvoton MA35D1 SoCs.
The MA35D1 EADC is a 12-bit ADC controller with eight external analog inputs. It supports single-ended conversion and four fixed differential input pairs: 0-4, 1-5, 2-6 and 3-7.
The controller provides four interrupt outputs, ADINT0 through ADINT3. ADINT0 through ADINT2 are associated with the corresponding conversion interrupt flags. ADINT3 is shared by the ADIF3 conversion interrupt and the ADCMPF0 through ADCMPF3 result-compare interrupt sources.
The initial driver uses sample module 0 and ADINT0 for conversion completion. It supports direct reads and externally triggered buffered capture. The driver supports an optional external reference supply and uses the internal 1.6 V reference when no external reference is provided.
The binding describes the optional DMA request and all four hardware interrupt outputs, including the result-compare sources sharing ADINT3. The initial driver does not use DMA or result-compare functionality.
Changes in v3:
- Document all four ADINT0 through ADINT3 hardware interrupt outputs and add interrupt-names.
- Document ADINT3 as the shared conversion and result-compare interrupt output.
- Remove the redundant minItems/maxItems constraints from the fixed interrupts items list.
- Rework the channel binding for the fixed differential pairs and use single-channel for single-ended inputs.
- Add #io-channel-cells so the EADC can be referenced by DT IIO consumers, and define the cell as the logical channel index.
- Add firmware-node channel translation so sparse or reordered channel child nodes are mapped by their logical channel index.
- Restrict unsupported generic ADC channel properties including bipolar, common-mode-channel, settling-time-us and oversampling-ratio.
- Explicitly configure INTPOS for end-of-conversion interrupt generation.
- Explicitly configure the ADINT0 sample-module source and clear unused sample-module routing.
- Reset the EADC control circuits during hardware initialization and clear inherited ADC interrupt enables, result-compare interrupt enables and sample-module trigger sources.
- Add timeout recovery using the EADC control-circuit reset, followed by IRQ synchronization and stale status/completion cleanup.
- Use iio_push_to_buffers_with_ts() for triggered buffered capture.
- Avoid mixing cleanup guards with goto paths in the trigger handler.
- Add ARCH_MA35 || COMPILE_TEST and HAS_IOMEM Kconfig dependencies.
- Drop redundant linux/bitops.h and linux/mod_devicetable.h includes.
- Use direct regmap helpers, explicit REGCACHE_NONE and regmap_assign_bits() where appropriate.
- Use fixed-width register offsets and kernel time/unit helpers.
- Use size_add() for channel array allocation.
- Request ADINT0 by interrupt name.
- Improve system suspend and resume handling.
- Address review comments from Jonathan Cameron, Rob Herring, Andy Shevchenko, Uwe Kleine-König and automated review/testing.
Changes in v2:
- Reworked the driver to use regmap.
- Added optional external reference supply support and IIO scale reporting.
- Added optional reset handling.
- Added externally triggered buffered capture.
- Added signed differential conversion handling.
- Added fixed differential-pair validation.
- Added system sleep support.
- Added optional DMA properties to the binding.
Patch 1 adds the devicetree binding.
Patch 2 adds the IIO driver.
Chi-Wen Weng (2):
dt-bindings: iio: adc: Add Nuvoton MA35D1 EADC
iio: adc: Add Nuvoton MA35D1 EADC driver
.../bindings/iio/adc/nuvoton,ma35d1-eadc.yaml | 175 ++++ drivers/iio/adc/Kconfig | 14 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/ma35d1_eadc.c | 849 ++++++++++++++++++ 4 files changed, 1039 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml create mode 100644 drivers/iio/adc/ma35d1_eadc.c
base-commit: d89e1d186ec9271bf78e62771779e9fc03bf0fc3
Signature · 1 line
2.25.1
PATCH v3 1/2 dt-bindings: iio: adc: Add Nuvoton MA35D1 EADC
From: Chi-Wen Weng <[email protected]>
Add a devicetree binding for the Enhanced Analog-to-Digital Converter (EADC) found on Nuvoton MA35D1 SoCs.
The controller provides eight external analog inputs which can be used as single-ended channels or as four fixed differential pairs: 0-4, 1-5, 2-6 and 3-7.
Describe the four hardware interrupt outputs, ADINT0 through ADINT3. ADINT0 through ADINT2 are associated with the corresponding conversion interrupt flags, while ADINT3 is shared by the ADIF3 conversion interrupt and the ADCMPF0 through ADCMPF3 result-compare interrupt sources.
Also describe the functional clock, optional reset, optional external reference supply and optional DMA request.
Expose the controller as an IIO channel provider through the DT #io-channel-cells property. The cell identifies the logical EADC channel selected by the reg value of the corresponding channel child node.
Restrict the channel binding to features supported by the MA35D1 EADC hardware and use the common single-channel representation for single-ended inputs.
--- .../bindings/iio/adc/nuvoton,ma35d1-eadc.yaml | 175 ++++++++++++++++++ 1 file changed, 175 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +175 -0
@@ -0,0 +1,175 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/adc/nuvoton,ma35d1-eadc.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Nuvoton MA35D1 Enhanced Analog-to-Digital Converter + +maintainers: + - Chi-Wen Weng <[email protected]> + +description: | + The Nuvoton MA35D1 Enhanced Analog-to-Digital Converter (EADC) is a + 12-bit successive approximation converter with eight external analog + inputs and one internally connected VBAT input. + + The external inputs can be used as eight single-ended channels or as + four fixed differential pairs: 0-4, 1-5, 2-6 and 3-7. The internally + connected VBAT input is not described by a channel child node. + + The controller provides four interrupt outputs, ADINT0 through ADINT3. + ADINT0, ADINT1 and ADINT2 are generated by the corresponding ADIF0, + ADIF1 and ADIF2 conversion interrupt flags. ADINT3 is shared by the + ADIF3 conversion interrupt flag and the ADCMPF0 through ADCMPF3 result + compare flags. + +properties: + compatible: + const: nuvoton,ma35d1-eadc + + reg: + maxItems: 1 + + interrupts: + items: + - description: ADINT0 conversion interrupt output generated by ADIF0 + - description: ADINT1 conversion interrupt output generated by ADIF1 + - description: ADINT2 conversion interrupt output generated by ADIF2 + - description: > + ADINT3 shared interrupt output generated by ADIF3 or by the + ADCMPF0 through ADCMPF3 result compare flags + + interrupt-names: + items: + - const: adint0 + - const: adint1 + - const: adint2 + - const: adint3 + + clocks: + maxItems: 1 + + resets: + maxItems: 1 + + vref-supply: + description: + Optional external reference voltage connected to the VREF pin. If this + supply is absent, the controller uses its internal 1.6 V reference. + + dmas: + maxItems: 1 + + dma-names: + const: rx + + '#io-channel-cells': + const: 1 + description: + The cell selects the logical EADC channel identified by the reg value + of the corresponding channel child node. + + '#address-cells': + const: 1 + + '#size-cells': + const: 0 + +patternProperties: + '^channel@[0-7]$': + type: object + $ref: /schemas/iio/adc/adc.yaml# + unevaluatedProperties: false + description: | + External EADC channel description. + + For a single-ended channel, single-channel specifies the external + analog input number (0 through 7), while reg provides the logical IIO + channel index. + + For a differential channel, omit single-channel. The reg value selects + one of the four fixed hardware differential pairs: reg 0 selects 0-4, + reg 1 selects 1-5, reg 2 selects 2-6 and reg 3 selects 3-7. + + properties: + reg: + maximum: 7 + + single-channel: + maximum: 7 + + # Differential pairs are fixed in hardware and are selected by reg. + diff-channels: false + + bipolar: false + + common-mode-channel: false + + settling-time-us: false + + oversampling-ratio: false + + allOf: + - if: + not: + required: + - single-channel + then: + properties: + reg: + maximum: 3 + + required: + - reg + +required: + - compatible + - reg + - interrupts + - interrupt-names + - clocks + - '#io-channel-cells' + - '#address-cells' + - '#size-cells' + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/nuvoton,ma35d1-clk.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/reset/nuvoton,ma35d1-reset.h> + + soc { + #address-cells = <2>; + #size-cells = <2>; + + adc@40430000 { + compatible = "nuvoton,ma35d1-eadc"; + reg = <0x0 0x40430000 0x0 0x10000>; + interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 66 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "adint0", "adint1", "adint2", "adint3"; + clocks = <&clk EADC_GATE>; + resets = <&reset MA35D1_RESET_EADC0>; + + #io-channel-cells = <1>; + #address-cells = <1>; + #size-cells = <0>; + + channel@0 { + reg = <0>; + single-channel = <0>; + }; + + channel@1 { + reg = <1>; + }; + }; + }; + +...
Signature · 1 line
2.25.1
[email protected]
AI reviewRe: [PATCH v3 1/2] dt-bindings: iio: adc: Add Nuvoton MA35D1 EADC
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] dt-bindings: iio: adc: channel schema inverts adc.yaml differential semantics - [Medium]…
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] dt-bindings: iio: adc: channel schema inverts adc.yaml differential semantics
- [Medium] dt-bindings: iio: adc: incomplete channel property restriction with adc.yaml --
commit f0823504c40ee953b211fa5196cd4cbe1be6611c
Author: Chi-Wen Weng <[email protected]>
dt-bindings: iio: adc: Add Nuvoton MA35D1 EADC
This commit adds the devicetree binding schema for the Nuvoton MA35D1 Enhanced Analog-to-Digital Converter (EADC), describing its interrupts, clocks, resets, reference supply, DMA support, and channel child nodes.
Quoted 5 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +0
-0
[ ... ]
Quoted 23 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +23
-0+patternProperties: + '^channel@[0-7]$': + type: object + $ref: /schemas/iio/adc/adc.yaml# + unevaluatedProperties: false + description: | + External EADC channel description. + + For a single-ended channel, single-channel specifies the external + analog input number (0 through 7), while reg provides the logical IIO + channel index. + + For a differential channel, omit single-channel. The reg value selects + one of the four fixed hardware differential pairs: reg 0 selects 0-4, + reg 1 selects 1-5, reg 2 selects 2-6 and reg 3 selects 3-7. + + properties: + reg: + maximum: 7 + + single-channel: + maximum: 7 +
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml line 111
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +10
-0+ # Differential pairs are fixed in hardware and are selected by reg. + diff-channels: false + + bipolar: false + + common-mode-channel: false + + settling-time-us: false + + oversampling-ratio: false
[Severity: Medium]
Does using unevaluatedProperties: false here leave other unsupported properties from Documentation/devicetree/bindings/iio/adc/adc.yaml permitted in channel@[0-7] child nodes?
Because adc.yaml is referenced with unevaluatedProperties: false and only five properties are explicitly set to false above, the remaining properties defined in adc.yaml (reference-sources, input-chopping, excitation-channels, excitation-current-nanoamp, and burn-out-current-nanoamp) will still pass schema validation.
Should patternProperties.'^channel@[0-7]$' use additionalProperties: false instead and list only the supported properties from adc.yaml?
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml line 121
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +10
-0+ + allOf: + - if: + not: + required: + - single-channel + then: + properties: + reg: + maximum: 3
[Severity: Medium]
Doesn't forbidding diff-channels and using the omission of single-channel to select differential pairs invert the standard semantics in Documentation/devicetree/bindings/iio/adc/adc.yaml?
In adc.yaml, properties.single-channel specifies:
single-channel:
$ref: /schemas/types.yaml#/definitions/uint32
description:
When devices combine single-ended and differential channels, allow the channel for a single element to be specified, independent of reg (as for differential channels). If this and diff-channels are not present reg shall be used instead.
Under adc.yaml, a child node with neither single-channel nor diff-channels (such as channel@1 with only reg = <1> in the example) represents a single-ended channel on input reg, whereas this schema treats it as differential pair 1-5.
Additionally, for single-ended channels, reg (0 through 7) acts as a logical channel index independent of the input pin selected by single-channel, while for differential channels reg (0 through 3) is forced to act as both the child node unit address and the hardware differential pair selector. Because sibling devicetree nodes cannot share the same reg value, a board that uses reg = <0> for a single-ended channel on another input (for instance, channel@0 with reg = <0> and single-channel = <2>) cannot also describe hardware differential pair 0-4, which requires reg = <0>.
Could differential channels use diff-channels constrained to the four valid hardware pairs (<0 4>, <1 5>, <2 6>, and <3 7>) so that reg remains purely the logical channel index?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Jonathan Cameron
maintainerRe: [PATCH v3 1/2] dt-bindings: iio: adc: Add Nuvoton MA35D1 EADC
In reply to Chi-Wen WengQuoted 18 lines · On Fri, 30 Oct 2026 11:15:00 +0800 Chi-Wen Weng <[email protected]> wrote:
From: Chi-Wen Weng <[email protected]>
Add a devicetree binding for the Enhanced Analog-to-Digital Converter (EADC) found on Nuvoton MA35D1 SoCs.
The controller provides eight external analog inputs which can be used as single-ended channels or as four fixed differential pairs: 0-4, 1-5, 2-6 and 3-7.
Describe the four hardware interrupt outputs, ADINT0 through ADINT3. ADINT0 through ADINT2 are associated with the corresponding conversion interrupt flags, while ADINT3 is shared by the ADIF3 conversion interrupt and the ADCMPF0 through ADCMPF3 result-compare interrupt sources.
Also describe the functional clock, optional reset, optional external reference supply and optional DMA request.
Quoted 8 lines
Expose the controller as an IIO channel provider through the DT #io-channel-cells property. The cell identifies the logical EADC channel selected by the reg value of the corresponding channel child node.
Restrict the channel binding to features supported by the MA35D1 EADC hardware and use the common single-channel representation for single-ended inputs.
This description seems a little verbose. Maybe only talk about the stuff we can't tell from the patch.
1. Why it needs a new binding and isn't similar enough to existing ones for reuse.
2. Any unusual aspects. Calling out the stuff that I note below on the per channel binding oddity more clearly would have been helpful.
For the other stuff the reader can just look at the patch,
Thanks,
Jonathan
Quoted 15 lines
Signed-off-by Chi-Wen Weng <[email protected]>--- .../bindings/iio/adc/nuvoton,ma35d1-eadc.yaml | 175 ++++++++++++++++++ 1 file changed, 175 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yamlDocumentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +2
-0@@ -0,0 +1,175 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml line 12
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +10
-0+--- +$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 <[email protected]> + +description: |
I think you only want to keep paragraphs. In which case > rather than |
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml line 19
Quoted 7 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +7
-0+ The Nuvoton MA35D1 Enhanced Analog-to-Digital Converter (EADC) is a + 12-bit successive approximation converter with eight external analog + inputs and one internally connected VBAT input. + + The external inputs can be used as eight single-ended channels or as + four fixed differential pairs: 0-4, 1-5, 2-6 and 3-7. The internally + connected VBAT input is not described by a channel child node.
How would you have a consumer of VBAT given the io-channel-cells comment about using the channel reg values for xlate?
Quoted 13 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +12
-0+ + The controller provides four interrupt outputs, ADINT0 through ADINT3. + ADINT0, ADINT1 and ADINT2 are generated by the corresponding ADIF0, + ADIF1 and ADIF2 conversion interrupt flags. ADINT3 is shared by the + ADIF3 conversion interrupt flag and the ADCMPF0 through ADCMPF3 result + compare flags. + +patternProperties: + '^channel@[0-7]$': + type: object + $ref: /schemas/iio/adc/adc.yaml# + unevaluatedProperties: false
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml line 93
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +10
-0+ description: | + External EADC channel description. + + For a single-ended channel, single-channel specifies the external + analog input number (0 through 7), while reg provides the logical IIO + channel index. + + For a differential channel, omit single-channel. The reg value selects + one of the four fixed hardware differential pairs: reg 0 selects 0-4, + reg 1 selects 1-5, reg 2 selects 2-6 and reg 3 selects 3-7.
Sorry, I wasn't paying attention on earlier versions. This is a very unintuitive binding particularly as we may want to have a single channel before a differential one. Better I think to use the differential channel bindings and restrict which ones are allowed to this fixed set.
Quoted 57 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +56
-0+ + properties: + reg: + maximum: 7 + + single-channel: + maximum: 7 + + # Differential pairs are fixed in hardware and are selected by reg. + diff-channels: false + + bipolar: false + + common-mode-channel: false + + settling-time-us: false + + oversampling-ratio: false + + allOf: + - if: + not: + required: + - single-channel + then: + properties: + reg: + maximum: 3 + + required: + - reg + + +examples: + - | + #include <dt-bindings/clock/nuvoton,ma35d1-clk.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/reset/nuvoton,ma35d1-reset.h> + + soc { + #address-cells = <2>; + #size-cells = <2>; + + adc@40430000 { + compatible = "nuvoton,ma35d1-eadc"; + reg = <0x0 0x40430000 0x0 0x10000>; + interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 66 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "adint0", "adint1", "adint2", "adint3"; + clocks = <&clk EADC_GATE>; + resets = <&reset MA35D1_RESET_EADC0>; + + #io-channel-cells = <1>;
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml line 170
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +10
-0+ #address-cells = <1>; + #size-cells = <0>; + + channel@0 { + reg = <0>; + single-channel = <0>; + }; + + channel@1 { + reg = <1>;
This looked very odd and is why I had a closer read of the docs above. To make this a reasonable binding to read this needs to do the full differential channel binding from adc.yaml with reg just being an index and not specifying channels at all (same as for the single channel case).
Quoted 5 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +5
-0+ }; + }; + }; + +...
Re: [PATCH v3 1/2] dt-bindings: iio: adc: Add Nuvoton MA35D1 EADC
Jonathan Cameron 於 2026/11/1 上午 08:19 寫道:
Quoted 178 lines
Quoted 18 lines · On Fri, 30 Oct 2026 11:15:00 +0800 Chi-Wen Weng <[email protected]> wrote:
From: Chi-Wen Weng <[email protected]>
Add a devicetree binding for the Enhanced Analog-to-Digital Converter (EADC) found on Nuvoton MA35D1 SoCs.
The controller provides eight external analog inputs which can be used as single-ended channels or as four fixed differential pairs: 0-4, 1-5, 2-6 and 3-7.
Describe the four hardware interrupt outputs, ADINT0 through ADINT3. ADINT0 through ADINT2 are associated with the corresponding conversion interrupt flags, while ADINT3 is shared by the ADIF3 conversion interrupt and the ADCMPF0 through ADCMPF3 result-compare interrupt sources.
Also describe the functional clock, optional reset, optional external reference supply and optional DMA request.
Quoted 8 lines
Expose the controller as an IIO channel provider through the DT #io-channel-cells property. The cell identifies the logical EADC channel selected by the reg value of the corresponding channel child node.
Restrict the channel binding to features supported by the MA35D1 EADC hardware and use the common single-channel representation for single-ended inputs.
This description seems a little verbose. Maybe only talk about the stuff we can't tell from the patch.
1. Why it needs a new binding and isn't similar enough to existing ones for reuse.
2. Any unusual aspects. Calling out the stuff that I note below on the per channel binding oddity more clearly would have been helpful.For the other stuff the reader can just look at the patch,
Thanks,
Jonathan
Quoted 15 lines
Signed-off-by Chi-Wen Weng <[email protected]>--- .../bindings/iio/adc/nuvoton,ma35d1-eadc.yaml | 175 ++++++++++++++++++ 1 file changed, 175 insertions(+)create mode 100644
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yamlDocumentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +2
-0@@ -0,0 +1,175 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml line 12
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +10
-0+--- +$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 <[email protected]> + +description: |I think you only want to keep paragraphs. In which case > rather than |
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml line 19
Quoted 7 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +7
-0+ The Nuvoton MA35D1 Enhanced Analog-to-Digital Converter (EADC) is a + 12-bit successive approximation converter with eight external analog + inputs and one internally connected VBAT input. + + The external inputs can be used as eight single-ended channels or as + four fixed differential pairs: 0-4, 1-5, 2-6 and 3-7. The internally + connected VBAT input is not described by a channel child node.How would you have a consumer of VBAT given the io-channel-cells comment about using the channel reg values for xlate?
Quoted 13 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +12
-0+ + The controller provides four interrupt outputs, ADINT0 through ADINT3. + ADINT0, ADINT1 and ADINT2 are generated by the corresponding ADIF0, + ADIF1 and ADIF2 conversion interrupt flags. ADINT3 is shared by the + ADIF3 conversion interrupt flag and the ADCMPF0 through ADCMPF3 result + compare flags. + +patternProperties: + '^channel@[0-7]$': + type: object + $ref: /schemas/iio/adc/adc.yaml# + unevaluatedProperties: falseDocumentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml line 93
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +10
-0+ description: | + External EADC channel description. + + For a single-ended channel, single-channel specifies the external + analog input number (0 through 7), while reg provides the logical IIO + channel index. + + For a differential channel, omit single-channel. The reg value selects + one of the four fixed hardware differential pairs: reg 0 selects 0-4, + reg 1 selects 1-5, reg 2 selects 2-6 and reg 3 selects 3-7.Sorry, I wasn't paying attention on earlier versions. This is a very unintuitive binding particularly as we may want to have a single channel before a differential one. Better I think to use the differential channel bindings and restrict which ones are allowed to this fixed set.
Quoted 65 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +64
-0+ + properties: + reg: + maximum: 7 + + single-channel: + maximum: 7 + + # Differential pairs are fixed in hardware and are selected by reg. + diff-channels: false + + bipolar: false + + common-mode-channel: false + + settling-time-us: false + + oversampling-ratio: false + + allOf: + - if: + not: + required: + - single-channel + then: + properties: + reg: + maximum: 3 + + required: + - reg + + +examples: + - | + #include <dt-bindings/clock/nuvoton,ma35d1-clk.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/reset/nuvoton,ma35d1-reset.h> + + soc { + #address-cells = <2>; + #size-cells = <2>; + + adc@40430000 { + compatible = "nuvoton,ma35d1-eadc"; + reg = <0x0 0x40430000 0x0 0x10000>; + interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 66 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "adint0", "adint1", "adint2", "adint3"; + clocks = <&clk EADC_GATE>; + resets = <&reset MA35D1_RESET_EADC0>; + + #io-channel-cells = <1>; + #address-cells = <1>; + #size-cells = <0>; + + channel@0 { + reg = <0>; + single-channel = <0>; + }; +
Quoted 15 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml line 170
Quoted 2 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +2
-0+ channel@1 { + reg = <1>;This looked very odd and is why I had a closer read of the docs above. To make this a reasonable binding to read this needs to do the full differential channel binding from
adc.yamlwith reg just being an index and not specifying channels at all (same as for the single channel case).Quoted 5 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +5
-0+ }; + }; + }; + +...
Hi Jonathan,
Thank you for the detailed review.
I agree that the current channel representation is unnecessarily unusual and does not make good use of the common ADC channel binding.
For v4 I will rework it so that reg is only the logical channel index, while the physical inputs are described explicitly:
- single-channel for a single-ended input
- diff-channels for a differential input
The differential channels will be restricted to the four combinations supported by the MA35D1 hardware:
<0 4>
<1 5>
<2 6>
<3 7>
I will also require each external channel node to explicitly describe either single-channel or diff-channels, instead of using the absence of single-channel to imply differential mode.
This should also make the example clearer, for example:
channel@0 {
reg = <0>;
single-channel = <2>;
};
channel@1 {
reg = <1>;
diff-channels = <0 4>;
};so that reg is clearly independent of the physical ADC input selection.
I will also switch the channel schema to additionalProperties: false and only expose the common ADC properties supported by this hardware, rather than inheriting unsupported properties from adc.yaml.
For the top-level description I will switch from the literal block style to the folded paragraph style and shorten the commit message to focus on the unusual hardware aspects rather than repeating information visible directly in the schema.
Regarding VBAT, thank you for pointing out the inconsistency with the #io-channel-cells description. The internal VBAT input is a fixed hardware connection rather than a board-routed input, so it should not need a channel child node. I will rework the provider/channel numbering description in v4 so that the internal VBAT channel, if exposed to an IIO consumer, has an unambiguous channel specifier without requiring a DT child node.
Thanks,
Chi-Wen
Re: [PATCH v3 1/2] dt-bindings: iio: adc: Add Nuvoton MA35D1 EADC
In reply to Chi-Wen WengQuoted 9 lines · Chi-Wen Weng
From: Chi-Wen Weng <[email protected]>
Add a devicetree binding for the Enhanced Analog-to-Digital Converter (EADC) found on Nuvoton MA35D1 SoCs.
The controller provides eight external analog inputs which can be used as single-ended channels or as four fixed differential pairs: 0-4, 1-5, 2-6 and 3-7.
Quoted 8 lines
Describe the four hardware interrupt outputs, ADINT0 through ADINT3. ADINT0 through ADINT2 are associated with the corresponding conversion interrupt flags, while ADINT3 is shared by the ADIF3 conversion interrupt and the ADCMPF0 through ADCMPF3 result-compare interrupt sources.
Also describe the functional clock, optional reset, optional external reference supply and optional DMA request.
Don't repeat the patch contents.
Quoted 9 lines
Expose the controller as an IIO channel provider through the DT #io-channel-cells property. The cell identifies the logical EADC channel selected by the reg value of the corresponding channel child node.
Restrict the channel binding to features supported by the MA35D1 EADC hardware and use the common single-channel representation for single-ended inputs.
And here even more.
Quoted 42 lines
Signed-off-by Chi-Wen Weng <[email protected]>--- .../bindings/iio/adc/nuvoton,ma35d1-eadc.yaml | 175 ++++++++++++++++++ 1 file changed, 175 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yamlDocumentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +29
-0@@ -0,0 +1,175 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/adc/nuvoton,ma35d1-eadc.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Nuvoton MA35D1 Enhanced Analog-to-Digital Converter + +maintainers: + - Chi-Wen Weng <[email protected]> + +description: | + The Nuvoton MA35D1 Enhanced Analog-to-Digital Converter (EADC) is a + 12-bit successive approximation converter with eight external analog + inputs and one internally connected VBAT input. + + The external inputs can be used as eight single-ended channels or as + four fixed differential pairs: 0-4, 1-5, 2-6 and 3-7. The internally + connected VBAT input is not described by a channel child node. + + The controller provides four interrupt outputs, ADINT0 through ADINT3. + ADINT0, ADINT1 and ADINT2 are generated by the corresponding ADIF0, + ADIF1 and ADIF2 conversion interrupt flags. ADINT3 is shared by the + ADIF3 conversion interrupt flag and the ADCMPF0 through ADCMPF3 result + compare flags. + +properties: + compatible: + const: nuvoton,ma35d1-eadc
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml line 39
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +10
-0+ + reg: + maxItems: 1 + + interrupts: + items: + - description: ADINT0 conversion interrupt output generated by ADIF0 + - description: ADINT1 conversion interrupt output generated by ADIF1 + - description: ADINT2 conversion interrupt output generated by ADIF2 + - description: >
Drop >
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml line 48
Quoted 9 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +9
-0+ ADINT3 shared interrupt output generated by ADIF3 or by the + ADCMPF0 through ADCMPF3 result compare flags + + interrupt-names: + items: + - const: adint0 + - const: adint1 + - const: adint2 + - const: adint3
"int" is redundant, so honestly names should rather go away completely.
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml line 54
Quoted 6 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +6
-0+ + clocks: + maxItems: 1 + + resets: + maxItems: 1
Best regards,
Krzysztof
Re: [PATCH v3 1/2] dt-bindings: iio: adc: Add Nuvoton MA35D1 EADC
Krzysztof Kozlowski 於 2026/11/2 下午 06:03 寫道:
Quoted 100 lines · Chi-Wen Weng
Quoted 9 lines
From: Chi-Wen Weng <[email protected]>
Add a devicetree binding for the Enhanced Analog-to-Digital Converter (EADC) found on Nuvoton MA35D1 SoCs.
The controller provides eight external analog inputs which can be used as single-ended channels or as four fixed differential pairs: 0-4, 1-5, 2-6 and 3-7.
Quoted 8 lines
Describe the four hardware interrupt outputs, ADINT0 through ADINT3. ADINT0 through ADINT2 are associated with the corresponding conversion interrupt flags, while ADINT3 is shared by the ADIF3 conversion interrupt and the ADCMPF0 through ADCMPF3 result-compare interrupt sources.
Also describe the functional clock, optional reset, optional external reference supply and optional DMA request.
Don't repeat the patch contents.
Quoted 9 lines
Expose the controller as an IIO channel provider through the DT #io-channel-cells property. The cell identifies the logical EADC channel selected by the reg value of the corresponding channel child node.
Restrict the channel binding to features supported by the MA35D1 EADC hardware and use the common single-channel representation for single-ended inputs.
And here even more.
Quoted 42 lines
Signed-off-by Chi-Wen Weng <[email protected]>--- .../bindings/iio/adc/nuvoton,ma35d1-eadc.yaml | 175 ++++++++++++++++++ 1 file changed, 175 insertions(+)create mode 100644
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yamlDocumentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +29
-0@@ -0,0 +1,175 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/adc/nuvoton,ma35d1-eadc.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Nuvoton MA35D1 Enhanced Analog-to-Digital Converter + +maintainers: + - Chi-Wen Weng <[email protected]> + +description: | + The Nuvoton MA35D1 Enhanced Analog-to-Digital Converter (EADC) is a + 12-bit successive approximation converter with eight external analog + inputs and one internally connected VBAT input. + + The external inputs can be used as eight single-ended channels or as + four fixed differential pairs: 0-4, 1-5, 2-6 and 3-7. The internally + connected VBAT input is not described by a channel child node. + + The controller provides four interrupt outputs, ADINT0 through ADINT3. + ADINT0, ADINT1 and ADINT2 are generated by the corresponding ADIF0, + ADIF1 and ADIF2 conversion interrupt flags. ADINT3 is shared by the + ADIF3 conversion interrupt flag and the ADCMPF0 through ADCMPF3 result + compare flags. + +properties: + compatible: + const: nuvoton,ma35d1-eadcDocumentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml line 39
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +10
-0+ + reg: + maxItems: 1 + + interrupts: + items: + - description: ADINT0 conversion interrupt output generated by ADIF0 + - description: ADINT1 conversion interrupt output generated by ADIF1 + - description: ADINT2 conversion interrupt output generated by ADIF2 + - description: >Drop >
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml line 48
Quoted 9 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +9
-0+ ADINT3 shared interrupt output generated by ADIF3 or by the + ADCMPF0 through ADCMPF3 result compare flags + + interrupt-names: + items: + - const: adint0 + - const: adint1 + - const: adint2 + - const: adint3"int" is redundant, so honestly names should rather go away completely.
Quoted 9 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml line 54
Quoted 6 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +6
-0+ + clocks: + maxItems: 1 + + resets: + maxItems: 1Best regards,
Krzysztof
Hi Krzysztof,
Thank you for the review.
I will shorten the commit message in v4 and keep it focused on the hardware aspects that are not obvious from the binding itself, rather than repeating the properties added by the patch.
I will also simplify the interrupt description. The detailed ADINT3 source information can be kept in the controller description where needed, rather than repeated in the interrupts property.
For interrupt-names, I agree that the current "adint0" through "adint3" names do not add useful information. The interrupt outputs have a fixed ordering, so I will remove interrupt-names from the binding and use the first interrupt by position in the driver.
I will address these together with the other binding changes in v4.
Thanks,
Chi-Wen
Re: [PATCH v3 1/2] dt-bindings: iio: adc: Add Nuvoton MA35D1 EADC
Quoted 16 lines · Chi-Wen Weng
Quoted 11 lines
"int" is redundant, so honestly names should rather go away completely.
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml line 54
Quoted 6 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +6
-0+ + clocks: + maxItems: 1 + + resets: + maxItems: 1Best regards,
KrzysztofHi Krzysztof,
Thank you for the review.
Quoted 7 lines
I will shorten the commit message in v4 and keep it focused on the hardware aspects that are not obvious from the binding itself, rather than repeating the properties added by the patch.
I will also simplify the interrupt description. The detailed ADINT3 source information can be kept in the controller description where needed, rather than repeated in the interrupts property.
That's LLM generated confirmation. Typical respond to prompt. Great way to get yourself on the deprioritized list.
Best regards,
Krzysztof
PATCH v3 2/2 iio: adc: Add Nuvoton MA35D1 EADC driver
In reply to Chi-Wen WengFrom: Chi-Wen Weng <[email protected]>
Add an IIO driver for the Enhanced Analog-to-Digital Converter (EADC) found on Nuvoton MA35D1 SoCs.
The driver supports direct conversion and externally triggered buffered capture using sample module 0. Both single-ended inputs and the four fixed differential input pairs are supported.
Use the internal 1.6 V reference when no external reference supply is provided, and report the reference voltage through the IIO scale interface.
Use ADINT0 for conversion completion. Configure the interrupt source and end-of-conversion position explicitly, and recover from conversion timeouts by resetting the EADC control circuits, synchronizing the IRQ and clearing stale conversion state before another conversion is started.
Reset the EADC control circuits during hardware initialization and sanitize interrupt, compare and sample-module trigger configuration so the driver does not depend on state left by firmware when the external reset control is unavailable.
Provide firmware-node channel translation so DT IIO consumers select channels by the logical channel index described by the channel node's reg property rather than by the position of the channel in the driver's channel array.
Add clock, optional reset and system sleep handling, and restore the controller configuration after resume.
--- drivers/iio/adc/Kconfig | 14 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/ma35d1_eadc.c | 849 ++++++++++++++++++++++++++++++++++ 3 files changed, 864 insertions(+) create mode 100644 drivers/iio/adc/ma35d1_eadc.c
drivers/iio/adc/Kconfig +14 -0
@@ -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
drivers/iio/adc/Makefile +1 -0
@@ -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
drivers/iio/adc/ma35d1_eadc.c new file +849 -0
849 lines not on this page · raw message
Signature · 1 line
2.25.1
Jonathan Cameron
maintainerRe: [PATCH v3 2/2] iio: adc: Add Nuvoton MA35D1 EADC driver
Quoted 29 lines · On Fri, 30 Oct 2026 11:15:01 +0800 Chi-Wen Weng <[email protected]> wrote:
From: Chi-Wen Weng <[email protected]>
Add an IIO driver for the Enhanced Analog-to-Digital Converter (EADC) found on Nuvoton MA35D1 SoCs.
The driver supports direct conversion and externally triggered buffered capture using sample module 0. Both single-ended inputs and the four fixed differential input pairs are supported.
Use the internal 1.6 V reference when no external reference supply is provided, and report the reference voltage through the IIO scale interface.
Use ADINT0 for conversion completion. Configure the interrupt source and end-of-conversion position explicitly, and recover from conversion timeouts by resetting the EADC control circuits, synchronizing the IRQ and clearing stale conversion state before another conversion is started.
Reset the EADC control circuits during hardware initialization and sanitize interrupt, compare and sample-module trigger configuration so the driver does not depend on state left by firmware when the external reset control is unavailable.
Provide firmware-node channel translation so DT IIO consumers select channels by the logical channel index described by the channel node's reg property rather than by the position of the channel in the driver's channel array.
Quoted 4 lines
Add clock, optional reset and system sleep handling, and restore the controller configuration after resume.
Signed-off-by Chi-Wen Weng <[email protected]>
Hi Chi-Wen
Various comments inline.
Thanks,
Jonathan
Quoted 13 lines
drivers/iio/adc/ma35d1_eadc.c new file +6
-0+ +static int ma35d1_adc_disable_irq (struct ma35d1_adc *adc) +{ +return regmap_clear_bits (adc->regmap,MA35D1_EADC_CTL ,MA35D1_EADC_CTL_ADCIEN0 ); +} +
drivers/iio/adc/ma35d1_eadc.c line 117
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+static int ma35d1_adc_setup_reference (struct ma35d1_adc *adc) +{ +struct regmap *map = adc->regmap; +int ret; + +/* +* MA35D1 TRM EADC reference control: PDREF selects the external VREF +* pin. With the internal reference selected, VREFSEL = 0 selects 1.6 V. +*/ +if (adc->vref) {
With change below to use the read_voltage function this would then be using a flag set there.
Quoted 29 lines
drivers/iio/adc/ma35d1_eadc.c new file +27
-0+ ret =regmap_set_bits (map,MA35D1_EADC_REFADJCTL , +MA35D1_EADC_REFADJCTL_PDREF ); +if (ret) +return ret; + }else { + ret =regmap_clear_bits (map,MA35D1_EADC_REFADJCTL , +MA35D1_EADC_REFADJCTL_PDREF ); +if (ret) +return ret; + + ret =regmap_update_bits (map,MA35D1_EADC_CTL , +MA35D1_EADC_CTL_VREFSEL_MASK , +MA35D1_EADC_CTL_VREFSEL_1V6 ); +if (ret) +return ret; + } + +fsleep (MA35D1_EADC_REF_STABLE_US ); + +return 0 ; +} +static int ma35d1_adc_abort_conversion (struct ma35d1_adc *adc) +{ +struct regmap *map = adc->regmap; +int ret; +int err; +
drivers/iio/adc/ma35d1_eadc.c
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+ ret =ma35d1_adc_disable_irq (adc); + +/* +* Reset the EADC control circuits to terminate a conversion which may +* still be in progress. EADCRST is cleared automatically by hardware +* when the reset completes and the EADC register values are preserved. +*/ + err =ma35d1_adc_reset_control (adc); +if (!ret) + ret = err;
Why not just set ret directly for this one?
drivers/iio/adc/ma35d1_eadc.c line 301
Quoted 6 lines
drivers/iio/adc/ma35d1_eadc.c new file +6
-0+ +/* Drain an interrupt which may already have been latched by the GIC. */ +synchronize_irq (adc->irq); + + err =regmap_write (map,MA35D1_EADC_STATUS2 , +MA35D1_EADC_STATUS2_ADIF0 );
I don't mind going slightly over 80 chars. I would do so for the line above.
drivers/iio/adc/ma35d1_eadc.c line 305
Quoted 4 lines
drivers/iio/adc/ma35d1_eadc.c new file +4
-0+if (!ret) + ret = err; + +reinit_completion (&adc->completion);
Why? It is reinit at start of read conversion - why else do we need to do it? Add a comment
Quoted 25 lines
drivers/iio/adc/ma35d1_eadc.c new file +25
-0+ +return ret; +} + +static int ma35d1_adc_read_conversion (struct ma35d1_adc *adc, +const struct iio_chan_spec *chan,u16 *raw) +{ +struct regmap *map = adc->regmap; +unsigned int val; +long timeout; +int ret; + +reinit_completion (&adc->completion); + + ret =regmap_write (map,MA35D1_EADC_STATUS2 ,MA35D1_EADC_STATUS2_ADIF0 ); +if (ret) +return ret; + + ret =ma35d1_adc_config_channel (adc, chan); +if (ret) +return ret; + + ret =regmap_set_bits (map,MA35D1_EADC_CTL ,MA35D1_EADC_CTL_ADCIEN0 ); +if (ret) +return ret;
drivers/iio/adc/ma35d1_eadc.c line 340
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+ + ret =regmap_write (map,MA35D1_EADC_SWTRG ,BIT (0 )); +if (ret) + goto disable_irq; + + timeout =wait_for_completion_timeout (&adc->completion, +MA35D1_EADC_TIMEOUT ); +if (!timeout) { + ret =ma35d1_adc_abort_conversion (adc); +return ret ?: -ETIMEDOUT ;
Timed out first, so I think that should be the return value even if the abort then failed. Maybe an error print is more appropriate for a failed abort so we don't loose that extra info.
Quoted 7 lines
drivers/iio/adc/ma35d1_eadc.c new file +7
-0+ } + + ret =ma35d1_adc_disable_irq (adc); +if (ret) +return ret; + + ret =regmap_read (map,MA35D1_EADC_DAT (0 ), &val);
drivers/iio/adc/ma35d1_eadc.c line 357
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+if (ret) +return ret; + +if (!(val &MA35D1_EADC_DAT_VALID )) +return -EIO ; + +if (val &MA35D1_EADC_DAT_OV ) +return -EOVERFLOW ; + + *raw =FIELD_GET (MA35D1_EADC_DAT_RESULT_MASK , val);
As below - I don't follow why we'd get 16 bits when I think all paths throw away everything above the 12th.
Quoted 20 lines
drivers/iio/adc/ma35d1_eadc.c new file +20
-0+ +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 ; +
drivers/iio/adc/ma35d1_eadc.c line 387
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+regmap_write (adc->regmap,MA35D1_EADC_STATUS2 , +MA35D1_EADC_STATUS2_ADIF0 ); +complete (&adc->completion); + +return IRQ_HANDLED ; +} + +static int ma35d1_adc_sign_extend (u16 raw) +{ +return sign_extend32 (raw &MA35D1_EADC_DAT_RESULT_12BIT ,11 );
If the result is always 12 bits, why is read_conversion returning 16?
Quoted 4 lines
drivers/iio/adc/ma35d1_eadc.c new file +4
-0+} + +static int ma35d1_adc_read_raw(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan,
drivers/iio/adc/ma35d1_eadc.c line 401
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+ int *val, int *val2, long mask) +{ + struct ma35d1_adc *adc = iio_priv(indio_dev); +u16 raw; +int ret; + +switch (mask) { +case IIO_CHAN_INFO_RAW : { +IIO_DEV_ACQUIRE_DIRECT_MODE (indio_dev, claim); +
No blank line here. We want the acquire and the error check tightly coupled visually.
Quoted 26 lines
drivers/iio/adc/ma35d1_eadc.c new file +26
-0+if (IIO_DEV_ACQUIRE_FAILED (claim)) +return -EBUSY ; + +guard (mutex)(&adc->lock); +if (adc->suspended) +return -EBUSY ; + + ret =ma35d1_adc_read_conversion (adc, chan, &raw); +if (ret) +return ret; + +if (chan->differential) + *val =ma35d1_adc_sign_extend (raw); +else + *val = raw &MA35D1_EADC_DAT_RESULT_12BIT ; + +return IIO_VAL_INT ; + } +case IIO_CHAN_INFO_SCALE : + *val = adc->vref_mV; +/* Differential mode has a VREF / 2048 LSB; single-ended uses /4096. */ + *val2 = chan->differential ?11 :12 ; +return IIO_VAL_FRACTIONAL_LOG2 ; +default : +return -EINVAL ; + }
drivers/iio/adc/ma35d1_eadc.c line 437
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-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; + +for_each_set_bit (bit, scan_mask,iio_get_masklength (indio_dev)) {
iio_for_each_active_channel()
drivers/iio/adc/ma35d1_eadc.c line 446
Quoted 9 lines
drivers/iio/adc/ma35d1_eadc.c new file +9
-0+const struct iio_chan_spec *chan = &indio_dev->channels[bit]; + +if (chan->type ==IIO_TIMESTAMP ) +continue ; + +if (chan->type !=IIO_VOLTAGE || scan_chan) +return -EINVAL ; + + scan_chan = chan;
This only makes sense if there is only one channel enabled. The driver needs to ensure that is the case (there is a standard validation callback for this).
drivers/iio/adc/ma35d1_eadc.c line 486
Quoted 12 lines
drivers/iio/adc/ma35d1_eadc.c new file +11
-0+ } + +if (!scan_chan) +return -EINVAL ; + + adc->scan_chan = scan_chan; + +return 0 ; +} + +static irqreturn_t ma35d1_adc_trigger_handler (int irq,void *p)
This looks like the device only supports onehot channels (one at a time) That's fine but then you should have the callbacks or available_channels set to indicate that.
drivers/iio/adc/ma35d1_eadc.c line 491
Quoted 5 lines
drivers/iio/adc/ma35d1_eadc.c new file +5
-0+{ +struct iio_poll_func *pf = p; +struct iio_dev *indio_dev = pf->indio_dev; +struct ma35d1_adc *adc =iio_priv (indio_dev); + IIO_DECLARE_BUFFER_WITH_TS(u16, scan, 1) = { };
Where the timestamp location is fixed (if present) I prefer
struct {
u16 channel;
aligned_s64 timestamp;
} scan;as it makes it easier to understand the layout. The macro is for the case where different numbers of channels can be enabled and there are enough of them to have the timestamp moving around depending on what is enabled.
Quoted 21 lines
drivers/iio/adc/ma35d1_eadc.c new file +20
-0+u16 raw; +int ret; + +mutex_lock (&adc->lock); + +if (!adc->suspended && adc->scan_chan) { + ret =ma35d1_adc_read_conversion (adc, adc->scan_chan, &raw); +if (!ret) { + scan[0 ] = raw &MA35D1_EADC_DAT_RESULT_12BIT ; +iio_push_to_buffers_with_ts (indio_dev, scan,sizeof (scan), + pf->timestamp); + } + } + +mutex_unlock (&adc->lock); + +iio_trigger_notify_done (indio_dev->trig); + +return IRQ_HANDLED ; +}
drivers/iio/adc/ma35d1_eadc.c line 526
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+ +static int ma35d1_adc_fwnode_xlate (struct iio_dev *indio_dev, +const struct fwnode_reference_args *iiospec) +{ +unsigned int i; + +if (iiospec->nargs !=1 ) +return -EINVAL ; + +for (i =0 ; i < indio_dev->num_channels; i++) {
for (unsigned int i = 0; ..
Quoted 7 lines
drivers/iio/adc/ma35d1_eadc.c new file +7
-0+const struct iio_chan_spec *chan = &indio_dev->channels[i]; + +if (chan->type ==IIO_VOLTAGE && + chan->channel == iiospec->args[0 ]) +return i; + } +
drivers/iio/adc/ma35d1_eadc.c line 543
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+return -EINVAL ; +} + +static const struct iio_info ma35d1_adc_info = { + .read_raw = ma35d1_adc_read_raw, + .update_scan_mode = ma35d1_adc_update_scan_mode, + .fwnode_xlate = ma35d1_adc_fwnode_xlate, +}; + +static void ma35d1_adc_init_channel(struct iio_chan_spec *chan, u32 index,
As below. Given C provides nice ways to set lots of structure elements helpers use those, rather than a fill the values in helper.
Quoted 42 lines
drivers/iio/adc/ma35d1_eadc.c new file +42
-0+ u32 vinp, u32 vinn, + unsigned int scan_index, + bool differential) +{ + chan->type = IIO_VOLTAGE; + chan->indexed =1 ; + chan->channel = index; + chan->address = vinp; + chan->scan_index = scan_index; + chan->info_mask_separate = +BIT (IIO_CHAN_INFO_RAW ) |BIT (IIO_CHAN_INFO_SCALE ); + chan->scan_type.format = differential ?IIO_SCAN_FORMAT_SIGNED_INT : +IIO_SCAN_FORMAT_UNSIGNED_INT ; + chan->scan_type.realbits =12 ; + chan->scan_type.storagebits =16 ; + chan->scan_type.endianness =IIO_CPU ; + +if (differential) { + chan->differential =1 ; + chan->channel2 = vinn; + } +} + +static int ma35d1_adc_parse_channels (struct iio_dev *indio_dev, +struct device *dev) +{ +DECLARE_BITMAP (used_regs,MA35D1_EADC_MAX_EXT_CHANNELS ); +struct iio_chan_spec *channels; +unsigned int scan_index; +int num_channels; +int ret; + +bitmap_zero (used_regs,MA35D1_EADC_MAX_EXT_CHANNELS ); + + num_channels =device_get_child_node_count (dev); +if (!num_channels) +return dev_err_probe (dev, -ENOENT ,"no ADC channels configured\n" ); + +if (num_channels >MA35D1_EADC_MAX_EXT_CHANNELS ) +return dev_err_probe (dev, -EINVAL ,"too many ADC channels\n" ); + + channels = devm_kcalloc(dev, size_add(num_channels, 1),
drivers/iio/adc/ma35d1_eadc.c line 595
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+ sizeof(*channels), GFP_KERNEL); +if (!channels) +return -ENOMEM ; + + scan_index =0 ; +device_for_each_child_node_scoped (dev, child) { +bool differential; +u32 reg; +u32 vinp; +u32 vinn;
Can combine these related u32s on oneline.
Quoted 19 lines
drivers/iio/adc/ma35d1_eadc.c new file +19
-0+ + 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, -EINVAL , +"invalid ADC channel index %u\n" , reg); + +if (test_and_set_bit (reg, used_regs)) +return dev_err_probe (dev, -EINVAL , +"duplicate ADC channel index %u\n" , reg); + +if (fwnode_property_present (child,"single-channel" )) { + ret =fwnode_property_read_u32 (child,"single-channel" , + &vinp); +if (ret) + return dev_err_probe(dev, ret,
drivers/iio/adc/ma35d1_eadc.c line 624
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+"invalid single-channel for index %u\n" , + reg); + +if (vinp >=MA35D1_EADC_MAX_EXT_CHANNELS ) +return dev_err_probe (dev, -EINVAL , +"invalid ADC input %u\n" , vinp); + + vinn =0 ; + differential = false; + }else {
This will change to match my requests for the binding so I won't look too closely at it in this revision.
drivers/iio/adc/ma35d1_eadc.c line 635
Quoted 11 lines
drivers/iio/adc/ma35d1_eadc.c new file +11
-0+if (reg >=4 ) +return dev_err_probe (dev, -EINVAL , +"invalid differential pair %u\n" , reg); + + vinp = reg; + vinn = reg +4 ; + differential = true; + } + +ma35d1_adc_init_channel (&channels[scan_index], reg, vinp, vinn, + scan_index, differential);
get rid of this and fill the structure in directly in the two different paths above - so have a single ended copy of this and the slightly different one for the differential channels. Bit of duplication but simpler code by putting it all in one place.
channels[scan_index] = (struct iio_chan_spec) {
.type = IIO_VOLTAGE,
.indexed = 1,
.channel = index,
.address = vinp,
.scan_index = scan_index,
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
BIT(IIO_CHAN_INFO_SCALE);
.scan_type = {
.format = IIO_SCAN_FORMAT_UNSIGNED_INT,
.realbits = 12,
.storagebits = 16,
.endianness = IIO_CPU,;
};Quoted 26 lines
drivers/iio/adc/ma35d1_eadc.c new file +24
-0+ scan_index++; + } + + channels[scan_index] =IIO_CHAN_SOFT_TIMESTAMP (scan_index); + + indio_dev->channels = channels; + indio_dev->num_channels = scan_index +1 ; + +return 0 ; +} + +static int ma35d1_adc_init_vref (struct ma35d1_adc *adc,struct device *dev) +{ +int ret; + + adc->vref =devm_regulator_get_optional (dev,"vref" ); + ret =PTR_ERR_OR_ZERO (adc->vref); +if (ret == -ENODEV ) { + adc->vref =NULL ; + adc->vref_mV = MA35D1_EADC_INTERNAL_VREF_mV; +return 0 ; + } +if (ret) +return dev_err_probe (dev, ret,"failed to get VREF supply\n" );
drivers/iio/adc/ma35d1_eadc.c line 669
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-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);
There are plenty of examples of how to use devm_regulator_get_enable_read_voltage() with optional regulators in tree. Follow one of those and this code will be a lot shorter.
Quoted 20 lines
drivers/iio/adc/ma35d1_eadc.c new file +19
-0+if (ret <=0 ) +return dev_err_probe (dev, ret ?: -EINVAL , +"failed to get VREF voltage\n" ); + + adc->vref_mV = ret / (MICRO /MILLI ); + +return 0 ; +} +static int ma35d1_adc_suspend (struct device *dev) +{ +struct iio_dev *indio_dev =dev_get_drvdata (dev); +struct ma35d1_adc *adc =iio_priv (indio_dev); +int ret; + + ret =iio_device_suspend_triggering (indio_dev); +if (ret) +return ret; + +mutex_lock (&adc->lock);
drivers/iio/adc/ma35d1_eadc.c line 783
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+ + ret =ma35d1_adc_disable_irq (adc); +if (ret) + goto resume_triggering; + +synchronize_irq (adc->irq); + + ret =regmap_clear_bits (adc->regmap,MA35D1_EADC_CTL , +MA35D1_EADC_CTL_ADCEN ); +if (ret)
If this fails, for consistency I think you should probably reenable the irq. If not add a comment on your thinking for why.
Quoted 15 lines
drivers/iio/adc/ma35d1_eadc.c new file +15
-0+ goto resume_triggering; + + adc->suspended = true; +clk_disable_unprepare (adc->clk); +mutex_unlock (&adc->lock); + +return 0 ; + +resume_triggering: +mutex_unlock (&adc->lock); +iio_device_resume_triggering (indio_dev); + +return ret; +} +
Re: [PATCH v3 2/2] iio: adc: Add Nuvoton MA35D1 EADC driver
Jonathan Cameron 於 2026/11/1 上午 08:53 寫道:
Quoted 574 lines
Quoted 29 lines · On Fri, 30 Oct 2026 11:15:01 +0800 Chi-Wen Weng <[email protected]> wrote:
From: Chi-Wen Weng <[email protected]>
Add an IIO driver for the Enhanced Analog-to-Digital Converter (EADC) found on Nuvoton MA35D1 SoCs.
The driver supports direct conversion and externally triggered buffered capture using sample module 0. Both single-ended inputs and the four fixed differential input pairs are supported.
Use the internal 1.6 V reference when no external reference supply is provided, and report the reference voltage through the IIO scale interface.
Use ADINT0 for conversion completion. Configure the interrupt source and end-of-conversion position explicitly, and recover from conversion timeouts by resetting the EADC control circuits, synchronizing the IRQ and clearing stale conversion state before another conversion is started.
Reset the EADC control circuits during hardware initialization and sanitize interrupt, compare and sample-module trigger configuration so the driver does not depend on state left by firmware when the external reset control is unavailable.
Provide firmware-node channel translation so DT IIO consumers select channels by the logical channel index described by the channel node's reg property rather than by the position of the channel in the driver's channel array.
Quoted 4 lines
Add clock, optional reset and system sleep handling, and restore the controller configuration after resume.
Signed-off-by Chi-Wen Weng <[email protected]>Hi Chi-Wen
Various comments inline.
Thanks,
Jonathan
Quoted 13 lines
drivers/iio/adc/ma35d1_eadc.c new file +6
-0+ +static int ma35d1_adc_disable_irq (struct ma35d1_adc *adc) +{ +return regmap_clear_bits (adc->regmap,MA35D1_EADC_CTL ,MA35D1_EADC_CTL_ADCIEN0 ); +} +drivers/iio/adc/ma35d1_eadc.c line 117
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+static int ma35d1_adc_setup_reference (struct ma35d1_adc *adc) +{ +struct regmap *map = adc->regmap; +int ret; + +/* +* MA35D1 TRM EADC reference control: PDREF selects the external VREF +* pin. With the internal reference selected, VREFSEL = 0 selects 1.6 V. +*/ +if (adc->vref) {With change below to use the
read_voltagefunction this would then be using a flag set there.Quoted 29 lines
drivers/iio/adc/ma35d1_eadc.c new file +27
-0+ ret =regmap_set_bits (map,MA35D1_EADC_REFADJCTL , +MA35D1_EADC_REFADJCTL_PDREF ); +if (ret) +return ret; + }else { + ret =regmap_clear_bits (map,MA35D1_EADC_REFADJCTL , +MA35D1_EADC_REFADJCTL_PDREF ); +if (ret) +return ret; + + ret =regmap_update_bits (map,MA35D1_EADC_CTL , +MA35D1_EADC_CTL_VREFSEL_MASK , +MA35D1_EADC_CTL_VREFSEL_1V6 ); +if (ret) +return ret; + } + +fsleep (MA35D1_EADC_REF_STABLE_US ); + +return 0 ; +} +static int ma35d1_adc_abort_conversion (struct ma35d1_adc *adc) +{ +struct regmap *map = adc->regmap; +int ret; +int err; +drivers/iio/adc/ma35d1_eadc.c
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+ ret =ma35d1_adc_disable_irq (adc); + +/* +* Reset the EADC control circuits to terminate a conversion which may +* still be in progress. EADCRST is cleared automatically by hardware +* when the reset completes and the EADC register values are preserved. +*/ + err =ma35d1_adc_reset_control (adc); +if (!ret) + ret = err;Why not just set ret directly for this one?
drivers/iio/adc/ma35d1_eadc.c line 301
Quoted 6 lines
drivers/iio/adc/ma35d1_eadc.c new file +6
-0+ +/* Drain an interrupt which may already have been latched by the GIC. */ +synchronize_irq (adc->irq); + + err =regmap_write (map,MA35D1_EADC_STATUS2 , +MA35D1_EADC_STATUS2_ADIF0 );I don't mind going slightly over 80 chars. I would do so for the line above.
drivers/iio/adc/ma35d1_eadc.c line 305
Quoted 4 lines
drivers/iio/adc/ma35d1_eadc.c new file +4
-0+if (!ret) + ret = err; + +reinit_completion (&adc->completion);Why? It is reinit at start of read conversion - why else do we need to do it? Add a comment
Quoted 25 lines
drivers/iio/adc/ma35d1_eadc.c new file +25
-0+ +return ret; +} + +static int ma35d1_adc_read_conversion (struct ma35d1_adc *adc, +const struct iio_chan_spec *chan,u16 *raw) +{ +struct regmap *map = adc->regmap; +unsigned int val; +long timeout; +int ret; + +reinit_completion (&adc->completion); + + ret =regmap_write (map,MA35D1_EADC_STATUS2 ,MA35D1_EADC_STATUS2_ADIF0 ); +if (ret) +return ret; + + ret =ma35d1_adc_config_channel (adc, chan); +if (ret) +return ret; + + ret =regmap_set_bits (map,MA35D1_EADC_CTL ,MA35D1_EADC_CTL_ADCIEN0 ); +if (ret) +return ret;drivers/iio/adc/ma35d1_eadc.c line 340
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+ + ret =regmap_write (map,MA35D1_EADC_SWTRG ,BIT (0 )); +if (ret) + goto disable_irq; + + timeout =wait_for_completion_timeout (&adc->completion, +MA35D1_EADC_TIMEOUT ); +if (!timeout) { + ret =ma35d1_adc_abort_conversion (adc); +return ret ?: -ETIMEDOUT ;Timed out first, so I think that should be the return value even if the abort then failed. Maybe an error print is more appropriate for a failed abort so we don't loose that extra info.
Quoted 7 lines
drivers/iio/adc/ma35d1_eadc.c new file +7
-0+ } + + ret =ma35d1_adc_disable_irq (adc); +if (ret) +return ret; + + ret =regmap_read (map,MA35D1_EADC_DAT (0 ), &val);drivers/iio/adc/ma35d1_eadc.c line 357
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+if (ret) +return ret; + +if (!(val &MA35D1_EADC_DAT_VALID )) +return -EIO ; + +if (val &MA35D1_EADC_DAT_OV ) +return -EOVERFLOW ; + + *raw =FIELD_GET (MA35D1_EADC_DAT_RESULT_MASK , val);As below - I don't follow why we'd get 16 bits when I think all paths throw away everything above the 12th.
Quoted 20 lines
drivers/iio/adc/ma35d1_eadc.c new file +20
-0+ +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 ; +drivers/iio/adc/ma35d1_eadc.c line 387
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+regmap_write (adc->regmap,MA35D1_EADC_STATUS2 , +MA35D1_EADC_STATUS2_ADIF0 ); +complete (&adc->completion); + +return IRQ_HANDLED ; +} + +static int ma35d1_adc_sign_extend (u16 raw) +{ +return sign_extend32 (raw &MA35D1_EADC_DAT_RESULT_12BIT ,11 );If the result is always 12 bits, why is
read_conversionreturning 16?Quoted 4 lines
drivers/iio/adc/ma35d1_eadc.c new file +4
-0+} + +static int ma35d1_adc_read_raw(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan,drivers/iio/adc/ma35d1_eadc.c line 401
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+ int *val, int *val2, long mask) +{ + struct ma35d1_adc *adc = iio_priv(indio_dev); +u16 raw; +int ret; + +switch (mask) { +case IIO_CHAN_INFO_RAW : { +IIO_DEV_ACQUIRE_DIRECT_MODE (indio_dev, claim); +No blank line here. We want the acquire and the error check tightly coupled visually.
Quoted 26 lines
drivers/iio/adc/ma35d1_eadc.c new file +26
-0+if (IIO_DEV_ACQUIRE_FAILED (claim)) +return -EBUSY ; + +guard (mutex)(&adc->lock); +if (adc->suspended) +return -EBUSY ; + + ret =ma35d1_adc_read_conversion (adc, chan, &raw); +if (ret) +return ret; + +if (chan->differential) + *val =ma35d1_adc_sign_extend (raw); +else + *val = raw &MA35D1_EADC_DAT_RESULT_12BIT ; + +return IIO_VAL_INT ; + } +case IIO_CHAN_INFO_SCALE : + *val = adc->vref_mV; +/* Differential mode has a VREF / 2048 LSB; single-ended uses /4096. */ + *val2 = chan->differential ?11 :12 ; +return IIO_VAL_FRACTIONAL_LOG2 ; +default : +return -EINVAL ; + }drivers/iio/adc/ma35d1_eadc.c line 437
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-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; + +for_each_set_bit (bit, scan_mask,iio_get_masklength (indio_dev)) {
iio_for_each_active_channel()drivers/iio/adc/ma35d1_eadc.c line 446
Quoted 9 lines
drivers/iio/adc/ma35d1_eadc.c new file +9
-0+const struct iio_chan_spec *chan = &indio_dev->channels[bit]; + +if (chan->type ==IIO_TIMESTAMP ) +continue ; + +if (chan->type !=IIO_VOLTAGE || scan_chan) +return -EINVAL ; + + scan_chan = chan;This only makes sense if there is only one channel enabled. The driver needs to ensure that is the case (there is a standard validation callback for this).
drivers/iio/adc/ma35d1_eadc.c line 486
Quoted 12 lines
drivers/iio/adc/ma35d1_eadc.c new file +11
-0+ } + +if (!scan_chan) +return -EINVAL ; + + adc->scan_chan = scan_chan; + +return 0 ; +} + +static irqreturn_t ma35d1_adc_trigger_handler (int irq,void *p)This looks like the device only supports onehot channels (one at a time) That's fine but then you should have the callbacks or
available_channelsset to indicate that.drivers/iio/adc/ma35d1_eadc.c line 491
Quoted 5 lines
drivers/iio/adc/ma35d1_eadc.c new file +5
-0+{ +struct iio_poll_func *pf = p; +struct iio_dev *indio_dev = pf->indio_dev; +struct ma35d1_adc *adc =iio_priv (indio_dev); + IIO_DECLARE_BUFFER_WITH_TS(u16, scan, 1) = { };Where the timestamp location is fixed (if present) I prefer
struct { u16 channel; aligned_s64 timestamp; } scan;as it makes it easier to understand the layout. The macro is for the case where different numbers of channels can be enabled and there are enough of them to have the timestamp moving around depending on what is enabled.
Quoted 21 lines
drivers/iio/adc/ma35d1_eadc.c new file +20
-0+u16 raw; +int ret; + +mutex_lock (&adc->lock); + +if (!adc->suspended && adc->scan_chan) { + ret =ma35d1_adc_read_conversion (adc, adc->scan_chan, &raw); +if (!ret) { + scan[0 ] = raw &MA35D1_EADC_DAT_RESULT_12BIT ; +iio_push_to_buffers_with_ts (indio_dev, scan,sizeof (scan), + pf->timestamp); + } + } + +mutex_unlock (&adc->lock); + +iio_trigger_notify_done (indio_dev->trig); + +return IRQ_HANDLED ; +}drivers/iio/adc/ma35d1_eadc.c line 526
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+ +static int ma35d1_adc_fwnode_xlate (struct iio_dev *indio_dev, +const struct fwnode_reference_args *iiospec) +{ +unsigned int i; + +if (iiospec->nargs !=1 ) +return -EINVAL ; + +for (i =0 ; i < indio_dev->num_channels; i++) {for (unsigned int i = 0; ..
Quoted 7 lines
drivers/iio/adc/ma35d1_eadc.c new file +7
-0+const struct iio_chan_spec *chan = &indio_dev->channels[i]; + +if (chan->type ==IIO_VOLTAGE && + chan->channel == iiospec->args[0 ]) +return i; + } +drivers/iio/adc/ma35d1_eadc.c line 543
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+return -EINVAL ; +} + +static const struct iio_info ma35d1_adc_info = { + .read_raw = ma35d1_adc_read_raw, + .update_scan_mode = ma35d1_adc_update_scan_mode, + .fwnode_xlate = ma35d1_adc_fwnode_xlate, +}; + +static void ma35d1_adc_init_channel(struct iio_chan_spec *chan, u32 index,As below. Given C provides nice ways to set lots of structure elements helpers use those, rather than a fill the values in helper.
Quoted 42 lines
drivers/iio/adc/ma35d1_eadc.c new file +42
-0+ u32 vinp, u32 vinn, + unsigned int scan_index, + bool differential) +{ + chan->type = IIO_VOLTAGE; + chan->indexed =1 ; + chan->channel = index; + chan->address = vinp; + chan->scan_index = scan_index; + chan->info_mask_separate = +BIT (IIO_CHAN_INFO_RAW ) |BIT (IIO_CHAN_INFO_SCALE ); + chan->scan_type.format = differential ?IIO_SCAN_FORMAT_SIGNED_INT : +IIO_SCAN_FORMAT_UNSIGNED_INT ; + chan->scan_type.realbits =12 ; + chan->scan_type.storagebits =16 ; + chan->scan_type.endianness =IIO_CPU ; + +if (differential) { + chan->differential =1 ; + chan->channel2 = vinn; + } +} + +static int ma35d1_adc_parse_channels (struct iio_dev *indio_dev, +struct device *dev) +{ +DECLARE_BITMAP (used_regs,MA35D1_EADC_MAX_EXT_CHANNELS ); +struct iio_chan_spec *channels; +unsigned int scan_index; +int num_channels; +int ret; + +bitmap_zero (used_regs,MA35D1_EADC_MAX_EXT_CHANNELS ); + + num_channels =device_get_child_node_count (dev); +if (!num_channels) +return dev_err_probe (dev, -ENOENT ,"no ADC channels configured\n" ); + +if (num_channels >MA35D1_EADC_MAX_EXT_CHANNELS ) +return dev_err_probe (dev, -EINVAL ,"too many ADC channels\n" ); + + channels = devm_kcalloc(dev, size_add(num_channels, 1),drivers/iio/adc/ma35d1_eadc.c line 595
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+ sizeof(*channels), GFP_KERNEL); +if (!channels) +return -ENOMEM ; + + scan_index =0 ; +device_for_each_child_node_scoped (dev, child) { +bool differential; +u32 reg; +u32 vinp; +u32 vinn;Can combine these related u32s on oneline.
Quoted 19 lines
drivers/iio/adc/ma35d1_eadc.c new file +19
-0+ + 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, -EINVAL , +"invalid ADC channel index %u\n" , reg); + +if (test_and_set_bit (reg, used_regs)) +return dev_err_probe (dev, -EINVAL , +"duplicate ADC channel index %u\n" , reg); + +if (fwnode_property_present (child,"single-channel" )) { + ret =fwnode_property_read_u32 (child,"single-channel" , + &vinp); +if (ret) + return dev_err_probe(dev, ret,drivers/iio/adc/ma35d1_eadc.c line 624
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+"invalid single-channel for index %u\n" , + reg); + +if (vinp >=MA35D1_EADC_MAX_EXT_CHANNELS ) +return dev_err_probe (dev, -EINVAL , +"invalid ADC input %u\n" , vinp); + + vinn =0 ; + differential = false; + }else {This will change to match my requests for the binding so I won't look too closely at it in this revision.
drivers/iio/adc/ma35d1_eadc.c line 635
Quoted 11 lines
drivers/iio/adc/ma35d1_eadc.c new file +11
-0+if (reg >=4 ) +return dev_err_probe (dev, -EINVAL , +"invalid differential pair %u\n" , reg); + + vinp = reg; + vinn = reg +4 ; + differential = true; + } + +ma35d1_adc_init_channel (&channels[scan_index], reg, vinp, vinn, + scan_index, differential);get rid of this and fill the structure in directly in the two different paths above - so have a single ended copy of this and the slightly different one for the differential channels. Bit of duplication but simpler code by putting it all in one place.
channels[scan_index] = (struct iio_chan_spec) { .type = IIO_VOLTAGE, .indexed = 1, .channel = index, .address = vinp, .scan_index = scan_index, .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE); .scan_type = { .format = IIO_SCAN_FORMAT_UNSIGNED_INT, .realbits = 12, .storagebits = 16, .endianness = IIO_CPU,; };Quoted 26 lines
drivers/iio/adc/ma35d1_eadc.c new file +24
-0+ scan_index++; + } + + channels[scan_index] =IIO_CHAN_SOFT_TIMESTAMP (scan_index); + + indio_dev->channels = channels; + indio_dev->num_channels = scan_index +1 ; + +return 0 ; +} + +static int ma35d1_adc_init_vref (struct ma35d1_adc *adc,struct device *dev) +{ +int ret; + + adc->vref =devm_regulator_get_optional (dev,"vref" ); + ret =PTR_ERR_OR_ZERO (adc->vref); +if (ret == -ENODEV ) { + adc->vref =NULL ; + adc->vref_mV = MA35D1_EADC_INTERNAL_VREF_mV; +return 0 ; + } +if (ret) +return dev_err_probe (dev, ret,"failed to get VREF supply\n" );drivers/iio/adc/ma35d1_eadc.c line 669
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-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);There are plenty of examples of how to use
devm_regulator_get_enable_read_voltage()with optional regulators in tree. Follow one of those and this code will be a lot shorter.Quoted 20 lines
drivers/iio/adc/ma35d1_eadc.c new file +19
-0+if (ret <=0 ) +return dev_err_probe (dev, ret ?: -EINVAL , +"failed to get VREF voltage\n" ); + + adc->vref_mV = ret / (MICRO /MILLI ); + +return 0 ; +} +static int ma35d1_adc_suspend (struct device *dev) +{ +struct iio_dev *indio_dev =dev_get_drvdata (dev); +struct ma35d1_adc *adc =iio_priv (indio_dev); +int ret; + + ret =iio_device_suspend_triggering (indio_dev); +if (ret) +return ret; + +mutex_lock (&adc->lock);drivers/iio/adc/ma35d1_eadc.c line 783
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+ + ret =ma35d1_adc_disable_irq (adc); +if (ret) + goto resume_triggering; + +synchronize_irq (adc->irq); + + ret =regmap_clear_bits (adc->regmap,MA35D1_EADC_CTL , +MA35D1_EADC_CTL_ADCEN ); +if (ret)If this fails, for consistency I think you should probably reenable the irq. If not add a comment on your thinking for why.
Quoted 16 lines
Quoted 5 lines
drivers/iio/adc/ma35d1_eadc.c new file +5
-0+ goto resume_triggering; + + adc->suspended = true; +clk_disable_unprepare (adc->clk); +mutex_unlock (&adc->lock);drivers/iio/adc/ma35d1_eadc.c line 798
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+ +return 0 ; + +resume_triggering: +mutex_unlock (&adc->lock); +iio_device_resume_triggering (indio_dev); + +return ret; +} +
Hi Jonathan,
Thank you for the detailed review.
I will address these comments in v4.
The channel parsing will be reworked together with the binding changes. reg will remain the logical channel index, while single-channel and diff-channels will describe the physical inputs. Accordingly, the IIO channel specification will use channel/channel2 for the physical input or differential pair, while address will keep the logical DT channel index used for firmware-node translation.
I will also remove ma35d1_adc_init_channel() and initialize the iio_chan_spec structures directly in the single-ended and differential parsing paths.
Regarding the one-hot scan-mask handling, the MA35D1 hardware itself is not limited to one sample module at a time. The EADC has one SAR converter shared by multiple sample modules. Multiple sample modules can be triggered together, after which the hardware priority arbitrator performs their conversions sequentially.
The initial driver, however, currently uses only sample module 0 and performs one voltage conversion for each buffered trigger. Therefore the buffered interface exposed by this version of the driver supports one active voltage channel per scan. I will make this limitation explicit using the standard one-hot scan-mask validation callback and use iio_for_each_active_channel() when selecting the active channel. Support for using multiple sample modules for multi-channel buffered scans can be added separately in the future.
Since the scan layout is therefore fixed to one u16 sample plus an optional timestamp, I will replace IIO_DECLARE_BUFFER_WITH_TS() with a fixed scan structure containing one u16 sample and an aligned timestamp.
For the conversion path I will make the returned ADC sample explicitly 12 bits. Differential values will then be sign-extended directly from that 12-bit value, rather than carrying a 16-bit intermediate result whose upper bits are discarded later.
For timeout recovery I will keep -ETIMEDOUT as the primary error. If resetting or aborting the timed-out conversion fails, I will report that separately instead of replacing the timeout return value. I will also remove the redundant completion reinitialization from the abort path, since the completion is reinitialized before each new conversion and the abort path already synchronizes the IRQ.
I will simplify the optional VREF handling using devm_regulator_get_enable_read_voltage() and keep a flag indicating whether the external reference is in use.
Regarding the suspend error path, ADCIEN0 is only enabled while a conversion is in progress and is normally disabled while the device is idle. The suspend path holds the device lock after triggering has been suspended, so there is no active conversion at that point. If disabling ADCEN fails, leaving ADCIEN0 disabled therefore matches the normal idle state, and the next conversion will enable it again. I will add a comment to make this state transition clear rather than unconditionally re-enabling the interrupt.
I will also take care of the smaller style points, including keeping the direct-mode acquire and error check together, using a loop-local variable in fwnode_xlate(), combining the related u32 declarations, and avoiding unnecessary line wrapping.
Thanks,
Chi-Wen
Jonathan Cameron
maintainerRe: [PATCH v3 2/2] iio: adc: Add Nuvoton MA35D1 EADC driver
Quoted 8 lines
Quoted 2 lines
Hi Jonathan,
Thank you for the detailed review.
I will address these comments in v4.
Hi,
A couple of process things.
Most of this didn't need a reply at all. When you agree with feedback and plan to modify the code, keep any description of that for the change log of v2.
Also, reply inline to individual comments. The reviewer has probably forgotten most of the details of what they commented on and so has to do the tiresome step of reading their own reply and then correlating that with what you have at the end. If you reply inline as I did during the review, the context they need is right there. Keep to tight focused comments where you want further feedback or disagree with the review.
...
Quoted 7 lines
Regarding the one-hot scan-mask handling, the MA35D1 hardware itself is not limited to one sample module at a time. The EADC has one SAR converter shared by multiple sample modules. Multiple sample modules can be triggered together, after which the hardware priority arbitrator performs their conversions sequentially.
Quoted 8 lines
The initial driver, however, currently uses only sample module 0 and performs one voltage conversion for each buffered trigger. Therefore the buffered interface exposed by this version of the driver supports one active voltage channel per scan. I will make this limitation explicit using the standard one-hot scan-mask validation callback and use
iio_for_each_active_channel()when selecting the active channel. Support for using multiple sample modules for multi-channel buffered scans can be added separately in the future.
This part would have been helpful inline next to the comment it is addressing.
Quoted 4 lines
Since the scan layout is therefore fixed to one u16 sample plus an optional timestamp, I will replace
IIO_DECLARE_BUFFER_WITH_TS()with a fixed scan structure containing one u16 sample and an aligned timestamp.
Jonathan
Re: [PATCH v3 2/2] iio: adc: Add Nuvoton MA35D1 EADC driver
In reply to Chi-Wen WengQuoted 29 lines · Chi-Wen Weng
Add an IIO driver for the Enhanced Analog-to-Digital Converter (EADC) found on Nuvoton MA35D1 SoCs.
The driver supports direct conversion and externally triggered buffered capture using sample module 0. Both single-ended inputs and the four fixed differential input pairs are supported.
Use the internal 1.6 V reference when no external reference supply is provided, and report the reference voltage through the IIO scale interface.
Use ADINT0 for conversion completion. Configure the interrupt source and end-of-conversion position explicitly, and recover from conversion timeouts by resetting the EADC control circuits, synchronizing the IRQ and clearing stale conversion state before another conversion is started.
Reset the EADC control circuits during hardware initialization and sanitize interrupt, compare and sample-module trigger configuration so the driver does not depend on state left by firmware when the external reset control is unavailable.
Provide firmware-node channel translation so DT IIO consumers select channels by the logical channel index described by the channel node's reg property rather than by the position of the channel in the driver's channel array.
Add clock, optional reset and system sleep handling, and restore the controller configuration after resume.
...
drivers/iio/adc/ma35d1_eadc.c line 100
Quoted 7 lines
drivers/iio/adc/ma35d1_eadc.c new file +6
-0+static const struct regmap_config ma35d1_adc_regmap_config = { + .reg_bits =32 , + .val_bits =32 , + .reg_stride =4 , + .max_register =MA35D1_EADC_REFADJCTL , + .cache_type =REGCACHE_NONE ,
This has to be justified. If it's indeed necessary, explain in the comment above why cache is disabled.
Quoted 1 line
+};
...
drivers/iio/adc/ma35d1_eadc.c line 152
Quoted 13 lines
drivers/iio/adc/ma35d1_eadc.c new file +13
-0+static int ma35d1_adc_reset_control (struct ma35d1_adc *adc) +{ +struct regmap *map = adc->regmap; +unsigned int val; +int ret; + + ret =regmap_set_bits (map,MA35D1_EADC_CTL ,MA35D1_EADC_CTL_EADCRST ); +if (ret) +return ret; + +return regmap_read_poll_timeout (map,MA35D1_EADC_CTL , val, + !(val &MA35D1_EADC_CTL_EADCRST ), +1 ,USEC_PER_MSEC );
Use numbers with multipliers, id est 1 * USEC_PER_MSEC.
Quoted 1 line
+}
...
drivers/iio/adc/ma35d1_eadc.c line 301
Quoted 5 lines
drivers/iio/adc/ma35d1_eadc.c new file +5
-0+/* Drain an interrupt which may already have been latched by the GIC. */ +synchronize_irq (adc->irq); + + err =regmap_write (map,MA35D1_EADC_STATUS2 , +MA35D1_EADC_STATUS2_ADIF0 );
It's perfectly a single line precisely with 80 characters in it.
Quoted 4 lines
drivers/iio/adc/ma35d1_eadc.c new file +4
-0+if (!ret) + ret = err; + +reinit_completion (&adc->completion);
...
drivers/iio/adc/ma35d1_eadc.c line 376
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +9
-0+static irqreturn_t ma35d1_adc_isr (int irq,void *data) +{ +struct iio_dev *indio_dev = data; +struct ma35d1_adc *adc =iio_priv (indio_dev); +unsigned int status; +int ret; + ret =regmap_read (adc->regmap,MA35D1_EADC_STATUS2 , &status); +if (ret || !(status &MA35D1_EADC_STATUS2_ADIF0 )) +return IRQ_NONE ;
Split this to two conditions, it will be better to maintain in a long term.
drivers/iio/adc/ma35d1_eadc.c line 379
Quoted 2 lines
drivers/iio/adc/ma35d1_eadc.c new file +2
-0+regmap_write (adc->regmap,MA35D1_EADC_STATUS2 , +MA35D1_EADC_STATUS2_ADIF0 );
Use local variable and make this a single line.
Btw, no return check here because...?
Quoted 4 lines
drivers/iio/adc/ma35d1_eadc.c new file +4
-0+complete (&adc->completion); + +return IRQ_HANDLED ; +}
...
Quoted 6 lines
drivers/iio/adc/ma35d1_eadc.c new file +6
-0+static int ma35d1_adc_read_raw (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan, +int *val,int *val2,long mask) +{ +struct ma35d1_adc *adc =iio_priv (indio_dev); +u16 raw;
drivers/iio/adc/ma35d1_eadc.c line 405
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+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);
+ blank line. Make guard()() to be visible in the code at a glance.
Quoted 8 lines
drivers/iio/adc/ma35d1_eadc.c new file +8
-0+if (adc->suspended) +return -EBUSY ; + + ret =ma35d1_adc_read_conversion (adc, chan, &raw); +if (ret) +return ret; + +if (chan->differential)
drivers/iio/adc/ma35d1_eadc.c line 423
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+ *val =ma35d1_adc_sign_extend (raw); + else + *val = raw &MA35D1_EADC_DAT_RESULT_12BIT ; + +return IIO_VAL_INT ; + } +case IIO_CHAN_INFO_SCALE : + *val = adc->vref_mV; +/* Differential mode has a VREF / 2048 LSB; single-ended uses /4096. */ + *val2 = chan->differential ?11 :12 ;
Perhaps make two definitions? 11 already second time appears in the code.
Quoted 5 lines
drivers/iio/adc/ma35d1_eadc.c new file +5
-0+return IIO_VAL_FRACTIONAL_LOG2 ; +default : +return -EINVAL ; + } +}
...
drivers/iio/adc/ma35d1_eadc.c line 438
Quoted 9 lines
drivers/iio/adc/ma35d1_eadc.c new file +9
-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; + +for_each_set_bit (bit, scan_mask,iio_get_masklength (indio_dev)) { +const struct iio_chan_spec *chan = &indio_dev->channels[bit];
IIRC we have a for_each macro for this.
Quoted 17 lines
drivers/iio/adc/ma35d1_eadc.c new file +17
-0+ +if (chan->type ==IIO_TIMESTAMP ) +continue ; + +if (chan->type !=IIO_VOLTAGE || scan_chan) +return -EINVAL ; + + scan_chan = chan; + } + +if (!scan_chan) +return -EINVAL ; + + adc->scan_chan = scan_chan; + +return 0 ; +}
...
drivers/iio/adc/ma35d1_eadc.c line 466
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-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 ;
Unroll to if return; return. As it's done just in above function.
Quoted 1 line
+}
...
drivers/iio/adc/ma35d1_eadc.c line 495
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-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; + +mutex_lock (&adc->lock);
Use guard()(). This might require to split the content into the helper function.
Quoted 15 lines
drivers/iio/adc/ma35d1_eadc.c new file +15
-0+if (!adc->suspended && adc->scan_chan) { + ret =ma35d1_adc_read_conversion (adc, adc->scan_chan, &raw); +if (!ret) { + scan[0 ] = raw &MA35D1_EADC_DAT_RESULT_12BIT ; +iio_push_to_buffers_with_ts (indio_dev, scan,sizeof (scan), + pf->timestamp); + } + } + +mutex_unlock (&adc->lock); + +iio_trigger_notify_done (indio_dev->trig); + +return IRQ_HANDLED ; +}
...
drivers/iio/adc/ma35d1_eadc.c line 521
Quoted 4 lines
drivers/iio/adc/ma35d1_eadc.c new file +4
-0+static int ma35d1_adc_fwnode_xlate (struct iio_dev *indio_dev, +const struct fwnode_reference_args *iiospec) +{ +unsigned int i;
Not used outside the loop.
drivers/iio/adc/ma35d1_eadc.c line 526
Quoted 4 lines
drivers/iio/adc/ma35d1_eadc.c new file +4
-0+if (iiospec->nargs !=1 ) +return -EINVAL ; + +for (i =0 ; i < indio_dev->num_channels; i++) {
for (unsigned int i = 0; i < indio_dev->num_channels; i++) {Quoted 9 lines
drivers/iio/adc/ma35d1_eadc.c new file +9
-0+const struct iio_chan_spec *chan = &indio_dev->channels[i]; + +if (chan->type ==IIO_VOLTAGE && + chan->channel == iiospec->args[0 ]) +return i; + } + +return -EINVAL ; +}
...
Quoted 7 lines
drivers/iio/adc/ma35d1_eadc.c new file +7
-0+static int ma35d1_adc_parse_channels (struct iio_dev *indio_dev, +struct device *dev) +{ +DECLARE_BITMAP (used_regs,MA35D1_EADC_MAX_EXT_CHANNELS ); +struct iio_chan_spec *channels; +unsigned int scan_index; +int num_channels;
drivers/iio/adc/ma35d1_eadc.c line 583
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+int ret; + +bitmap_zero (used_regs,MA35D1_EADC_MAX_EXT_CHANNELS ); + + num_channels =device_get_child_node_count (dev); +if (!num_channels) +return dev_err_probe (dev, -ENOENT ,"no ADC channels configured\n" ); + +if (num_channels >MA35D1_EADC_MAX_EXT_CHANNELS ) +return dev_err_probe (dev, -EINVAL ,"too many ADC channels\n" );
-ENOSPC
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+ + channels =devm_kcalloc (dev,size_add (num_channels,1 ), +sizeof (*channels),GFP_KERNEL ); +if (!channels) +return -ENOMEM ; + + scan_index =0 ; +device_for_each_child_node_scoped (dev, child) { +bool differential; +u32 reg;
drivers/iio/adc/ma35d1_eadc.c line 603
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+u32 vinp; +u32 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, -EINVAL,
-ENOSPC
drivers/iio/adc/ma35d1_eadc.c line 608
Quoted 5 lines
drivers/iio/adc/ma35d1_eadc.c new file +5
-0+ "invalid ADC channel index %u\n", reg); + +if (test_and_set_bit (reg, used_regs)) +return dev_err_probe (dev, -EINVAL , +"duplicate ADC channel index %u\n" , reg);
-EEXIST
drivers/iio/adc/ma35d1_eadc.c line 620
Quoted 11 lines
drivers/iio/adc/ma35d1_eadc.c new file +11
-0+if (fwnode_property_present (child,"single-channel" )) { + ret =fwnode_property_read_u32 (child,"single-channel" , + &vinp); +if (ret) +return dev_err_probe (dev, ret, +"invalid single-channel for index %u\n" , + reg); + +if (vinp >=MA35D1_EADC_MAX_EXT_CHANNELS ) +return dev_err_probe (dev, -EINVAL , +"invalid ADC input %u\n" , vinp);
-ECHRNG
drivers/iio/adc/ma35d1_eadc.c line 626
Quoted 5 lines
drivers/iio/adc/ma35d1_eadc.c new file +5
-0+ vinn =0 ; + differential = false; + }else { +if (reg >=4 ) + return dev_err_probe(dev, -EINVAL,
Ditto.
Quoted 19 lines
drivers/iio/adc/ma35d1_eadc.c new file +19
-0+ "invalid differential pair %u\n", reg); + + vinp = reg; + vinn = reg +4 ; + differential = true; + } + +ma35d1_adc_init_channel (&channels[scan_index], reg, vinp, vinn, + scan_index, differential); + scan_index++; + } + + channels[scan_index] =IIO_CHAN_SOFT_TIMESTAMP (scan_index); + + indio_dev->channels = channels; + indio_dev->num_channels = scan_index +1 ; + +return 0 ; +}
...
Quoted 14 lines
drivers/iio/adc/ma35d1_eadc.c new file +14
-0+static int ma35d1_adc_init_vref (struct ma35d1_adc *adc,struct device *dev) +{ +int ret; + + adc->vref =devm_regulator_get_optional (dev,"vref" ); + ret =PTR_ERR_OR_ZERO (adc->vref); +if (ret == -ENODEV ) { + adc->vref =NULL ; + adc->vref_mV = MA35D1_EADC_INTERNAL_VREF_mV; +return 0 ; + } +if (ret) +return dev_err_probe (dev, ret,"failed to get VREF supply\n" ); +
drivers/iio/adc/ma35d1_eadc.c line 670
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-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 )
Too many nested branches. Unroll it
drivers/iio/adc/ma35d1_eadc.c line 672
Quoted 2 lines
drivers/iio/adc/ma35d1_eadc.c new file +2
-0+return dev_err_probe (dev, ret ?: -EINVAL , +"failed to get VREF voltage\n" );
if (ret < 0) return dev_err_probe(dev, ret, "failed to get VREF voltage\n"); if (ret == 0) return dev_err_probe(dev, -EINVAL, "wrong VREF voltage\n");
Quoted 4 lines
drivers/iio/adc/ma35d1_eadc.c new file +4
-0+ adc->vref_mV = ret / (MICRO /MILLI ); + +return 0 ; +}
...
drivers/iio/adc/ma35d1_eadc.c line 766
Quoted 4 lines
drivers/iio/adc/ma35d1_eadc.c new file +4
-0+static int ma35d1_adc_suspend (struct device *dev) +{ +struct iio_dev *indio_dev =dev_get_drvdata (dev); +struct ma35d1_adc *adc =iio_priv (indio_dev);
struct regmap *map = adc->regmap;
Quoted 6 lines
drivers/iio/adc/ma35d1_eadc.c new file +6
-0+int ret; + + ret =iio_device_suspend_triggering (indio_dev); +if (ret) +return ret; +
drivers/iio/adc/ma35d1_eadc.c line 782
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+mutex_lock (&adc->lock); + + ret =ma35d1_adc_disable_irq (adc); +if (ret) + goto resume_triggering; + +synchronize_irq (adc->irq); + + ret =regmap_clear_bits (adc->regmap,MA35D1_EADC_CTL , +MA35D1_EADC_CTL_ADCEN );
ret = regmap_clear_bits(map, MA35D1_EADC_CTL, MA35D1_EADC_CTL_ADCEN);
Quoted 15 lines
drivers/iio/adc/ma35d1_eadc.c new file +15
-0+if (ret) + goto resume_triggering; + + adc->suspended = true; +clk_disable_unprepare (adc->clk); +mutex_unlock (&adc->lock); + +return 0 ; + +resume_triggering: +mutex_unlock (&adc->lock); +iio_device_resume_triggering (indio_dev); + +return ret; +}
Signature · 2 lines
With Best Regards, Andy Shevchenko