Re: [PATCH v5 3/5] iio: pressure: bmp280: Add support for BMP380 sensor family

From: Andy Shevchenko
Date: Mon Aug 08 2022 - 05:09:11 EST


On Sun, Aug 7, 2022 at 1:56 PM Angel Iglesias <ang.iglesiasg@xxxxxxxxx> 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