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

[Jonathan Cameron]

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

> On Sat,  7 Mar 2026 15:47:57 +0530
> Rajveer Chaudhari <rajveer.chaudhari.linux@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> >  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,  
>