Re: [PATCH 2/2] iio: dac: add AD5413 driver

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 17 Oct 2026 03:23:54 +0800
Bruce Tsao via B4 Relay <devnull+bruce.tsao.analog.com@kernel.org> wrote:

> From: Bruce Tsao <bruce.tsao@analog.com>
> 
> The AD5413 is a single-channel, 14-bit, voltage and current output
> digital-to-analog converter (DAC) with programmable output ranges,
> slew rate control and on-chip diagnostics.
> 
> Add an IIO driver supporting the +-10.5 V voltage range and the
> 0-24 mA current range, output enable, slew rate control and device
> reset over the SPI interface.
> 
> The device powers up with the SPI CRC enabled, so every frame carries
> a CRC-8 byte and the interface is configured for SPI mode 1. This lets
> the driver bring the device up directly from a cold boot without any
> pre-conditioning.
> 
> Signed-off-by: Bruce Tsao <bruce.tsao@analog.com>
Hi Bruce

Good state for a v1.  Main thing in my feedback is take
another look at the endian conversions. It think those
have evolved into being way more complex than they need to be.

Jonathan

> diff --git a/drivers/iio/dac/ad5413.c b/drivers/iio/dac/ad5413.c
> new file mode 100644
> index 000000000000..96cce32fc640
> --- /dev/null
> +++ b/drivers/iio/dac/ad5413.c
> @@ -0,0 +1,666 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * AD5413 Digital to analog converters driver
> + *
> + * Copyright 2026 Analog Devices Inc.
> + */
> +#include <linux/bitfield.h>
> +#include <linux/crc8.h>
> +#include <linux/delay.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iopoll.h>
> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/reset.h>
> +#include <linux/spi/spi.h>
> +#include <linux/units.h>
> +#include <linux/util_macros.h>
> +
> +/* AD5413 registers definition */
> +#define AD5413_REG_NOP 0x00
> +#define AD5413_REG_DAC_INPUT 0x01
> +#define AD5413_REG_DAC_OUTPUT 0x02
> +#define AD5413_REG_CLEAR_CODE 0x03
> +#define AD5413_REG_USER_GAIN 0x04
> +#define AD5413_REG_USER_OFFSET 0x05
> +#define AD5413_REG_DAC_CONFIG 0x06
> +#define AD5413_REG_DAC_CONFIG_SR_STEP_MSK GENMASK(15, 13)
Mixing fields and register defines can make for hard reading. I sugest
using a bit of indenting to make them visually different.
#define AD5413_REG_DAC_CONFIG 0x06
#define   AD5413_REG_DAC_CONFIG_SR_STEP_MSK GENMASK(15, 13)
#define   AD5413_REG_DAC_CONFIG_SR_CLOCK_MSK GENMASK(12, 9)
#define   AD5413_REG_DAC_CONFIG_SR_EN_MSK BIT(8)
#define   AD5413_REG_DAC_CONFIG_RSET_EXT_EN_MSK BIT(7)
#define   AD5413_REG_DAC_CONFIG_OUT_EN_MSK BIT(6)
#define   AD5413_REG_DAC_CONFIG_INT_EN_MSK BIT(5)
#define   AD5413_REG_DAC_CONFIG_OVRNG_EN_MSK BIT(4)
#define   AD5413_REG_DAC_CONFIG_RANGE_MSK GENMASK(3, 0)
define AD5413_REG_SW_LDAC 0x07

etc. If you have values of fields defined, then indent them
even further.

