Re: [bmp280 v1 2/6] iio: pressure: bmp280: reduce overhead on read with MODE_FORCED

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Sun,  3 Aug 2025 16:07:58 +0200
Achim Gratz <Achim.Gratz@Stromeko.DE> wrote:

> When measuring with MODE_FORCED, each read through sysfs triggers a
> new measurement cycle through aLL channels with the current channel
> configuration, even though we can only access a single channel.
> Reduce the incurred overhead (especially for higher oversampling_ratio
> settings) by temporarily switching off the unused channels.  This
> savea about a third of the acquisition time when reading all three
> channels in succession:
> 
> | oversampling | max/full | max/skip | time/full | time/skip |
> |              |     [ms] |     [ms] |      [ms] |      [ms] |
> |--------------+----------+----------+-----------+-----------|
> |           16 |      339 |      120 |       315 |       127 |
> |            8 |      174 |       65 |       166 |        76 |
> |            4 |       90 |       38 |        94 |        53 |
> |            2 |       49 |       24 |        59 |        41 |
> |            1 |       28 |       17 |        36 |        33 |
> 
> The results are from an I²C connected sensor at 400kHz, so there is
> considerable overhead from the changing the channel configuration,
> most noticeably with low oversampling_ratio values.  Faster
> communication will reduce this overhead further; and since there is
> still a net reduction in acquisition time even for
> oversampling_ratio=1 switching off the channels is always done.
> 
> Note: The IIR filters will process a slightly noisier input signal.
> 
> Signed-off-by: Achim Gratz <Achim.Gratz@Stromeko.DE>
Hi Achim.

I'm not really sure what the algorithm implemented here is and what the
various local variables actually mean as state.  Please add some
more comments to the code.

> ---
>  drivers/iio/pressure/bmp280-core.c | 138 ++++++++++++++++++++++++++---
>  drivers/iio/pressure/bmp280.h      |   7 +-
>  2 files changed, 129 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index 3213dcadba28..858974a64306 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -619,18 +619,66 @@ static int bmp280_read_raw_impl(struct iio_dev *indio_dev,
>  	struct bmp280_data *data = iio_priv(indio_dev);
>  	int chan_value;
>  	int ret;
> +	int prev_oversampling_humid, prev_oversampling_press, prev_oversampling_temp;
> +	int temp_oversampling_humid, temp_oversampling_press, temp_oversampling_temp;
> +	int switch_off, switch_threshold = -1;
>  
>  	guard(mutex)(&data->lock);
>  
> +	prev_oversampling_humid = temp_oversampling_humid = data->oversampling_humid;
> +	prev_oversampling_press = temp_oversampling_press = data->oversampling_press;
> +	prev_oversampling_temp  = temp_oversampling_temp  = data->oversampling_temp;
> +
>  	switch (mask) {
>  	case IIO_CHAN_INFO_PROCESSED:
> +		/* switch off unused channels */
> +		switch_off = 0;

So this is saying if 'anything can be switched off'? 
switch_off_something or some naming like that.

> +		switch (chan->type) {
> +		case IIO_HUMIDITYRELATIVE:
> +			temp_oversampling_press = 0-1;

0 - 1
etc. So always spaces around operators.
I'm not at all sure on the logic behind this maths though.
Seems it ultimately gets store into a u8.  Add some comments on what the algorithm
is doing.


> +			switch_off |= (prev_oversampling_press > switch_threshold);
> +			/* can't be switched off as it is needed for compensation */
What can't be switched off?  The temperature I guess.
> +			temp_oversampling_temp  = 1-1;
1 - 1
> +			break;
> +		case IIO_PRESSURE:
> +			temp_oversampling_humid = 0-1;
> +			switch_off |= (prev_oversampling_humid > switch_threshold);
> +			/* can't be switched off as it is needed for compensation */
> +			temp_oversampling_temp  = 1-1;
> +			break;
> +		case IIO_TEMP:
> +			temp_oversampling_humid = 0-1;
> +			temp_oversampling_press = 0-1;
> +			switch_off = (prev_oversampling_humid > switch_threshold) |
> +				     (prev_oversampling_press > switch_threshold);
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		if (switch_off) {
> +			data->oversampling_humid = temp_oversampling_humid;
> +			data->oversampling_press = temp_oversampling_press;
> +			data->oversampling_temp  = temp_oversampling_temp;
> +			ret = data->chip_info->chip_config(data);
> +			if (ret)
> +				goto restore;
> +		}
> +
>  		ret = data->chip_info->set_mode(data, BMP280_FORCED);
>  		if (ret)
> -			return ret;
> -
> +			goto restore;
>  		ret = data->chip_info->wait_conv(data);
>  		if (ret)
> -			return ret;
> +			goto restore;
> +
> +		if (switch_off) {
> +			data->oversampling_humid = prev_oversampling_humid;
> +			data->oversampling_press = prev_oversampling_press;
> +			data->oversampling_temp  = prev_oversampling_temp;
> +			ret = data->chip_info->chip_config(data);
> +			if (ret)
> +				return ret;
> +		}
>  
>  		switch (chan->type) {
>  		case IIO_HUMIDITYRELATIVE:
> @@ -661,13 +709,55 @@ static int bmp280_read_raw_impl(struct iio_dev *indio_dev,
>  			return -EINVAL;
>  		}
>  	case IIO_CHAN_INFO_RAW:
> +		/* switch off unused channels */
> +		switch_off = 0;
> +		switch (chan->type) {
> +		case IIO_HUMIDITYRELATIVE:
> +			temp_oversampling_press = 0-1;
> +			switch_off |= (prev_oversampling_press > switch_threshold);
> +			/* can't be switched off as it is needed for compensation */
> +			temp_oversampling_temp  = 1-1;
> +			break;
> +		case IIO_PRESSURE:
> +			temp_oversampling_humid = 0-1;
> +			switch_off |= (prev_oversampling_humid > switch_threshold);
> +			/* can't be switched off as it is needed for compensation */
> +			temp_oversampling_temp  = 1-1;
> +			break;
> +		case IIO_TEMP:
> +			temp_oversampling_humid = 0-1;
> +			temp_oversampling_press = 0-1;
> +			switch_off = (prev_oversampling_humid > switch_threshold) |
> +				     (prev_oversampling_press > switch_threshold);
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		if (switch_off) {
> +			data->oversampling_humid = temp_oversampling_humid;
> +			data->oversampling_press = temp_oversampling_press;
> +			data->oversampling_temp  = temp_oversampling_temp;
> +			ret = data->chip_info->chip_config(data);
> +			if (ret)
> +				goto restore;
> +		}
> +
>  		ret = data->chip_info->set_mode(data, BMP280_FORCED);
>  		if (ret)
> -			return ret;
> -
> +			goto restore;
>  		ret = data->chip_info->wait_conv(data);
>  		if (ret)
> -			return ret;
> +			goto restore;
> +
> +		if (switch_off) {
> +			data->oversampling_humid = prev_oversampling_humid;
> +			data->oversampling_press = prev_oversampling_press;
> +			data->oversampling_temp  = prev_oversampling_temp;
> +			data->chip_info->chip_config(data);
> +			if (ret) {
> +				return ret;
> +			}
> +		}
>  
>  		switch (chan->type) {
>  		case IIO_HUMIDITYRELATIVE:
> @@ -741,6 +831,15 @@ static int bmp280_read_raw_impl(struct iio_dev *indio_dev,
>  	default:
>  		return -EINVAL;
>  	}
> +restore:
Maybe worth factoring out he contents of the switch case that has goto restore
into a separate function so that the scope of the restore is same as the code
that is calling it.  (i.e. avoid a goto jumping out of a switch.

> +	if (switch_off) {
> +	/* restore channel configuration */
> +		data->oversampling_humid = prev_oversampling_humid;
> +		data->oversampling_press = prev_oversampling_press;
> +		data->oversampling_temp  = prev_oversampling_temp;
> +		data->chip_info->chip_config(data);
> +	}
> +	return ret;
>  }