Re: [PATCH v4 3/6] iio: pressure: bmp280: Introduce new cleanup routines

[Jonathan Cameron <jic23@kernel.org>]

On Sun,  7 Apr 2024 19:29:17 +0200
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> Introduce new linux/cleanup.h with the guard(mutex) functionality
> in the {read,write}_raw() functions.
> 
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Suggested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
A could of corners of dead code that you can remove.

In general looks good.

Jonathan

> ---
>  drivers/iio/pressure/bmp280-core.c | 129 +++++++++++++----------------
>  1 file changed, 58 insertions(+), 71 deletions(-)
> 
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index 50bdf79011bc..51bcdf8cede6 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -27,6 +27,7 @@
>  
>  #include <linux/bitops.h>
>  #include <linux/bitfield.h>
> +#include <linux/cleanup.h>
>  #include <linux/completion.h>
>  #include <linux/delay.h>
>  #include <linux/device.h>
> @@ -499,77 +500,69 @@ static int bme280_read_humid(struct bmp280_data *data, int *val, int *val2)
>  	return IIO_VAL_INT;
>  }
>  
> -static int bmp_read_raw(struct iio_dev *indio_dev,
> -			struct iio_chan_spec const *chan,
> -			int *val, int *val2, long mask)
> +static int bmp_read_raw_impl(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long mask)
>  {
>  	struct bmp280_data *data = iio_priv(indio_dev);
> -	int ret;
>  
> -	pm_runtime_get_sync(data->dev);
> -	mutex_lock(&data->lock);
> +	guard(mutex)(&data->lock);
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_PROCESSED:
>  		switch (chan->type) {
>  		case IIO_HUMIDITYRELATIVE:
> -			ret = data->chip_info->read_humid(data, val, val2);
> -			break;
> +			return data->chip_info->read_humid(data, val, val2);
>  		case IIO_PRESSURE:
> -			ret = data->chip_info->read_press(data, val, val2);
> -			break;
> +			return data->chip_info->read_press(data, val, val2);
>  		case IIO_TEMP:
> -			ret = data->chip_info->read_temp(data, val, val2);
> -			break;
> +			return data->chip_info->read_temp(data, val, val2);
>  		default:
> -			ret = -EINVAL;
> -			break;
> +			return -EINVAL;
>  		}
> -		break;
>  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>  		switch (chan->type) {
>  		case IIO_HUMIDITYRELATIVE:
>  			*val = 1 << data->oversampling_humid;
> -			ret = IIO_VAL_INT;
> -			break;
> +			return IIO_VAL_INT;
>  		case IIO_PRESSURE:
>  			*val = 1 << data->oversampling_press;
> -			ret = IIO_VAL_INT;
> -			break;
> +			return IIO_VAL_INT;
>  		case IIO_TEMP:
>  			*val = 1 << data->oversampling_temp;
> -			ret = IIO_VAL_INT;
> -			break;
> +			return IIO_VAL_INT;
>  		default:
> -			ret = -EINVAL;
> -			break;
> +			return -EINVAL;
>  		}
> -		break;
>  	case IIO_CHAN_INFO_SAMP_FREQ:
> -		if (!data->chip_info->sampling_freq_avail) {
> -			ret = -EINVAL;
> -			break;
> -		}
> +		if (!data->chip_info->sampling_freq_avail)
> +			return -EINVAL;
>  
>  		*val = data->chip_info->sampling_freq_avail[data->sampling_freq][0];
>  		*val2 = data->chip_info->sampling_freq_avail[data->sampling_freq][1];
> -		ret = IIO_VAL_INT_PLUS_MICRO;
> -		break;
> +		return IIO_VAL_INT_PLUS_MICRO;
>  	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> -		if (!data->chip_info->iir_filter_coeffs_avail) {
> -			ret = -EINVAL;
> -			break;
> -		}
> +		if (!data->chip_info->iir_filter_coeffs_avail)
> +			return -EINVAL;
>  
>  		*val = (1 << data->iir_filter_coeff) - 1;
> -		ret = IIO_VAL_INT;
> -		break;
> +		return IIO_VAL_INT;
>  	default:
> -		ret = -EINVAL;
> -		break;
> +		return -EINVAL;
>  	}
>  
> -	mutex_unlock(&data->lock);
> +	return 0;
As below. I don't think you can get here.  I hope all paths have already returned.

