Re: [PATCH v2 08/27] iio: accel: adxl367: 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 there is a convenient wrapper function to which
> the handling can be moved. Do that instead of introducing
> another layer of wrappers. In others an outer wrapper is added
> which claims direct mode, runs the original function with the
> scoped claim logic removed, releases direct mode and then checks
> for errors.
> 
> Cc: Cosmin Tanislav <demonsingur@gmail.com>
> Reviewed-by: David Lechner <dlechner@baylibre.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---

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

>  drivers/iio/accel/adxl367.c | 194 ++++++++++++++++++++----------------
>  1 file changed, 106 insertions(+), 88 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl367.c b/drivers/iio/accel/adxl367.c
> index a48ac0d7bd96..add4053e7a02 100644
> --- a/drivers/iio/accel/adxl367.c
> +++ b/drivers/iio/accel/adxl367.c
> @@ -477,45 +477,42 @@ static int adxl367_set_fifo_watermark(struct
> adxl367_state *st,
>  static int adxl367_set_range(struct iio_dev *indio_dev,
>  			     enum adxl367_range range)
>  {
> -	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> -		struct adxl367_state *st = iio_priv(indio_dev);
> -		int ret;
> +	struct adxl367_state *st = iio_priv(indio_dev);
> +	int ret;
>  
> -		guard(mutex)(&st->lock);
> +	guard(mutex)(&st->lock);
>  
> -		ret = adxl367_set_measure_en(st, false);
> -		if (ret)
> -			return ret;
> +	ret = adxl367_set_measure_en(st, false);
> +	if (ret)
> +		return ret;
>  
> -		ret = regmap_update_bits(st->regmap, ADXL367_REG_FILTER_CTL,
> -					 ADXL367_FILTER_CTL_RANGE_MASK,
> -					
> FIELD_PREP(ADXL367_FILTER_CTL_RANGE_MASK,
> -						    range));
> -		if (ret)
> -			return ret;
> +	ret = regmap_update_bits(st->regmap, ADXL367_REG_FILTER_CTL,
> +				 ADXL367_FILTER_CTL_RANGE_MASK,
> +				 FIELD_PREP(ADXL367_FILTER_CTL_RANGE_MASK,
> +					    range));
> +	if (ret)
> +		return ret;
>  
> -		adxl367_scale_act_thresholds(st, st->range, range);
> +	adxl367_scale_act_thresholds(st, st->range, range);
>  
> -		/* Activity thresholds depend on range */
> -		ret = _adxl367_set_act_threshold(st, ADXL367_ACTIVITY,
> -						 st->act_threshold);
> -		if (ret)
> -			return ret;
> +	/* Activity thresholds depend on range */
> +	ret = _adxl367_set_act_threshold(st, ADXL367_ACTIVITY,
> +					 st->act_threshold);
> +	if (ret)
> +		return ret;
>  
> -		ret = _adxl367_set_act_threshold(st, ADXL367_INACTIVITY,
> -						 st->inact_threshold);
> -		if (ret)
> -			return ret;
> +	ret = _adxl367_set_act_threshold(st, ADXL367_INACTIVITY,
> +					 st->inact_threshold);
> +	if (ret)
> +		return ret;
>  
> -		ret = adxl367_set_measure_en(st, true);
> -		if (ret)
> -			return ret;
> +	ret = adxl367_set_measure_en(st, true);
> +	if (ret)
> +		return ret;
>  
> -		st->range = range;
> +	st->range = range;
>  
> -		return 0;
> -	}
> -	unreachable();
> +	return 0;
>  }
>  
>  static int adxl367_time_ms_to_samples(struct adxl367_state *st, unsigned int
> ms)
> @@ -620,23 +617,20 @@ static int _adxl367_set_odr(struct adxl367_state *st,
> enum adxl367_odr odr)
>  
>  static int adxl367_set_odr(struct iio_dev *indio_dev, enum adxl367_odr odr)
>  {
> -	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> -		struct adxl367_state *st = iio_priv(indio_dev);
> -		int ret;
> +	struct adxl367_state *st = iio_priv(indio_dev);
> +	int ret;
>  
> -		guard(mutex)(&st->lock);
> +	guard(mutex)(&st->lock);
>  
> -		ret = adxl367_set_measure_en(st, false);
> -		if (ret)
> -			return ret;
> +	ret = adxl367_set_measure_en(st, false);
> +	if (ret)
> +		return ret;
>  
> -		ret = _adxl367_set_odr(st, odr);
> -		if (ret)
> -			return ret;
> +	ret = _adxl367_set_odr(st, odr);
> +	if (ret)
> +		return ret;
>  
> -		return adxl367_set_measure_en(st, true);
> -	}
> -	unreachable();
> +	return adxl367_set_measure_en(st, true);
>  }
>  
>  static int adxl367_set_temp_adc_en(struct adxl367_state *st, unsigned int
> reg,
> @@ -725,32 +719,29 @@ static int adxl367_read_sample(struct iio_dev
> *indio_dev,
>  			       struct iio_chan_spec const *chan,
>  			       int *val)
>  {
> -	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> -		struct adxl367_state *st = iio_priv(indio_dev);
> -		u16 sample;
> -		int ret;
> +	struct adxl367_state *st = iio_priv(indio_dev);
> +	u16 sample;
> +	int ret;
>  
> -		guard(mutex)(&st->lock);
> +	guard(mutex)(&st->lock);
>  
> -		ret = adxl367_set_temp_adc_reg_en(st, chan->address, true);
> -		if (ret)
> -			return ret;
> +	ret = adxl367_set_temp_adc_reg_en(st, chan->address, true);
> +	if (ret)
> +		return ret;
>  
> -		ret = regmap_bulk_read(st->regmap, chan->address, &st-
> >sample_buf,
> -				       sizeof(st->sample_buf));
> -		if (ret)
> -			return ret;
> +	ret = regmap_bulk_read(st->regmap, chan->address, &st->sample_buf,
> +			       sizeof(st->sample_buf));
> +	if (ret)
> +		return ret;
>  
> -		sample = FIELD_GET(ADXL367_DATA_MASK, be16_to_cpu(st-
> >sample_buf));
> -		*val = sign_extend32(sample, chan->scan_type.realbits - 1);
> +	sample = FIELD_GET(ADXL367_DATA_MASK, be16_to_cpu(st->sample_buf));
> +	*val = sign_extend32(sample, chan->scan_type.realbits - 1);
>  
> -		ret = adxl367_set_temp_adc_reg_en(st, chan->address, false);
> -		if (ret)
> -			return ret;
> +	ret = adxl367_set_temp_adc_reg_en(st, chan->address, false);
> +	if (ret)
> +		return ret;
>  
> -		return IIO_VAL_INT;
> -	}
> -	unreachable();
> +	return IIO_VAL_INT;
>  }
>  
>  static int adxl367_get_status(struct adxl367_state *st, u8 *status,
> @@ -852,10 +843,15 @@ static int adxl367_read_raw(struct iio_dev *indio_dev,
>  			    int *val, int *val2, long info)
>  {
>  	struct adxl367_state *st = iio_priv(indio_dev);
> +	int ret;
>  
>  	switch (info) {
>  	case IIO_CHAN_INFO_RAW:
> -		return adxl367_read_sample(indio_dev, chan, val);
> +		if (!iio_device_claim_direct(indio_dev))
> +			return -EBUSY;
> +		ret = adxl367_read_sample(indio_dev, chan, val);
> +		iio_device_release_direct(indio_dev);
> +		return ret;
>  	case IIO_CHAN_INFO_SCALE:
>  		switch (chan->type) {
>  		case IIO_ACCEL: {
> @@ -912,7 +908,12 @@ static int adxl367_write_raw(struct iio_dev *indio_dev,
>  		if (ret)
>  			return ret;
>  
> -		return adxl367_set_odr(indio_dev, odr);
> +		if (!iio_device_claim_direct(indio_dev))
> +			return -EBUSY;
> +
> +		ret = adxl367_set_odr(indio_dev, odr);
> +		iio_device_release_direct(indio_dev);
> +		return ret;
>  	}
>  	case IIO_CHAN_INFO_SCALE: {
>  		enum adxl367_range range;
> @@ -921,7 +922,12 @@ static int adxl367_write_raw(struct iio_dev *indio_dev,
>  		if (ret)
>  			return ret;
>  
> -		return adxl367_set_range(indio_dev, range);
> +		if (!iio_device_claim_direct(indio_dev))
> +			return -EBUSY;
> +
> +		ret = adxl367_set_range(indio_dev, range);
> +		iio_device_release_direct(indio_dev);
> +		return ret;
>  	}
>  	default:
>  		return -EINVAL;
> @@ -1069,13 +1075,15 @@ static int adxl367_read_event_config(struct iio_dev
> *indio_dev,
>  	}
>  }
>  
> -static int adxl367_write_event_config(struct iio_dev *indio_dev,
> -				      const struct iio_chan_spec *chan,
> -				      enum iio_event_type type,
> -				      enum iio_event_direction dir,
> -				      bool state)
> +static int __adxl367_write_event_config(struct iio_dev *indio_dev,
> +					const struct iio_chan_spec *chan,
> +					enum iio_event_type type,
> +					enum iio_event_direction dir,
> +					bool state)
>  {
> +	struct adxl367_state *st = iio_priv(indio_dev);
>  	enum adxl367_activity_type act;
> +	int ret;
>  
>  	switch (dir) {
>  	case IIO_EV_DIR_RISING:
> @@ -1088,28 +1096,38 @@ static int adxl367_write_event_config(struct iio_dev
> *indio_dev,
>  		return -EINVAL;
>  	}
>  
> -	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> -		struct adxl367_state *st = iio_priv(indio_dev);
> -		int ret;
> +	guard(mutex)(&st->lock);
> +
> +	ret = adxl367_set_measure_en(st, false);
> +	if (ret)
> +		return ret;
>  
> -		guard(mutex)(&st->lock);
> +	ret = adxl367_set_act_interrupt_en(st, act, state);
> +	if (ret)
> +		return ret;
>  
> -		ret = adxl367_set_measure_en(st, false);
> -		if (ret)
> -			return ret;
> +	ret = adxl367_set_act_en(st, act, state ? ADCL367_ACT_REF_ENABLED
> +				 : ADXL367_ACT_DISABLED);
> +	if (ret)
> +		return ret;
>  
> -		ret = adxl367_set_act_interrupt_en(st, act, state);
> -		if (ret)
> -			return ret;
> +	return adxl367_set_measure_en(st, true);
> +}
>  
> -		ret = adxl367_set_act_en(st, act, state ?
> ADCL367_ACT_REF_ENABLED
> -					 : ADXL367_ACT_DISABLED);
> -		if (ret)
> -			return ret;
> +static int adxl367_write_event_config(struct iio_dev *indio_dev,
> +				      const struct iio_chan_spec *chan,
> +				      enum iio_event_type type,
> +				      enum iio_event_direction dir,
> +				      bool state)
> +{
> +	int ret;
>  
> -		return adxl367_set_measure_en(st, true);
> -	}
> -	unreachable();
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;
> +
> +	ret = __adxl367_write_event_config(indio_dev, chan, type, dir,
> state);
> +	iio_device_release_direct(indio_dev);
> +	return ret;
>  }
>  
>  static ssize_t adxl367_get_fifo_enabled(struct device *dev,