diff mbox series

[RFC,v2,1/4] iio: adc: ad7380: do not use iio_device_claim_direct_scoped anymore

Message ID 20241224-ad7380-add-alert-support-v2-1-7c89b2bf7cb3@baylibre.com (mailing list archive)
State Changes Requested
Headers show
Series iio: adc: ad7380: add alert support | expand

Commit Message

Julien Stephan Dec. 24, 2024, 9:34 a.m. UTC 
Rollback and remove the scoped version of iio_dvice_claim_direct_mode.

Signed-off-by: Julien Stephan <jstephan@baylibre.com>
---
 drivers/iio/adc/ad7380.c | 89 ++++++++++++++++++++++++++++--------------------
 1 file changed, 53 insertions(+), 36 deletions(-)

Comments

Uwe Kleine-König Dec. 27, 2024, 8:43 a.m. UTC | #1 
Hello Julien,

On Tue, Dec 24, 2024 at 10:34:30AM +0100, Julien Stephan wrote:
> Rollback and remove the scoped version of iio_dvice_claim_direct_mode.

Is this a preparation for one of the later patches in this series?
Mentioning the reasoning for this change in the commit log would be
good.

Best regards
Uwe
Jonathan Cameron Dec. 28, 2024, 1:49 p.m. UTC | #2 
On Tue, 24 Dec 2024 10:34:30 +0100
Julien Stephan <jstephan@baylibre.com> wrote:

> Rollback and remove the scoped version of iio_dvice_claim_direct_mode.
> 
> Signed-off-by: Julien Stephan <jstephan@baylibre.com>
> ---
>  drivers/iio/adc/ad7380.c | 89 ++++++++++++++++++++++++++++--------------------
>  1 file changed, 53 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
> index 4f32cb22f140442b831dc9a4f275e88e4ab2388e..4e26e0e7ac1d5a1c4c67118dbc34f2921fc171a4 100644
> --- a/drivers/iio/adc/ad7380.c
> +++ b/drivers/iio/adc/ad7380.c
> @@ -675,15 +675,21 @@ static const struct regmap_config ad7380_regmap_config = {
>  static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
>  				     u32 writeval, u32 *readval)
>  {
> -	iio_device_claim_direct_scoped(return  -EBUSY, indio_dev) {
> -		struct ad7380_state *st = iio_priv(indio_dev);
> +	struct ad7380_state *st = iio_priv(indio_dev);
> +	int ret;
>  
> -		if (readval)
> -			return regmap_read(st->regmap, reg, readval);
> -		else
> -			return regmap_write(st->regmap, reg, writeval);
> -	}
> -	unreachable();
> +	ret = iio_device_claim_direct_mode(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	if (readval)
> +		ret = regmap_read(st->regmap, reg, readval);
> +	else
> +		ret = regmap_write(st->regmap, reg, writeval);
> +
> +	iio_device_release_direct_mode(indio_dev);
> +
> +	return ret;
>  }
>  
>  /*
> @@ -920,6 +926,7 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
>  {
>  	struct ad7380_state *st = iio_priv(indio_dev);
>  	const struct iio_scan_type *scan_type;
> +	int ret;
>  
>  	scan_type = iio_get_current_scan_type(indio_dev, chan);
>  
> @@ -928,11 +935,16 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
>  
>  	switch (info) {
>  	case IIO_CHAN_INFO_RAW:
> -		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> -			return ad7380_read_direct(st, chan->scan_index,
> -						  scan_type, val);
> -		}
> -		unreachable();
> +		ret = iio_device_claim_direct_mode(indio_dev);
> +		if (ret)
> +			return ret;
> +
> +		ret = ad7380_read_direct(st, chan->scan_index,
> +					 scan_type, val);
> +
> +		iio_device_release_direct_mode(indio_dev);
> +
> +		return ret;
>  	case IIO_CHAN_INFO_SCALE:
>  		/*
>  		 * According to the datasheet, the LSB size is:
> @@ -1024,31 +1036,36 @@ static int ad7380_write_raw(struct iio_dev *indio_dev,
>  		/* always enable resolution boost when oversampling is enabled */
>  		boost = osr > 0 ? 1 : 0;
>  
> -		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> -			ret = regmap_update_bits(st->regmap,
> -					AD7380_REG_ADDR_CONFIG1,
> -					AD7380_CONFIG1_OSR | AD7380_CONFIG1_RES,
> -					FIELD_PREP(AD7380_CONFIG1_OSR, osr) |
> -					FIELD_PREP(AD7380_CONFIG1_RES, boost));
> +		ret = iio_device_claim_direct_mode(indio_dev);
> +		if (ret)
> +			return ret;
>  
> -			if (ret)
> -				return ret;
> +		ret = regmap_update_bits(st->regmap,
> +					 AD7380_REG_ADDR_CONFIG1,
> +					 AD7380_CONFIG1_OSR | AD7380_CONFIG1_RES,
> +					 FIELD_PREP(AD7380_CONFIG1_OSR, osr) |
> +					 FIELD_PREP(AD7380_CONFIG1_RES, boost));
>  
> -			st->oversampling_ratio = val;
> -			st->resolution_boost_enabled = boost;
> -
> -			/*
> -			 * Perform a soft reset. This will flush the oversampling
> -			 * block and FIFO but will maintain the content of the
> -			 * configurable registers.
> -			 */
> -			return regmap_update_bits(st->regmap,
> -					AD7380_REG_ADDR_CONFIG2,
> -					AD7380_CONFIG2_RESET,
> -					FIELD_PREP(AD7380_CONFIG2_RESET,
> -						   AD7380_CONFIG2_RESET_SOFT));
> -		}
> -		unreachable();
> +		if (ret)
> +			goto err;
> +
> +		st->oversampling_ratio = val;
> +		st->resolution_boost_enabled = boost;
> +
> +		/*
> +		 * Perform a soft reset. This will flush the oversampling
> +		 * block and FIFO but will maintain the content of the
> +		 * configurable registers.
> +		 */
> +		ret = regmap_update_bits(st->regmap,
> +					 AD7380_REG_ADDR_CONFIG2,
> +					 AD7380_CONFIG2_RESET,
> +					 FIELD_PREP(AD7380_CONFIG2_RESET,
> +						    AD7380_CONFIG2_RESET_SOFT));
> +err:
Labels within switch statements can become hard to maintainer / read.

