| 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: <ZtBqNAYlZSEhd_20@smile.fi.intel.com>
Date: Thu, 29 Aug 2024 15:31:48 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Vasileios Amoiridis <vassilisamir@...il.com>
Cc: jic23@...nel.org, lars@...afoo.de, robh@...nel.org, krzk+dt@...nel.org,
conor+dt@...nel.org, 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,
christophe.jaillet@...adoo.fr
Subject: Re: [PATCH v4 4/7] iio: pressure: bmp280: Use sleep and forced mode
for oneshot captures
On Wed, Aug 28, 2024 at 10:51:24PM +0200, Vasileios Amoiridis wrote:
> This commit adds forced mode support in sensors BMP28x, BME28x, BMP3xx
s/This commit, adds/Add/
The imperative mode is documented in Submitting Patches.
> 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
Déjà-vu feeling... Ah, first line is the same!
> 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 int bmp280_wait_conv(struct bmp280_data *data)
> +{
> + unsigned int reg;
> + int ret, meas_time;
Why meas_time is signed?
Also, please name it with a unit suffix
unsigned int meas_time_us;
(and check the rest of the patch for the similar).
> +
> +
A single blank line is enough. Also check all patches for this.
> + /* Check if we are using a BME280 device */
> + if (data->oversampling_humid)
> + meas_time += BIT(data->oversampling_humid) * BMP280_MEAS_DUR +
> + BMP280_PRESS_HUMID_MEAS_OFFSET;
Indentation issue, the same seems in all of similar expressions in this patch.
Also play with this form and check if it looks better
meas_time += BMP280_PRESS_HUMID_MEAS_OFFSET +
BIT(data->oversampling_humid) * BMP280_MEAS_DUR;
(at least I found it better to read as first we apply constants, followed by
longer variable-based calculations).
> + /* Pressure measurement time */
> + meas_time += BIT(data->oversampling_press) * BMP280_MEAS_DUR +
> + BMP280_PRESS_HUMID_MEAS_OFFSET;
> +
> + /* Temperature measurement time */
> + meas_time += BIT(data->oversampling_temp) * BMP280_MEAS_DUR;
> +
> + /* Waiting time according to the BM(P/E)2 Sensor API */
> + fsleep(meas_time);
> +
> + 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;
> +}
...
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;
This _feels_ like a separate change. I haven't found anything explicitly
describing it in the commit message. Did I miss it?
...
> + /*
> + * According to the BMP3 Sensor API, the sensor needs 5000ms
I believe it's a typo in unit suffix.
If not, this should be very well described to explain why 5 seconds is needed.
> + * in order to go to the sleep mode.
> + */
> + fsleep(5000);
...
> +{
> + 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(data->dev,
> + "Could not flush IIR filter constants.\n");
Temporary variable for data->dev?
> + return ret;
> + }
> + break;
> + default:
> + 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(data->dev, "failed to write power control register\n");
> + return ret;
> + }
> +
> + data->op_mode = mode;
> +
> + 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
> + };
Mind the comma at the end.
And in programming hardware we quite often operate with power-of-2 things, so I
recommend to have 8 per line,
static const int time_conv_press[] = {
0, 1050, 1785, 3045, 5670, 10920, 21420, 42420, /* 0-7 */
84420, /* 8 */
};
> + static const int time_conv_temp[] = {
> + 0, 1050, 1105, 1575, 2205, 3465, 6090, 11340, 21840
> + };
Ditto.
> +
Stray blank line. This is a definition block, we don't need blank lines in it.
> + int meas_time;
> +
> + meas_time = 4 * USEC_PER_MSEC + time_conv_temp[data->oversampling_temp]
> + + time_conv_press[data->oversampling_press];
> +
> + /* Measurement time mentioned in Chapter 2, Table 4 of the datasheet. */
> + fsleep(meas_time);
> +
> + return 0;
> +}
...
> /* From datasheet's table 4: electrical characteristics */
With this change the comment seems odd. Can you elaborate more?
> - usleep_range(2500, 3000);
> + fsleep(data->start_up_time + 500);
Also, can we name it start_up_time_us?
It's fine to postpone renaming if it takes too many unrelated changes.
...
> + usleep_range(2500, 3000);
fsleep()? Comment?
...
> usleep_range(data->start_up_time, data->start_up_time + 100);
This is already in the code, but maybe switching to fsleep() and adding
a comment to explain how it's calculated (based on the spec? Reference?),
so in a separate change?
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists