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

From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Angel Iglesias <ang.iglesiasg@gmail.com>
Cc: linux-iio <linux-iio@vger.kernel.org>,
	Jonathan Cameron <jic23@kernel.org>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Nikita Yushchenko <nikita.yoush@cogentembedded.com>,
	Ulf Hansson <ulf.hansson@linaro.org>,
	"Rafael J. Wysocki" <rafael.j.wysocki@intel.com>,
	Paul Cercueil <paul@crapouillou.net>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v5 3/5] iio: pressure: bmp280: Add support for BMP380 sensor family
Date: Mon, 8 Aug 2022 11:08:29 +0200	[thread overview]
Message-ID: <CAHp75VexAQ5BQEyoAJq9r8uY1MussTy12bUyLd5z6GtJSbf0NQ@mail.gmail.com> (raw)
In-Reply-To: <462251c4bca30b53f06307043ad52d859eb8d5c1.1659872590.git.ang.iglesiasg@gmail.com>

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