Id' suggest factoring out the bits between claim and release as a single
__ad7380_write_oversample() or something along those lines.

Otherwise looks good to me and thanks for doing this.

Jonathan


> +		iio_device_release_direct_mode(indio_dev);
> +
> +		return ret;
>  	default:
>  		return -EINVAL;
>  	}
>
Jonathan Cameron Dec. 28, 2024, 1:51 p.m. UTC | #3 
On Fri, 27 Dec 2024 09:43:31 +0100
Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

> Hello Julien,
> 
> On Tue, Dec 24, 2024 at 10:34:30AM +0100, Julien Stephan wrote:
> > Rollback and remove the scoped version of iio_dvice_claim_direct_mode.  
> 
> Is this a preparation for one of the later patches in this series?
> Mentioning the reasoning for this change in the commit log would be
> good.
> 
> Best regards
> Uwe

I'd guess this is in response to a comment I made recently.
The conditional scoped handlers are turning out to be a real pain so
my plan is to rip them out entirely.

There are readability problems, compiler and linker handling issues
and a mess of other reasons these are much harder to use than they
originally seemed.

I've still very keen on the rest of the cleanup.h stuff, just not this
corner!

Jonathan
diff mbox series

Patch

diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
index 4f32cb22f140442b831dc9a4f275e88e4ab2388e..4e26e0e7ac1d5a1c4c67118dbc34f2921fc171a4 100644
--- a/drivers/iio/adc/ad7380.c
+++ b/drivers/iio/adc/ad7380.c
@@ -675,15 +675,21 @@  static const struct regmap_config ad7380_regmap_config = {
 static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
 				     u32 writeval, u32 *readval)
 {
-	iio_device_claim_direct_scoped(return  -EBUSY, indio_dev) {
-		struct ad7380_state *st = iio_priv(indio_dev);
+	struct ad7380_state *st = iio_priv(indio_dev);
+	int ret;
 
-		if (readval)
-			return regmap_read(st->regmap, reg, readval);
-		else
-			return regmap_write(st->regmap, reg, writeval);
-	}
-	unreachable();
+	ret = iio_device_claim_direct_mode(indio_dev);
+	if (ret)
+		return ret;
+
+	if (readval)
+		ret = regmap_read(st->regmap, reg, readval);
+	else
+		ret = regmap_write(st->regmap, reg, writeval);
+
+	iio_device_release_direct_mode(indio_dev);
+
+	return ret;
 }
 
 /*
@@ -920,6 +926,7 @@  static int ad7380_read_raw(struct iio_dev *indio_dev,
 {
 	struct ad7380_state *st = iio_priv(indio_dev);
 	const struct iio_scan_type *scan_type;
+	int ret;
 
 	scan_type = iio_get_current_scan_type(indio_dev, chan);
 
@@ -928,11 +935,16 @@  static int ad7380_read_raw(struct iio_dev *indio_dev,
 
 	switch (info) {
 	case IIO_CHAN_INFO_RAW:
-		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
-			return ad7380_read_direct(st, chan->scan_index,
-						  scan_type, val);
-		}
-		unreachable();
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+
+		ret = ad7380_read_direct(st, chan->scan_index,
+					 scan_type, val);
+
+		iio_device_release_direct_mode(indio_dev);
+
+		return ret;
 	case IIO_CHAN_INFO_SCALE:
 		/*
 		 * According to the datasheet, the LSB size is:
@@ -1024,31 +1036,36 @@  static int ad7380_write_raw(struct iio_dev *indio_dev,
 		/* always enable resolution boost when oversampling is enabled */
 		boost = osr > 0 ? 1 : 0;
 
