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

[Rodrigo Alencar]

On 26/02/21 02:16PM, David Lechner wrote:
> 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.

Most applications for this part are in fact for frequency generation, like:
- Agile local oscillator (LO) frequency synthesis
- Programmable clock generators
- FM chirp source for radar and scanning systems
- Fast frequency hopping

The device has been made to be too flexible, so that its operation modes
have sub-operation modes that allows to handle frequency, scale and phase.
But at a specific timestamp, the output signal will always be a CW, i.e.
a sine or cosine wave.
 
> > 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.

That is part of the reason for this RFC. Dividing those top-level modes
into channels allows for better organization, as they can operate together,
i.e., phase or scale can be provided by single-tone profile, while
frequency is controlled by the digital ramp generator (see Mode Priority
section in the datasheet). Also, it allows to explore the most of standard
ABIs like, scale, frequency, phase, sampling_frequency and enable.
Putting everything into a single channel would make things a lot messy
to interface with.

> 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?

Yes, the exact amount of bytes needs to be sent when writing specific 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.

I would have to have 3 configs, one of them for a single 16-bit register.
Also, regmap_spi does not seem to support 64-bit registers (maybe I am wrong).
Additionally, single tone modes and RAM control modes are profile based and
they share the same registers, so I suppose that having to control the
register cache manually would be beneficial to switch RAM mode ON/OFF.
I understand that the digital design of the chip is not one of the best,
and a lot of unneeded complications are pushed to be handled in software.

> > 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  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 12: sampling_frequency value: 100000000.000000
> >     altvoltage2:  (output)
> >     27 channel-specific attributes found:
> >         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 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
> >     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  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  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
> > 1 debug attributes found:
> >         debug attr  0: direct_reg_access value: 0x2
> 
> 
> This is a lot of custom attributes!

yes, specially for the digital ramp generator, where we have range
sets of increment, decrement, min and max for each DDS parameter:
scale, phase, frequency.

> 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. 

Not directly, there is often a conversion whenever we are dealing
with scale, phase, frequency or sampling frequency.

> 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.

Not sure, if that makes things simpler, specially for the application
the would interface with this. I think having the attributes as is
would be the whole point of using the IIO subsystem.

-- 
Kind regards,

Rodrigo Alencar