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

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <d0ed98480b600c48c36b45b84dd21285b021ee9b.camel@gmail.com>
Date:   Mon, 06 Feb 2023 00:39:08 +0100
From:   Angel Iglesias <ang.iglesiasg@...il.com>
To:     Jonathan Cameron <jic23@...nel.org>
Cc:     linux-iio@...r.kernel.org, Lars-Peter Clausen <lars@...afoo.de>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Andreas Klinger <ak@...klinger.de>, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 5/7] iio: pressure: bmp280: Add support for new
 sensor BMP580

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@...il.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@...il.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