Re: [PATCH v1 07/10] iio: pressure: bmp280: Use sleep and forced mode for oneshot captures

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 11 Jul 2024 23:15:55 +0200
Vasileios Amoiridis <vassilisamir@gmail.com> 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@gmail.com>
Various minor comments inline.

Thanks,

Jonathan

> ---
>  drivers/iio/pressure/bmp280-core.c | 276 +++++++++++++++++++++++++++--
>  drivers/iio/pressure/bmp280.h      |  12 ++
>  2 files changed, 269 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index 9c99373d66ec..fc8d42880eb8 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -145,6 +145,12 @@ enum bmp280_scan {
>  	BME280_HUMID,
>  };
>  }
>  
> +static int bmp280_set_mode(struct bmp280_data *data, u8 mode)
> +{
> +	int ret;
> +
> +	switch (mode) {
> +	case BMP280_SLEEP:
> +		ret = regmap_write_bits(data->regmap, BMP280_REG_CTRL_MEAS,
> +					BMP280_MODE_MASK, BMP280_MODE_SLEEP);

Use a local variable for the BMP280_MODE_* and then have the regmap_write_bits()
after the switch statement.

Could even make it a const data look up given you are getting a value
based on the enum.

> +		break;
> +	case BMP280_FORCED:
> +		ret = regmap_write_bits(data->regmap, BMP280_REG_CTRL_MEAS,
> +					BMP280_MODE_MASK, BMP280_MODE_FORCED);
> +		break;
> +	case BMP280_NORMAL:
> +		ret = regmap_write_bits(data->regmap, BMP280_REG_CTRL_MEAS,
> +					BMP280_MODE_MASK, BMP280_MODE_NORMAL);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (ret) {
> +		dev_err(data->dev, "failed to  write ctrl_meas register\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int bmp280_wait_conv(struct bmp280_data *data)
> +{
> +	unsigned int reg;
> +	int ret, meas_time;
> +
> +	meas_time = BMP280_MEAS_OFFSET;
> +
> +	if (data->oversampling_humid)
> +		meas_time += (1 << data->oversampling_humid) * BMP280_MEAS_DUR +
> +			       BMP280_PRESS_HUMID_MEAS_OFFSET;
Add a comment on why, if oversampling_humid is not set we end up with
no time for measuring humidity.  The MEAS_OFFSET is less than one MEAS_DUR
so not it's not a case of that already incorporating the time.


> +
> +	/* Pressure measurement time */
> +	meas_time += (1 << data->oversampling_press) * BMP280_MEAS_DUR +
> +		      BMP280_PRESS_HUMID_MEAS_OFFSET;
> +
> +	/* Temperature measurement time */
> +	meas_time += (1 << data->oversampling_temp) * BMP280_MEAS_DUR;
> +
> +	usleep_range(meas_time, meas_time * 12 / 10);
> +
> +	ret = regmap_read(data->regmap, BMP280_REG_STATUS, &reg);
> +	if (ret) {
> +		dev_err(data->dev, "failed to read status register\n");
> +		return ret;
> +	}
> +	if (reg & BMP280_REG_STATUS_MEAS_BIT) {
> +		dev_err(data->dev, "Measurement cycle didn't complete\n");
> +		return -EBUSY;
> +	}
> +
> +	return 0;
> +}
> +
>  static int bmp280_chip_config(struct bmp280_data *data)
>  {
>  	u8 osrs = FIELD_PREP(BMP280_OSRS_TEMP_MASK, data->oversampling_temp + 1) |
> @@ -994,7 +1078,7 @@ static int bmp280_chip_config(struct bmp280_data *data)
>  				BMP280_OSRS_TEMP_MASK |
>  				BMP280_OSRS_PRESS_MASK |
>  				BMP280_MODE_MASK,
> -				osrs | BMP280_MODE_NORMAL);
> +				osrs | BMP280_MODE_SLEEP);
>  	if (ret) {
>  		dev_err(data->dev, "failed to write ctrl_meas register\n");
>  		return ret;
> @@ -1100,6 +1184,8 @@ const struct bmp280_chip_info bmp280_chip_info = {
>  	.read_temp = bmp280_read_temp,
>  	.read_press = bmp280_read_press,
>  	.read_calib = bmp280_read_calib,
> +	.set_mode = bmp280_set_mode,
> +	.wait_conv = bmp280_wait_conv,
>  	.preinit = bmp280_preinit,
>  
>  	.trigger_handler = bmp280_trigger_handler,
> @@ -1218,6 +1304,8 @@ const struct bmp280_chip_info bme280_chip_info = {
>  	.read_press = bmp280_read_press,
>  	.read_humid = bme280_read_humid,
>  	.read_calib = bme280_read_calib,
> +	.set_mode = bmp280_set_mode,
> +	.wait_conv = bmp280_wait_conv,
>  	.preinit = bmp280_preinit,
>  
>  	.trigger_handler = bme280_trigger_handler,
> @@ -1505,6 +1593,75 @@ static int bmp380_preinit(struct bmp280_data *data)
>  	return bmp380_cmd(data, BMP380_CMD_SOFT_RESET);
>  }
>  
> +static int bmp380_set_mode(struct bmp280_data *data, u8 mode)
> +{
> +	int ret;
> +
> +	switch (mode) {
> +	case BMP280_SLEEP:
> +		ret = regmap_write_bits(data->regmap, BMP380_REG_POWER_CONTROL,
> +					BMP380_MODE_MASK,
> +					FIELD_PREP(BMP380_MODE_MASK,
> +						   BMP380_MODE_SLEEP));
As above. I'd use a local variable to stash the MODE* that you are going
to write or just look it up in a const array.

> +		break;
> +	case BMP280_FORCED:
> +		ret = regmap_write_bits(data->regmap, BMP380_REG_POWER_CONTROL,
> +					BMP380_MODE_MASK,
> +					FIELD_PREP(BMP380_MODE_MASK,
> +						   BMP380_MODE_FORCED));
> +		break;
> +	case BMP280_NORMAL:
> +		ret = regmap_write_bits(data->regmap, BMP380_REG_POWER_CONTROL,
> +					BMP380_MODE_MASK,
> +					FIELD_PREP(BMP380_MODE_MASK,
> +						   BMP380_MODE_NORMAL));
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (ret) {
> +		dev_err(data->dev, "failed to  write power control register\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int bmp380_wait_conv(struct bmp280_data *data)
> +{
> +	unsigned int reg;
> +	int ret, meas_time;
> +
> +	/* Offset measurement time */
> +	meas_time = BMP380_MEAS_OFFSET;
> +
> +	/* Pressure measurement time */
> +	meas_time += (1 << data->oversampling_press) * BMP380_MEAS_DUR +
> +		      BMP380_PRESS_MEAS_OFFSET;
> +
> +	/* Temperature measurement time */
> +	meas_time += (1 << data->oversampling_temp) * BMP380_MEAS_DUR +
> +		      BMP380_TEMP_MEAS_OFFSET;
> +
> +	usleep_range(meas_time, meas_time * 12 / 10);
> +
> +	ret = regmap_read(data->regmap, BMP380_REG_STATUS, &reg);
> +	if (ret) {
> +		dev_err(data->dev, "failed to read status register\n");
> +		return ret;
> +	}
> +
> +	if (!(reg & BMP380_STATUS_DRDY_PRESS_MASK) ||
> +	    !(reg & BMP380_STATUS_DRDY_TEMP_MASK)) {
> +		pr_info("Meas_time: %d\n", meas_time);

Why as pr_info?  Seems like part of the dev_err.

> +		dev_err(data->dev, "Measurement cycle didn't complete\n");
> +		return -EBUSY;
> +	}
> +
> +	return 0;
> +}
> +
>  static int bmp380_chip_config(struct bmp280_data *data)
>  {
>  	bool change = false, aux;
> @@ -1565,17 +1722,15 @@ static int bmp380_chip_config(struct bmp280_data *data)
>  		 * Resets sensor measurement loop toggling between sleep and
>  		 * normal operating modes.
>  		 */
> -		ret = regmap_write_bits(data->regmap, BMP380_REG_POWER_CONTROL,
> -					BMP380_MODE_MASK,
> -					FIELD_PREP(BMP380_MODE_MASK, BMP380_MODE_SLEEP));
> +		ret = bmp380_set_mode(data, BMP280_SLEEP);
>  		if (ret) {
>  			dev_err(data->dev, "failed to set sleep mode\n");
>  			return ret;
>  		}
> -		usleep_range(2000, 2500);
> -		ret = regmap_write_bits(data->regmap, BMP380_REG_POWER_CONTROL,
> -					BMP380_MODE_MASK,
> -					FIELD_PREP(BMP380_MODE_MASK, BMP380_MODE_NORMAL));
> +
> +		usleep_range(data->start_up_time, data->start_up_time + 500);
> +
> +		ret = bmp380_set_mode(data, BMP280_NORMAL);
>  		if (ret) {
>  			dev_err(data->dev, "failed to set normal mode\n");
>  			return ret;
> @@ -1601,6 +1756,17 @@ static int bmp380_chip_config(struct bmp280_data *data)
>  		}
>  	}
>  
> +	/* Dummy read to empty data registers. */
> +	ret = bmp380_read_press(data, &tmp);
> +	if (ret)
> +		return ret;
> +
> +	ret = bmp380_set_mode(data, BMP280_SLEEP);
> +	if (ret) {
> +		dev_err(data->dev, "failed to set sleep mode\n");
> +		return ret;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -1693,6 +1859,8 @@ const struct bmp280_chip_info bmp380_chip_info = {
>  	.read_temp = bmp380_read_temp,
>  	.read_press = bmp380_read_press,
>  	.read_calib = bmp380_read_calib,
> +	.set_mode = bmp380_set_mode,
> +	.wait_conv = bmp380_wait_conv,
>  	.preinit = bmp380_preinit,
>  
>  	.trigger_handler = bmp380_trigger_handler,
> @@ -2080,6 +2248,65 @@ static int bmp580_preinit(struct bmp280_data *data)
>  	return PTR_ERR_OR_ZERO(devm_nvmem_register(config.dev, &config));
>  }
>  
> +static int bmp580_set_mode(struct bmp280_data *data, u8 mode)
> +{
> +	int ret;
> +
> +	switch (mode) {
> +	case BMP280_SLEEP:
> +		ret = regmap_write_bits(data->regmap, BMP580_REG_ODR_CONFIG,
> +					BMP580_MODE_MASK,
> +					FIELD_PREP(BMP580_MODE_MASK,
> +						   BMP580_MODE_SLEEP));
> +		break;
> +	case BMP280_FORCED:
> +		ret = regmap_set_bits(data->regmap, BMP580_REG_DSP_CONFIG,
> +				      BMP580_DSP_IIR_FORCED_FLUSH);
> +
check that ret.

> +		ret = regmap_write_bits(data->regmap, BMP580_REG_ODR_CONFIG,
> +					BMP580_MODE_MASK,
> +					FIELD_PREP(BMP580_MODE_MASK,
> +						   BMP580_MODE_FORCED));
This one is more complex so a switch statement makes sense here.
> +		break;
> +	case BMP280_NORMAL:
> +		ret = regmap_write_bits(data->regmap, BMP580_REG_ODR_CONFIG,
> +					BMP580_MODE_MASK,
> +					FIELD_PREP(BMP580_MODE_MASK,
> +						   BMP580_MODE_NORMAL));
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (ret) {
> +		dev_err(data->dev, "failed to  write power control register\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +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};
> +	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;
> +}
> +
one blank line only.
> +
>  static int bmp580_chip_config(struct bmp280_data *data)
>  {
>  	bool change = false, aux;
> @@ -2150,17 +2377,6 @@ static int bmp580_chip_config(struct bmp280_data *data)
>  		return ret;
>  	}
>  
> -	/* Restore sensor to normal operation mode */
> -	ret = regmap_write_bits(data->regmap, BMP580_REG_ODR_CONFIG,
> -				BMP580_MODE_MASK,
> -				FIELD_PREP(BMP580_MODE_MASK, BMP580_MODE_NORMAL));
> -	if (ret) {
> -		dev_err(data->dev, "failed to set normal mode\n");
> -		return ret;
> -	}
> -	/* From datasheet's table 4: electrical characteristics */
> -	usleep_range(3000, 3500);
> -
>  	if (change) {
>  		/*
>  		 * Check if ODR and OSR settings are valid or we are
> @@ -2256,6 +2472,8 @@ const struct bmp280_chip_info bmp580_chip_info = {
>  	.chip_config = bmp580_chip_config,
>  	.read_temp = bmp580_read_temp,
>  	.read_press = bmp580_read_press,
> +	.set_mode = bmp580_set_mode,
> +	.wait_conv = bmp580_wait_conv,
>  	.preinit = bmp580_preinit,
>  
>  	.trigger_handler = bmp580_trigger_handler,
> @@ -2503,6 +2721,16 @@ static int bmp180_read_press(struct bmp280_data *data, u32 *comp_press)
>  	return 0;
>  }
>  
> +static int bmp180_set_mode(struct bmp280_data *data, u8 mode)
> +{
> +	return 0;
Add a comment on why these are stubs.  It's in the patch description, but
better to have it recorded in the code.

> +}
> +
> +static int bmp180_wait_conv(struct bmp280_data *data)
> +{
> +	return 0;
> +}
> +
>