Re: [PATCH RFC v4 10/10] docs: iio: add documentation for ad9910 driver

[David Lechner <dlechner@baylibre.com>]

On 5/11/26 10:02 AM, Rodrigo Alencar wrote:
> On 26/05/11 09:46AM, David Lechner wrote:
>> On 5/10/26 4:30 AM, Rodrigo Alencar wrote:
>>> On 26/05/09 06:42PM, David Lechner wrote:
>>>> On 5/8/26 12:00 PM, Rodrigo Alencar via B4 Relay wrote:
>>>>> From: Rodrigo Alencar <rodrigo.alencar@analog.com>
>>>>>
>>>>> Add documentation for the AD9910 DDS IIO driver, which describes channels,
>>>>> DDS modes, attributes and ABI usage examples.
>>>
>>> ...
>>>
>>>>> +       must be a power of 2.
>>>>> +
>>>>> +   * - ``frequency_offset``
>>>>> +     - Hz
>>>>> +     - Base FTW to which scaled parallel data is added. Range :math:`[0, f_{SYSCLK}/2)`.
>>>>> +
>>>>> +   * - ``phase_offset``
>>>>> +     - rad
>>>>> +     - Base phase for polar modulation. Lower 8 bits of POW register.
>>>>> +       Range :math:`[0, 2\pi/256)`.
>>>>> +
>>>>> +   * - ``scale_offset``
>>>>> +     - fractional
>>>>> +     - Base amplitude for polar modulation. Lower 6 bits of ASF register.
>>>>> +       Range :math:`[0, 1/256)`.
>>>>> +
>>>>
>>>> I guess there was some discussion on these attributes. I see some of these in the
>>>> ad9832 driver in staging, but I'm guessing they are new ABI. It isn't clear to
>>>> me from the documentation here what they actually do though. I guess they are
>>>> just basic transformations on the input signal?
>>>
>>> Not sure how the ABI is not clear:
>>>
>>> 	For a channel that allows amplitude control through buffers, this
>>> 	represents the value for a base amplitude scale. The actual output
>>> 	amplitude scale is a result with the sum of this value.
>>>
>>> So yes, it is a basic transformation.
>>
>> I didn't have time to read the ABI docs yet. For scale_offset though,
>> how is that different from the existing offset attribute?
> 
> I suppose that existing offset ABI is applied to (raw * scale), mostly for
> voltage channels, here the scale_offset is an offset to the scale itself.


Ah, so a very general case would be (raw * (scale + scale_offset)) + offset

when the scale can change as a function of time and comes from an external
source.

>  
>>>
>>>>
>>>> And a practical note, they should be "frequencyscale". I don't like that it is
>>>> harder to read, but it is easier for a machine to parse.
>>>
>>> Parsers like the ones in libiio is not having problems with that.
>>>
>>>>> +Usage examples
>>>>> +^^^^^^^^^^^^^^
>>>>> +
>>>>> +Set parallel port frequency modulation with a scale of 16 and a 50 MHz
>>>>> +offset:
>>>>> +
>>>>> +.. code-block:: bash
>>>>> +
>>>>> +  echo 16 > /sys/bus/iio/devices/iio:device0/out_altvoltage113_frequency_scale
>>>>> +  echo 50000000 > /sys/bus/iio/devices/iio:device0/out_altvoltage113_frequency_offset
>>>>> +
>>>>> +Digital ramp generator (DRG)
>>>>> +----------------------------
>>>>> +
>>>>> +The DRG produces linear frequency, phase or amplitude sweeps using dedicated
>>>>> +hardware. It is controlled through three channels: a parent control channel
>>>>> +(``digital_ramp_generator``) and two child ramp channels
>>>>> +(``digital_ramp_up``, ``digital_ramp_down``). DRG destination is set when
>>>>> +ramp attributes are written, i.e. writing to ``frequency`` or ``frequency_roc``
>>>>> +sets the destination to frequency.
>>>>
>>>> Would it be better to say that the destination is set when the the
>>>> value is non-zero? Otherwise, how would one change the destination
>>>> once set?
>>>
>>> Destination is only one, so you just need to write phase or phase_roc, if you want
>>> to target phase then. Does that not sound intuitive?
>>
>> I was thinking about if you needed to change the configuration.
>> If you set it to phase, then want to change it to frequency, how
>> could you do that if 0 is a valid value for phase?
>>
>> Also how could you know which is selected by reading back the
>> values if 0 is a valid value?
> 
> This is where Jonathan raised some concerns, so it is a good oportunity for you
> to provide your inputs! Right now, I am returning -EBUSY on read of an attribute
> where its destination is not selected. As pointed out, the destination selection
> is happening when writting to the attribute. In the previous patch, Jonathan
> suggested frequency_active, phase_active and scale_active to track mode priority,
> and It could be leveraged here for DRG destination selection. I havent gone for
> that because I was not willing to add that to all the channels given that it is
> mostly used for debugging, so I added frequency_source, phase_source and
> amplitude_source to debugfs instead.

The "last write wins" with the others changing to EBUSY makes more sense to
me now. If the docs said that, I missed it. Otherwise, that would be a helpful
thing to add to the docs here.

> 
> Destination selection for RAM mode is firmware based at this point.

Seems reasonable.

> Destination selection for Parallel mode is still not clear... could use
> those *_active attributes or separate channels.

Since there are _offset attributes proposed for parallel input already,
could we just make it the same where you have to write one of those
attributes?

> 
>>>
>>> Zero is a valid value to be written.
>>>
>>>>
>