Re: [PATCH v2 4/7] iio: pressure: bmp280: Use sleep and forced mode for oneshot captures

[Jonathan Cameron]

On Fri, 26 Jul 2024 01:10:36 +0200
Vasileios Amoiridis <vassilisamir@xxxxxxxxx> wrote:

> This commit adds forced mode support in sensors BMP28x, BME28x, BMP3xx
> and BMP58x. Sensors BMP18x and BMP085 are old and do not support this
> feature so their operation is not affected at all.
> 
> Essentially, up to now, the rest of the sensors were used in normal mode
> all the time. This means that they are continuously doing measurements
> even though these measurements are not used. Even though the sensor does
> provide PM support, to cover all the possible use cases, the sensor needs
> to go into sleep mode and wake up whenever necessary.
> 
> This commit, adds sleep and forced mode support. Essentially, the sensor
> sleeps all the time except for when a measurement is requested. When there
> is a request for a measurement, the sensor is put into forced mode, starts
> the measurement and after it is done we read the output and we put it again
> in sleep mode.
> 
> For really fast and more deterministic measurements, the triggered buffer
> interface can be used, since the sensor is still used in normal mode for
> that use case.
> 
> This commit does not add though support for DEEP STANDBY, Low Power NORMAL
> and CONTINUOUS modes, supported only by the BMP58x version.
> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@xxxxxxxxx>
One question inline about the corner case of buffered capture in progress
when the machine is suspended.  We'd like the device to carry on feeding
us data on resume. Does that happen?

Jonathan


>  	.trigger_handler = bmp380_trigger_handler,
> @@ -2085,6 +2239,64 @@ static int bmp580_preinit(struct bmp280_data *data)
>  	return PTR_ERR_OR_ZERO(devm_nvmem_register(config.dev, &config));
>  }
>  
> +static const u8 bmp580_operation_mode[] = { BMP580_MODE_SLEEP,
> +					    BMP580_MODE_FORCED,
> +					    BMP580_MODE_NORMAL };
> +


> +
> +static int bmp580_wait_conv(struct bmp280_data *data)
> +{
> +	/*
> +	 * Taken from datasheet, Section 2 "Specification, Table 3 "Electrical
> +	 * characteristics
> +	 */
> +	const int time_conv_press[] = { 0, 1050, 1785, 3045, 5670, 10920, 21420,
> +					42420, 84420};
> +	const int time_conv_temp[] = { 0, 1050, 1105, 1575, 2205, 3465, 6090,
> +				       11340, 21840};
space before }

Also stick a static in front of them or Colin will ;)
Aim being to makes sure they aren't pointlessly allocated on the stack
if the compiler doesn't do something clever with them.

> +	int meas_time;
> +
> +	meas_time = 4000 + time_conv_temp[data->oversampling_temp] +
> +			   time_conv_press[data->oversampling_press];
> +
> +	usleep_range(meas_time, meas_time * 12 / 10);
> +
> +	return 0;
> +}
>
>  
> +/* Keep compatibility with future generations of the sensor */
> +static int bmp180_set_mode(struct bmp280_data *data, enum bmp280_op_mode mode)
> +{
> +	return 0;
> +}
> +
> +/* Keep compatibility with future generations of the sensor */
> +static int bmp180_wait_conv(struct bmp280_data *data)
> +{
> +	return 0;
> +}
> +
> +/* Keep compatibility with future generations of the sensor */

What does this comment mean?  I'm in favour of course, but don't understand
why it is here and above the stub calls.


> @@ -2825,6 +3048,9 @@ static int bmp280_runtime_suspend(struct device *dev)
>  	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>  	struct bmp280_data *data = iio_priv(indio_dev);
>  
> +	data->chip_info->set_mode(data, BMP280_SLEEP);

What happens if the device is in buffered mode and you suspend?
I'd expect to see the power mode stashed somewhere and restored in resume.

> +
> +	usleep_range(2500, 3000);
>  	return regulator_bulk_disable(BMP280_NUM_SUPPLIES, data->supplies);
>  }
>