-		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
-			ret = regmap_update_bits(st->regmap,
-					AD7380_REG_ADDR_CONFIG1,
-					AD7380_CONFIG1_OSR | AD7380_CONFIG1_RES,
-					FIELD_PREP(AD7380_CONFIG1_OSR, osr) |
-					FIELD_PREP(AD7380_CONFIG1_RES, boost));
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
 
-			if (ret)
-				return ret;
+		ret = regmap_update_bits(st->regmap,
+					 AD7380_REG_ADDR_CONFIG1,
+					 AD7380_CONFIG1_OSR | AD7380_CONFIG1_RES,
+					 FIELD_PREP(AD7380_CONFIG1_OSR, osr) |
+					 FIELD_PREP(AD7380_CONFIG1_RES, boost));
 
-			st->oversampling_ratio = val;
-			st->resolution_boost_enabled = boost;
-
-			/*
-			 * Perform a soft reset. This will flush the oversampling
-			 * block and FIFO but will maintain the content of the
-			 * configurable registers.
-			 */
-			return regmap_update_bits(st->regmap,
-					AD7380_REG_ADDR_CONFIG2,
-					AD7380_CONFIG2_RESET,
-					FIELD_PREP(AD7380_CONFIG2_RESET,
-						   AD7380_CONFIG2_RESET_SOFT));
-		}
-		unreachable();
+		if (ret)
+			goto err;
+
+		st->oversampling_ratio = val;
+		st->resolution_boost_enabled = boost;
+
+		/*
+		 * Perform a soft reset. This will flush the oversampling
+		 * block and FIFO but will maintain the content of the
+		 * configurable registers.
+		 */
+		ret = regmap_update_bits(st->regmap,
+					 AD7380_REG_ADDR_CONFIG2,
+					 AD7380_CONFIG2_RESET,
+					 FIELD_PREP(AD7380_CONFIG2_RESET,
+						    AD7380_CONFIG2_RESET_SOFT));
+err:
+		iio_device_release_direct_mode(indio_dev);
+
+		return ret;
 	default:
 		return -EINVAL;
 	}