iio: dac: Add AD5413 support #2786
Conversation
The AD5413 is a single-channel, 14-bit DAC with programmable current or voltage output. It communicates via SPI at clock rates up to 50 MHz. The output can be configured as either voltage or current and is available on a single terminal. Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad5413.pdf Signed-off-by: Bruce Tsao <[email protected]>
BruceTsaoADI
requested review from
nunojsa,
mhennerich,
dbogdan,
btogorean,
chegbeli,
buha,
amiclaus,
rbolboac,
danmois,
CiprianRegus and
machschmitt
as code owners
| { | ||
| unsigned int tmp, tmparray[2], size; | ||
| const struct ad5413_range *range; | ||
| int *index, ret; |
There was a problem hiding this comment.
drivers/iio/dac/ad5413.c: In function 'ad5413_parse_dt':
::warning file=drivers/iio/dac/ad5413.c,line=590,col=14::gcc_fanalayzer: unused variable 'index' [-Wunused-variable]
590 | int *index, ret;
| ^~~~~
There was a problem hiding this comment.
I’ve removed the unused index variable from the ad5413_parse_dt() signature so that only int ret remains. This eliminates the -Wunused-variable warning. Please let me know if you spot any other unused declarations.
| else | ||
| indio_dev->channels = ad5413_current_ch; | ||
|
|
||
| ret = ad5413_init(st); | ||
| if (ret < 0) { | ||
| dev_err(&spi->dev, "AD5413 init failed\n"); | ||
| return ret; | ||
| } | ||
|
|
||
| return devm_iio_device_register(&st->spi->dev, indio_dev); |
There was a problem hiding this comment.
drivers/iio/dac/ad5413.c: In function 'ad5413_probe':
::warning file=drivers/iio/dac/ad5413.c,line=724,col=5::gcc_fanalayzer: this 'else' clause does not guard... [-Wmisleading-indentation]
724 | else
| ^~~~
drivers/iio/dac/ad5413.c:727:9: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'else'
727 | ret = ad5413_init(st);
| ^~~
There was a problem hiding this comment.
Thanks for spotting that. I’ve refactored the if/else block to use braces so the indentation matches the actual control flow. This removes the misleading indentation and eliminates the warning. Let me know if you’d like any further adjustments.
| enum: | ||
| - [-10500000, 10500000] | ||
| - [-12600000, 12600000] |
There was a problem hiding this comment.
enum doesn't work like that. See other bindings that already have this same property for what works.
| enum: | |
| - [-10500000, 10500000] | |
| - [-12600000, 12600000] | |
| adi,range-microvolt: | |
| description: Voltage output range specified as <minimum, maximum> | |
| oneOf: | |
| - items: | |
| - const: -10500000 | |
| - const: 10500000 | |
| - items: | |
| - const: -12600000 | |
| - const: 12600000 |
There was a problem hiding this comment.
I’ve updated the adi,range-microvolt binding to use oneOf instead of enum.
|
|
||
| adi,range-microamp: | ||
| description: Current output range <min, max> in microamps | ||
| enum: |
There was a problem hiding this comment.
Same here.
Will also want to call out in the commit message why there is only one option here (and why adi,range-microvolt doesn't have a default: - the same reason - but it isn't obvious until later, so best to mention it before)
There was a problem hiding this comment.
I’ve updated adi,range-microamp to use oneOf with the single supported span (0–24 000 μA).
| description: | | ||
| Output digital slew control time in microseconds |
There was a problem hiding this comment.
| description: | | |
| Output digital slew control time in microseconds | |
| description: Output digital slew control time in microseconds |
There was a problem hiding this comment.
I’ve changed the adi,slew-time-us property to use an inline description instead of a multi-line block.
| allOf: | ||
| - $ref: /schemas/spi/spi-peripheral-props.yaml# | ||
| - if: | ||
| required: [adi, range-microamp] |
There was a problem hiding this comment.
| required: [adi, range-microamp] | |
| required: [ 'adi,range-microamp' ] |
There was a problem hiding this comment.
I’ve updated the required array to quote the property name as ['adi,range-microamp'].
| properties: | ||
| adi,range-microvolt: false | ||
| - if: | ||
| required: [adi, range-microvolt] |
There was a problem hiding this comment.
| required: [adi, range-microvolt] | |
| required: [ 'adi,range-microvolt' ] |
| /* 3) read slew-time（optional） */ | ||
| ret = device_property_read_u32(&st->spi->dev, | ||
| "adi,slew-time-us", &tmp); | ||
| if (ret) { |
There was a problem hiding this comment.
Checking all of the device_property_ returns in this function could be more strict. There are different errors for "property is missing" (-EINAL) and other error caused, e.g. by typos in the .dts file (e.g. a not enough items in the array).
There was a problem hiding this comment.
Thanks for the catch. I’ve updated the adi,slew-time-us handling so that:
-ENODATA and -EINVAL are treated as “property not provided” and default to 0
Any other negative return is treated as a real parsing error (logged via dev_err) and aborts probe
Please let me know if this covers all cases or if further tweaks are needed.
|
|
||
| #define AD5413_DAC_CHAN(_chan_type) { \ | ||
| .type = (_chan_type), \ | ||
| .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_RAW) | \ |
There was a problem hiding this comment.
IIO_CHAN_INFO_RAW is for sure separate, not shared_by_type. Since there aren't versions of this chip with more than one channel, it isn't clear if SCALE and OFFSET should also be separate, so could go either way on those.
There was a problem hiding this comment.
Thank you for pointing that out. I’ve removed IIO_CHAN_INFO_RAW from info_mask_shared_by_type and added it to info_mask_separate to ensure RAW is treated as a separate attribute. SCALE and OFFSET remain in info_mask_shared_by_type since this device only has one channel. Please let me know if you’d prefer SCALE and OFFSET to be marked separate as well.
| int *index, ret; | ||
|
|
||
| /* 1) voltage */ | ||
| ret = device_property_read_u32_array(&st->spi->dev, |
There was a problem hiding this comment.
This will allow the +/- 12.6 range to be given in the devicetree but set up the chip for +/-10.5V, but OVRNG_EN isn't implemented yet. So we should not allow that or now or implement OVRNG_EN.
It also wouldn't hurt to check the current values as well (even if there is only one option, someone might try to put something else in the .dts file)
There was a problem hiding this comment.
Thank you for catching that. I’ve made the following updates:
Voltage range: The DT binding and driver now only allow ±10.5 V. Any attempt to use ±12.6 V is rejected with -EINVAL until we implement the OVRNG_EN bit.
Current range: Added explicit validation so that adi,range-microamp must be exactly 0–24 000 µA; anything else also returns -EINVAL.
With these checks in place, we prevent the chip from being configured for unsupported ranges. OVRNG_EN support can be added in a follow-up patch once fully tested.
|
|
||
| struct ad5413_range { | ||
| int reg; | ||
| int min; |
There was a problem hiding this comment.
Upstream prefers units on macros and variables, e.g. min_uv here.
Same applies throughout the driver. (It would especially help to make the raw_read function easier to understand)
There was a problem hiding this comment.
Thank you for the suggestion. I’ve renamed all range struct members and related macros to include explicit unit suffixes (e.g. min_uv/max_uv for microvolt ranges), which should improve readability and prevent confusion. Please let me know if any other identifiers need unit annotations.
Summary
This PR adds initial support for the Analog Devices AD5413, a 14-bit single-channel DAC capable of voltage and current output.
Key changes
ad5413.cunderdrivers/iio/dac/adi,ad5413.yamlunderDocumentation/devicetree/bindings/iio/dac/Datasheet:
https://www.analog.com/media/en/technical-documentation/data-sheets/ad5413.pdf
PR Description
necessary to understand them. List any dependencies required for this change.
any space), or simply check them after publishing the PR.
description and try to push all related PRs simultaneously.
PR Type
PR Checklist