Re: [PATCH v5 13/13] docs: iio: add documentation for ad9910 driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Abuse of IIO channels to create a configuration hierarchy and property namespace.
- [High] Implicit multiplexer switching and '-EBUSY' reads on inactive DRG attributes breaks state save/restore tools.
- [Medium] Overloading the `scale_roc` numerical attribute with a magic sentinel value to enable pin-controlled mode.
--

commit f680c44805df9c1351d6484cf4f4d1ff94df8c8c
Author: Rodrigo Alencar <rodrigo.alencar@analog.com>

docs: iio: add documentation for ad9910 driver
    
Add documentation for the AD9910 DDS IIO driver, which describes channels,
DDS modes, attributes and ABI usage examples.

> diff --git a/Documentation/iio/ad9910.rst b/Documentation/iio/ad9910.rst
> new file mode 100644
> index 0000000000000..dbcf8f8a1dda4
> --- /dev/null
> +++ b/Documentation/iio/ad9910.rst
> @@ -0,0 +1,666 @@
[ ... ]
> +Channel hierarchy
> +=================
> +
> +The driver exposes the following IIO output channels, each identified by a
> +unique channel number and a human-readable label. The ``phy`` channel is the
> +root of the hierarchy. Changing its ``sampling_frequency`` reconfigures the
> +system clock (SYSCLK) which affects all other channels. Most of the
> +mode-specific channels have an ``enable`` attribute that turns the mode on/off.
> +
> +.. flat-table::
> +   :header-rows: 1
> +
> +   * - Channel
> +     - Label
> +     - Parent
> +     - Description
> +
> +   * - ``out_altvoltage100``
> +     - ``phy``
> +     -
> +     - Physical output: system clock and profile control.
> +       See `Physical channel`_.
> +
> +   * - ``out_altvoltage110`` ... ``out_altvoltage117``
> +     - ``profile0`` ... ``profile7``
> +     - ``phy``
> +     - Single tone control: frequency, phase, amplitude.

Are these multiple IIO channels actually representing distinct physical
signal paths (e.g., individual DAC outputs)?

In the IIO subsystem, channels should typically represent actual hardware
inputs or outputs rather than software configuration namespaces or internal
modes. Since the AD9910 has a single physical output, exposing these
mode-specific groups (like phy, profile0-7, parallel_port, etc.) as
separate out_altvoltage channels might severely confuse generic userspace
clients like libiio, which could incorrectly detect 15+ distinct voltage
outputs.

Could this configuration hierarchy be mapped to IIO device attributes or a
single channel's extended attributes instead?

[ ... ]
> +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``).
> +
> +The DRG can target only one destination at a time (frequency, phase or
> +amplitude). Destination selection follows a "last write wins" policy: writing
> +any value (including zero) to a destination-specific attribute (e.g.
> +``frequency``, ``frequency_roc``, ``phase``, ``phase_roc``, ``scale`` or
> +``scale_roc``) switches the DRG destination accordingly. Reading an attribute
> +whose destination is not currently active returns ``-EBUSY``.

Could returning -EBUSY on read operations for inactive attributes prevent
generic userspace diagnostic and state-tracking tools (like iio_info or
sysfsutils) from successfully querying the device's full state?

Additionally, the implicit "last write wins" behavior might cause state
restoration tools to inadvertently scramble the active DRG destination,
since they typically write sysfs attributes in an arbitrary or alphabetical
order.

Would it be better to use an explicit multiplexer attribute (e.g.,
drg_destination) and allow inactive attributes to remain readable?

[ ... ]
> +Output Shift Keying (OSK)
> +-------------------------
> +
> +OSK controls the output amplitude envelope, allowing the output to be ramped
> +on/off rather than switched abruptly.
> +
> +.. flat-table::
> +   :header-rows: 1
> +
> +   * - Attribute
> +     - Unit
> +     - Description
> +
> +   * - ``en``
> +     - boolean (0 or 1)
> +     - Enable/disable OSK.
> +
> +   * - ``scale``
> +     - fractional
> +     - Target amplitude for the OSK ramp. 14-bit ASF field. Range: :math:`[0, 1)`.
> +
> +   * - ``scale_roc``
> +     - 1/s
> +     - Amplitude scale rate of change. Writing a non-zero value enables
> +       automatic OSK and selects the closest hardware step size. Writing ``0``
> +       disables automatic ramping (manual control of the ASF register using
> +       ``scale``). Writing the maximum available value enables pin-controlled
> +       immediate transition with no ramping.

Is overloading this numerical rate-of-change attribute with a magic sentinel
value (the maximum available value) to enable pin-controlled mode the
intended sysfs design?

This approach might make the UAPI brittle and difficult to discover,
especially if the maximum calculated rate changes due to clock
configurations. Could a dedicated boolean or enum attribute be used to
enable the pin-controlled mode instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260517-ad9910-iio-driver-v5-0-31599c88314a@analog.com?part=13