[v3] iio: accel: convert to guard(mutex)

[PATCH v3 2/3] iio: accel: adxl355: convert to guard(mutex)

Posted by Rajveer Chaudhari 1 month ago

Replace manual mutex_lock/mutex_unlock pairs with guard(mutex) in
adxl355_data_rdy_trigger_set_state(), adxl355_set_odr(),
adxl355_set_hpf_3db() and adxl355_set_calibbias(). Remove all
goto labels and return directly on error paths.

v3: Remove all remaining gotos and return directly where possible.
v2: Split into separate patch per driver.

Signed-off-by: Rajveer Chaudhari <rajveer.chaudhari.linux@gmail.com>
---
 drivers/iio/accel/adxl355_core.c | 81 ++++++++++++++------------------
 1 file changed, 34 insertions(+), 47 deletions(-)

diff --git a/drivers/iio/accel/adxl355_core.c b/drivers/iio/accel/adxl355_core.c
index 1c1d64d5cbcb..af606e2ab8d4 100644
--- a/drivers/iio/accel/adxl355_core.c
+++ b/drivers/iio/accel/adxl355_core.c
@@ -9,6 +9,7 @@
 
 #include <linux/bits.h>
 #include <linux/bitfield.h>
+#include <linux/cleanup.h>
 #include <linux/iio/buffer.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/trigger.h>
