Re: [PATCH v2 12/12] iio: dac: ad5686: add gain control support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] ABI Violation: Exposing a writable sysfs attribute to fix a static hardware configuration.
- [Medium] State Inconsistency: `st->double_scale` is not rolled back if hardware synchronization fails.
- [Low] Data Race: Lockless read of `st->double_scale` can race with concurrent writes.
--

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

iio: dac: ad5686: add gain control support

This commit adds support for a 2x gain multiplier applied to the output
voltage. This provides a gain control through a bit field in the control
register or a dedicated GAIN pin, exposing the scale property as configurable.

> diff --git a/drivers/iio/dac/ad5686.c b/drivers/iio/dac/ad5686.c
> index d9022c6a32016..d49390af71f90 100644
> --- a/drivers/iio/dac/ad5686.c
> +++ b/drivers/iio/dac/ad5686.c

[ ... ]

> @@ -192,9 +196,14 @@ static int ad5686_read_raw(struct iio_dev *indio_dev,
>  			GENMASK(chan->scan_type.realbits - 1, 0);
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_SCALE:
> -		*val = st->vref_mv;
> -		*val2 = chan->scan_type.realbits;
> -		return IIO_VAL_FRACTIONAL_LOG2;
> +		if (st->double_scale) {
> +			*val = st->scale_avail[2];
> +			*val2 = st->scale_avail[3];
> +		} else {
> +			*val = st->scale_avail[0];
> +			*val2 = st->scale_avail[1];
> +		}
> +		return IIO_VAL_INT_PLUS_NANO;

[Severity: Low]
Is it possible for a data race to occur here?

In ad5686_read_raw(), the mutable software state st->double_scale is read
to determine the returned scale values without holding the st->lock mutex.

Because st->double_scale can be concurrently modified by ad5686_write_raw()
(which properly acquires st->lock), this can lead to reading a prematurely
updated state before the hardware configuration has actually completed.

>  	}
>  	return -EINVAL;
>  }

[ ... ]

> @@ -216,6 +225,63 @@ static int ad5686_write_raw(struct iio_dev *indio_dev,
>  
>  		return ad5686_write(st, AD5686_CMD_WRITE_INPUT_N_UPDATE_N,
>  				    chan->address, val << chan->scan_type.shift);
> +	case IIO_CHAN_INFO_SCALE:
> +		if (val == st->scale_avail[0] && val2 == st->scale_avail[1])
> +			st->double_scale = false;
> +		else if (val == st->scale_avail[2] && val2 == st->scale_avail[3])
> +			st->double_scale = true;
> +		else
> +			return -EINVAL;
> +
> +		switch (st->chip_info->regmap_type) {
> +		case AD5310_REGMAP:
> +			return ad5310_control_sync(st);
> +		case AD5683_REGMAP:
> +			return ad5683_control_sync(st);

[Severity: Medium]
Should st->double_scale be restored to its previous value if the hardware
synchronization fails?

In ad5686_write_raw(), st->double_scale is updated before the driver attempts
to push the change to the DAC via ad5310_control_sync(st) or
ad5683_control_sync(st). 

If the underlying SPI/I2C write fails, the function immediately returns the
error code without rolling back st->double_scale.

This could cause the driver's cached state to diverge from the actual
hardware configuration, causing future reads of the scale to return the
incorrect updated value.

> +		case AD5686_REGMAP:
> +			/*
> +			 * Even if the gain pin is hardwired on the board, the
> +			 * user is able to control the scale such that it
> +			 * matches the actual gain setting.
> +			 */
> +			gpiod_set_value_cansleep(st->gain_gpio,
> +						 st->double_scale ? 1 : 0);
> +			return 0;

[Severity: Medium]
Does this expose a writable sysfs attribute to fix a static hardware
configuration?

The commit introduces a writable scale sysfs attribute to allow users to manually
match the software scale to a hardwired gain pin. Static hardware properties
should typically be described via firmware properties (e.g., Device Tree) so
the driver can expose the correct, immutable scale at initialization.

If the GAIN GPIO is absent, writing to the scale attribute returns success but
does not change the physical hardware state, leading to a silent desynchronization
between the reported scale and the actual physical DAC output.

Could this mislead generic IIO clients by presenting multiple available scales
that they believe they can dynamically toggle?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-ad5686-new-features-v2-0-70b423f5c76d@analog.com?part=12