On Sun, 7 Apr 2024 19:29:17 +0200 Vasileios Amoiridis <vassilisamir@xxxxxxxxx> wrote: > Introduce new linux/cleanup.h with the guard(mutex) functionality > in the {read,write}_raw() functions. > > Suggested-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> > Suggested-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> > Signed-off-by: Vasileios Amoiridis <vassilisamir@xxxxxxxxx> 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. > +} >