Re: [PATCH v2 11/27] iio: adc: ad4695: Stop using iio_device_claim_direct_scoped()

["Nuno Sá" <noname.nuno@gmail.com>]

On Sun, 2025-02-09 at 18:06 +0000, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> This complex cleanup.h use case of conditional guards has proved
> to be more trouble that it is worth in terms of false positive compiler
> warnings and hard to read code.
> 
> Move directly to the new claim/release_direct() that allow sparse
> to check for unbalanced context.  In some cases code is factored
> out to utility functions that can do a direct return with the
> claim and release around the call.
> 
> Reviewed-by: David Lechner <dlechner@baylibre.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> v2: Typo in commit description (David).
> Note there are several sets current in flight that touch this driver.
> I'll rebase as necessary depending on what order the dependencies resolve.
> ---

Reviewed-by: Nuno Sa <nuno.sa@analog.com>

>  drivers/iio/adc/ad4695.c | 240 ++++++++++++++++++++++-----------------
>  1 file changed, 133 insertions(+), 107 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
> index 13cf01d35301..4bb22f4d739b 100644
> --- a/drivers/iio/adc/ad4695.c
> +++ b/drivers/iio/adc/ad4695.c
> @@ -738,6 +738,25 @@ static int ad4695_read_one_sample(struct ad4695_state
> *st, unsigned int address)
>  	return spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
>  }
>  
> +static int __ad4695_read_info_raw(struct ad4695_state *st,
> +				  struct iio_chan_spec const *chan,
> +				  int *val)
> +{
> +	u8 realbits = chan->scan_type.realbits;
> +	int ret;
> +
> +	ret = ad4695_read_one_sample(st, chan->address);
> +	if (ret)
> +		return ret;
> +
> +	if (chan->scan_type.sign == 's')
> +		*val = sign_extend32(st->raw_data, realbits - 1);
> +	else
> +		*val = st->raw_data;
> +
> +	return IIO_VAL_INT;
> +}
> +
>  static int ad4695_read_raw(struct iio_dev *indio_dev,
>  			   struct iio_chan_spec const *chan,
>  			   int *val, int *val2, long mask)
> @@ -750,19 +769,12 @@ static int ad4695_read_raw(struct iio_dev *indio_dev,
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> -		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> -			ret = ad4695_read_one_sample(st, chan->address);
> -			if (ret)
> -				return ret;
> -
> -			if (chan->scan_type.sign == 's')
> -				*val = sign_extend32(st->raw_data, realbits -
> 1);
> -			else
> -				*val = st->raw_data;
>  
> -			return IIO_VAL_INT;
> -		}
> -		unreachable();
> +		if (!iio_device_claim_direct(indio_dev))
> +			return -EBUSY;
> +		ret = __ad4695_read_info_raw(st, chan, val);
> +		iio_device_release_direct(indio_dev);
> +		return ret;
>  	case IIO_CHAN_INFO_SCALE:
>  		switch (chan->type) {
>  		case IIO_VOLTAGE:
> @@ -800,45 +812,45 @@ static int ad4695_read_raw(struct iio_dev *indio_dev,
>  	case IIO_CHAN_INFO_CALIBSCALE:
>  		switch (chan->type) {
>  		case IIO_VOLTAGE:
> -			iio_device_claim_direct_scoped(return -EBUSY,
> indio_dev) {
> -				ret = regmap_read(st->regmap16,
> -					AD4695_REG_GAIN_IN(chan->scan_index),
> -					&reg_val);
> -				if (ret)
> -					return ret;
> +			if (!iio_device_claim_direct(indio_dev))
> +				return -EBUSY;
> +			ret = regmap_read(st->regmap16,
> +					  AD4695_REG_GAIN_IN(chan-
> >scan_index),
> +					  &reg_val);
> +			iio_device_release_direct(indio_dev);
> +			if (ret)
> +				return ret;
>  
> -				*val = reg_val;
> -				*val2 = 15;
> +			*val = reg_val;
> +			*val2 = 15;
>  
> -				return IIO_VAL_FRACTIONAL_LOG2;
> -			}
> -			unreachable();
> +			return IIO_VAL_FRACTIONAL_LOG2;
>  		default:
>  			return -EINVAL;
>  		}
>  	case IIO_CHAN_INFO_CALIBBIAS:
>  		switch (chan->type) {
>  		case IIO_VOLTAGE:
> -			iio_device_claim_direct_scoped(return -EBUSY,
> indio_dev) {
> -				ret = regmap_read(st->regmap16,
> -					AD4695_REG_OFFSET_IN(chan-
> >scan_index),
> -					&reg_val);
> -				if (ret)
> -					return ret;
> -
> -				tmp = sign_extend32(reg_val, 15);
> +			if (!iio_device_claim_direct(indio_dev))
> +				return -EBUSY;
> +			ret = regmap_read(st->regmap16,
> +					  AD4695_REG_OFFSET_IN(chan-
> >scan_index),
> +					  &reg_val);
> +			iio_device_release_direct(indio_dev);
> +			if (ret)
> +				return ret;
>  
> -				*val = tmp / 4;
> -				*val2 = abs(tmp) % 4 * MICRO / 4;
> +			tmp = sign_extend32(reg_val, 15);
>  
> -				if (tmp < 0 && *val2) {
> -					*val *= -1;
> -					*val2 *= -1;
> -				}
> +			*val = tmp / 4;
> +			*val2 = abs(tmp) % 4 * MICRO / 4;
>  
> -				return IIO_VAL_INT_PLUS_MICRO;
> +			if (tmp < 0 && *val2) {
> +				*val *= -1;
> +				*val2 *= -1;
>  			}
> -			unreachable();
> +
> +			return IIO_VAL_INT_PLUS_MICRO;
>  		default:
>  			return -EINVAL;
>  		}
> @@ -847,64 +859,75 @@ static int ad4695_read_raw(struct iio_dev *indio_dev,
>  	}
>  }
>  
> -static int ad4695_write_raw(struct iio_dev *indio_dev,
> -			    struct iio_chan_spec const *chan,
> -			    int val, int val2, long mask)
> +static int __ad4695_write_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      int val, int val2, long mask)
>  {
>  	struct ad4695_state *st = iio_priv(indio_dev);
>  	unsigned int reg_val;
>  
> -	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> -		switch (mask) {
> -		case IIO_CHAN_INFO_CALIBSCALE:
> -			switch (chan->type) {
> -			case IIO_VOLTAGE:
> -				if (val < 0 || val2 < 0)
> -					reg_val = 0;
> -				else if (val > 1)
> -					reg_val = U16_MAX;
> -				else
> -					reg_val = (val * (1 << 16) +
> -						   mul_u64_u32_div(val2, 1 <<
> 16,
> -								   MICRO)) /
> 2;
> -
> -				return regmap_write(st->regmap16,
> -					AD4695_REG_GAIN_IN(chan->scan_index),
> -					reg_val);
> -			default:
> -				return -EINVAL;
> -			}
> -		case IIO_CHAN_INFO_CALIBBIAS:
> -			switch (chan->type) {
> -			case IIO_VOLTAGE:
> -				if (val2 >= 0 && val > S16_MAX / 4)
> -					reg_val = S16_MAX;
> -				else if ((val2 < 0 ? -val : val) < S16_MIN /
> 4)
> -					reg_val = S16_MIN;
> -				else if (val2 < 0)
> -					reg_val = clamp_t(int,
> -						-(val * 4 + -val2 * 4 /
> MICRO),
> -						S16_MIN, S16_MAX);
> -				else if (val < 0)
> -					reg_val = clamp_t(int,
> -						val * 4 - val2 * 4 / MICRO,
> -						S16_MIN, S16_MAX);
> -				else
> -					reg_val = clamp_t(int,
> -						val * 4 + val2 * 4 / MICRO,
> -						S16_MIN, S16_MAX);
> -
> -				return regmap_write(st->regmap16,
> -					AD4695_REG_OFFSET_IN(chan-
> >scan_index),
> -					reg_val);
> -			default:
> -				return -EINVAL;
> -			}
> +	switch (mask) {
> +	case IIO_CHAN_INFO_CALIBSCALE:
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			if (val < 0 || val2 < 0)
> +				reg_val = 0;
> +			else if (val > 1)
> +				reg_val = U16_MAX;
> +			else
> +				reg_val = (val * (1 << 16) +
> +					   mul_u64_u32_div(val2, 1 << 16,
> +							   MICRO)) / 2;
> +
> +			return regmap_write(st->regmap16,
> +					    AD4695_REG_GAIN_IN(chan-
> >scan_index),
> +					    reg_val);
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			if (val2 >= 0 && val > S16_MAX / 4)
> +				reg_val = S16_MAX;
> +			else if ((val2 < 0 ? -val : val) < S16_MIN / 4)
> +				reg_val = S16_MIN;
> +			else if (val2 < 0)
> +				reg_val = clamp_t(int,
> +						  -(val * 4 + -val2 * 4 /
> MICRO),
> +						  S16_MIN, S16_MAX);
> +			else if (val < 0)
> +				reg_val = clamp_t(int,
> +						  val * 4 - val2 * 4 / MICRO,
> +						  S16_MIN, S16_MAX);
> +			else
> +				reg_val = clamp_t(int,
> +						  val * 4 + val2 * 4 / MICRO,
> +						  S16_MIN, S16_MAX);
> +
> +			return regmap_write(st->regmap16,
> +					    AD4695_REG_OFFSET_IN(chan-
> >scan_index),
> +					    reg_val);
>  		default:
>  			return -EINVAL;
>  		}
> +	default:
> +		return -EINVAL;
>  	}
> -	unreachable();
> +}
> +
> +static int ad4695_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int val, int val2, long mask)
> +{
> +	int ret;
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;
> +	ret = __ad4695_write_raw(indio_dev, chan, val, val2, mask);
> +	iio_device_release_direct(indio_dev);
> +
> +	return ret;
>  }
>  
>  static int ad4695_read_avail(struct iio_dev *indio_dev,
> @@ -954,26 +977,29 @@ static int ad4695_debugfs_reg_access(struct iio_dev
> *indio_dev,
>  				     unsigned int *readval)
>  {
>  	struct ad4695_state *st = iio_priv(indio_dev);
> -
> -	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> -		if (readval) {
> -			if (regmap_check_range_table(st->regmap, reg,
> -						    
> &ad4695_regmap_rd_table))
> -				return regmap_read(st->regmap, reg, readval);
> -			if (regmap_check_range_table(st->regmap16, reg,
> -						    
> &ad4695_regmap16_rd_table))
> -				return regmap_read(st->regmap16, reg,
> readval);
> -		} else {
> -			if (regmap_check_range_table(st->regmap, reg,
> -						    
> &ad4695_regmap_wr_table))
> -				return regmap_write(st->regmap, reg,
> writeval);
> -			if (regmap_check_range_table(st->regmap16, reg,
> -						    
> &ad4695_regmap16_wr_table))
> -				return regmap_write(st->regmap16, reg,
> writeval);
> -		}
> +	int ret = -EINVAL;
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;
> +
> +	if (readval) {
> +		if (regmap_check_range_table(st->regmap, reg,
> +					     &ad4695_regmap_rd_table))
> +			ret = regmap_read(st->regmap, reg, readval);
> +		if (regmap_check_range_table(st->regmap16, reg,
> +					     &ad4695_regmap16_rd_table))
> +			ret = regmap_read(st->regmap16, reg, readval);
> +	} else {
> +		if (regmap_check_range_table(st->regmap, reg,
> +					     &ad4695_regmap_wr_table))
> +			ret = regmap_write(st->regmap, reg, writeval);
> +		if (regmap_check_range_table(st->regmap16, reg,
> +					     &ad4695_regmap16_wr_table))
> +			ret = regmap_write(st->regmap16, reg, writeval);
>  	}
> +	iio_device_release_direct(indio_dev);
>  
> -	return -EINVAL;
> +	return ret;
>  }
>  
>  static const struct iio_info ad4695_info = {