[v3] Series to add triggered buffer support to BMP280 driver

[PATCH v3 2/6] iio: pressure: Introduce new cleanup routines to BMP280 driver *_raw() functions

Posted by Vasileios Amoiridis 1 year, 10 months ago

Introduce the 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>
---
 drivers/iio/pressure/bmp280-core.c | 125 +++++++++++++----------------
 1 file changed, 58 insertions(+), 67 deletions(-)

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index 871b2214121b..f7a13ff6f26c 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -460,77 +460,74 @@ static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
 	return IIO_VAL_INT;
 }
 
-static int bmp280_read_raw(struct iio_dev *indio_dev,
-			   struct iio_chan_spec const *chan,
-			   int *val, int *val2, long mask)
+static int bmp280_read_raw_guarded(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;
+		return 0;
 	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;
+		return 0;
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		if (!data->chip_info->sampling_freq_avail) {
-			ret = -EINVAL;
-			break;
+			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;
+			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;
+}
+
+
+static int bmp280_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 = bmp280_read_raw_guarded(indio_dev, chan, val, val2, mask);
 	pm_runtime_mark_last_busy(data->dev);
 	pm_runtime_put_autosuspend(data->dev);
 
@@ -662,13 +659,13 @@ static int bmp280_write_iir_filter_coeffs(struct bmp280_data *data, int val)
 	return -EINVAL;
 }
 
-static int bmp280_write_raw(struct iio_dev *indio_dev,
-			    struct iio_chan_spec const *chan,
-			    int val, int val2, long mask)
+static int bmp280_write_raw_guarded(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.
 	 * If an error happens applying new settings, will try restore
@@ -677,46 +674,40 @@ static int bmp280_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 = bmp280_write_oversampling_ratio_humid(data, val);
-			break;
+			return bmp280_write_oversampling_ratio_humid(data, val);
 		case IIO_PRESSURE:
-			ret = bmp280_write_oversampling_ratio_press(data, val);
-			break;
+			return bmp280_write_oversampling_ratio_press(data, val);
 		case IIO_TEMP:
-			ret = bmp280_write_oversampling_ratio_temp(data, val);
-			break;
+			return bmp280_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;
+		return 0;
 	case IIO_CHAN_INFO_SAMP_FREQ:
-		pm_runtime_get_sync(data->dev);
-		mutex_lock(&data->lock);
-		ret = bmp280_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 bmp280_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 = bmp280_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 bmp280_write_iir_filter_coeffs(data, val);
 	default:
 		return -EINVAL;
 	}
 
+	return 0;
+}
+
+static int bmp280_write_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 = 0;
+
+	pm_runtime_get_sync(data->dev);
+	ret = bmp280_write_raw_guarded(indio_dev, chan, val, val2, mask);
+	pm_runtime_mark_last_busy(data->dev);
+	pm_runtime_put_autosuspend(data->dev);
+
 	return ret;
 }
 
-- 
2.25.1

Re: [PATCH v3 2/6] iio: pressure: Introduce new cleanup routines to BMP280 driver *_raw() functions

Posted by Jonathan Cameron 1 year, 10 months ago

On Tue, 19 Mar 2024 01:29:21 +0100
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> Introduce the 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>
> ---
>  drivers/iio/pressure/bmp280-core.c | 125 +++++++++++++----------------
>  1 file changed, 58 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index 871b2214121b..f7a13ff6f26c 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -460,77 +460,74 @@ static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
>  	return IIO_VAL_INT;
>  }
>  
> -static int bmp280_read_raw(struct iio_dev *indio_dev,
> -			   struct iio_chan_spec const *chan,
> -			   int *val, int *val2, long mask)
> +static int bmp280_read_raw_guarded(struct iio_dev *indio_dev,

Why do we need the guarded naming?  It always took the lock, no
change just because we are doing that in a neater fashion.

I don't see a reason for that name.  Better to use something like
_impl, or _internal as the prefix.

I don't want to see people calling every function that uses guard
_guarded().


> +				   struct iio_chan_spec const *chan,
> +				   int *val, int *val2, long mask)
Otherwise, I didn't find anything beyond what Andy already pointed out.
So looks good in general.

Jonathan

Re: [PATCH v3 2/6] iio: pressure: Introduce new cleanup routines to BMP280 driver *_raw() functions

Posted by Andy Shevchenko 1 year, 10 months ago

On Tue, Mar 19, 2024 at 01:29:21AM +0100, Vasileios Amoiridis wrote:
> Introduce the new linux/cleanup.h with the guard(mutex) functionality
> in the {read/write}_raw() functions

Missing period at the end of the sentence.

...

>  	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;
> +		return 0;

Now dead code.

>  	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;
> +		return 0;

Ditto.

>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		if (!data->chip_info->sampling_freq_avail) {
> -			ret = -EINVAL;
> -			break;
> +			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;
> +			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;

Ditto.

...

> +
> +

One blank line is enough.

> +static int bmp280_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long mask)

...

>  	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 = bmp280_write_oversampling_ratio_humid(data, val);
> -			break;
> +			return bmp280_write_oversampling_ratio_humid(data, val);
>  		case IIO_PRESSURE:
> -			ret = bmp280_write_oversampling_ratio_press(data, val);
> -			break;
> +			return bmp280_write_oversampling_ratio_press(data, val);
>  		case IIO_TEMP:
> -			ret = bmp280_write_oversampling_ratio_temp(data, val);
> -			break;
> +			return bmp280_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;
> +		return 0;
>  	case IIO_CHAN_INFO_SAMP_FREQ:
> -		pm_runtime_get_sync(data->dev);
> -		mutex_lock(&data->lock);
> -		ret = bmp280_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 bmp280_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 = bmp280_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 bmp280_write_iir_filter_coeffs(data, val);
>  	default:
>  		return -EINVAL;
>  	}
>  
> +	return 0;

As per above, dead code.

...

> +static int bmp280_write_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 = 0;

Useless assignment.

> +
> +	pm_runtime_get_sync(data->dev);
> +	ret = bmp280_write_raw_guarded(indio_dev, chan, val, val2, mask);
> +	pm_runtime_mark_last_busy(data->dev);
> +	pm_runtime_put_autosuspend(data->dev);
> +
>  	return ret;
>  }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/6] iio: pressure: Introduce new cleanup routines to BMP280 driver *_raw() functions

Posted by Andy Shevchenko 1 year, 10 months ago

On Wed, Mar 20, 2024 at 01:00:01PM +0200, Andy Shevchenko wrote:
> On Tue, Mar 19, 2024 at 01:29:21AM +0100, Vasileios Amoiridis wrote:
> > Introduce the new linux/cleanup.h with the guard(mutex) functionality
> > in the {read/write}_raw() functions
> 
> Missing period at the end of the sentence.

And also

{read,write}_raw()

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 1/6] iio: pressure: BMP280 core driver headers sorting
[PATCH v3 2/6] iio: pressure: Introduce new cleanup routines to BMP280 driver *_raw() functions
[PATCH v3 3/6] iio: pressure: Generalize read_{temp/press/humid}() functions
[PATCH v3 4/6] iio: pressure: Add SCALE and RAW values for channels
[PATCH v3 5/6] iio: pressure: Add timestamp and scan_masks for BMP280 driver
[PATCH v3 6/6] iio: pressure: Add triggered buffer support for BMP280 driver