Re: [PATCH v8 3/3] iio: dac: Add AD5529R DAC driver support

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Incorrect endianness configuration for 16-bit regmap value format, causing byte-swapped SPI data transmission.
- [High] The driver attempts an SPI transaction (software reset) immediately after enabling power regulators, failing to account for internal Power-On Reset (POR) delay.
- [Medium] External reference voltage is ignored in IIO scale and offset calculations.
- [Low] The 20 ns delay (`ndelay(20)`) used to hold the hardware reset line active is too short for reliable software-controlled assertion.
--

--- Patch [3]: [PATCH v8 3/3] iio: dac: Add AD5529R DAC driver support ---
commit 2d3e31b4283b225ad0ff6048ec906544c72c44b0
Author: Janani Sunil <[email protected]>

iio: dac: Add AD5529R DAC driver support
    
Add support for AD5529R 16-channel, 12/16 bit Digital to Analog Converter
from Analog Devices. The device communicates over SPI and supports
per-channel output range configuration. An optional external 4.096V
reference can be used in place of the internal reference.

> diff --git a/drivers/iio/dac/ad5529r.c b/drivers/iio/dac/ad5529r.c
> new file mode 100644
> index 0000000000000..1ea0ddfdda994
> --- /dev/null
> +++ b/drivers/iio/dac/ad5529r.c

[ ... ]

> +static int ad5529r_reset(struct ad5529r_state *st)
> +{
> +	struct reset_control *rst;
> +	int ret;
> +
> +	rst = devm_reset_control_get_optional_exclusive(&st->spi->dev, NULL);
> +	if (IS_ERR(rst))
> +		return PTR_ERR(rst);
> +
> +	if (rst) {
> +		ret = reset_control_assert(rst);
> +		if (ret)
> +			return ret;
> +
> +		/* Minimum reset low width (t_reset) is 20 ns per datasheet. */
> +		ndelay(20);

[Severity: Low]
While the datasheet minimum is 20 ns, is this ndelay(20) too short for
reliable software-controlled assertion?

Depending on bus latencies, PCB capacitance, and pull-up resistor values,
driving a physical line via software GPIO for such a short pulse might not
reach the logic threshold to reliably trigger a reset.

> +
> +		ret = reset_control_deassert(rst);
> +		if (ret)
> +			return ret;
> +	} else {
> +		ret = regmap_write(st->regmap_8bit, AD5529R_REG_INTERFACE_CONFIG_A,
> +				   AD5529R_INTERFACE_CONFIG_A_SW_RESET);

[Severity: High]
Will this SPI transaction fail if regulators were just enabled dynamically in
ad5529r_probe()?

The device requires 10 ms for digital initialization after power-up. If we
execute this software reset immediately without a preceding delay, the command
might be ignored or the device may set a NOT_READY_ERR.

> +		if (ret)
> +			return ret;
> +	}

[ ... ]

> +static int ad5529r_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	struct ad5529r_state *st = iio_priv(indio_dev);
> +	unsigned int reg_addr, reg_val_h;
> +	int ret, range_idx, span_mv;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:

[ ... ]

> +	case IIO_CHAN_INFO_SCALE:
> +		range_idx = st->output_range_idx[chan->channel];
> +
> +		span_mv = ad5529r_output_ranges_mV[range_idx][1] -
> +			  ad5529r_output_ranges_mV[range_idx][0];
> +		*val = span_mv;

[Severity: Medium]
Does this scale calculation report incorrect values if a non-default external
reference voltage is supplied?

This calculation strictly uses the hardcoded millivolt spans from
ad5529r_output_ranges_mV. If a user supplies an external reference of a
different voltage, scaling shouldn't be statically tied to this table (which
likely assumes a 4.096V reference).

> +		*val2 = st->model_data->resolution;
> +
> +		return IIO_VAL_FRACTIONAL_LOG2;

[ ... ]

> +static int ad5529r_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct iio_dev *indio_dev;
> +	struct ad5529r_state *st;
> +	struct regmap_config regmap_8bit_cfg;
> +	struct regmap_config regmap_16bit_cfg;

[ ... ]

> +	regmap_16bit_cfg = (struct regmap_config) {
> +		.name = "ad5529r-16bit",
> +		.reg_bits = 16,
> +		.val_bits = 16,
> +		.max_register = AD5529R_MAX_REGISTER,
> +		.read_flag_mask = AD5529R_SPI_READ_FLAG,
> +		.val_format_endian = REGMAP_ENDIAN_LITTLE,

[Severity: High]
Can this cause hardware malfunction due to byte-swapping SPI data transmission?

This configures the regmap to byteswap the 16-bit values into little-endian
format. Since reg_format_endian isn't explicitly defined, it defaults the
register address to REGMAP_ENDIAN_BIG. Mixed endianness across a single SPI
transaction is highly unusual for Analog Devices DACs, which typically expect
MSB-first for the entire frame.

> +		.rd_table = &ad5529r_16bit_readable_table,
> +		.wr_table = &ad5529r_16bit_writeable_table,
> +		.reg_stride = 2,
> +		.reg_base = dev_addr << AD5529R_ADDR_SHIFT,
> +	};

[ ... ]

> +	ret = devm_regulator_get_enable_optional(dev, "vref");

[Severity: Medium]
Should we query the voltage of the external vref regulator here via
regulator_get_voltage()?

The regulator is retrieved and enabled, but its voltage is never read, which
leads to the hardcoded span issue in ad5529r_read_raw() if the voltage differs
from the default reference.

> +	if (ret == -ENODEV)
> +		external_vref = false;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3