Re: [PATCH v2 3/3] Documentation: iio: Add AD5529R Documentation

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 8 May 2026 13:55:49 +0200
Janani Sunil <janani.sunil@analog.com> wrote:

> Add documentation for AD5529R high voltage, 16-channel 12/16 bit DAC
Whilst it is good to have documentation for devices - I've made some
comments below on not providing documentation of standard things (too much
duplication) and being careful to work out who the document is for.
These tend to be for users and board integrators etc so we don't tend
to have much about the internals of the driver.  For that see driver!

Jonathan

> 
> Signed-off-by: Janani Sunil <janani.sunil@analog.com>
> ---
>  Documentation/iio/ad5529r.rst | 216 ++++++++++++++++++++++++++++++++++++++++++
>  Documentation/iio/index.rst   |   1 +
>  MAINTAINERS                   |   1 +
>  3 files changed, 218 insertions(+)
> 
> diff --git a/Documentation/iio/ad5529r.rst b/Documentation/iio/ad5529r.rst
> new file mode 100644
> index 000000000000..41fea1521790
> --- /dev/null
> +++ b/Documentation/iio/ad5529r.rst
> @@ -0,0 +1,216 @@
> +.. SPDX-License-Identifier: GPL-2.0-only
> +
> +==============
> +AD5529R driver
> +==============
> +
> +Device driver for Analog Devices Inc. AD5529R 16-Channel 12/16-bit High Voltage DAC.
> +The module name is ``ad5529r``.
> +
> +Supported devices
> +=================
> +
> +* `AD5529R <https://www.analog.com/en/products/ad5529r.html>`_
> +
> +Description
> +===========
> +
> +The AD5529R is a 16-channel, 12-bit or 16-bit, high voltage, buffered voltage output

Long line.  Wrap to 80 chars consistently

> +digital-to-analog converter (DAC) with an integrated precision reference.
> +The device operates from unipolar and bipolar supplies and is guaranteed
> +monotonic. It has built-in rail-to-rail output buffers that can source or
> +sink up to 25mA.
> +
> +Hardware Features
> +=================
> +
> +* 16 independent 12-bit or 16-bit DAC channels
> +* Independently programmable output ranges:
> +
> +  - 0V to 5V (current driver default)

With such wide ranges we have in some previous drivers made it a device
tree constraint.  It rarely makes sense to switch between them at runtime
as these are really about what circuit is downstream of the DAC.
That needs to be there from initial driver otherwise we have a
backwards compatibility problem.  A 'default' of smallest range should
be safe though (gets messier for some device where 'smallest' could be
unipolar or bipolar)

> +  - 0V to 10V
> +  - 0V to 20V
> +  - 0V to 40V
> +  - ±5V
> +  - ±10V
> +  - ±15V
> +  - ±20V
> +
> +* 4.096V precision reference (12ppm/°C maximum)
> +* Built-in function generation capabilities (hardware support)

I'd drop the "(hardware support)"  Given you are talking about functions
of the hardware, bit odd if the hardware didn't support them!
 
> +* Output voltage and current monitoring (hardware support)
> +* Temperature monitoring with 8 on-chip sensors (hardware support)
> +* Over-temperature protection
> +* SPI interface with CRC error detection support
> +
> +Current Driver Implementation
> +=============================
> +
> +The current driver provides basic DAC functionality with the following features:
> +
> +* Basic DAC output control for all 16 channels
> +* Scale attributes for voltage conversion (0-5V default range)
> +* SPI communication with regmap support
> +* Reset control framework support
> +* Automatic hardware variant detection (16-bit vs 12-bit) based on product ID
> +* Debugfs register access for development
> +
> +SPI Configuration:
> +
> +* **Mode**: Supports SPI mode 0 and mode 3 (default: mode 0)

In what sense is it the default? I'd drop that as just depends on the DT.

