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

[Vasileios Amoiridis <vassilisamir@gmail.com>]

On Sun, Jul 28, 2024 at 04:57:24PM +0100, Jonathan Cameron wrote:
> On Fri, 26 Jul 2024 01:10:36 +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>
> 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
> 

Hi Jonathan,

This is actually a corner case that I couldn't think of. I will have to think
it a bit more and come back on that.

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

Ack.

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

This is for the bm(p/e)(2/3/5)80 devices which actually use those functions.
Maybe instead of "future" I should have put "newer".

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

As said before, I will investigate it and come back with more info.

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