Re: [PATCH v6 5/5] iio: dac: Add AD5529R DAC driver support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Heap buffer overflow during device tree channel parsing
- [High] Incorrect Regmap endianness configuration corrupts SPI communication
- [Medium] Incorrect regulator presence detection when CONFIG_REGULATOR=n
- [Medium] Missing timing delay between hardware reset assert and deassert
- [Medium] Potential data misalignment for 12-bit device variant
--

--- Patch [5]: [PATCH v6 5/5] iio: dac: Add AD5529R DAC driver support ---
commit e8333f8884287977f99dcd3fe867d9426fc20f71
Author: Janani Sunil <janani.sunil@analog.com>

iio: dac: Add AD5529R DAC driver support

Add support for AD5529R 16-channel, 12/16 bit Digital to Analog Converter
from Analog Devices.

> diff --git a/drivers/iio/dac/ad5529r.c b/drivers/iio/dac/ad5529r.c
> new file mode 100644
> index 0000000000000..c279dc530d688
> --- /dev/null
> +++ b/drivers/iio/dac/ad5529r.c
> @@ -0,0 +1,502 @@

[ ... ]

> +static const struct regmap_config ad5529r_regmap_16bit_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]
Is this Little Endian configuration correct? Analog Devices SPI peripherals 
typically use Big Endian (MSB first) for 16-bit payloads. Could this 
byte-swap and corrupt the SPI data on transmission?

> +	.rd_table = &ad5529r_16bit_readable_table,
> +	.wr_table = &ad5529r_16bit_writeable_table,
> +	.reg_stride = 2,
> +};

[ ... ]

> +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;
> +
> +		ret = reset_control_deassert(rst);

[Severity: Medium]
Does the hardware require a minimum reset pulse width? Calling 
reset_control_assert() and reset_control_deassert() sequentially without a 
delay usually generates a sub-microsecond pulse that might be too short 
for the device to properly initialize.

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

[ ... ]

> +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:
> +		/*
> +		 * Read from DAC_INPUT_A register rather than DAC_DATA_READBACK.
> +		 * The DAC operates in transparent mode and directly reflects
> +		 * whatever value is written to the INPUT_A register.
> +		 */
> +		reg_addr = AD5529R_REG_DAC_INPUT_A(chan->channel);
> +		ret = regmap_read(st->regmap_16bit, reg_addr, &reg_val_h);
> +		if (ret)
> +			return ret;
> +
> +		*val = reg_val_h;

[Severity: Medium]
Does the 12-bit variant of this chip expect left-aligned data in the 16-bit 
register? If so, reading the unshifted register value could return incorrect 
scale data for the 12-bit device.

> +
> +		return IIO_VAL_INT;

[ ... ]

> +static int ad5529r_write_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;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (val < 0 || val > GENMASK(st->model_data->resolution - 1, 0))
> +			return -EINVAL;
> +
> +		reg_addr = AD5529R_REG_DAC_INPUT_A(chan->channel);
> +
> +		return regmap_write(st->regmap_16bit, reg_addr, val);

[Severity: Medium]
Similar to the read path, if the 12-bit payload needs to be left-aligned, 
writing it directly here without shifting could cause the hardware to 
interpret it as a much smaller value. Should this value be shifted based on 
the device resolution?

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

[ ... ]

> +static int ad5529r_parse_channel_ranges(struct device *dev,
> +					struct ad5529r_state *st)
> +{
> +	s32 vals[2];
> +	int ret, range_idx;
> +	u32 ch;
> +
> +	device_for_each_child_node_scoped(dev, child) {
> +		ret = fwnode_property_read_u32(child, "reg", &ch);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Missing reg property in channel node\n");
> +
> +		if (ch >= 16)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "Channel %u exceeds maximum 15\n",
> +					     ch);

[ ... ]

> +		st->output_range_idx[ch] = range_idx;
> +		ret = regmap_write(st->regmap_16bit,
> +				   AD5529R_REG_OUT_RANGE(ch), range_idx);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to configure range for ch %u\n",
> +					     ch);
> +
> +		st->channels[st->num_channels++] = AD5529R_DAC_CHANNEL(ch);

[Severity: High]
Could this overflow the channels array? While there is a check for ch >= 16 
above, there doesn't seem to be a check on the total number of child nodes. 
If a malformed device tree provides more than 16 child nodes (for instance, 
by duplicating reg properties), st->num_channels could exceed 15 and write 
out of bounds.

> +	}
> +
> +	return 0;
> +}

[ ... ]

> +static int ad5529r_probe(struct spi_device *spi)
> +{

[ ... ]

> +	ret = devm_regulator_get_enable_optional(dev, "vref");
> +	if (ret == -ENODEV)
> +		external_vref = false;
> +	else if (!ret)
> +		external_vref = true;

[Severity: Medium]
Can this result in incorrect regulator configuration if CONFIG_REGULATOR is 
disabled? The dummy implementation of devm_regulator_get_enable_optional() 
returns 0 in that case, which would cause this to set external_vref = true 
and disable the internal reference even if no external reference exists on 
the board.

> +	else
> +		return dev_err_probe(dev, ret,
> +				     "Failed to get and enable vref regulator\n");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715-ad5529r-driver-v6-0-cfdf8b9f5ee3@analog.com?part=5