lkml.org 
[lkml]   [2023]   [Feb]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v3 5/7] iio: pressure: bmp280: Add support for new sensor BMP580
    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.

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

    > + return IIO_VAL_INT;
    > +}


    \
     
     \ /
      Last update: 2023-03-27 00:10    [W:2.167 / U:0.344 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site