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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Inconsistent and incorrect endianness configuration in 16-bit regmap.
- [High] Missing bit shift for 12-bit data variant.
--

commit 30ecf6c6be9fe7d7d56ed009cc4fbd27ef838d98
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. The device communicates over SPI and supports
per-channel output range configuration.

> diff --git a/drivers/iio/dac/ad5529r.c b/drivers/iio/dac/ad5529r.c
> new file mode 100644
> index 0000000000000..52601cbf28d5e
> --- /dev/null
> +++ b/drivers/iio/dac/ad5529r.c
[ ... ]
> +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: High]
Does this read of reg_val_h need a bit shift for the 12-bit variant of the
device?

Since 12-bit and 16-bit variants typically share a 16-bit register format,
the 12-bit variant often expects its data to be left-justified. By returning
the 16-bit register value directly without right-shifting, will this read
report a value 16 times higher than the actual scale for 12-bit models?

> +
> +		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: High]
Similarly, will writing the raw 12-bit value directly to the 16-bit register
without left-justification alignment cause the 12-bit variant to output
voltages significantly lower than intended?

> +	default:
> +		return -EINVAL;
> +	}
> +}
[ ... ]
> +static int ad5529r_probe(struct spi_device *spi)
> +{
> +	struct regmap_config regmap_16bit_cfg;
> +	struct regmap_config regmap_8bit_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]
Will this little-endian configuration break the standard SPI MSB-first
protocol for 16-bit register values?

Standard SPI DACs usually expect big-endian ordering. Since the register
address defaults to big-endian, using REGMAP_ENDIAN_LITTLE for the values
could cause mixed endianness in the SPI transaction, which might result in
completely incorrect 16-bit register reads and writes over the bus.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827-ad5529r-driver-v10-0-38f2be07b824@analog.com?part=3