> +}
> +
> +static int bmp_read_raw(struct iio_dev *indio_dev,
> +			struct iio_chan_spec const *chan,
> +			int *val, int *val2, long mask)
> +{
> +	struct bmp280_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	pm_runtime_get_sync(data->dev);
> +	ret = bmp_read_raw_impl(indio_dev, chan, val, val2, mask);
>  	pm_runtime_mark_last_busy(data->dev);
>  	pm_runtime_put_autosuspend(data->dev);
>  
> @@ -697,12 +690,13 @@ static int bmp_write_iir_filter_coeffs(struct bmp280_data *data, int val)
>  	return -EINVAL;
>  }
>  
> -static int bmp_write_raw(struct iio_dev *indio_dev,
> -			 struct iio_chan_spec const *chan,
> -			 int val, int val2, long mask)
> +static int bmp_write_raw_impl(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      int val, int val2, long mask)
>  {
>  	struct bmp280_data *data = iio_priv(indio_dev);
> -	int ret = 0;
> +
> +	guard(mutex)(&data->lock);
>  
>  	/*
>  	 * Helper functions to update sensor running configuration.
> @@ -712,46 +706,39 @@ static int bmp_write_raw(struct iio_dev *indio_dev,
>  	 */
>  	switch (mask) {
>  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> -		pm_runtime_get_sync(data->dev);
> -		mutex_lock(&data->lock);
>  		switch (chan->type) {
>  		case IIO_HUMIDITYRELATIVE:
> -			ret = bmp_write_oversampling_ratio_humid(data, val);
> -			break;
> +			return bmp_write_oversampling_ratio_humid(data, val);
>  		case IIO_PRESSURE:
> -			ret = bmp_write_oversampling_ratio_press(data, val);
> -			break;
> +			return bmp_write_oversampling_ratio_press(data, val);
>  		case IIO_TEMP:
> -			ret = bmp_write_oversampling_ratio_temp(data, val);
> -			break;
> +			return bmp_write_oversampling_ratio_temp(data, val);
>  		default:
> -			ret = -EINVAL;
> -			break;
> +			return -EINVAL;
>  		}
> -		mutex_unlock(&data->lock);
> -		pm_runtime_mark_last_busy(data->dev);
> -		pm_runtime_put_autosuspend(data->dev);
> -		break;
>  	case IIO_CHAN_INFO_SAMP_FREQ:
> -		pm_runtime_get_sync(data->dev);
> -		mutex_lock(&data->lock);
> -		ret = bmp_write_sampling_frequency(data, val, val2);
> -		mutex_unlock(&data->lock);
> -		pm_runtime_mark_last_busy(data->dev);
> -		pm_runtime_put_autosuspend(data->dev);
> -		break;
> +		return bmp_write_sampling_frequency(data, val, val2);
>  	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> -		pm_runtime_get_sync(data->dev);
> -		mutex_lock(&data->lock);
> -		ret = bmp_write_iir_filter_coeffs(data, val);
> -		mutex_unlock(&data->lock);
> -		pm_runtime_mark_last_busy(data->dev);
> -		pm_runtime_put_autosuspend(data->dev);
> -		break;
> +		return bmp_write_iir_filter_coeffs(data, val);
>  	default:
>  		return -EINVAL;
>  	}
>  
> +	return 0;
I'm fairly sure you can't get here so this is dead code. Hene
drop this final return.

> +}

>