| 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
| ||
|
Date: Mon, 8 Aug 2022 11:08:29 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Angel Iglesias <ang.iglesiasg@...il.com>
Cc: linux-iio <linux-iio@...r.kernel.org>,
Jonathan Cameron <jic23@...nel.org>,
Lars-Peter Clausen <lars@...afoo.de>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Nikita Yushchenko <nikita.yoush@...entembedded.com>,
Ulf Hansson <ulf.hansson@...aro.org>,
"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
Paul Cercueil <paul@...pouillou.net>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 3/5] iio: pressure: bmp280: Add support for BMP380
sensor family
On Sun, Aug 7, 2022 at 1:56 PM Angel Iglesias <ang.iglesiasg@...il.com> wrote:
>
> Adds compatibility with the new generation of this sensor, the BMP380
>
> Includes basic sensor initialization to do pressure and temp
> measurements and allows tuning oversampling settings for each channel.
>
> The compensation algorithms are adapted from the device datasheet and
> the repository https://github.com/BoschSensortec/BMP3-Sensor-API
Missed period.
...
> * https://ae-bst.resource.bosch.com/media/_tech/media/datasheets/BST-BMP180-DS000-121.pdf
> * https://ae-bst.resource.bosch.com/media/_tech/media/datasheets/BST-BMP280-DS001-12.pdf
> * https://ae-bst.resource.bosch.com/media/_tech/media/datasheets/BST-BME280_DS001-11.pdf
Seems like the above links are unresponsive now? Perhaps you may fix
them as well in a separate patch?
> + * https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmp388-ds001.pdf
...
> +/* See datasheet Section 3.11.1. */
Again, a bit of consistency in (one-line) comments, please.
> +struct bmp380_calib {
> + u16 T1;
> + u16 T2;
> + s8 T3;
> + s16 P1;
> + s16 P2;
> + s8 P3;
> + s8 P4;
> + u16 P5;
> + u16 P6;
> + s8 P7;
> + s8 P8;
> + s16 P9;
> + s8 P10;
> + s8 P11;
> +};
__packed ?
...
> +/* Send a command to BMP3XX sensors */
> + /* check if device is ready to process a command */
> + /* send command to process */
> + /* wait for 2ms for command to be processed */
> + /* check for command processing error */
Consistency, please!
...
> + dev_dbg(data->dev, "Command 0x%X processed successfully\n", cmd);
Useless.
...
> +/*
> + * Returns temperature in DegC, resolution is 0.01 DegC. Output value of
DegC?! Perhaps "Celsius degrees"?
> + * "5123" equals 51.23 DegC. t_fine carries fine temperature as global
> + * value.
> + *
> + * Taken from datasheet, Section Appendix 9, "Compensation formula" and repo
> + * https://github.com/BoschSensortec/BMP3-Sensor-API
Missed period.
> + */
...
> + return (s32) comp_temp;
Do you need casting?
...
> +/*
> + * Returns pressure in Pa as unsigned 32 bit integer in fractional Pascal.
an unsigned
32-bit
> + * Output value of "9528709" represents 9528709/100 = 95287.09 Pa = 952.8709 hPa
> + *
> + * Taken from datasheet, Section 9.3. "Pressure compensation" and repository
> + * https://github.com/BoschSensortec/BMP3-Sensor-API
Missed period.
> + */
...
> + var1 = ((s64)data->t_fine) * ((s64)data->t_fine);
Too many parentheses?
...
> + /*
> + * Dividing by 10 followed by multiplying by 10 to avoid
> + * possible overflow caused by (uncomp_data->pressure * partial_data4)
Missed period.
> + */
...
> + return (u32)comp_press;
Do you need casting?
...
> + /* reading was skipped */
The useless comment.
> + dev_err(data->dev, "reading pressure skipped\n");
...
> + /* Compensated pressure is in cPa (centipascals) */
> + *val2 = 100000;
Anything to use from units.h?
...
> + ret = regmap_write_bits(data->regmap, BMP380_REG_OSR,
> + BMP380_OSRS_TEMP_MASK | BMP380_OSRS_PRESS_MASK,
> + osrs);
> + if (ret < 0) {
Do all these ' < 0' parts make any sense?
> + }
...
> + { .compatible = "bosch,bmp380", .data = (void *)BMP380_CHIP_ID },
> { .compatible = "bosch,bme280", .data = (void *)BME280_CHIP_ID },
> { .compatible = "bosch,bmp280", .data = (void *)BMP280_CHIP_ID },
> { .compatible = "bosch,bmp180", .data = (void *)BMP180_CHIP_ID },
See below.
...
> + {"bmp380", BMP380_CHIP_ID },
> {"bmp280", BMP280_CHIP_ID },
> {"bmp180", BMP180_CHIP_ID },
> {"bmp085", BMP180_CHIP_ID },
Can we actually keep them forward-ordered? (Add 380 after 280 here and
in a separate patch sort the rest, or other way around, sort as a
prerequisite patch)
...
> +#define BMP380_MIN_TEMP -4000
> +#define BMP380_MAX_TEMP 8500
> +#define BMP380_MIN_PRES 3000000
> +#define BMP380_MAX_PRES 12500000
Units?
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists