Re: [PATCH RFC 0/8] AD9910 Direct Digital Synthesizer

[David Lechner]

On 2/20/26 10:46 AM, Rodrigo Alencar via B4 Relay wrote:
> This patch series adds support for the Analog Devices AD9910 DDS.
> This is an RFC so that we can agree/discuss on the design that follows:
> 
> The AD9910 DDS core can be driven through several independent mechanisms:
> single tone profiles, a digital ramp generator, an internal RAM playback
> engine, a parallel data port, and output shift keying. Each of these

This makes is sound more like a DAC than a frequency generator. altvoltage
specifically means an AC voltage (sine wave), so these arbitrary outputs
don't fit that.

> represents a distinct signal path into the DDS accumulator, so the driver
> models them as separate IIO output channels (all IIO_ALTVOLTAGE type).

Generally IIO channels represent the physical input/output, not the
internal channels.

Ideally we would just have the one channel here with a mode selection
attribute. Documentation can tell us which modes use which attributes.

> This per-channel separation allows userspace to configure each mode
> independently through its own set of sysfs attributes, and to
> enable/disable modes individually via IIO_CHAN_INFO_ENABLE, relying on
> the hardware's own mode selection architecture.
> 
> The AD9910 register map is not suited for the regmap framework: register
> widths vary across the map (16, 32, and 64 bits). The driver instead

Does it break things if you read/write 64 bits from/to non-64-bit registers?

In other drivers for chips like this, we've just created 2 regmaps, i.e.
one for 16-bit regs and one for 32-bit regs. Seems better than
re-implementing a reg cache.

> implements direct SPI access helpers with a software register cache, using
> type-specific read/write/update functions (ad9910_reg{16,32,64}_{read,
> write,update}) that handle endianness conversion and cache coherency.
> 
> Registers are cached for several reasons. The control/function registers
> (CFR1, CFR2) are frequently queried to determine the current operating
> mode (e.g., checking RAM_ENABLE before every profile register access),
> and caching avoids repeated SPI read transactions for what are
> essentially state checks. The cache also enables efficient
> read-modify-write updates on multi-byte registers: the update functions
> merge new field values with the cached register content without issuing
> a SPI read, and skip the write entirely when the value is unchanged.
> Finally, the profile registers serve dual purposes depending on whether
> RAM mode is active -- they hold single tone parameters (FTW, POW, ASF)
> in normal operation but are repurposed for RAM playback configuration
> (start/end address, step rate, operating mode) when RAM is enabled. A
> shadow register array (reg_profile[]) preserves the inactive mode's
> settings across transitions, so no state is lost when switching between
> single tone and RAM operation.
> 
> RAM data is loaded through a write-only binary sysfs attribute
> (ram_data). Userspace writes the waveform data as a raw binary buffer
> (up to 4096 bytes for the full 1024x32-bit RAM), and the driver
> transfers it to the device in a single SPI transaction. Per-profile
> start/end addresses and playback parameters (operating mode, step rate,
> no-dwell control) are configured through the RAM channel's ext_info
> attributes.
> 
> Streaming data to the DDS core through the parallel data port at the
> PD_CLK rate is not covered by this series. That functionality would
> be added in a separate patch series, building on top of the IIO backend
> infrastructure to provide a proper buffered data path.
> 
> As I am pushing implementation, as lot has been done already without much
> supervision or agreement, still I would be interested on hearing about
> the design choices discussed above. Here is the output for the iio_info
> at this point:
> 
> 5 channels found:
>     altvoltage1:  (output)
>     9 channel-specific attributes found:
>         attr  0: en value: 0
>         attr  1: frequency_offset value: 0.000000
>         attr  2: frequency_scale value: 1
>         attr  3: label value: parallel_port
>         attr  4: phase_offset value: 0.000000
>         attr  5: powerdown value: 0
>         attr  6: profile value: 0
>         attr  7: sampling_frequency value: 100000000.000000
>         attr  8: scale_offset value: 0.000000
>     altvoltage3:  (output)
>     13 channel-specific attributes found:
>         attr  0: address_end value: 1023
>         attr  1: address_start value: 0
>         attr  2: destination value: frequency
>         attr  3: destination_available value:
>           frequency phase amplitude polar
>         attr  4: en value: 0
>         attr  5: frequency value: 0.000000
>         attr  6: label value: ram_control
>         attr  7: operating_mode value: direct_switch
>         attr  8: operating_mode_available value:
>           direct_switch ramp_up bidirectional
>           bidirectional_continuous ramp_up_continuous
>           sequenced sequenced_continuous
>         attr  9: phase value: 0.000000
>         attr 10: powerdown value: 0
>         attr 11: profile value: 0
>         attr 12: sampling_frequency value: 100000000.000000
>     altvoltage2:  (output)
>     27 channel-specific attributes found:
>         attr  0: burst_count value: 0
>         attr  1: burst_delay value: 0.000000030
>         attr  2: control_en value: 0
>         attr  3: decrement_sampling_frequency value: 100000000.000000
>         attr  4: destination value: frequency
>         attr  5: destination_available value: frequency phase amplitude
>         attr  6: en value: 0
>         attr  7: frequency_decrement value: 0.000000
>         attr  8: frequency_increment value: 0.000000
>         attr  9: frequency_max value: 0.000000
>         attr 10: frequency_min value: 0.000000
>         attr 11: increment_sampling_frequency value: 100000000.000000
>         attr 12: label value: digital_ramp_generator
>         attr 13: operating_mode value: bidirectional_continuous
>         attr 14: operating_mode_available value:
> 		  bidirectional ramp_down ramp_up bidirectional_continuous
>         attr 15: phase_decrement value: 0.000000000
>         attr 16: phase_increment value: 0.000000000
>         attr 17: phase_max value: 0.000000000
>         attr 18: phase_min value: 0.000000000
>         attr 19: powerdown value: 0
>         attr 20: profile value: 0
>         attr 21: ramp_delay value: 0.000000020
>         attr 22: scale_decrement value: 0.000000000
>         attr 23: scale_increment value: 0.000000000
>         attr 24: scale_max value: 0.000000000
>         attr 25: scale_min value: 0.000000000
>         attr 26: toggle_en value: 0
>     altvoltage0:  (output)
>     6 channel-specific attributes found:
>         attr  0: frequency value: 0.000000
>         attr  1: label value: single_tone
>         attr  2: phase value: 0.000000
>         attr  3: powerdown value: 0
>         attr  4: profile value: 0
>         attr  5: scale value: 0.000000
>     altvoltage4:  (output)
>     8 channel-specific attributes found:
>         attr  0: en value: 0
>         attr  1: label value: output_shift_keying
>         attr  2: pinctrl_en value: 0
>         attr  3: powerdown value: 0
>         attr  4: profile value: 0
>         attr  5: sampling_frequency value: 100000000.000000
>         attr  6: scale value: 0.000000
>         attr  7: scale_increment value: 0.000000
> 3 device-specific attributes found:
>         attr  0: ram_data ERROR: Permission denied (13)
>         attr  1: sysclk_frequency value: 400000000
>         attr  2: waiting_for_supplier value: 0
> 1 debug attributes found:
>         debug attr  0: direct_reg_access value: 0x2


