diff mbox series

[v2,08/27] iio: accel: adxl367: Stop using iio_device_claim_direct_scoped()

Message ID 20250209180624.701140-9-jic23@kernel.org (mailing list archive)
State Accepted
Headers show
Series iio: improve handling of direct mode claim and release | expand

Commit Message

Jonathan Cameron Feb. 9, 2025, 6:06 p.m. UTC 
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>
---
 drivers/iio/accel/adxl367.c | 194 ++++++++++++++++++++----------------
 1 file changed, 106 insertions(+), 88 deletions(-)

Comments

Nuno Sá Feb. 17, 2025, 10:44 a.m. UTC | #1 
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,
diff mbox series

Patch

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,