iio: adc: Add Nuvoton MA35D1 EADC support
Subsystems (3)
v2 cover iio: adc: Add Nuvoton MA35D1 EADC support
From: Chi-Wen Weng <[email protected]>
This series adds devicetree binding and IIO driver support for the Nuvoton MA35D1 Enhanced ADC controller.
The MA35D1 EADC controller supports external analog input channels, fixed differential input pairs, an optional external reference voltage, up to four end-of-conversion interrupts, an optional reset line, and an ADC-to-memory DMA request.
This initial driver supports interrupt-driven direct raw reads and triggered buffered capture. Buffered capture is paced by an external IIO trigger and is limited to a single enabled voltage channel per scan in this version. Multi-channel buffered scans and DMA support can be added later.
The driver supports both single-ended external channels and the fixed hardware differential input pairs. Differential conversions are reported as signed two's complement values. The ADC scale is reported using either the optional external vref-supply or the internal 1.6 V reference.
Changes in v2:
- Update the binding to allow up to four interrupt entries for ADINT0 to ADINT3.
- Add optional vref-supply, dmas, dma-names and resets properties to the binding.
- Keep diff-channels in the binding and restrict differential inputs to the fixed hardware pairs.
- Limit DT child channel nodes to external channels 0 to 7.
- Use regmap for register access.
- Add optional reset handling.
- Add optional external reference handling and IIO scale reporting.
- Use the internal 1.6 V reference when no vref-supply is present.
- Handle differential conversions as signed two's complement values.
- Use an external IIO trigger for buffered capture instead of a self-retriggering EOC loop.
- Limit buffered capture to a single enabled voltage channel per scan.
- Rework suspend and resume to save and restore the buffered capture state.
- Use devm_mutex_init() and scoped child-node iteration.
- Use the new scan_type.format field for buffered scan type metadata.
- Drop datasheet_name from channel setup.
- Address header, helper naming and cleanup comments from review.
Chi-Wen Weng (2):
dt-bindings: iio: adc: Add Nuvoton MA35D1 EADC
iio: adc: Add Nuvoton MA35D1 EADC driver
.../bindings/iio/adc/nuvoton,ma35d1-eadc.yaml | 163 ++++ drivers/iio/adc/Kconfig | 12 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/ma35d1_eadc.c | 740 ++++++++++++++++++ 4 files changed, 916 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml create mode 100644 drivers/iio/adc/ma35d1_eadc.c
Signature · 1 line
2.25.1
PATCH v2 1/2 dt-bindings: iio: adc: Add Nuvoton MA35D1 EADC
From: Chi-Wen Weng <[email protected]>
Add devicetree binding for the Enhanced ADC controller found on Nuvoton MA35D1 SoCs.
The controller has one register region, up to four end-of-conversion interrupts, one functional clock, and an optional reset line. The binding also describes the optional external reference supply and the optional DMA request supported by the hardware.
ADC input pins are described using standard channel child nodes. The external ADC input channels are 0 to 7. Differential inputs are described with diff-channels and are limited to the fixed hardware pairs.
--- .../bindings/iio/adc/nuvoton,ma35d1-eadc.yaml | 163 ++++++++++++++++++ 1 file changed, 163 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +163 -0
@@ -0,0 +1,163 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/adc/nuvoton,ma35d1-eadc.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Nuvoton MA35D1 Enhanced Analog to Digital Converter + +maintainers: + - Chi-Wen Weng <[email protected]> + +description: + The Nuvoton MA35D1 Enhanced Analog to Digital Converter is a 12-bit + ADC controller with 8 external input channels, 4 fixed differential + input pairs and 1 internal VBAT input. Board wired ADC inputs are + described using channel child nodes. + +properties: + compatible: + const: nuvoton,ma35d1-eadc + + reg: + maxItems: 1 + + interrupts: + minItems: 1 + maxItems: 4 + items: + - description: ADINT0 end-of-conversion interrupt + - description: ADINT1 end-of-conversion interrupt + - description: ADINT2 end-of-conversion interrupt + - description: ADINT3 end-of-conversion interrupt + + clocks: + maxItems: 1 + + resets: + maxItems: 1 + + vref-supply: + description: + External reference voltage supply connected to the VREF pin. If this + property is omitted, the internal 1.6 V reference is used. + + dmas: + maxItems: 1 + + dma-names: + const: rx + + '#address-cells': + const: 1 + + '#size-cells': + const: 0 + +patternProperties: + '^channel@[0-7]$': + type: object + $ref: adc.yaml + unevaluatedProperties: false + + properties: + reg: + maximum: 7 + + diff-channels: true + + allOf: + - if: + properties: + reg: + const: 0 + then: + properties: + diff-channels: + items: + - const: 0 + - const: 4 + - if: + properties: + reg: + const: 1 + then: + properties: + diff-channels: + items: + - const: 1 + - const: 5 + - if: + properties: + reg: + const: 2 + then: + properties: + diff-channels: + items: + - const: 2 + - const: 6 + - if: + properties: + reg: + const: 3 + then: + properties: + diff-channels: + items: + - const: 3 + - const: 7 + - if: + properties: + reg: + enum: [4, 5, 6, 7] + then: + not: + required: + - diff-channels + + required: + - reg + +required: + - compatible + - reg + - interrupts + - clocks + - '#address-cells' + - '#size-cells' + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/nuvoton,ma35d1-clk.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/reset/nuvoton,ma35d1-reset.h> + + soc { + #address-cells = <2>; + #size-cells = <2>; + + adc@40430000 { + compatible = "nuvoton,ma35d1-eadc"; + reg = <0x0 0x40430000 0x0 0x10000>; + interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&clk EADC_GATE>; + resets = <&reset MA35D1_RESET_EADC0>; + + #address-cells = <1>; + #size-cells = <0>; + + channel@0 { + reg = <0>; + }; + + channel@2 { + reg = <2>; + diff-channels = <2 6>; + }; + }; + }; +...
Signature · 1 line
2.25.1
Rob Herring (Arm)
maintainerRe: [PATCH v2 1/2] dt-bindings: iio: adc: Add Nuvoton MA35D1 EADC
Quoted 14 lines · Chi-Wen Weng
From: Chi-Wen Weng <[email protected]>
Add devicetree binding for the Enhanced ADC controller found on Nuvoton MA35D1 SoCs.
The controller has one register region, up to four end-of-conversion interrupts, one functional clock, and an optional reset line. The binding also describes the optional external reference supply and the optional DMA request supported by the hardware.
ADC input pins are described using standard channel child nodes. The external ADC input channels are 0 to 7. Differential inputs are described with diff-channels and are limited to the fixed hardware pairs.
Quoted 6 lines
Signed-off-by Chi-Wen Weng <[email protected]>--- .../bindings/iio/adc/nuvoton,ma35d1-eadc.yaml | 163 ++++++++++++++++++ 1 file changed, 163 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml
My bot found errors running 'make dt_binding_check' on your patch:
yamllint warnings/errors:
dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml: properties:interrupts: {'minItems': 1, 'maxItems': 4, 'items': [{'description': 'ADINT0 end-of-conversion interrupt'}, {'description': 'ADINT1 end-of-conversion interrupt'}, {'description': 'ADINT2 end-of-conversion interrupt'}, {'description': 'ADINT3 end-of-conversion interrupt'}]} should not be valid under {'required': ['maxItems']}
hint: "maxItems" is not needed with an "items" list
from schema $id: http://devicetree.org/meta-schemas/items.yaml
doc reference errors (make refcheckdocs):
See https://patchwork.kernel.org/project/devicetree/patch/[email protected]
The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
Re: [PATCH v2 1/2] dt-bindings: iio: adc: Add Nuvoton MA35D1 EADC
In reply to Chi-Wen WengQuoted 10 lines · On Mon, 13 Jul 2026 16:11:26 +0800 Chi-Wen Weng <[email protected]> wrote:
From: Chi-Wen Weng <[email protected]>
Add devicetree binding for the Enhanced ADC controller found on Nuvoton MA35D1 SoCs.
The controller has one register region, up to four end-of-conversion interrupts, one functional clock, and an optional reset line. The binding also describes the optional external reference supply and the optional DMA request supported by the hardware.
Quoted 5 lines
ADC input pins are described using standard channel child nodes. The external ADC input channels are 0 to 7. Differential inputs are described with diff-channels and are limited to the fixed hardware pairs.
Signed-off-by Chi-Wen Weng <[email protected]>
Hi, A few comments below,
Quoted 10 lines
--- .../bindings/iio/adc/nuvoton,ma35d1-eadc.yaml | 163 ++++++++++++++++++ 1 file changed, 163 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yamlDocumentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +0
-0
...
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml line 32
Quoted 8 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +8
-0+ interrupts: + minItems: 1 + maxItems: 4 + items: + - description: ADINT0 end-of-conversion interrupt + - description: ADINT1 end-of-conversion interrupt + - description: ADINT2 end-of-conversion interrupt + - description: ADINT3 end-of-conversion interrupt
Not sure this doc helps. What are these interrupts for? The driver only uses one of them so why are there 4?
May well need interrupt-names to allow gaps in the list to work but hard to tell without more information.
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml line 67
Quoted 12 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +12
-0+ +patternProperties: + '^channel@[0-7]$': + type: object + $ref: adc.yaml + unevaluatedProperties: false + + properties: + reg: + maximum: 7 + + diff-channels: true
Do you need that explicitly. I think adc.yaml already brings that in.
Quoted 29 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +29
-0+ + allOf: + - if: + properties: + reg: + const: 0 + then: + properties: + diff-channels: + items: + - const: 0 + - const: 4 + - if: + properties: + reg: + const: 1 + then: + properties: + diff-channels: + items: + - const: 1 + - const: 5 + - if: + properties: + reg: + const: 2 + then: + properties: + diff-channels:
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml line 106
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +10
-0+ items: + - const: 2 + - const: 6 + - if: + properties: + reg: + const: 3 + then: + properties: + diff-channels:
For these take a look at the description of diff-channels. It states that where the pairs are hard wired you can just use reg and not specify diff-channels. Seems to apply here and will greatly simplify this binding.
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml line 113
Quoted 7 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +7
-0+ items: + - const: 3 + - const: 7 + - if: + properties: + reg: + enum: [4, 5, 6, 7]
What are these channels? There is a reference to one internally wired one so what are the other 3?
Quoted 7 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,ma35d1-eadc.yaml new file +7
-0+ then: + not: + required: + - diff-channels + + required: + - reg
Re: [PATCH v2 1/2] dt-bindings: iio: adc: Add Nuvoton MA35D1 EADC
Hi Jonathan,
Thank you for the review.
Quoted 5 lines
Not sure this doc helps. What are these interrupts for? The driver only uses one of them so why are there 4?
May well need interrupt-names to allow gaps in the list to work but hard to tell without more information.
The hardware has four EADC interrupt outputs, ADINT0 to ADINT3. The conversion-complete source for each interrupt can be selected through the EADC interrupt source registers.
However, this initial driver only uses sample module 0 and routes its end-of-conversion event to ADINT0. So the binding does not need to describe the unused interrupt outputs at this stage.
I will simplify this in v3 and document only one interrupt entry for ADINT0. If support for the other ADINT lines is added later, the binding can be extended with interrupt-names at that point.
I will also drop the redundant maxItems from the interrupts property, as reported by Rob's bot.
Quoted 1 line
Do you need that explicitly. I think
adc.yamlalready brings that in.
No, it is not needed. I will drop the explicit diff-channels property from the binding.
Quoted 4 lines
For these take a look at the description of diff-channels. It states that where the pairs are hard wired you can just use reg and not specify diff-channels. Seems to apply here and will greatly simplify this binding.
Yes, the differential pairs are fixed in hardware. The valid pairs are 0-4, 1-5, 2-6 and 3-7.
I will rework the channel binding to avoid diff-channels for these fixed pairs. For fixed differential channels, reg will identify the hardware pair. For single-ended inputs, I will use single-channel so that the single-ended input number is explicit and not confused with a fixed differential pair index.
That should remove the large allOf block and make the binding much simpler.
Quoted 2 lines
What are these channels? There is a reference to one internally wired one so what are the other 3?
Channels 4 to 7 are external ADC input pins. They can be used as single-ended inputs, and in differential mode they are the fixed negative inputs for channels 0 to 3.
The internally wired channel is channel 8, which is the VBAT input. This binding only describes the external ADC input pins, so I will clarify the description to avoid confusion.
Thanks again. I will address these issues in the next revision.
Best regards,
Chi-Wen
Rob Herring
maintainerRe: [PATCH v2 1/2] dt-bindings: iio: adc: Add Nuvoton MA35D1 EADC
Quoted 15 lines · Chi-Wen Weng
Hi Jonathan,
Thank you for the review.
Quoted 5 lines
Not sure this doc helps. What are these interrupts for? The driver only uses one of them so why are there 4?
May well need interrupt-names to allow gaps in the list to work but hard to tell without more information.
The hardware has four EADC interrupt outputs, ADINT0 to ADINT3. The conversion-complete source for each interrupt can be selected through the EADC interrupt source registers.
Quoted 7 lines
However, this initial driver only uses sample module 0 and routes its end-of-conversion event to ADINT0. So the binding does not need to describe the unused interrupt outputs at this stage.
I will simplify this in v3 and document only one interrupt entry for ADINT0. If support for the other ADINT lines is added later, the binding can be extended with interrupt-names at that point.
No, document what the h/w has, not what a driver currently uses. The only exception I can think of here would be if only 1 interrupt pin is usable on a given system. Then it would be just 1 interrupt with interrupt-names to define which one is used.
Rob
Re: [PATCH v2 1/2] dt-bindings: iio: adc: Add Nuvoton MA35D1 EADC
Hi Rob,
Thank you for the clarification.
Quoted 4 lines
No, document what the h/w has, not what a driver currently uses. The only exception I can think of here would be if only 1 interrupt pin is usable on a given system. Then it would be just 1 interrupt with interrupt-names to define which one is used.
Understood. I incorrectly based the binding on the current driver implementation rather than the hardware resources.
MA35D1 has one EADC controller, EADC0, and all four interrupt outputs, ADINT0 through ADINT3, are connected to separate GIC interrupt lines.
ADINT0, ADINT1 and ADINT2 are generated from the corresponding ADIF0, ADIF1 and ADIF2 conversion interrupt flags. ADINT3 is a shared interrupt output and can be generated by ADIF3 or by any of the ADCMPF0 through ADCMPF3 result-compare flags.
I will therefore keep all four interrupt entries in the v3 binding and add interrupt-names for "adint0", "adint1", "adint2" and "adint3". The ADINT3 description will state that it is shared by the conversion and result-compare interrupt sources.
The initial driver will continue to request and use only "adint0", but that implementation limitation will not restrict the hardware description in the binding.
I will also remove the redundant maxItems property because the fixed items list already specifies the number of interrupt entries.
Thanks,
Chi-Wen
PATCH v2 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 Nuvoton MA35D1 Enhanced ADC controller.
The driver supports interrupt-driven direct raw reads and triggered buffered capture paced by an external IIO trigger. Buffered capture is limited to a single enabled voltage channel per scan in this initial implementation.
Channels are described by firmware child nodes. Single-ended external channels and the fixed hardware differential input pairs are supported. The driver reports IIO scale from either an optional external reference supply or the internal 1.6 V reference.
The driver also handles the functional clock and optional reset line. DMA support is not used by this initial driver.
--- drivers/iio/adc/Kconfig | 12 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/ma35d1_eadc.c | 740 ++++++++++++++++++++++++++++++++++ 3 files changed, 753 insertions(+) create mode 100644 drivers/iio/adc/ma35d1_eadc.c
drivers/iio/adc/Kconfig +12 -0
@@ -981,6 +981,18 @@ config LTC2497 To compile this driver as a module, choose M here: the module will be called ltc2497. +config MA35D1_EADC + tristate "Nuvoton MA35D1 EADC driver" + select IIO_BUFFER + select IIO_TRIGGERED_BUFFER + select REGMAP_MMIO + help + Say yes here to build support for the Nuvoton MA35D1 + Enhanced Analog to Digital Converter. + + To compile this driver as a module, choose M here: the module will be + called ma35d1_eadc. + config MAX1027 tristate "Maxim max1027 ADC driver" depends on SPI
drivers/iio/adc/Makefile +1 -0
@@ -85,6 +85,7 @@ obj-$(CONFIG_LTC2471) += ltc2471.o obj-$(CONFIG_LTC2485) += ltc2485.o obj-$(CONFIG_LTC2496) += ltc2496.o ltc2497-core.o obj-$(CONFIG_LTC2497) += ltc2497.o ltc2497-core.o +obj-$(CONFIG_MA35D1_EADC) += ma35d1_eadc.o obj-$(CONFIG_MAX1027) += max1027.o obj-$(CONFIG_MAX11100) += max11100.o obj-$(CONFIG_MAX1118) += max1118.o
drivers/iio/adc/ma35d1_eadc.c new file +740 -0
740 lines not on this page · raw message
Signature · 1 line
2.25.1
[email protected]
AI reviewRe: [PATCH v2 2/2] iio: adc: Add Nuvoton MA35D1 EADC driver
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [High] `ma35d1_adc_update_scan_mode` incorrectly rejects valid scan masks that include the timestamp…
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] ma35d1_adc_update_scan_mode incorrectly rejects valid scan masks that include the timestamp channel.
- [High] A timed-out conversion can leave a pending ISR that spuriously completes the next conversion, leading to stale data.
- [Low] The driver uses non-existent macros IIO_DEV_ACQUIRE_DIRECT_MODE and IIO_DEV_ACQUIRE_FAILED.
- [Low] The driver attempts to assign to a non-existent format field in struct iio_scan_type.
- [Low] The MA35D1_EADC Kconfig entry lacks an architecture or COMPILE_TEST dependency.
- [Low] Mixing guard() with goto for error handling violates cleanup.h rules.
--
--- Patch [2]: [PATCH v2 2/2] iio: adc: Add Nuvoton MA35D1 EADC driver --- commit e4312453e3f2955d13e6052b2ffd1d6a2a5739a1
Author: Chi-Wen Weng <[email protected]>
iio: adc: Add Nuvoton MA35D1 EADC driver
This commit adds an IIO driver for the Nuvoton MA35D1 Enhanced ADC controller. It supports direct raw reads and triggered buffered capture using an external IIO trigger.
Quoted 4 lines
drivers/iio/adc/Kconfig +0
-0
[ ... ]
drivers/iio/adc/Kconfig config LTC2497 · line 848
Quoted 6 lines
drivers/iio/adc/Kconfig +2
-0@@ -844,6 +844,18 @@ config LTC2497 To compile this driver as a module, choose M here: the module will be called ltc2497. +config MA35D1_EADC + tristate "Nuvoton MA35D1 EADC driver"
[Severity: Low]
Is this missing a dependency on the architecture or COMPILE_TEST? Without something like depends on ARCH_MA35 || COMPILE_TEST, this will prompt users during kernel configuration on all architectures.
Quoted 3 lines
drivers/iio/adc/Kconfig +3
-0+ select IIO_BUFFER + select IIO_TRIGGERED_BUFFER + select REGMAP_MMIO
[ ... ]
Quoted 5 lines
drivers/iio/adc/ma35d1_eadc.c new file +0
-0
[ ... ]
Quoted 3 lines
drivers/iio/adc/ma35d1_eadc.c new file +3
-0+static int ma35d1_adc_read_conversion (struct ma35d1_adc *adc, +const struct iio_chan_spec *chan,u16 *raw) +{
[ ... ]
drivers/iio/adc/ma35d1_eadc.c line 271
Quoted 9 lines
drivers/iio/adc/ma35d1_eadc.c new file +9
-0+ timeout =wait_for_completion_timeout (&adc->completion, +MA35D1_EADC_TIMEOUT ); + + ret =ma35d1_adc_disable_irq (adc); +if (ret) +return ret; + +if (!timeout) +return -ETIMEDOUT ;
[Severity: High]
Does this timeout path need a call to synchronize_irq()?
If the hardware interrupt fires right as the timeout expires, the completion could be signalled by the delayed ISR after we return -ETIMEDOUT. The next conversion would call reinit_completion(), but the pending ISR might immediately complete it, returning stale data or failing.
Quoted 2 lines
drivers/iio/adc/ma35d1_eadc.c new file +2
-0+ + ret =regmap_read (adc->regmap,MA35D1_EADC_DAT (0 ), &val);
[ ... ]
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, +int *val,int *val2,long mask) +{
drivers/iio/adc/ma35d1_eadc.c line 329
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+struct ma35d1_adc *adc =iio_priv (indio_dev); +u16 raw; +int ret; + +switch (mask) { +case IIO_CHAN_INFO_RAW : { +IIO_DEV_ACQUIRE_DIRECT_MODE (indio_dev, claim); + +if (IIO_DEV_ACQUIRE_FAILED (claim)) +return -EBUSY ;
[Severity: Low]
Will this compile? The IIO_DEV_ACQUIRE_DIRECT_MODE and IIO_DEV_ACQUIRE_FAILED macros do not appear to exist in the IIO subsystem.
Should this use iio_device_claim_direct_mode() oriio_device_claim_direct_scoped() instead?
Quoted 3 lines
drivers/iio/adc/ma35d1_eadc.c new file +3
-0+ +guard (mutex)(&adc->lock); +if (adc->suspended)
[ ... ]
Quoted 6 lines
drivers/iio/adc/ma35d1_eadc.c new file +6
-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;
drivers/iio/adc/ma35d1_eadc.c line 370
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+unsigned int count; + + count =0 ; +for_each_set_bit (bit, scan_mask, indio_dev->masklength) { + scan_chan = &indio_dev->channels[bit]; + count++; + } + +if (count !=1 || scan_chan->type !=IIO_VOLTAGE ) +return -EINVAL ;
[Severity: High]
Does this logic unintentionally reject valid scan masks that include the timestamp channel?
If a userspace application enables one voltage channel and the timestamp channel, the mask will have 2 bits set. The loop will increment count for both, causing count != 1 to trigger and reject the configuration.
Quoted 2 lines
drivers/iio/adc/ma35d1_eadc.c new file +2
-0+ + adc->scan_chan = scan_chan;
[ ... ]
drivers/iio/adc/ma35d1_eadc.c line 415
Quoted 12 lines
drivers/iio/adc/ma35d1_eadc.c new file +12
-0+static irqreturn_t ma35d1_adc_trigger_handler (int irq,void *p) +{ +struct iio_poll_func *pf = p; +struct iio_dev *indio_dev = pf->indio_dev; +struct ma35d1_adc *adc =iio_priv (indio_dev); + IIO_DECLARE_BUFFER_WITH_TS(u16, scan, 1) = { }; +u16 raw; +int ret; + +guard (mutex)(&adc->lock); +if (adc->suspended || !adc->scan_chan) + goto done;
[Severity: Low]
This isn't a runtime bug, but does this violate the cleanup.h guidelines?
The documentation for cleanup helpers states that we should never mix goto-based error handling with guard() declarations in the same function.
Quoted 2 lines
drivers/iio/adc/ma35d1_eadc.c new file +2
-0+ + ret =ma35d1_adc_read_conversion (adc, adc->scan_chan, &raw);
[ ... ]
drivers/iio/adc/ma35d1_eadc.c line 451
Quoted 12 lines
drivers/iio/adc/ma35d1_eadc.c new file +12
-0+static void ma35d1_adc_init_channel (struct iio_chan_spec *chan,u32 vinp, +u32 vinn,unsigned int scan_index, +bool differential) +{ + chan->type =IIO_VOLTAGE ; + chan->indexed =1 ; + chan->channel = vinp; + chan->scan_index = scan_index; + chan->info_mask_separate =BIT (IIO_CHAN_INFO_RAW ) | +BIT (IIO_CHAN_INFO_SCALE ); + chan->scan_type.format = differential ?IIO_SCAN_FORMAT_SIGNED_INT : +IIO_SCAN_FORMAT_UNSIGNED_INT ;
[Severity: Low]
Does struct iio_scan_type have a format field?
It looks like the IIO API uses a char sign field instead, and IIO_SCAN_FORMAT_SIGNED_INT does not exist. This will likely cause a build failure.
Quoted 4 lines
drivers/iio/adc/ma35d1_eadc.c new file +4
-0+ chan->scan_type.realbits =12 ; + chan->scan_type.storagebits =16 ; + chan->scan_type.endianness =IIO_CPU ; +
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
Re: [PATCH v2 2/2] iio: adc: Add Nuvoton MA35D1 EADC driver
In reply to Chi-Wen WengQuoted 14 lines · Chi-Wen Weng
Add an IIO driver for the Nuvoton MA35D1 Enhanced ADC controller.
The driver supports interrupt-driven direct raw reads and triggered buffered capture paced by an external IIO trigger. Buffered capture is limited to a single enabled voltage channel per scan in this initial implementation.
Channels are described by firmware child nodes. Single-ended external channels and the fixed hardware differential input pairs are supported. The driver reports IIO scale from either an optional external reference supply or the internal 1.6 V reference.
The driver also handles the functional clock and optional reset line. DMA support is not used by this initial driver.
...
drivers/iio/adc/ma35d1_eadc.c line 9
Quoted 2 lines
drivers/iio/adc/ma35d1_eadc.c new file +2
-0+#include <linux/bitfield.h> +#include <linux/bitmap.h>
bitmap.h implies bitops.h...
Quoted 1 line
+#include <
linux/bitops.h>
...so you may drop this one.
drivers/iio/adc/ma35d1_eadc.c line 19
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +9
-0+#include <linux/cleanup.h> +#include <linux/clk.h> +#include <linux/completion.h> +#include <linux/delay.h> +#include <linux/err.h> +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/jiffies.h> +#include <linux/mod_devicetable.h>
Uwe made a series to get rid of this header. The platform_device.h seems the one which will provide the thing.
Quoted 9 lines
drivers/iio/adc/ma35d1_eadc.c new file +9
-0+#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/platform_device.h> +#include <linux/pm.h> +#include <linux/property.h> +#include <linux/regmap.h> +#include <linux/regulator/consumer.h> +#include <linux/reset.h> +#include <linux/types.h>
...
drivers/iio/adc/ma35d1_eadc.c line 43
Quoted 8 lines
drivers/iio/adc/ma35d1_eadc.c new file +8
-0+#define MA35D1_EADC_DAT (n) (0x00 + (n) * 0x04) +#define MA35D1_EADC_CTL 0x50 +#define MA35D1_EADC_SWTRG 0x54 +#define MA35D1_EADC_SCTL (n) (0x80 + (n) * 0x04) +#define MA35D1_EADC_INTSRC0 0xd0 +#define MA35D1_EADC_STATUS2 0xf8 +#define MA35D1_EADC_SELSMP0 0x140 +#define MA35D1_EADC_REFADJCTL 0x150
Please, make all register offsets fixed width, exempli gratia
define MA35D1_EADC_STATUS2 0x0f8
...
Quoted 1 line
+#define
MA35D1_EADC_INTSRC0_SPLIEN(n)BIT(n)
Useless? Can't BIT() be used directly? (Note, I don't know if it's good or bad suggestion, wanting to understand a bit more.)
...
drivers/iio/adc/ma35d1_eadc.c line 67
Quoted 2 lines
drivers/iio/adc/ma35d1_eadc.c new file +2
-0+#define MA35D1_EADC_MAX_EXT_CHANNELS 8 +#define MA35D1_EADC_INTERNAL_VREF_MV 1600
_mV (yes, as per SI).
drivers/iio/adc/ma35d1_eadc.c line 70
Quoted 3 lines
drivers/iio/adc/ma35d1_eadc.c new file +3
-0+#define MA35D1_EADC_TIMEOUT msecs_to_jiffies(1000) +#define MA35D1_EADC_RESET_DELAY_US 10 +#define MA35D1_EADC_REF_STABLE_US 1000
(1 * USEC_PER_MSEC) ?
If go this way, include time.h for the multiplier definition.
...
drivers/iio/adc/ma35d1_eadc.c line 81
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+struct ma35d1_adc { +struct regmap *regmap; +struct clk *clk; +struct reset_control *rst; +struct regulator *vref; +struct completion completion; +/* Protects conversion state and register accesses from PM transitions. */ +struct mutex lock; +const struct iio_chan_spec *scan_chan; +unsigned int vref_mv;
_mV
Quoted 3 lines
drivers/iio/adc/ma35d1_eadc.c new file +3
-0+int irq; +bool suspended; +};
...
drivers/iio/adc/ma35d1_eadc.c line 90
Quoted 5 lines
drivers/iio/adc/ma35d1_eadc.c new file +5
-0+static const struct regmap_config ma35d1_adc_regmap_config = { + .reg_bits =32 , + .val_bits =32 , + .reg_stride =4 , + .max_register =MA35D1_EADC_REFADJCTL ,
No cache?
Quoted 1 line
+};
...
drivers/iio/adc/ma35d1_eadc.c line 98
Quoted 6 lines
drivers/iio/adc/ma35d1_eadc.c new file +6
-0+static bool ma35d1_adc_valid_diff_pair (unsigned int vinp,unsigned int vinn) +{ +return (vinp ==0 && vinn ==4 ) || + (vinp ==1 && vinn ==5 ) || + (vinp ==2 && vinn ==6 ) || + (vinp ==3 && vinn ==7 );
Wondering if this is just
return (vinp >= 0 && vinp < 4 && (vinn == vinp + 4));
But I'm fine with the original, perhaps compiler may optimise that.
Quoted 1 line
+}
...
drivers/iio/adc/ma35d1_eadc.c line 111
Quoted 11 lines
drivers/iio/adc/ma35d1_eadc.c new file +11
-0+static int ma35d1_adc_set_bits (struct ma35d1_adc *adc,unsigned int reg, +unsigned int bits) +{ +return regmap_set_bits (adc->regmap, reg, bits); +} + +static int ma35d1_adc_clear_bits (struct ma35d1_adc *adc,unsigned int reg, +unsigned int bits) +{ +return regmap_clear_bits (adc->regmap, reg, bits); +}
What's the point in these wrappers?
...
drivers/iio/adc/ma35d1_eadc.c line 116
Quoted 4 lines
drivers/iio/adc/ma35d1_eadc.c new file +4
-0+static int ma35d1_adc_disable_irq (struct ma35d1_adc *adc) +{ +return ma35d1_adc_clear_bits (adc,MA35D1_EADC_CTL , +MA35D1_EADC_CTL_ADCIEN0 );
I would make it one line (*yes, 84 characters).
Quoted 1 line
+}
...
Quoted 11 lines
drivers/iio/adc/ma35d1_eadc.c new file +11
-0+static int ma35d1_adc_setup_reference (struct ma35d1_adc *adc) +{ +int ret; + +if (adc->vref) { + ret =regmap_set_bits (adc->regmap,MA35D1_EADC_REFADJCTL , +MA35D1_EADC_REFADJCTL_PDREF ); +if (ret) +return ret; + }else { + ret = regmap_clear_bits(adc->regmap, MA35D1_EADC_REFADJCTL,
drivers/iio/adc/ma35d1_eadc.c line 139
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+ MA35D1_EADC_REFADJCTL_PDREF); +if (ret) +return ret; + + ret =regmap_update_bits (adc->regmap,MA35D1_EADC_CTL , +MA35D1_EADC_CTL_VREFSEL_MASK , +MA35D1_EADC_CTL_VREFSEL_1V6 ); +if (ret) +return ret; + }
Add a short comment with reference to a datasheet table/section/et cetera.
Quoted 4 lines
drivers/iio/adc/ma35d1_eadc.c new file +4
-0+fsleep (MA35D1_EADC_REF_STABLE_US ); + +return 0 ; +}
...
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+static int ma35d1_adc_hw_init (struct ma35d1_adc *adc) +{ +int ret; + + ret =ma35d1_adc_disable_irq (adc); +if (ret) +return ret; + + ret =ma35d1_adc_setup_reference (adc); +if (ret)
drivers/iio/adc/ma35d1_eadc.c line 165
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+return ret; + + ret =regmap_update_bits (adc->regmap,MA35D1_EADC_SELSMP0 , +MA35D1_EADC_SELSMP0_SMPT0_MASK , +MA35D1_EADC_SELSMP_LONG ); +if (ret) +return ret; + + ret =regmap_write (adc->regmap,MA35D1_EADC_STATUS2 , +MA35D1_EADC_STATUS2_ADIF0 );
With
static regmap *map = adc->regmap;
this becomes
ret = regmap_write(map, MA35D1_EADC_STATUS2, MA35D1_EADC_STATUS2_ADIF0);
exactly one line. Apply this trick everywhere and it might help reduce amount of LoC.
Quoted 5 lines
drivers/iio/adc/ma35d1_eadc.c new file +5
-0+if (ret) +return ret; + +return ma35d1_adc_set_bits (adc,MA35D1_EADC_CTL ,MA35D1_EADC_CTL_ADCEN ); +}
...
Quoted 4 lines
drivers/iio/adc/ma35d1_eadc.c new file +4
-0+static int ma35d1_adc_config_channel (struct ma35d1_adc *adc, +const struct iio_chan_spec *chan) +{ +unsigned int ctl;
drivers/iio/adc/ma35d1_eadc.c line 222
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+int ret; + + ctl = chan->differential ?MA35D1_EADC_CTL_DIFFEN | +MA35D1_EADC_CTL_DMOF :0 ; + + ret =regmap_update_bits (adc->regmap,MA35D1_EADC_CTL , +MA35D1_EADC_CTL_DIFFEN | +MA35D1_EADC_CTL_DMOF , ctl); +if (ret) +return ret;
regmap_assign_bits()
Quoted 12 lines
drivers/iio/adc/ma35d1_eadc.c new file +12
-0+ ret =regmap_update_bits (adc->regmap,MA35D1_EADC_SCTL (0 ), +MA35D1_EADC_SCTL_CHSEL_MASK | +MA35D1_EADC_SCTL_TRGSEL_MASK , +FIELD_PREP (MA35D1_EADC_SCTL_CHSEL_MASK , + chan->channel)); +if (ret) +return ret; + +return regmap_update_bits (adc->regmap,MA35D1_EADC_INTSRC0 , +MA35D1_EADC_INTSRC0_SPLIEN (0 ), +MA35D1_EADC_INTSRC0_SPLIEN (0 )); +}
...
Quoted 6 lines
drivers/iio/adc/ma35d1_eadc.c new file +6
-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;
drivers/iio/adc/ma35d1_eadc.c line 370
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+unsigned int count; + + count =0 ; +for_each_set_bit (bit, scan_mask, indio_dev->masklength) { + scan_chan = &indio_dev->channels[bit]; + count++; + } + +if (count !=1 || scan_chan->type !=IIO_VOLTAGE ) +return -EINVAL ;
This is interesting check. First of all, it checks if the only a single bit is set in the whole mask, then it checks the type of the *last* listed channel. The latter seems fragile to me, as it depends on ordering (yes, the limitation to a single enabled channel helps).
With that being said, the both checks can be unrolled to the find_next_bit().
/* Find the first enabled channel to scan */ bit = find_next_bit(scan_mask, indio_dev->masklength, 0); if (bit >= indio_dev->masklength) return -EINVAL; /* Check that the type of the channel is what we are looking for */ scan_chan = &indio_dev->channels[bit]; if (scan_chan->type != IIO_VOLTAGE) return -EINVAL; /* Check that this is the only enabled channel */ bit = find_next_bit(scan_mask, indio_dev->masklength, bit); if (bit < indio_dev->masklength) return -EINVAL;
Not sure that this will generate less code, though. So up to you. One also might argue that the original code is easier to understand.
Quoted 4 lines
drivers/iio/adc/ma35d1_eadc.c new file +4
-0+ adc->scan_chan = scan_chan; + +return 0 ; +}
...
drivers/iio/adc/ma35d1_eadc.c line 415
Quoted 12 lines
drivers/iio/adc/ma35d1_eadc.c new file +12
-0+static irqreturn_t ma35d1_adc_trigger_handler (int irq,void *p) +{ +struct iio_poll_func *pf = p; +struct iio_dev *indio_dev = pf->indio_dev; +struct ma35d1_adc *adc =iio_priv (indio_dev); + IIO_DECLARE_BUFFER_WITH_TS(u16, scan, 1) = { }; +u16 raw; +int ret; + +guard (mutex)(&adc->lock); +if (adc->suspended || !adc->scan_chan) + goto done;
This is usually leads to a mess in the compiler. This is written in the top of cleanup.h that goto should be avoided while using guard()() or similar macros.
Quoted 12 lines
drivers/iio/adc/ma35d1_eadc.c new file +12
-0+ ret =ma35d1_adc_read_conversion (adc, adc->scan_chan, &raw); +if (ret) + goto done; + + scan[0 ] = raw &MA35D1_EADC_DAT_RESULT_12BIT ; +iio_push_to_buffers_with_timestamp (indio_dev, scan, pf->timestamp); + +done: +iio_trigger_notify_done (indio_dev->trig); + +return IRQ_HANDLED ; +}
...
drivers/iio/adc/ma35d1_eadc.c line 449
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+static void ma35d1_adc_init_channel (struct iio_chan_spec *chan,u32 vinp, +u32 vinn,unsigned int scan_index, +bool differential) +{ + chan->type =IIO_VOLTAGE ; + chan->indexed =1 ; + chan->channel = vinp; + chan->scan_index = scan_index; + chan->info_mask_separate =BIT (IIO_CHAN_INFO_RAW ) | +BIT (IIO_CHAN_INFO_SCALE );
Broken indentation, better to have
chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE);
OR
chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE);
Quoted 11 lines
drivers/iio/adc/ma35d1_eadc.c new file +11
-0+ 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; + } +}
...
drivers/iio/adc/ma35d1_eadc.c line 468
Quoted 7 lines
drivers/iio/adc/ma35d1_eadc.c new file +7
-0+static struct iio_chan_spec +ma35d1_adc_timestamp_channel (unsigned int scan_index) +{ +struct iio_chan_spec chan =IIO_CHAN_SOFT_TIMESTAMP (scan_index); + +return chan; +}
Useless wrapper. Just put _SOFT_TIMESTAMP() in place.
...
Quoted 5 lines
drivers/iio/adc/ma35d1_eadc.c new file +5
-0+static int ma35d1_adc_parse_channels (struct iio_dev *indio_dev, +struct device *dev) +{ +DECLARE_BITMAP (used_channels,MA35D1_EADC_MAX_EXT_CHANNELS ); +struct iio_chan_spec *channels;
drivers/iio/adc/ma35d1_eadc.c line 484
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+unsigned int scan_index; +int num_channels; +int ret; + +bitmap_zero (used_channels,MA35D1_EADC_MAX_EXT_CHANNELS ); + + num_channels =device_get_child_node_count (dev); +if (!num_channels) +return dev_err_probe (dev, -ENODATA , +"no ADC channels configured\n" );
I would return -ENOENT, as it's usual error code for 0 count in counting APIs in the kernel. Also you can put it on a single line.
drivers/iio/adc/ma35d1_eadc.c line 489
Quoted 4 lines
drivers/iio/adc/ma35d1_eadc.c new file +4
-0+if (num_channels >MA35D1_EADC_MAX_EXT_CHANNELS ) +return dev_err_probe (dev, -EINVAL ,"too many ADC channels\n" ); + + channels =devm_kcalloc (dev, num_channels +1 ,sizeof (*channels),GFP_KERNEL );
size_add() ?
Quoted 15 lines
drivers/iio/adc/ma35d1_eadc.c new file +15
-0+if (!channels) +return -ENOMEM ; + + scan_index =0 ; +device_for_each_child_node_scoped (dev, child) { +u32 diff[2 ]; +u32 reg; +u32 vinn; +bool differential; + + ret =fwnode_property_read_u32 (child,"reg" , ®); +if (ret) +return dev_err_probe (dev, ret, +"missing channel reg property\n" ); +
drivers/iio/adc/ma35d1_eadc.c line 514
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +9
-0+if (reg >=MA35D1_EADC_MAX_EXT_CHANNELS ) +return dev_err_probe (dev, -EINVAL , +"invalid ADC channel %u\n" , reg); + +if (test_and_set_bit (reg, used_channels)) +return dev_err_probe (dev, -EINVAL , +"duplicate ADC channel %u\n" , reg); + differential = false; + vinn =0 ;
Make it an 'else' branch.
Quoted 36 lines
drivers/iio/adc/ma35d1_eadc.c new file +36
-0+if (fwnode_property_present (child,"diff-channels" )) { + ret =fwnode_property_read_u32_array (child,"diff-channels" , diff, +ARRAY_SIZE (diff)); +if (ret) +return dev_err_probe (dev, ret, +"invalid diff-channels for channel %u\n" , + reg); + +if (diff[0 ] != reg || + !ma35d1_adc_valid_diff_pair (diff[0 ], diff[1 ])) +return dev_err_probe (dev, -EINVAL , +"invalid differential ADC channel %u-%u\n" , + diff[0 ], diff[1 ]); + +if (test_and_set_bit (diff[1 ], used_channels)) +return dev_err_probe (dev, -EINVAL , +"ADC channel %u already used\n" , + diff[1 ]); + + vinn = diff[1 ]; + differential = true; + } + +ma35d1_adc_init_channel (&channels[scan_index], reg, vinn, + scan_index, differential); + scan_index++; + } + + channels[scan_index] =ma35d1_adc_timestamp_channel (scan_index); + + indio_dev->channels = channels; + indio_dev->num_channels = scan_index +1 ; + indio_dev->masklength = indio_dev->num_channels; + +return 0 ; +}
...
Quoted 5 lines
drivers/iio/adc/ma35d1_eadc.c new file +4
-0+static int ma35d1_adc_init_vref (struct ma35d1_adc *adc,struct device *dev) +{ +int ret; + adc->vref =devm_regulator_get_optional (dev,"vref" );
drivers/iio/adc/ma35d1_eadc.c line 566
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +10
-0+if (IS_ERR (adc->vref)) { +if (PTR_ERR (adc->vref) != -ENODEV ) +return dev_err_probe (dev,PTR_ERR (adc->vref), +"failed to get VREF supply\n" ); + + adc->vref =NULL ; + adc->vref_mv =MA35D1_EADC_INTERNAL_VREF_MV ; + +return 0 ; + }
It can be unnested:
if (PTR_ERR(adc->vref) == -ENODEV) {
adc->vref = NULL;
adc->vref_mV = MA35D1_EADC_INTERNAL_VREF_mV;
return 0;
}
if (IS_ERR(adc->vref))
return dev_err_probe(dev, PTR_ERR(adc->vref),
"failed to get VREF supply\n");Quoted 4 lines
drivers/iio/adc/ma35d1_eadc.c new file +4
-0+ ret =regulator_enable (adc->vref); +if (ret) +return dev_err_probe (dev, ret,"failed to enable VREF supply\n" ); +
drivers/iio/adc/ma35d1_eadc.c line 581
Quoted 10 lines
drivers/iio/adc/ma35d1_eadc.c new file +9
-0+ ret =devm_add_action_or_reset (dev, ma35d1_adc_vref_disable, adc->vref); +if (ret) +return ret; + + ret =regulator_get_voltage (adc->vref); +if (ret <=0 ) +return dev_err_probe (dev, ret ?: -EINVAL , +"failed to get VREF voltage\n" ); + adc->vref_mv = ret /1000 ;
(MICRO / MILLI)
instead of plain 1000.
Quoted 2 lines
(quoted diff) +2
-0+ return 0; +}
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v2 2/2] iio: adc: Add Nuvoton MA35D1 EADC driver
Hi Andy,
Thank you for the detailed review.
Quoted 5 lines
bitmap.himpliesbitops.h...Quoted 1 line
+#include <
linux/bitops.h>...so you may drop this one.
Will do. I will drop linux/bitops.h. I will also drop linux/mod_devicetable.h since platform_device.h is enough here.
Quoted 3 lines
Please, make all register offsets fixed width, exempli gratia
define MA35D1_EADC_STATUS2
0x0f8
Will fix all register offsets to use fixed-width hex values.
Quoted 3 lines
Quoted 1 line
+#define MA35D1_EADC_INTSRC0_SPLIEN(n) BIT(n)
Useless? Can't
BIT()be used directly?
The field selects which sample module raises ADINT0. This initial driver only uses sample module 0, so I will replace this helper with a fixed named bit for SPLIEN0.
Quoted 3 lines
Quoted 1 line
+#define MA35D1_EADC_INTERNAL_VREF_MV 1600
_mV(yes, as per SI).
Will rename the macro and field to use _mV.
Quoted 4 lines
Quoted 1 line
+#define MA35D1_EADC_REF_STABLE_US 1000
(1 *
USEC_PER_MSEC) ?
If go this way, includetime.hfor the multiplier definition.
Will change this to use USEC_PER_MSEC and include linux/time.h.
Quoted 7 lines
Quoted 5 lines
+static const struct
regmap_configma35d1_adc_regmap_config= { + .reg_bits = 32,
+ .val_bits = 32,
+ .reg_stride = 4,
+ .max_register =MA35D1_EADC_REFADJCTL,No cache?
No register cache is intended for this MMIO ADC block. I will make that explicit by setting .cache_type = REGCACHE_NONE.
Quoted 5 lines
Quoted 1 line
+static bool
ma35d1_adc_valid_diff_pair(unsigned int vinp, unsigned int vinn)Wondering if this is just
return (vinp >= 0 && vinp < 4 && (vinn == vinp + 4));
Yes, the valid differential pairs are 0-4, 1-5, 2-6 and 3-7. Since vinp is unsigned, I will simplify this to:
return vinp < 4 && vinn == vinp + 4;
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+static int ma35d1_adc_set_bits(...) +static int ma35d1_adc_clear_bits(...)What's the point in these wrappers?
There is no strong reason to keep them. I will remove the wrappers and use regmap_set_bits() / regmap_clear_bits() directly.
Quoted 3 lines
Quoted 1 line
+static int
ma35d1_adc_setup_reference(struct ma35d1_adc *adc)Add a short comment with reference to a datasheet table/section/et cetera.
Will add a short comment describing the REFADJCTL/PDREF and VREFSEL settings according to the MA35D1 EADC reference control description.
Quoted 5 lines
With
static regmap *map =
adc->regmap;this becomes ...
Will use a local regmap pointer in functions that access several registers. That should make the code shorter and easier to read.
Quoted 5 lines
Quoted 3 lines
+ ret = regmap_update_bits(adc->regmap,
MA35D1_EADC_CTL, + MA35D1_EADC_CTL_DIFFEN |
+ MA35D1_EADC_CTL_DMOF, ctl);
regmap_assign_bits()
Will use regmap_assign_bits() for the differential mode bits.
Quoted 3 lines
Quoted 1 line
+static int
ma35d1_adc_update_scan_mode(...)This is interesting check...
I will rework this to explicitly accept exactly one IIO_VOLTAGE scan channel. This avoids depending on the last bit found in the scan mask.
Quoted 5 lines
Quoted 3 lines
+ guard(mutex)(&adc->lock);
+ if (adc->suspended|| !adc->scan_chan)
+ goto done;This is usually leads to a mess in the compiler...
Will fix this by removing the goto from the trigger handler and using a scoped_guard() block instead.
Quoted 4 lines
Quoted 2 lines
+
chan->info_mask_separate=BIT(IIO_CHAN_INFO_RAW)| + BIT(IIO_CHAN_INFO_SCALE);Broken indentation...
Will fix the indentation.
Quoted 4 lines
drivers/iio/adc/ma35d1_eadc.c line 463
Quoted 2 lines
drivers/iio/adc/ma35d1_eadc.c new file +2
-0+static struct iio_chan_spec +ma35d1_adc_timestamp_channel (unsigned int scan_index)Useless wrapper. Just put
_SOFT_TIMESTAMP()in place.
Will remove the helper and assign the timestamp channel directly using a compound literal.
Quoted 5 lines
Quoted 3 lines
+ if (!num_channels)
+ returndev_err_probe(dev, -ENODATA, + "no ADC channels configured\n");I would return
-ENOENT...
Will change this to -ENOENT.
Quoted 3 lines
Quoted 1 line
+ channels =
devm_kcalloc(dev, num_channels + 1, sizeof(*channels), GFP_KERNEL);
size_add()?
Will use size_add(num_channels, 1).
Quoted 4 lines
Quoted 2 lines
+ differential = false;
+ vinn = 0;Make it an 'else' branch.
Will do.
Quoted 3 lines
Quoted 1 line
+
adc->vref=devm_regulator_get_optional(dev, "vref");It can be unnested:
Will unnest the optional regulator handling in v3.
Quoted 5 lines
Quoted 1 line
+
adc->vref_mv= ret / 1000;(MICRO / MILLI)
instead of plain 1000.
Will fix this as well. The regulator API returns the voltage in microvolts, while the driver stores the reference in millivolts for IIO scale reporting. I will rename the field to vref_mV and use:
adc->vref_mV = ret / (MICRO / MILLI);
with linux/units.h included, instead of using the plain 1000 constant.
Thanks again for the review. I will address these comments in the next revision.
Best regards,
Chi-Wen
Re: [PATCH v2 2/2] iio: adc: Add Nuvoton MA35D1 EADC driver
In reply to Chi-Wen WengHello,
drivers/iio/adc/ma35d1_eadc.c line 22
Quoted 3 lines · Chi-Wen Weng
drivers/iio/adc/ma35d1_eadc.c new file +2
-0+#include <linux/mod_devicetable.h> +#include <linux/platform_device.h>
Please rely on <linux/platform_device.h> to provide of_device_id and drop the include for <linux/mod_devicetable.h>.
Best regards
Uwe
Re: [PATCH v2 2/2] iio: adc: Add Nuvoton MA35D1 EADC driver
Hi Uwe,
Thank you for the review.
Quoted 2 lines
Please rely on <
linux/platform_device.h> to provideof_device_idand drop the include for <linux/mod_devicetable.h>.
Will do. I will drop the explicit linux/mod_devicetable.h include in the next revision and rely on linux/platform_device.h instead.
Best regards,
Chi-Wen