Re: [PATCH v4 11/12] iio: dac: ad5686: add triggered buffer support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The ad5686_trigger_handler fails to convert Big Endian buffer data to native CPU endianness before passing it to the write operations, causing scrambled DAC values on Little Endian systems.
--

commit b9bb35e4f394ef5515930983f7b76ce0cae1335e
Author: Rodrigo Alencar <rodrigo.alencar@analog.com>

iio: dac: ad5686: add triggered buffer support

This commit implements a trigger handler for the AD5686 DAC driver to update 
all channels simultaneously when the LDAC GPIO is available. It also flushes 
multiple channel writes at once using the sync() operation.

> diff --git a/drivers/iio/dac/ad5686.c b/drivers/iio/dac/ad5686.c
> index db175e77b0b7c..4dc681eb077d6 100644
> --- a/drivers/iio/dac/ad5686.c
> +++ b/drivers/iio/dac/ad5686.c
[ ... ]
> @@ -467,6 +472,60 @@ const struct ad5686_chip_info ad5679r_chip_info = {
>  };
>  EXPORT_SYMBOL_NS_GPL(ad5679r_chip_info, "IIO_AD5686");
>  
> +static irqreturn_t ad5686_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct iio_buffer *buffer = indio_dev->buffer;
> +	struct ad5686_state *st = iio_priv(indio_dev);
> +	u16 val[AD5686_MAX_CHANNELS] = { };
> +	unsigned int scan_count, ch, i;
> +	bool async_update;
> +	int ret;
> +	u8 cmd;
> +
> +	ret = iio_pop_from_buffer(buffer, val);
> +	if (ret) {
> +		iio_trigger_notify_done(indio_dev->trig);
> +		return IRQ_HANDLED;
> +	}
> +
> +	guard(mutex)(&st->lock);
> +
> +	scan_count = bitmap_weight(indio_dev->active_scan_mask,
> +				   iio_get_masklength(indio_dev));
> +	async_update = st->ldac_gpio && scan_count > 1;
> +	if (async_update) {
> +		/* use LDAC to update all channels simultaneously */
> +		cmd = AD5686_CMD_WRITE_INPUT_N;
> +		gpiod_set_value_cansleep(st->ldac_gpio, 0);
> +	} else {
> +		cmd = AD5686_CMD_WRITE_INPUT_N_UPDATE_N;
> +	}
> +
> +	i = 0;
> +	iio_for_each_active_channel(indio_dev, ch) {
> +		ret = st->ops->write(st, cmd, indio_dev->channels[ch].address, val[i++]);

[Severity: High]
Will this send scrambled data on Little Endian CPUs?

Since the driver declares IIO_BE, the data populated by iio_pop_from_buffer() 
into the val array is in Big Endian format. Passing val[i++] directly to 
st->ops->write() as a native u16 without using be16_to_cpu() means the bytes 
might be incorrectly byte-swapped before transmission.

> +		if (ret)
> +			break;
> +	}
> +
> +	/*
> +	 * If sync() is available, it is called here regardless of write
> +	 * failure to allow bus implementation to reset. In that case, partial
> +	 * writes are unlikely as the write operations would just queue up
> +	 * the transfers.
> +	 */
> +	if (st->ops->sync)
> +		st->ops->sync(st);
> +
> +	if (async_update)
> +		gpiod_set_value_cansleep(st->ldac_gpio, 1);
> +
> +	iio_trigger_notify_done(indio_dev->trig);
> +	return IRQ_HANDLED;
> +}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623-ad5686-new-features-v4-0-28962a57db0f@analog.com?part=11