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

[Angel Iglesias]

On lun, 2022-08-08 at 11:08 +0200, Andy Shevchenko wrote:
> 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?

Sure. Should I include this patch in this next version of this series, or should
I send the patch as a standalone change?

> > + *
> > 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?

Yup, operator precedence of typecast precedes multiplication.

> ...
> 
> > +       /*
> > +        * 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?

Not really, should fall on the implicit conversion rules of the compiler as it
works like an assignment, right?. Also, after shifting 40 bits to the right,
there's no risk of overflow, comp_press can be a u32 from the start instead of a
u64.

> ...
> 
> > +               /* 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?

I've checked regmap calls and return 0 when call is successful, so the answer is
no

> > +       }
> 
> ...
> 
> > +       { .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)

Sure!

> ...
> 
> > +#define BMP380_MIN_TEMP                        -4000
> > +#define BMP380_MAX_TEMP                        8500
> > +#define BMP380_MIN_PRES                        3000000
> > +#define BMP380_MAX_PRES                        12500000
> 
> Units?

Units as in I should use units.h constants or also comment in which units are
these constants?

> 
> --
> With Best Regards,
> Andy Shevchenko

Kind regards,
Angel