@@ -261,16 +262,13 @@ static int adxl355_data_rdy_trigger_set_state(struct iio_trigger *trig,
 {
 	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
 	struct adxl355_data *data = iio_priv(indio_dev);
-	int ret;
 
-	mutex_lock(&data->lock);
-	ret = regmap_update_bits(data->regmap, ADXL355_POWER_CTL_REG,
+	guard(mutex)(&data->lock);
+	
+	return regmap_update_bits(data->regmap, ADXL355_POWER_CTL_REG,
 				 ADXL355_POWER_CTL_DRDY_MSK,
 				 FIELD_PREP(ADXL355_POWER_CTL_DRDY_MSK,
 					    state ? 0 : 1));
-	mutex_unlock(&data->lock);
-
-	return ret;
 }
 
 static void adxl355_fill_3db_frequency_table(struct adxl355_data *data)
@@ -409,38 +407,34 @@ static int adxl355_set_odr(struct adxl355_data *data,
 {
 	int ret;
 
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
 
 	if (data->odr == odr) {
-		mutex_unlock(&data->lock);
 		return 0;
 	}
 
 	ret = adxl355_set_op_mode(data, ADXL355_STANDBY);
 	if (ret)
-		goto err_unlock;
+		return ret;
 
 	ret = regmap_update_bits(data->regmap, ADXL355_FILTER_REG,
 				 ADXL355_FILTER_ODR_MSK,
 				 FIELD_PREP(ADXL355_FILTER_ODR_MSK, odr));
-	if (ret)
-		goto err_set_opmode;
+	if (ret){
+		adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
+		return ret;
+	}
 
 	data->odr = odr;
 	adxl355_fill_3db_frequency_table(data);
 
 	ret = adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
-	if (ret)
-		goto err_set_opmode;
+	if (ret){
+		adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
+		return ret;
+	}
 
-	mutex_unlock(&data->lock);
 	return 0;
-
-err_set_opmode:
-	adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
-err_unlock:
-	mutex_unlock(&data->lock);
-	return ret;
 }
 
 static int adxl355_set_hpf_3db(struct adxl355_data *data,
@@ -448,37 +442,33 @@ static int adxl355_set_hpf_3db(struct adxl355_data *data,
 {
 	int ret;
 
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
 
 	if (data->hpf_3db == hpf) {
-		mutex_unlock(&data->lock);
 		return 0;
 	}
 
 	ret = adxl355_set_op_mode(data, ADXL355_STANDBY);
 	if (ret)
-		goto err_unlock;
+		return ret;
 
 	ret = regmap_update_bits(data->regmap, ADXL355_FILTER_REG,
 				 ADXL355_FILTER_HPF_MSK,
 				 FIELD_PREP(ADXL355_FILTER_HPF_MSK, hpf));
-	if (ret)
-		goto err_set_opmode;
+	if (ret){
+		adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
+		return ret;
+	}
 
 	data->hpf_3db = hpf;
 
 	ret = adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
-	if (ret)
-		goto err_set_opmode;
+	if (ret){
+		adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
+		return ret;
+	}
 
-	mutex_unlock(&data->lock);
 	return 0;
-
-err_set_opmode:
-	adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
-err_unlock:
-	mutex_unlock(&data->lock);
-	return ret;
 }
 
 static int adxl355_set_calibbias(struct adxl355_data *data,
@@ -486,33 +476,30 @@ static int adxl355_set_calibbias(struct adxl355_data *data,
 {
 	int ret;
 
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
 
 	ret = adxl355_set_op_mode(data, ADXL355_STANDBY);
 	if (ret)
-		goto err_unlock;
+		return ret;
 
 	put_unaligned_be16(calibbias, data->transf_buf);
 	ret = regmap_bulk_write(data->regmap,
 				adxl355_chans[chan].offset_reg,
 				data->transf_buf, 2);
-	if (ret)
-		goto err_set_opmode;
+	if (ret){
+		adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
+		return ret;
+	}
 
 	data->calibbias[chan] = calibbias;
 
 	ret = adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
-	if (ret)
-		goto err_set_opmode;
+	if (ret){
+		adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
+		return ret;
+	}
 
-	mutex_unlock(&data->lock);
 	return 0;
-
-err_set_opmode:
-	adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
-err_unlock:
-	mutex_unlock(&data->lock);
-	return ret;
 }
 
 static int adxl355_read_raw(struct iio_dev *indio_dev,
-- 
2.53.0

Re: [PATCH v3 2/3] iio: accel: adxl355: convert to guard(mutex)

Posted by Jonathan Cameron 1 month ago

On Sat,  7 Mar 2026 15:47:57 +0530
Rajveer Chaudhari <rajveer.chaudhari.linux@gmail.com> wrote:

> Replace manual mutex_lock/mutex_unlock pairs with guard(mutex) in
> adxl355_data_rdy_trigger_set_state(), adxl355_set_odr(),
> adxl355_set_hpf_3db() and adxl355_set_calibbias(). Remove all
> goto labels and return directly on error paths.
> 
> v3: Remove all remaining gotos and return directly where possible.
> v2: Split into separate patch per driver.

Same issue for the change log needing to be below the ---


> 
> Signed-off-by: Rajveer Chaudhari <rajveer.chaudhari.linux@gmail.com>
> ---
>  drivers/iio/accel/adxl355_core.c | 81 ++++++++++++++------------------
>  1 file changed, 34 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl355_core.c b/drivers/iio/accel/adxl355_core.c
> index 1c1d64d5cbcb..af606e2ab8d4 100644
> --- a/drivers/iio/accel/adxl355_core.c
> +++ b/drivers/iio/accel/adxl355_core.c
>

...

>  
>  static void adxl355_fill_3db_frequency_table(struct adxl355_data *data)
> @@ -409,38 +407,34 @@ static int adxl355_set_odr(struct adxl355_data *data,
>  {
>  	int ret;
>  
> -	mutex_lock(&data->lock);
> +	guard(mutex)(&data->lock);
>  
>  	if (data->odr == odr) {
> -		mutex_unlock(&data->lock);
>  		return 0;
>  	}
>  
>  	ret = adxl355_set_op_mode(data, ADXL355_STANDBY);
>  	if (ret)
> -		goto err_unlock;
> +		return ret;
>  
>  	ret = regmap_update_bits(data->regmap, ADXL355_FILTER_REG,
>  				 ADXL355_FILTER_ODR_MSK,
>  				 FIELD_PREP(ADXL355_FILTER_ODR_MSK, odr));
> -	if (ret)
> -		goto err_set_opmode;
> +	if (ret){
> +		adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
See below.

> +		return ret;
> +	}
>  
>  	data->odr = odr;
>  	adxl355_fill_3db_frequency_table(data);
>  
>  	ret = adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
> -	if (ret)
> -		goto err_set_opmode;
> +	if (ret){
> +		adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
> +		return ret;
> +	}
>  
> -	mutex_unlock(&data->lock);
>  	return 0;
> -
> -err_set_opmode:
> -	adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
> -err_unlock:
> -	mutex_unlock(&data->lock);
> -	return ret;
>  }
>  
>  static int adxl355_set_hpf_3db(struct adxl355_data *data,
> @@ -448,37 +442,33 @@ static int adxl355_set_hpf_3db(struct adxl355_data *data,
>  {
>  	int ret;
>  
> -	mutex_lock(&data->lock);
> +	guard(mutex)(&data->lock);
>  
>  	if (data->hpf_3db == hpf) {
> -		mutex_unlock(&data->lock);
>  		return 0;
>  	}
>  
>  	ret = adxl355_set_op_mode(data, ADXL355_STANDBY);
>  	if (ret)
> -		goto err_unlock;
> +		return ret;
>  
>  	ret = regmap_update_bits(data->regmap, ADXL355_FILTER_REG,
>  				 ADXL355_FILTER_HPF_MSK,
>  				 FIELD_PREP(ADXL355_FILTER_HPF_MSK, hpf));
> -	if (ret)
> -		goto err_set_opmode;
> +	if (ret){
> +		adxl355_set_op_mode(data, ADXL355_MEASUREMENT);

Similar to below.

> +		return ret;
> +	}
>  
>  	data->hpf_3db = hpf;
>  
>  	ret = adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
> -	if (ret)
> -		goto err_set_opmode;
> +	if (ret){
> +		adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
> +		return ret;
> +	}
>  
> -	mutex_unlock(&data->lock);
>  	return 0;
> -
> -err_set_opmode:
> -	adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
> -err_unlock:
> -	mutex_unlock(&data->lock);
> -	return ret;
>  }
>  
>  static int adxl355_set_calibbias(struct adxl355_data *data,
> @@ -486,33 +476,30 @@ static int adxl355_set_calibbias(struct adxl355_data *data,
>  {
>  	int ret;
>  
> -	mutex_lock(&data->lock);
> +	guard(mutex)(&data->lock);
>  
>  	ret = adxl355_set_op_mode(data, ADXL355_STANDBY);
>  	if (ret)
> -		goto err_unlock;
> +		return ret;
>  
>  	put_unaligned_be16(calibbias, data->transf_buf);
>  	ret = regmap_bulk_write(data->regmap,
>  				adxl355_chans[chan].offset_reg,
>  				data->transf_buf, 2);
> -	if (ret)
> -		goto err_set_opmode;
> +	if (ret){
Whilst this does answer the feedback you got on v2 wrt to not mixing
gotos (see comments on this in cleanup.h) it leads to inelegant code
due to the duplication.

There are a couple of techniques to avoid this. The most appropriate
one here is to use a helper function called something like
do_adxl355_set_calibbias() which has all the code
done between set_op_mode and unsetting it.

Then you can unconditionally unset the op_mode before checking
if there as an error.  You will need to be a little clever with
ret code handling though.


> +		adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
> +		return ret;
> +	}
>  
>  	data->calibbias[chan] = calibbias;
>  
>  	ret = adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
> -	if (ret)
> -		goto err_set_opmode;
> +	if (ret){

Run checkpatch.pl over your patch. Should be a space before that {

> +		adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
> +		return ret;
> +	}
>  
> -	mutex_unlock(&data->lock);
>  	return 0;
> -
> -err_set_opmode:
> -	adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
> -err_unlock:
> -	mutex_unlock(&data->lock);
> -	return ret;
>  }
>  
>  static int adxl355_read_raw(struct iio_dev *indio_dev,

Re: [PATCH v3 2/3] iio: accel: adxl355: convert to guard(mutex)

Posted by Jonathan Cameron 1 month ago

On Sat, 7 Mar 2026 10:39:49 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Sat,  7 Mar 2026 15:47:57 +0530
> Rajveer Chaudhari <rajveer.chaudhari.linux@gmail.com> wrote:
> 
> > Replace manual mutex_lock/mutex_unlock pairs with guard(mutex) in
> > adxl355_data_rdy_trigger_set_state(), adxl355_set_odr(),
> > adxl355_set_hpf_3db() and adxl355_set_calibbias(). Remove all
> > goto labels and return directly on error paths.
> > 
> > v3: Remove all remaining gotos and return directly where possible.
> > v2: Split into separate patch per driver.  
> 
> Same issue for the change log needing to be below the ---

One more thing. Sometimes guard() brings enough extra complexity it isn't
the right tool for the job. Whilst you can explore the suggestions for
helper functions I make below, I'm not sure they will lead to easier to
read and less error prone code.  So it might not be a good idea to use
them in this driver at all!

Jonathan

> 
> 
> > 
> > Signed-off-by: Rajveer Chaudhari <rajveer.chaudhari.linux@gmail.com>
> > ---
> >  drivers/iio/accel/adxl355_core.c | 81 ++++++++++++++------------------
> >  1 file changed, 34 insertions(+), 47 deletions(-)
> > 
> > diff --git a/drivers/iio/accel/adxl355_core.c b/drivers/iio/accel/adxl355_core.c
> > index 1c1d64d5cbcb..af606e2ab8d4 100644
> > --- a/drivers/iio/accel/adxl355_core.c
> > +++ b/drivers/iio/accel/adxl355_core.c
> >  
> 
> ...
> 
> >  
> >  static void adxl355_fill_3db_frequency_table(struct adxl355_data *data)
> > @@ -409,38 +407,34 @@ static int adxl355_set_odr(struct adxl355_data *data,
> >  {
> >  	int ret;
> >  
> > -	mutex_lock(&data->lock);
> > +	guard(mutex)(&data->lock);
> >  
> >  	if (data->odr == odr) {
> > -		mutex_unlock(&data->lock);
> >  		return 0;
> >  	}
> >  
> >  	ret = adxl355_set_op_mode(data, ADXL355_STANDBY);
> >  	if (ret)
> > -		goto err_unlock;
> > +		return ret;
> >  
> >  	ret = regmap_update_bits(data->regmap, ADXL355_FILTER_REG,
> >  				 ADXL355_FILTER_ODR_MSK,
> >  				 FIELD_PREP(ADXL355_FILTER_ODR_MSK, odr));
> > -	if (ret)
> > -		goto err_set_opmode;
> > +	if (ret){
> > +		adxl355_set_op_mode(data, ADXL355_MEASUREMENT);  
> See below.
> 
> > +		return ret;
> > +	}
> >  
> >  	data->odr = odr;
> >  	adxl355_fill_3db_frequency_table(data);
> >  
> >  	ret = adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
> > -	if (ret)
> > -		goto err_set_opmode;
> > +	if (ret){
> > +		adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
> > +		return ret;
> > +	}
> >  
> > -	mutex_unlock(&data->lock);
> >  	return 0;
> > -
> > -err_set_opmode:
> > -	adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
> > -err_unlock:
> > -	mutex_unlock(&data->lock);
> > -	return ret;
> >  }
> >  
> >  static int adxl355_set_hpf_3db(struct adxl355_data *data,
> > @@ -448,37 +442,33 @@ static int adxl355_set_hpf_3db(struct adxl355_data *data,
> >  {
> >  	int ret;
> >  
> > -	mutex_lock(&data->lock);
> > +	guard(mutex)(&data->lock);
> >  
> >  	if (data->hpf_3db == hpf) {
> > -		mutex_unlock(&data->lock);
> >  		return 0;
> >  	}
> >  
> >  	ret = adxl355_set_op_mode(data, ADXL355_STANDBY);
> >  	if (ret)
> > -		goto err_unlock;
> > +		return ret;
> >  
> >  	ret = regmap_update_bits(data->regmap, ADXL355_FILTER_REG,
> >  				 ADXL355_FILTER_HPF_MSK,
> >  				 FIELD_PREP(ADXL355_FILTER_HPF_MSK, hpf));
> > -	if (ret)
> > -		goto err_set_opmode;
> > +	if (ret){
> > +		adxl355_set_op_mode(data, ADXL355_MEASUREMENT);  
> 
> Similar to below.
> 
> > +		return ret;
> > +	}
> >  
> >  	data->hpf_3db = hpf;
> >  
> >  	ret = adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
> > -	if (ret)
> > -		goto err_set_opmode;
> > +	if (ret){
> > +		adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
> > +		return ret;
> > +	}
> >  
> > -	mutex_unlock(&data->lock);
> >  	return 0;
> > -
> > -err_set_opmode:
> > -	adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
> > -err_unlock:
> > -	mutex_unlock(&data->lock);
> > -	return ret;
> >  }
> >  
> >  static int adxl355_set_calibbias(struct adxl355_data *data,
> > @@ -486,33 +476,30 @@ static int adxl355_set_calibbias(struct adxl355_data *data,
> >  {
> >  	int ret;
> >  
> > -	mutex_lock(&data->lock);
> > +	guard(mutex)(&data->lock);
> >  
> >  	ret = adxl355_set_op_mode(data, ADXL355_STANDBY);
> >  	if (ret)
> > -		goto err_unlock;
> > +		return ret;
> >  
> >  	put_unaligned_be16(calibbias, data->transf_buf);
> >  	ret = regmap_bulk_write(data->regmap,
> >  				adxl355_chans[chan].offset_reg,
> >  				data->transf_buf, 2);
> > -	if (ret)
> > -		goto err_set_opmode;
> > +	if (ret){  
> Whilst this does answer the feedback you got on v2 wrt to not mixing
> gotos (see comments on this in cleanup.h) it leads to inelegant code
> due to the duplication.
> 
> There are a couple of techniques to avoid this. The most appropriate
> one here is to use a helper function called something like
> do_adxl355_set_calibbias() which has all the code
> done between set_op_mode and unsetting it.
> 
> Then you can unconditionally unset the op_mode before checking
> if there as an error.  You will need to be a little clever with
> ret code handling though.
> 
> 
> > +		adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
> > +		return ret;
> > +	}
> >  
> >  	data->calibbias[chan] = calibbias;
> >  
> >  	ret = adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
> > -	if (ret)
> > -		goto err_set_opmode;
> > +	if (ret){  
> 
> Run checkpatch.pl over your patch. Should be a space before that {
> 
> > +		adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
> > +		return ret;
> > +	}
> >  
> > -	mutex_unlock(&data->lock);
> >  	return 0;
> > -
> > -err_set_opmode:
> > -	adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
> > -err_unlock:
> > -	mutex_unlock(&data->lock);
> > -	return ret;
> >  }
> >  
> >  static int adxl355_read_raw(struct iio_dev *indio_dev,  
>

[PATCH v3 1/3] iio: accel: adxl313: convert to guard(mutex)
[PATCH v3 2/3] iio: accel: adxl355: convert to guard(mutex)
[PATCH v3 3/3] iio: accel: adxl372: convert to guard(mutex)