This is a lot of custom attributes!

It looks like a lot of these are just exposing registers directly, which
usually isn't the best if we want something that can be reused. However,
this looks pretty complex so coming up with something generic is probably
not worth the effort. 

Instead, I would suggest to create a firmware file format that
describes how the chip should be programmed. And in the driver call
firmware_upload_register() to create a sysfs interface where the
firmware can be loaded/replaced at runtime. This way, there is just
one attribute write needed to set all of the parameters at once.

This could probably be as simple as something that just contains
the value of each register to be programmed and the driver can
write all of the registers just before enabling the output.

> 
> Kind regards,
> 
> Rodrigo Alencar
> 
> Signed-off-by: Rodrigo Alencar <rodrigo.alencar@xxxxxxxxxx>
> ---
> Rodrigo Alencar (8):
>       dt-bindings: iio: frequency: add ad9910
>       iio: frequency: ad9910: initial driver implementation
>       iio: frequency: ad9910: add simple parallel port mode support
>       iio: frequency: ad9910: expose sysclk_frequency device attribute
>       iio: frequency: ad9910: add digital ramp generator support
>       iio: frequency: ad9910: add RAM mode support
>       iio: frequency: ad9910: add output shift keying support
>       iio: frequency: ad9910: add channel labels
> 
>  .../bindings/iio/frequency/adi,ad9910.yaml         |  236 +++
>  MAINTAINERS                                        |    8 +
>  drivers/iio/frequency/Kconfig                      |   18 +
>  drivers/iio/frequency/Makefile                     |    1 +
>  drivers/iio/frequency/ad9910.c                     | 2153 ++++++++++++++++++++
>  5 files changed, 2416 insertions(+)
> ---
> base-commit: cce8de7f9744a210a4441ca8a667a9950515eea7
> change-id: 20260218-ad9910-iio-driver-9b3d214c251f
> 
> Best regards,