> +#define AD5413_REG_DAC_CONFIG_SR_CLOCK_MSK GENMASK(12, 9)
> +#define AD5413_REG_DAC_CONFIG_SR_EN_MSK BIT(8)
> +#define AD5413_REG_DAC_CONFIG_RSET_EXT_EN_MSK BIT(7)
> +#define AD5413_REG_DAC_CONFIG_OUT_EN_MSK BIT(6)
> +#define AD5413_REG_DAC_CONFIG_INT_EN_MSK BIT(5)
> +#define AD5413_REG_DAC_CONFIG_OVRNG_EN_MSK BIT(4)
> +#define AD5413_REG_DAC_CONFIG_RANGE_MSK GENMASK(3, 0)
> +#define AD5413_REG_SW_LDAC 0x07
> +#define AD5413_REG_KEY 0x08
> +#define AD5413_REG_KEY_REG_ADDR_MASK GENMASK(20, 16)
> +#define AD5413_REG_KEY_CODE_MASK GENMASK(15, 0)
> +#define AD5413_REG_GP_CONFIG1 0x09
> +#define AD5413_REG_GP_CONFIG2 0x0A
> +#define AD5413_REG_DIGITAL_DIAG_CONFIG 0x10
> +#define AD5413_REG_FAULT_PIN_CONFIG 0x12
> +#define AD5413_REG_TWO_STAGE_READBACK_SELECT 0x13
> +#define AD5413_REG_DIGITAL_DIAG_RESULTS 0x14
> +#define AD5413_REG_DIGITAL_DIAG_CAL_MEM_UNREFRESHED_MSK BIT(15)
> +#define AD5413_REG_DIGITAL_DIAG_SLEW_BUSY_MSK BIT(14)
> +#define AD5413_REG_ANALOG_DIAG_RESULTS 0x15
> +#define AD5413_REG_STATUS 0x16
> +#define AD5413_REG_CHIP_ID 0x17
> +#define AD5413_REG_FREQ_MONITOR 0x18
> +#define AD5413_REG_DEVICE_ID_3 0x1C
> +
> +/* Special Key Codes (bits[15:0]) */
> +#define AD5413_REG_KEY_CODE_RESET_1 0x15FA
> +#define AD5413_REG_KEY_CODE_RESET_2 0xAF51
> +#define AD5413_REG_KEY_CODE_CALIB_MEM_REFRESH 0xFCBA
> +
> +/* DIGITAL_DIAG_RESULTS Register (Addr = 0x14) Busy Flags */
> +
> +#define AD5413_WR_FLAG_MSK(x) (0x80u | ((x) & 0x1F))
> +
> +/* CRC-8 polynomial x^8 + x^2 + x + 1, enabled by default on the device */
> +#define AD5413_CRC8_POLY 0x07
> +
> +#define AD5413_FULL_SCALE_MICRO (16383ULL * MICRO)
> +
> +struct ad5413_range {
> +	int reg;
> +	int min;
> +	int max;
> +};
> +
> +struct ad5413_state {
> +	struct spi_device *spi;
> +	struct regulator_bulk_data supplies[3];
> +	struct mutex lock; /* Protects all register access via SPI */

Not necessary to protect data accesses in general as spi will do that.
The point you want to make is about rmw sequences.

> +	const struct ad5413_range *out_range;
> +	unsigned int slew_time;
> +	bool pwr_down;
> +	u8 crc8_tbl[CRC8_TABLE_SIZE];
> +	__be32 d32[3] __aligned(IIO_DMA_MINALIGN);

As below. I'd throw in a d8 array to simplify the data mangling.

> +};
> +
> +/*
> + * Output ranges corresponding to DAC_CONFIG[3:0] bits.
> + * Defined by the following symbolic identifiers:
> + *   AD5413_RANGE_PLUSMINUS_10_5V - ±10.5 V voltage range
> + *   AD5413_RANGE_0MA_24MA        - 0 mA to 24 mA current range
> + */
> +enum ad5413_output_range {
> +	AD5413_RANGE_PLUSMINUS_10_5V = 0x3,
> +	AD5413_RANGE_0MA_24MA = 0x9,
> +};
> +
> +static const struct ad5413_range ad5413_voltage_range[] = { {

Given only one element, why the array?

> +	.reg = AD5413_RANGE_PLUSMINUS_10_5V,
> +	.min = -10500000,
> +	.max = 10500000,
> +} };
> +
> +static const struct ad5413_range ad5413_current_range[] = { {

Same on this one.

> +	.reg = AD5413_RANGE_0MA_24MA,
> +	.min = 0,
> +	.max = 24000,
> +} };