> +* **Frequency**: Up to 50 MHz (typically tested at lower frequencies)
> +* **Word Size**: 16-bit transactions
> +
> +.. note::
> +   The device default configuration uses address decrement mode (ADDR_ASCENSION=0)
> +   for multi-byte SPI transactions. Therefore, all 16-bit register addresses are
> +   incremented by 1 in the driver to access the last byte first, allowing the
> +   hardware to decrement and access the complete multi-byte register correctly.

This doc is a slightly odd mix of stuff for users (most of it) and
driver details like this. I'd move this to a comment in the code and
keep the doc for users.

> +
> +IIO Attributes (Currently Implemented)
> +======================================
> +
> +Basic DAC Control
> +-----------------
> +
> +For each of the 16 channels (0-15):
> +
> +**out_voltageY_raw**
> +  Raw DAC code (12-bit: 0-4095, 16-bit: 0-65535)
> +
> +  * Read: Returns the current DAC register value
> +  * Write: Sets the DAC output code
> +
> +**out_voltageY_scale**
> +  Scale factor for voltage conversion (millivolts per LSB)
> +
> +  Based on the formula: VOUTn = A × D/2^N + B, where A=5V, B=0V, N=resolution
> +
> +  * 16-bit: 0.076294 mV/LSB (5V ÷ 2^16 = 5V ÷ 65536 = 0.076294mV)
> +  * 12-bit: 1.220703 mV/LSB (5V ÷ 2^12 = 5V ÷ 4096 = 1.220703mV)
> +  * Read-only attribute
> +
> +Debug Interface
> +===============
> +
> +**Register Access**
> +
> +The driver provides debugfs register access for debugging and development:
> +
> +``/sys/kernel/debug/iio/iio:deviceX/direct_reg_access``
> +  Direct register read/write access. Format:
> +
> +  * Read: ``echo <register_address> > direct_reg_access; cat direct_reg_access``
> +  * Write: ``echo <register_address> <value> > direct_reg_access``
> +
> +Usage examples
> +==============
> +
> +Basic DAC Output Control
> +------------------------
> +
> +.. code-block:: bash
> +
> +    # Set channel 0 to mid-scale (approximately 2.5V with 0V to 5V range)
> +    echo "32768" > /sys/bus/iio/devices/iio:device0/out_voltage0_raw
> +
> +    # Set channel 15 to full scale
> +    echo "65535" > /sys/bus/iio/devices/iio:device0/out_voltage15_raw
> +
> +    # Read current value from channel 5
> +    cat /sys/bus/iio/devices/iio:device0/out_voltage5_raw
> +
> +Scale Attributes
> +----------------
> +
> +.. code-block:: bash
> +
> +    # Read scale factor (millivolts per LSB)
> +    cat /sys/bus/iio/devices/iio:device0/out_voltage0_scale
> +    # Output: 0.076294 (for 16-bit) or 1.220703 (for 12-bit)
> +
> +    # Convert raw to voltage: voltage_mv = raw * scale
> +    # Formula: VOUTn = A × D/2^N + B where A=5V, B=0V

This stuff is very standard.  Consider if it is worth documenting
for this specific part.  To me it isn't..

> +
> +Register Access for Development
> +-------------------------------

Likewise this section is very standard so I'd not expect per device
docs for it.

> +
> +.. code-block:: bash
> +
> +    # Navigate to debugfs directory
> +    cd /sys/kernel/debug/iio/iio:device0/
> +
> +    # Read device product ID (register 0x04)
> +    echo 4 > direct_reg_access
> +    cat direct_reg_access
> +
> +    # Write to a 16-bit configuration register (example: LDAC_HW_SW register 0x19)
> +    echo "0x019 0xAA11" > direct_reg_access
> +    cat direct_reg_access
> +    # Output: 0xAA11
> +
> +    # Write to DAC channel registers (16-bit values)
> +    echo "0x149 32768" > direct_reg_access   # DAC channel 0 mid-scale
> +    echo "0x14B 65535" > direct_reg_access   # DAC channel 1 full-scale
> +
> +    # Read back DAC register values
> +    echo 0x149 > direct_reg_access && cat direct_reg_access  # Read channel 0
> +    echo 0x14B > direct_reg_access && cat direct_reg_access  # Read channel 1
> +
> +.. note::
> +   For 16-bit registers, use hexadecimal format for addresses (0x019, 0x149, etc.).

