| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20240721221156.GD325365@vamoiridPC>
Date: Mon, 22 Jul 2024 00:11:56 +0200
From: Vasileios Amoiridis <vassilisamir@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Vasileios Amoiridis <vassilisamir@...il.com>, lars@...afoo.de,
robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
andriy.shevchenko@...ux.intel.com, ang.iglesiasg@...il.com,
linus.walleij@...aro.org, biju.das.jz@...renesas.com,
javier.carrasco.cruz@...il.com, semen.protsenko@...aro.org,
579lpy@...il.com, ak@...klinger.de, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 07/10] iio: pressure: bmp280: Use sleep and forced
mode for oneshot captures
On Sat, Jul 20, 2024 at 12:28:02PM +0100, Jonathan Cameron wrote:
> On Thu, 11 Jul 2024 23:15:55 +0200
> Vasileios Amoiridis <vassilisamir@...il.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@...il.com>
> Various minor comments inline.
>
> Thanks,
>
> Jonathan
>
Hi Jonathan,
Thanks again for the amazing feedback!
My answers inline.
Cheers,
Vasilis
> > ---
> > drivers/iio/pressure/bmp280-core.c | 276 +++++++++++++++++++++++++++--
> > drivers/iio/pressure/bmp280.h | 12 ++
> > 2 files changed, 269 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> > index 9c99373d66ec..fc8d42880eb8 100644
> > --- a/drivers/iio/pressure/bmp280-core.c
> > +++ b/drivers/iio/pressure/bmp280-core.c
> > @@ -145,6 +145,12 @@ enum bmp280_scan {
> > BME280_HUMID,
> > };
> > }
> >
> > +static int bmp280_set_mode(struct bmp280_data *data, u8 mode)
> > +{
> > + int ret;
> > +
> > + switch (mode) {
> > + case BMP280_SLEEP:
> > + ret = regmap_write_bits(data->regmap, BMP280_REG_CTRL_MEAS,
> > + BMP280_MODE_MASK, BMP280_MODE_SLEEP);
>
> Use a local variable for the BMP280_MODE_* and then have the regmap_write_bits()
> after the switch statement.
>
> Could even make it a const data look up given you are getting a value
> based on the enum.
>
I like a lot both approaches, I feel like the const array one I like it more.
> > + break;
> > + case BMP280_FORCED:
> > + ret = regmap_write_bits(data->regmap, BMP280_REG_CTRL_MEAS,
> > + BMP280_MODE_MASK, BMP280_MODE_FORCED);
> > + break;
> > + case BMP280_NORMAL:
> > + ret = regmap_write_bits(data->regmap, BMP280_REG_CTRL_MEAS,
> > + BMP280_MODE_MASK, BMP280_MODE_NORMAL);
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + if (ret) {
> > + dev_err(data->dev, "failed to write ctrl_meas register\n");
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int bmp280_wait_conv(struct bmp280_data *data)
> > +{
> > + unsigned int reg;
> > + int ret, meas_time;
> > +
> > + meas_time = BMP280_MEAS_OFFSET;
> > +
> > + if (data->oversampling_humid)
> > + meas_time += (1 << data->oversampling_humid) * BMP280_MEAS_DUR +
> > + BMP280_PRESS_HUMID_MEAS_OFFSET;
> Add a comment on why, if oversampling_humid is not set we end up with
> no time for measuring humidity. The MEAS_OFFSET is less than one MEAS_DUR
> so not it's not a case of that already incorporating the time.
>
This is a check for if we use the BME280 or the BMP280 since humidity is a
feature of only the BME280 sensor. I should add it, thanks.
>
> > +
> > + /* Pressure measurement time */
> > + meas_time += (1 << data->oversampling_press) * BMP280_MEAS_DUR +
> > + BMP280_PRESS_HUMID_MEAS_OFFSET;
> > +
> > + /* Temperature measurement time */
> > + meas_time += (1 << data->oversampling_temp) * BMP280_MEAS_DUR;
> > +
> > + usleep_range(meas_time, meas_time * 12 / 10);
> > +
> > + ret = regmap_read(data->regmap, BMP280_REG_STATUS, ®);
> > + 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 bmp280_chip_config(struct bmp280_data *data)
> > {
> > u8 osrs = FIELD_PREP(BMP280_OSRS_TEMP_MASK, data->oversampling_temp + 1) |
> > @@ -994,7 +1078,7 @@ static int bmp280_chip_config(struct bmp280_data *data)
> > BMP280_OSRS_TEMP_MASK |
> > BMP280_OSRS_PRESS_MASK |
> > BMP280_MODE_MASK,
> > - osrs | BMP280_MODE_NORMAL);
> > + osrs | BMP280_MODE_SLEEP);
> > if (ret) {
> > dev_err(data->dev, "failed to write ctrl_meas register\n");
> > return ret;
> > @@ -1100,6 +1184,8 @@ const struct bmp280_chip_info bmp280_chip_info = {
> > .read_temp = bmp280_read_temp,
> > .read_press = bmp280_read_press,
> > .read_calib = bmp280_read_calib,
> > + .set_mode = bmp280_set_mode,
> > + .wait_conv = bmp280_wait_conv,
> > .preinit = bmp280_preinit,
> >
> > .trigger_handler = bmp280_trigger_handler,
> > @@ -1218,6 +1304,8 @@ const struct bmp280_chip_info bme280_chip_info = {
> > .read_press = bmp280_read_press,
> > .read_humid = bme280_read_humid,
> > .read_calib = bme280_read_calib,
> > + .set_mode = bmp280_set_mode,
> > + .wait_conv = bmp280_wait_conv,
> > .preinit = bmp280_preinit,
> >
> > .trigger_handler = bme280_trigger_handler,
> > @@ -1505,6 +1593,75 @@ static int bmp380_preinit(struct bmp280_data *data)
> > return bmp380_cmd(data, BMP380_CMD_SOFT_RESET);
> > }
> >
> > +static int bmp380_set_mode(struct bmp280_data *data, u8 mode)
> > +{
> > + int ret;
> > +
> > + switch (mode) {
> > + case BMP280_SLEEP:
> > + ret = regmap_write_bits(data->regmap, BMP380_REG_POWER_CONTROL,
> > + BMP380_MODE_MASK,
> > + FIELD_PREP(BMP380_MODE_MASK,
> > + BMP380_MODE_SLEEP));
> As above. I'd use a local variable to stash the MODE* that you are going
> to write or just look it up in a const array.
>
Ack.
> > + break;
> > + case BMP280_FORCED:
> > + ret = regmap_write_bits(data->regmap, BMP380_REG_POWER_CONTROL,
> > + BMP380_MODE_MASK,
> > + FIELD_PREP(BMP380_MODE_MASK,
> > + BMP380_MODE_FORCED));
> > + break;
> > + case BMP280_NORMAL:
> > + ret = regmap_write_bits(data->regmap, BMP380_REG_POWER_CONTROL,
> > + BMP380_MODE_MASK,
> > + FIELD_PREP(BMP380_MODE_MASK,
> > + BMP380_MODE_NORMAL));
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + if (ret) {
> > + dev_err(data->dev, "failed to write power control register\n");
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int bmp380_wait_conv(struct bmp280_data *data)
> > +{
> > + unsigned int reg;
> > + int ret, meas_time;
> > +
> > + /* Offset measurement time */
> > + meas_time = BMP380_MEAS_OFFSET;
> > +
> > + /* Pressure measurement time */
> > + meas_time += (1 << data->oversampling_press) * BMP380_MEAS_DUR +
> > + BMP380_PRESS_MEAS_OFFSET;
> > +
> > + /* Temperature measurement time */
> > + meas_time += (1 << data->oversampling_temp) * BMP380_MEAS_DUR +
> > + BMP380_TEMP_MEAS_OFFSET;
> > +
> > + usleep_range(meas_time, meas_time * 12 / 10);
> > +
> > + ret = regmap_read(data->regmap, BMP380_REG_STATUS, ®);
> > + 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)) {
> > + pr_info("Meas_time: %d\n", meas_time);
>
> Why as pr_info? Seems like part of the dev_err.
>
This is one of my forgotten "debug" messages...
> > + dev_err(data->dev, "Measurement cycle didn't complete\n");
> > + return -EBUSY;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int bmp380_chip_config(struct bmp280_data *data)
> > {
> > bool change = false, aux;
> > @@ -1565,17 +1722,15 @@ static int bmp380_chip_config(struct bmp280_data *data)
> > * Resets sensor measurement loop toggling between sleep and
> > * normal operating modes.
> > */
> > - ret = regmap_write_bits(data->regmap, BMP380_REG_POWER_CONTROL,
> > - BMP380_MODE_MASK,
> > - FIELD_PREP(BMP380_MODE_MASK, BMP380_MODE_SLEEP));
> > + ret = bmp380_set_mode(data, BMP280_SLEEP);
> > if (ret) {
> > dev_err(data->dev, "failed to set sleep mode\n");
> > return ret;
> > }
> > - usleep_range(2000, 2500);
> > - ret = regmap_write_bits(data->regmap, BMP380_REG_POWER_CONTROL,
> > - BMP380_MODE_MASK,
> > - FIELD_PREP(BMP380_MODE_MASK, BMP380_MODE_NORMAL));
> > +
> > + usleep_range(data->start_up_time, data->start_up_time + 500);
> > +
> > + ret = bmp380_set_mode(data, BMP280_NORMAL);
> > if (ret) {
> > dev_err(data->dev, "failed to set normal mode\n");
> > return ret;
> > @@ -1601,6 +1756,17 @@ static int bmp380_chip_config(struct bmp280_data *data)
> > }
> > }
> >
> > + /* Dummy read to empty data registers. */
> > + ret = bmp380_read_press(data, &tmp);
> > + if (ret)
> > + return ret;
> > +
> > + ret = bmp380_set_mode(data, BMP280_SLEEP);
> > + if (ret) {
> > + dev_err(data->dev, "failed to set sleep mode\n");
> > + return ret;
> > + }
> > +
> > return 0;
> > }
> >
> > @@ -1693,6 +1859,8 @@ const struct bmp280_chip_info bmp380_chip_info = {
> > .read_temp = bmp380_read_temp,
> > .read_press = bmp380_read_press,
> > .read_calib = bmp380_read_calib,
> > + .set_mode = bmp380_set_mode,
> > + .wait_conv = bmp380_wait_conv,
> > .preinit = bmp380_preinit,
> >
> > .trigger_handler = bmp380_trigger_handler,
> > @@ -2080,6 +2248,65 @@ static int bmp580_preinit(struct bmp280_data *data)
> > return PTR_ERR_OR_ZERO(devm_nvmem_register(config.dev, &config));
> > }
> >
> > +static int bmp580_set_mode(struct bmp280_data *data, u8 mode)
> > +{
> > + int ret;
> > +
> > + switch (mode) {
> > + case BMP280_SLEEP:
> > + ret = regmap_write_bits(data->regmap, BMP580_REG_ODR_CONFIG,
> > + BMP580_MODE_MASK,
> > + FIELD_PREP(BMP580_MODE_MASK,
> > + BMP580_MODE_SLEEP));
> > + break;
> > + case BMP280_FORCED:
> > + ret = regmap_set_bits(data->regmap, BMP580_REG_DSP_CONFIG,
> > + BMP580_DSP_IIR_FORCED_FLUSH);
> > +
> check that ret.
>
True, will do.
> > + ret = regmap_write_bits(data->regmap, BMP580_REG_ODR_CONFIG,
> > + BMP580_MODE_MASK,
> > + FIELD_PREP(BMP580_MODE_MASK,
> > + BMP580_MODE_FORCED));
> This one is more complex so a switch statement makes sense here.
> > + break;
> > + case BMP280_NORMAL:
> > + ret = regmap_write_bits(data->regmap, BMP580_REG_ODR_CONFIG,
> > + BMP580_MODE_MASK,
> > + FIELD_PREP(BMP580_MODE_MASK,
> > + BMP580_MODE_NORMAL));
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + if (ret) {
> > + dev_err(data->dev, "failed to write power control register\n");
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +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};
> > + 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;
> > +}
> > +
> one blank line only.
True, will do.
> > +
> > static int bmp580_chip_config(struct bmp280_data *data)
> > {
> > bool change = false, aux;
> > @@ -2150,17 +2377,6 @@ static int bmp580_chip_config(struct bmp280_data *data)
> > return ret;
> > }
> >
> > - /* Restore sensor to normal operation mode */
> > - ret = regmap_write_bits(data->regmap, BMP580_REG_ODR_CONFIG,
> > - BMP580_MODE_MASK,
> > - FIELD_PREP(BMP580_MODE_MASK, BMP580_MODE_NORMAL));
> > - if (ret) {
> > - dev_err(data->dev, "failed to set normal mode\n");
> > - return ret;
> > - }
> > - /* From datasheet's table 4: electrical characteristics */
> > - usleep_range(3000, 3500);
> > -
> > if (change) {
> > /*
> > * Check if ODR and OSR settings are valid or we are
> > @@ -2256,6 +2472,8 @@ const struct bmp280_chip_info bmp580_chip_info = {
> > .chip_config = bmp580_chip_config,
> > .read_temp = bmp580_read_temp,
> > .read_press = bmp580_read_press,
> > + .set_mode = bmp580_set_mode,
> > + .wait_conv = bmp580_wait_conv,
> > .preinit = bmp580_preinit,
> >
> > .trigger_handler = bmp580_trigger_handler,
> > @@ -2503,6 +2721,16 @@ static int bmp180_read_press(struct bmp280_data *data, u32 *comp_press)
> > return 0;
> > }
> >
> > +static int bmp180_set_mode(struct bmp280_data *data, u8 mode)
> > +{
> > + return 0;
> Add a comment on why these are stubs. It's in the patch description, but
> better to have it recorded in the code.
>
I didn't add because as you can see below there is another function exactly
like this one that doesn't have one. Should I add also to the other one?
> > +}
> > +
> > +static int bmp180_wait_conv(struct bmp280_data *data)
> > +{
> > + return 0;
> > +}
> > +
> >
Powered by blists - more mailing lists