Re: [PATCH v3 5/7] iio: pressure: bmp280: Add support for new sensor BMP580

[Angel Iglesias <ang.iglesiasg@gmail.com>]

On Sun, 2023-02-05 at 14:53 +0000, Jonathan Cameron wrote:
> On Sun, 29 Jan 2023 02:33:07 +0100
> Angel Iglesias <ang.iglesiasg@gmail.com> wrote:
> 
> > Adds compatibility with the new sensor generation, the BMP580.
> > 
> > The measurement and initialization codepaths are adapted from
> > the device datasheet and the repository from manufacturer at
> > https://github.com/boschsensortec/BMP5-Sensor-API.
> > 
> > Signed-off-by: Angel Iglesias <ang.iglesiasg@gmail.com>
> > 
> 
> Hi Angel,
> 
> As you are doing one more version anyway, a few really minor comments inline.
> 
> Thanks,
> 
> Jonathan
> 
> > diff --git a/drivers/iio/pressure/bmp280-core.c
> > b/drivers/iio/pressure/bmp280-core.c
> > index 22addaaa5393..c65fb4025ad9 100644
> > --- a/drivers/iio/pressure/bmp280-core.c
> > +++ b/drivers/iio/pressure/bmp280-core.c
> 
> >  /*
> >   * These enums are used for indexing into the array of compensation
> >   * parameters for BMP280.
> > @@ -1216,6 +1252,303 @@ const struct bmp280_chip_info bmp380_chip_info = {
> >  };
> >  EXPORT_SYMBOL_NS(bmp380_chip_info, IIO_BMP280);
> >  
> > +/*
> > + * BMP5xx soft reset procedure
> 
> Wild cards are often a bad idea, even in comments.  Tend to end up covering
> some device that works differently.  With that in mind, not sure this comment
> adds anything over the function name.
> 
> > + */
> > +static int bmp580_soft_reset(struct bmp280_data *data)
> > +{
> > +       unsigned int reg;
> > +       int ret;
> > +
> > +       /* Write reset word to CMD register */
> Not that informative as comments go.

Understood!

> 
> > +       ret = regmap_write(data->regmap, BMP580_REG_CMD,
> > BMP580_CMD_SOFT_RESET);
> > +       if (ret) {
> > +               dev_err(data->dev, "failed to send reset command to
> > device\n");
> > +               return ret;
> > +       }
> > +       /* Wait 2ms for reset completion */
> nor is this one - drop them both.
> > +       usleep_range(2000, 2500);
> > +
> > +       /* Dummy read of chip_id */
> Now this one is good as not obvious why read is here so keep it!
> > +       ret = regmap_read(data->regmap, BMP580_REG_CHIP_ID, &reg);
> > +       if (ret) {
> > +               dev_err(data->dev, "failed to reestablish comms after
> > reset\n");
> > +               return ret;
> > +       }
> > +
> > +       /* Check if POR bit is set on interrupt reg */
> Not sure the comment adds anything not obviously from code.  I'd be inclined
> to drop it.
> > +       ret = regmap_read(data->regmap, BMP580_REG_INT_STATUS, &reg);
> > +       if (ret) {
> > +               dev_err(data->dev, "error reading interrupt status
> > register\n");
> > +               return ret;
> > +       }
> > +       if (!(reg & BMP580_INT_STATUS_POR_MASK)) {
> > +               dev_err(data->dev, "error resetting sensor\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +/*
> > + * Contrary to previous sensors families, compensation algorithm is
> > builtin.
> > + * We are only required to read the register raw data and adapt the ranges
> > + * for what is expected on IIO ABI.
> > + */
> > +
> > +static int bmp580_read_temp(struct bmp280_data *data, int *val)
> > +{
> > +       s32 raw_temp;
> > +       int ret;
> > +
> > +       ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, data-
> > >buf,
> > +                              sizeof(data->buf));
> > +       if (ret) {
> > +               dev_err(data->dev, "failed to read temperature\n");
> > +               return ret;
> > +       }
> > +
> > +       raw_temp = get_unaligned_le24(data->buf);
> > +       if (raw_temp == BMP580_TEMP_SKIPPED) {
> > +               dev_err(data->dev, "reading temperature skipped\n");
> > +               return -EIO;
> > +       }
> > +
> > +       /*
> > +        * Temperature is returned in Celsius degrees in fractional
> > +        * form down 2^16. We reescale by x1000 to return milli Celsius
> > +        * to respect IIO ABI.
> > +        */
> > +       *val = (raw_temp * 1000) >> 16;
> 
> Why not use IIO_VAL_FRACTION_LOG2 and keep the precision?

Although this sensor has a resolution of 1/2^16, its absolute accuracy is of
+/- 0.5ºC so I suppose in the end we aren't really losing precision. But in the
future a high accuracy variant of the sensor might pop up (like the bmp384 in
the previous gen) so I think is a good idea to keep the precision. Thanks for
the heads up!

> > +       return IIO_VAL_INT;
> > +}
> 
> 
Thanks for your time!
Angel