Are there non 16-bit registers where we shouldn't use hexadecimal?  Otherwise this
note seems odd.

> +   Values can be decimal (32768) or hexadecimal (0xAA11). Register addresses shown
> +   include the +1 offset required for decrement mode operation.
Ah. This is worth noting as users need to be aware of it so keep this bit.
> +
> +Device Tree Configuration
> +=========================
> +
> +Basic configuration example:
> +
> +.. code-block:: devicetree
> +
> +    &spi0 {
> +        status = "okay";
> +
> +        ad5529r@0 {
> +            compatible = "adi,ad5529r";
> +            reg = <0>;
> +            spi-max-frequency = <25000000>;
> +
> +            vdd-supply = <&vdd_regulator>;
> +            avdd-supply = <&avdd_regulator>;
> +            hvdd-supply = <&hvdd_regulator>;
> +            hvss-supply = <&hvss_regulator>;
> +
> +            reset-gpios = <&gpio0 87 GPIO_ACTIVE_LOW>;
> +        };
> +    };
> +
> +For complete device tree binding documentation, see:
> +``Documentation/devicetree/bindings/iio/dac/adi,ad5529r.yaml``

Hard no to replicating the device tree example - that is just
noise. The cross reference is enough.

> +
> +Driver Architecture
> +===================
> +
> +The driver is structured as follows:
> +
> +* **Core**: Basic SPI communication and device initialization
> +* **IIO Interface**: Standard IIO DAC channel interface with scale attributes
> +* **Dual Regmap**: Uses standard regmap-spi for both 8-bit and 16-bit register access
> +* **Reset Framework**: Reset control support

Not seeing value in anything except the dual regmap and again this
is mixing user stuff with driver internal details.  That stuff probably
belongs in the driver.

> +
> +Development Notes
> +=================
> +
> +* The driver uses standard regmap-spi for both 8-bit and 16-bit register access
> +* SPI mode 0 (CPOL=0, CPHA=0) is typically used

Drop that.  We don't care as long as both the options the dt-binding allows work fine.

> +* Reset control framework support for device initialization
> +* Register addresses are incremented by 1 for 16-bit registers due to decrement mode addressing
> +* Scale attributes provide voltage conversion for 0-5V range
> +* Automatic regmap selection based on register address (≤0x13: 8-bit, >0x13: 16-bit)

All this is driver internal stuff so I'd drop it.

> +
> +References
> +==========
> +
> +* AD5529R Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad5529r.pdf
> +* Linux IIO Subsystem: https://www.kernel.org/doc/html/latest/driver-api/iio/index.html
I'd skip that IIO generic docs reference. Not seeing it as adding much in this file.

> diff --git a/Documentation/iio/index.rst b/Documentation/iio/index.rst
> index 007e0a1fcc5a..27f2ab41f05e 100644
> --- a/Documentation/iio/index.rst
> +++ b/Documentation/iio/index.rst
> @@ -25,6 +25,7 @@ Industrial I/O Kernel Drivers
>     ad4062
>     ad4691
>     ad4695
> +   ad5529r
>     ad7191
>     ad7380
>     ad7606
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 143714e27d51..41f42eb1adf2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1513,6 +1513,7 @@ L:	linux-iio@vger.kernel.org
>  S:	Supported
>  W:	https://ez.analog.com/linux-software-drivers
>  F:	Documentation/devicetree/bindings/iio/dac/adi,ad5529r.yaml
> +F:	Documentation/iio/ad5529r.rst
>  F:	drivers/iio/dac/ad5529r.c
>  
>  ANALOG DEVICES INC AD5706R DRIVER
>