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

[Vasileios Amoiridis <vassilisamir@gmail.com>]

On Tue, Sep 03, 2024 at 05:26:38PM +0300, Andy Shevchenko wrote:
> 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.

Hi Andy,

Thanks for finding again the time to review this!

> 
> ...
> 
> > +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.
> 

True, will fix that.

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

ACK.

> 
> > +	/* 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.
> 

ACK.

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

ACK.

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

Why would I use && instead of || ? I just need one of the 2 to be true
(one of the 2 measurements is not complete) and I can trigger the error
action.

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

ACK.

> > +	/* 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.
> 

True, I can do that.

> > +	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).
> 

ACK.

> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Best regards,
Vasilis