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

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

On Fri, Aug 23, 2024 at 08:17:11PM +0200, Vasileios Amoiridis 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.

...

> +static const u8 bmp280_operation_mode[] = { BMP280_MODE_SLEEP,
> +					    BMP280_MODE_FORCED,
> +					    BMP280_MODE_NORMAL };

Better style is

static const u8 bmp280_operation_mode[] = {
	BMP280_MODE_SLEEP, BMP280_MODE_FORCED, BMP280_MODE_NORMAL,
};

Also note comma at the end.

...

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

		BIT(data->oversampling_humid)

> +			       BMP280_PRESS_HUMID_MEAS_OFFSET;

> +	/* Pressure measurement time */
> +	meas_time += (1 << data->oversampling_press) * BMP280_MEAS_DUR +

Ditto.

> +		      BMP280_PRESS_HUMID_MEAS_OFFSET;

> +	/* Temperature measurement time */
> +	meas_time += (1 << data->oversampling_temp) * BMP280_MEAS_DUR;

Ditto.

> +	usleep_range(meas_time, meas_time * 12 / 10);

fsleep() ?

> +	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 const u8 bmp380_operation_mode[] = { BMP380_MODE_SLEEP,
> +					    BMP380_MODE_FORCED,
> +					    BMP380_MODE_NORMAL };

As per above.

...

> +static int bmp380_wait_conv(struct bmp280_data *data)
> +{

As per above comments against bmp280_wait_conv().

> +	ret = regmap_read(data->regmap, BMP380_REG_STATUS, &reg);
> +	if (ret) {
> +		dev_err(data->dev, "failed to read status register\n");
> +		return ret;
> +	}

> +

Choose one style (with or without blank line), as in the above you have no
blank line in the similar situation.

> +	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;
> +}

...

> +		usleep_range(data->start_up_time, data->start_up_time + 500);

fsleep() ? Comment?

...

> +static const u8 bmp580_operation_mode[] = { BMP580_MODE_SLEEP,
> +					    BMP580_MODE_FORCED,
> +					    BMP580_MODE_NORMAL };

As per above.

...

> +	switch (mode) {
> +	case BMP280_SLEEP:
> +		break;
> +	case BMP280_FORCED:
> +		ret = regmap_set_bits(data->regmap, BMP580_REG_DSP_CONFIG,
> +				      BMP580_DSP_IIR_FORCED_FLUSH);
> +		if (ret) {
> +			dev_err(data->dev,
> +				"Could not flush IIR filter constants.\n");
> +			return ret;
> +		}
> +		break;
> +	case BMP280_NORMAL:
> +		break;

Can be unified with _SLEEP case.

> +	default:
> +		return -EINVAL;
> +	}

...

> +static int bmp580_wait_conv(struct bmp280_data *data)
> +{
> +	/*
> +	 * Taken from datasheet, Section 2 "Specification, Table 3 "Electrical
> +	 * characteristics

Missing period.

> +	 */
> +	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};

Please, start values on the next line after {. Also make }; to be on a separate line.

> +	int meas_time;
> +
> +	meas_time = 4000 + time_conv_temp[data->oversampling_temp] +
> +			   time_conv_press[data->oversampling_press];

4 * USEC_PER_MSEC ?

> +	usleep_range(meas_time, meas_time * 12 / 10);

Comment? fsleep() ?

> +	return 0;
> +}

...

> +	usleep_range(2500, 3000);

fsleep() ?

-- 
With Best Regards,
Andy Shevchenko