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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Mon, Sep 02, 2024 at 08:42:19PM +0200, Vasileios Amoiridis wrote:
> Add 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.
> 
> The idea is that the sensor is by default in sleep mode, wakes up in
> forced mode when a oneshot capture is requested, or in normal mode
> when the buffer is enabled. The difference lays in the fact that in
> forced mode, the sensor does only one conversion and goes back to sleep
> while in normal mode, the sensor does continuous measurements with the
> frequency that was set in the ODR registers.
> 
> The bmpX_chip_config() functions which are responsible for applying
> the requested configuration to the sensor, are modified accordingly
> in order to set the sensor by default in sleep mode.
> 
> DEEP STANDBY, Low Power NORMAL and CONTINUOUS modes, supported only by
> the BMP58x version, are not added.

...

> +static int bmp280_wait_conv(struct bmp280_data *data)
> +{
> +	unsigned int reg, meas_time_us;
> +	int ret;
> +
> +	/* Check if we are using a BME280 device */
> +	if (data->oversampling_humid)
> +		meas_time_us += BMP280_PRESS_HUMID_MEAS_OFFSET +
> +				(BIT(data->oversampling_humid) * BMP280_MEAS_DUR);

The outer parentheses are not needed.

> +	/* Pressure measurement time */
> +	meas_time_us += BMP280_PRESS_HUMID_MEAS_OFFSET +
> +			(BIT(data->oversampling_press) * BMP280_MEAS_DUR);

Ditto.

> +	/* Temperature measurement time */
> +	meas_time_us += BIT(data->oversampling_temp) * BMP280_MEAS_DUR;
> +
> +	/* Waiting time according to the BM(P/E)2 Sensor API */
> +	fsleep(meas_time_us);
> +
> +	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 bmp380_wait_conv(struct bmp280_data *data)
> +{
> +	unsigned int reg;
> +	int ret, meas_time_us;
> +
> +	/* Offset measurement time */
> +	meas_time_us = BMP380_MEAS_OFFSET;
> +
> +	/* Pressure measurement time */
> +	meas_time_us += BMP380_PRESS_MEAS_OFFSET +
> +		     (BIT(data->oversampling_press) * BMP380_MEAS_DUR);

Ditto.

> +	/* Temperature measurement time */
> +	meas_time_us += BMP380_TEMP_MEAS_OFFSET +
> +		     (BIT(data->oversampling_temp) * BMP380_MEAS_DUR);

Ditto.

> +	/* Measurement time defined in Datasheet Section 3.9.2 */
> +	fsleep(meas_time_us);
> +
> +	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)) {
> +		dev_err(data->dev, "Measurement cycle didn't complete.\n");
> +		return -EBUSY;
> +	}

Alternatively

	if (!((reg & BMP380_STATUS_DRDY_PRESS_MASK) &&
	    !(reg & BMP380_STATUS_DRDY_TEMP_MASK)) {
		dev_err(data->dev, "Measurement cycle didn't complete.\n");
		return -EBUSY;
	}

> +	return 0;
> +}

...

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

> +	meas_time_us = 4 * USEC_PER_MSEC + time_conv_temp[data->oversampling_temp]
> +			 + time_conv_press[data->oversampling_press];

	meas_time_us = 4 * USEC_PER_MSEC + time_conv_temp[data->oversampling_temp] +
		       time_conv_press[data->oversampling_press];

OR

	meas_time_us = 4 * USEC_PER_MSEC +
		       time_conv_temp[data->oversampling_temp] +
		       time_conv_press[data->oversampling_press];


> +	/* Measurement time mentioned in Chapter 2, Table 4 of the datasheet. */

Since there is a constant in use (4ms) it would be nice to explain it
separately, the rest kinda obvious from the variable names.
So it allows roughly understand the timeout value without even looking into
the datasheet.

> +	fsleep(meas_time_us);
> +
> +	return 0;
> +}

...

> +	fsleep(data->start_up_time + 500);

Ditto.

Something like

	/* 500us margin for ... */

(but write the real meaning of it).

-- 
With Best Regards,
Andy Shevchenko