> +static int ad5413_spi_reg_read(struct ad5413_state *st, unsigned int addr,
> +			       u16 *val)
> +{
> +	struct spi_transfer t[] = {
> +		{
> +			.tx_buf = &st->d32[0],
> +			.len = 4,
> +			.cs_change = 1,
> +		},
> +		{
> +			.tx_buf = &st->d32[1],
> +			.rx_buf = &st->d32[2],
> +			.len = 4,
> +		},
> +	};
> +	u8 sel[3], nop[3];
> +	int ret;
> +
> +	/* Two-stage readback: select the register to read back. */
> +	sel[0] = AD5413_WR_FLAG_MSK(AD5413_REG_TWO_STAGE_READBACK_SELECT);
> +	sel[1] = 0;
> +	sel[2] = addr;
> +	st->d32[0] = cpu_to_be32((sel[0] << 24) | (sel[1] << 16) |
> +				 (sel[2] << 8) |
> +				 crc8(st->crc8_tbl, sel, sizeof(sel), 0));

See below. I suspect similarly overly complicated endian dance.

> +
> +	/* NOP frame clocks the selected register contents out on SDO. */
> +	nop[0] = AD5413_WR_FLAG_MSK(AD5413_REG_NOP);
> +	nop[1] = 0;
> +	nop[2] = 0;
> +	st->d32[1] = cpu_to_be32((nop[0] << 24) | (nop[1] << 16) |
> +				 (nop[2] << 8) |
> +				 crc8(st->crc8_tbl, nop, sizeof(nop), 0));
> +
> +	ret = spi_sync_transfer(st->spi, t, ARRAY_SIZE(t));
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * The readback CRC covers the live FAULT pin status bit, which can
> +	 * toggle between capture and clock-out, so it is not validated here.
> +	 */
> +	*val = be32_to_cpu(st->d32[2]) >> 8;
> +
> +	return 0;
> +}
> +
> +static int ad5413_spi_reg_write(struct ad5413_state *st, unsigned int addr,
> +				unsigned int val)
> +{
> +	u8 buf[3] = { };
	u8 buf[3] = {
		[0] = AD...

> +
> +	buf[0] = AD5413_WR_FLAG_MSK(addr);
> +	buf[1] = val >> 8;
> +	buf[2] = val;

This is fun as that's a put_unaligned_be16 which you then do
another endian swap on.

> +
> +	st->d32[0] = cpu_to_be32((buf[0] << 24) | (buf[1] << 16) |
> +				 (buf[2] << 8) |

Ok. I'm lost.

In both cass we start with
	buf[0] = AD5413_WR_FLAG_MSK(addr),
	buf[1] = (val >> 8) & 0xff;
	buf[2] = val & 0xff;

Then you build a native endian integer. So on a little endian cpu that means
	
	v[0] = crc8(buf..)
	v[1] = val & 0xff;
	v[2] = (val >> 8) & 0xff;
	v[3] = AD5413_WR_FLAG_MSK(addr)

	d32 is that swapped (pretend it's u8 array)
	d32[0] = AD5413_WR_FLAG_MSK(addr);
	d32[1] = (val >> 8) & 0xff;
	d32[2] =  val & 0xff;
	d32[3] = crc8(buf..)

on a big endian
	v[0] = AD5413_WR_FLAG_MSK(addr)
	v[1] = (val >> 8) & 0xff;
	v[2] = val & 0xff;
	v[3] = crc8(buf..)
d32 is not swapped. (again pretend u8 array
	d32[0] = AD5413_WR_FLAG_MSK(addr);
	d32[1] = (val >> 8) & 0xff;
	d32[2] =  val & 0xff;
	d32[3] = crc8(buf..)
(snap which is good).

Fun bit, is the 3 bytes are right for computing the crc8 so... This is all
equivalent of:
static int ad5413_spi_reg_write(struct ad5413_state *st, unsigned int addr,
				unsigned int val)
{
	st->d8[0] = AD5413_WR_FLAG_MSK(addr);
	put_unaligned_be16(&st->d8[1], val);
	st->d8[3] = crc8(st->crc8_tbl, buf, 3, 0);

	return spi_write(st->spi, st->d8, sizeof(st->d8));
}
As long as you have a st->d8[4] array to use (and I didn't unwind
that all incorrectly).

> +				 crc8(st->crc8_tbl, buf, sizeof(buf), 0));
> +
> +	return spi_write(st->spi, &st->d32[0], sizeof(st->d32[0]));
> +}
> +
> +static int ad5413_spi_write_mask(struct ad5413_state *st, unsigned int addr,
> +				 unsigned long mask, unsigned int val)
> +{
> +	u16 regval;
> +	int ret;
> +
> +	ret = ad5413_spi_reg_read(st, addr, &regval);
> +	if (ret)
> +		return ret;
> +
> +	return ad5413_spi_reg_write(st, addr, (regval & ~mask) | val);
> +}


> +
> +static int ad5413_slew_rate_config(struct ad5413_state *st)
> +{
> +	unsigned int sr_clk_idx = 0, sr_step_idx = 0;
> +	int i, res;
> +	s64 diff_new, diff_old = S64_MAX;
> +	u64 sr_step, calc_slew_time;
> +
> +	/*
> +	 * The slew time can be determined by using the formula:
> +	 * Slew Time = (Full Scale Out / (Step Size x Update Clk Freq))
> +	 * where Slew time is expressed in microseconds
> +	 * Given the desired slew time, the following algorithm determines the
> +	 * best match for the step size and the update clock frequency.
> +	 */
> +	for (i = 0; i < ARRAY_SIZE(ad5413_sr_clk); i++) {

	for (int i = 0;

is acceptable these days and makes the scope explicit.

> +		/*
> +		 * Go through each valid update clock freq and determine a raw
> +		 * value for the step size by using the formula:
> +		 * Step Size = Full Scale Out / (Update Clk Freq * Slew Time)
> +		 */
> +		sr_step = div64_u64(AD5413_FULL_SCALE_MICRO,
> +				    (u64)ad5413_sr_clk[i] * st->slew_time);
> +		/*
> +		 * After a raw value for step size was determined, find the
> +		 * closest valid match
> +		 */
> +		res = find_closest(sr_step, ad5413_sr_step, ARRAY_SIZE(ad5413_sr_step));
> +
> +		/* Calculate the slew time */
> +		calc_slew_time = div_u64(AD5413_FULL_SCALE_MICRO,
> +					 ad5413_sr_step[res] * ad5413_sr_clk[i]);
> +		/*
> +		 * Determine with how many microseconds the calculated slew time
> +		 * is different from the desired slew time and store the diff
> +		 * for the next iteration
> +		 */
> +		diff_new = abs(st->slew_time - calc_slew_time);
> +		if (diff_new < diff_old) {
> +			diff_old = diff_new;
> +			sr_clk_idx = i;
> +			sr_step_idx = res;
> +		}
> +	}
> +
> +	return ad5413_slew_rate_set(st, sr_clk_idx, sr_step_idx);
> +}

> +
> +static int ad5413_internal_buffers_en(struct ad5413_state *st, bool enable)
> +{
> +	int ret;
> +
> +	ret = ad5413_spi_write_mask(st,
> +				    AD5413_REG_DAC_CONFIG,
> +				    AD5413_REG_DAC_CONFIG_INT_EN_MSK,
> +				    FIELD_PREP(AD5413_REG_DAC_CONFIG_INT_EN_MSK,
> +					       enable));
> +	if (ret < 0)
> +		return ret;
I'd not expect a function with that name to return > 0 so perhaps
just if (ret)?

> +
> +	/* Wait to allow time for the internal calibrations to complete */

The function name and parameters are probably clear enough that you can
drop the comment.

> +	return ad5413_wait_for_task_complete(st,
> +					     AD5413_REG_DIGITAL_DIAG_RESULTS,
> +					     AD5413_REG_DIGITAL_DIAG_CAL_MEM_UNREFRESHED_MSK);
> +}

> +static int ad5413_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan, int *val,
> +			   int *val2, long info)
> +{
> +	struct ad5413_state *st = iio_priv(indio_dev);
> +	int max, min, ret;
> +	u16 regval;
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_RAW: {
> +		guard(mutex)(&st->lock);
> +		ret = ad5413_spi_reg_read(st, AD5413_REG_DAC_INPUT, &regval);
> +		if (ret)
> +			return ret;
> +
> +		/* 14-bit DAC data is in bits [15:2] of the register. */
> +		*val = regval >> 2;
> +		return IIO_VAL_INT;
> +	}
> +	case IIO_CHAN_INFO_SCALE:
> +		min = st->out_range->min;
> +		max = st->out_range->max;
> +		*val = (max - min) / 1000;
> +		*val2 = 14;
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	case IIO_CHAN_INFO_OFFSET:
> +		min = st->out_range->min;
> +		max = st->out_range->max;
> +		*val = div_s64((s64)min * BIT(14), max - min);
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad5413_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan, int val, int val2,
> +			    long info)
> +{
> +	struct ad5413_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_RAW: {
> +		guard(mutex)(&st->lock);
> +		/* 14-bit DAC data is in bits [15:2] of the register. */
> +		ret = ad5413_spi_reg_write(st, AD5413_REG_DAC_INPUT, val << 2);
> +		return ret;

		return ad54...

> +	}
> +	default:
> +		return -EINVAL;
> +	}
> +}

> +static ssize_t ad5413_write_powerdown(struct iio_dev *indio_dev, uintptr_t priv,
> +				      struct iio_chan_spec const *chan,
> +				      const char *buf, size_t len)
> +{
> +	struct ad5413_state *st = iio_priv(indio_dev);
> +	bool pwr_down;
> +	unsigned int dac_config_mode, val;
> +	unsigned long dac_config_msk;

Where nothing else dictates ordering, reverse xmas tree preferred.

> +	int ret;
> +
> +	ret = kstrtobool(buf, &pwr_down);
> +	if (ret)
> +		return ret;
> +
> +	val = pwr_down ? 0 : 1;
> +
> +	dac_config_mode = FIELD_PREP(AD5413_REG_DAC_CONFIG_OUT_EN_MSK, (val)) |
> +			  FIELD_PREP(AD5413_REG_DAC_CONFIG_INT_EN_MSK, (val));
> +	dac_config_msk = AD5413_REG_DAC_CONFIG_OUT_EN_MSK |
> +			 AD5413_REG_DAC_CONFIG_INT_EN_MSK;
> +
> +	guard(mutex)(&st->lock);
> +	ret = ad5413_spi_write_mask(st, AD5413_REG_DAC_CONFIG, dac_config_msk,
> +				    dac_config_mode);
> +	if (ret < 0)
> +		return ret;
> +
> +	st->pwr_down = pwr_down;
> +
> +	return len;
> +}

> +static const struct iio_chan_spec ad5413_voltage_ch[] = {
> +	AD5413_DAC_CHAN(IIO_VOLTAGE),
> +};
> +
> +static const struct iio_chan_spec ad5413_current_ch[] = {
> +	AD5413_DAC_CHAN(IIO_CURRENT),

Why an array?  For these, given you are just copying them in place
maybe use a macro that has (struct iio_chan_spec) { values }
so that you can just have the assignment in code without the
need for these two template arrays.

> +};
> +
> +static int ad5413_parse_fw(struct ad5413_state *st)
> +{
> +	struct device *dev = &st->spi->dev;
> +	int ret;
> +
> +	if (device_property_read_bool(dev, "adi,chan-current-mode"))
> +		st->out_range = &ad5413_current_range[0];

I ask this above, but it was motivated by this. If there is only
one structure why do we want an array for these?

> +	else
> +		st->out_range = &ad5413_voltage_range[0];
> +
> +	st->slew_time = 0;
> +	if (device_property_present(dev, "adi,slew-time-us")) {
> +		ret = device_property_read_u32(dev,
> +					       "adi,slew-time-us",
> +					       &st->slew_time);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "Failed to read adi,slew-time-us\n");
> +
> +		if (st->slew_time > 1 * MICRO)
> +			return dev_err_probe(dev, -EINVAL, "adi,slew-time-us out of range\n");
> +	}
> +
> +	return 0;
> +}
> +
> +static int ad5413_init(struct ad5413_state *st)
> +{
> +	struct reset_control *reset;
> +	u16 regval;
> +	int ret;
> +
> +	reset = devm_reset_control_get_optional_exclusive(&st->spi->dev, NULL);
> +	if (IS_ERR(reset))
> +		return PTR_ERR(reset);
> +
> +	if (reset) {
> +		/* Hard reset via the reset controller */
> +		ret = reset_control_assert(reset);
> +		if (ret)
> +			return ret;
> +
> +		fsleep(100);

For most sleeps a comment that refers to the datasheet is useful
so if people hit problems in future they understand why the sleeps take
particular values. If the datasheet is uninformative on a particular
value just say something about having found it to work in practice.

> +
> +		ret = reset_control_deassert(reset);
> +		if (ret)
> +			return ret;
> +
> +		fsleep(100);
> +	} else {
> +		/*
> +		 * CRC is enabled by default after power-on/reset. The SPI
> +		 * helpers always append the CRC byte, so the device accepts
> +		 * these soft-reset key frames without any bootstrap step.
> +		 */
> +		ret = ad5413_spi_reg_write(st, AD5413_REG_KEY,
> +					   AD5413_REG_KEY_CODE_RESET_1);
> +		if (ret)
> +			return ret;
> +
> +		ret = ad5413_spi_reg_write(st, AD5413_REG_KEY,
> +					   AD5413_REG_KEY_CODE_RESET_2);
> +		if (ret)
> +			return ret;
> +
> +		fsleep(100);
> +	}
> +
> +	ret = ad5413_calib_mem_refresh(st);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = ad5413_spi_reg_read(st, AD5413_REG_DIGITAL_DIAG_RESULTS, &regval);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad5413_spi_reg_write(st, AD5413_REG_DIGITAL_DIAG_RESULTS, regval);

Whilst it doesn't matter in practice, I'd rather this only wrote the write 1 to clear
bits rather than also writing the read only ones up the top. Acts as documentation
about what this is doing.

> +	if (ret < 0)
> +		return ret;
> +
> +	ret = ad5413_set_out_range(st, st->out_range->reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (st->slew_time) {
> +		ret = ad5413_slew_rate_config(st);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	ret = ad5413_internal_buffers_en(st, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	return ad5413_spi_write_mask(st,
> +				    AD5413_REG_DAC_CONFIG,
> +				    AD5413_REG_DAC_CONFIG_OUT_EN_MSK,
> +				    FIELD_PREP(AD5413_REG_DAC_CONFIG_OUT_EN_MSK, 1));
> +}
}