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

[Vasileios Amoiridis]

On Sat, Oct 12, 2024 at 05:03:33PM +0100, Jonathan Cameron wrote:
> On Mon,  7 Oct 2024 21:49:42 +0200
> Vasileios Amoiridis <vassilisamir@xxxxxxxxx> 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.
> > 
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@xxxxxxxxx>
> Hi Vasilieos
> 
> Given it looks like you'll be doing a v9 anyway, a few comments inline
> on some minor simplifications and potential readability improvements.
> 
> Thanks,
> 
> Jonathan
> 

Hi Jonathan,

> 
> > ---
> >  drivers/iio/pressure/bmp280-core.c | 296 +++++++++++++++++++++++++++--
> >  drivers/iio/pressure/bmp280.h      |  21 ++
> >  2 files changed, 296 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> > index 6811619c6f11..9ad29cf4c2ac 100644
> > --- a/drivers/iio/pressure/bmp280-core.c
> > +++ b/drivers/iio/pressure/bmp280-core.c
> > @@ -16,6 +16,11 @@
> 
> 
> > @@ -1522,6 +1610,71 @@ static int bmp380_preinit(struct bmp280_data *data)
> >  	return bmp380_cmd(data, BMP380_CMD_SOFT_RESET);
> >  }
> >  
> > +static const u8 bmp380_operation_mode[] = {
> > +	BMP380_MODE_SLEEP, BMP380_MODE_FORCED, BMP380_MODE_NORMAL,
> > +};
> 
> As below - I'd assign these to specific entries to make the fairly obvious association
> even more obvious!
> 

ACK! Makes total sense. Thanks for your time!

Cheers,
Vasilis

> > +
> > +static int bmp380_set_mode(struct bmp280_data *data, enum bmp280_op_mode mode)
> > +{
> > +	int ret;
> > +
> > +	switch (mode) {
> > +	case BMP280_SLEEP:
> > +	case BMP280_FORCED:
> > +	case BMP280_NORMAL:
> > +		break;
> > +	default:
> > +		return -EINVAL;
> 
> Currently there aren't others. So the compiler should shout if you try to pass
> something else. Hence this check shouldn't be needed.
> 
> > +	}
> > +
> > +	ret = regmap_write_bits(data->regmap, BMP380_REG_POWER_CONTROL,
> > +				BMP380_MODE_MASK,
> > +				FIELD_PREP(BMP380_MODE_MASK,
> > +					   bmp380_operation_mode[mode]));
> > +	if (ret) {
> > +		dev_err(data->dev, "failed to  write power control register.\n");
> > +		return ret;
> > +	}
> > +
> > +	data->op_mode = mode;
> > +
> > +	return 0;
> > +}
> >
> 
> >  	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,
> 
> For these, explicit setting will make it more obvious.
> 	[BMP280_SLEEP] = BMP580_MODE_SLEEP,
> etc
> 
> > +};
> > +
> > +static int bmp580_set_mode(struct bmp280_data *data, enum bmp280_op_mode mode)
> > +{
> > +	struct device *dev = data->dev;
> > +	int ret;
> > +
> > +	switch (mode) {
> > +	case BMP280_SLEEP:
> > +	case BMP280_NORMAL:
> > +		break;
> > +	case BMP280_FORCED:
> > +		ret = regmap_set_bits(data->regmap, BMP580_REG_DSP_CONFIG,
> > +				      BMP580_DSP_IIR_FORCED_FLUSH);
> > +		if (ret) {
> > +			dev_err(dev, "Could not flush IIR filter constants.\n");
> > +			return ret;
> > +		}
> > +		break;
> > +	default:
> There are only the values above, and we should hopefully be able to rely
> on compiler warnings to shout at us if a future modification adds more.
> 
> So should be able to drop the default here.
> 
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = regmap_write_bits(data->regmap, BMP580_REG_ODR_CONFIG,
> > +				BMP580_MODE_MASK,
> > +				FIELD_PREP(BMP580_MODE_MASK,
> > +					   bmp580_operation_mode[mode]));
> > +	if (ret) {
> > +		dev_err(dev, "failed to  write power control register.\n");
> > +		return ret;
> > +	}
> > +
> > +	data->op_mode = mode;
> > +
> > +	return 0;
> > +}
>