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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On sáb, 2022-06-25 at 16:46 +0100, Jonathan Cameron wrote:
> On Sat, 25 Jun 2022 17:09:12 +0200
> Angel Iglesias <ang.iglesiasg@xxxxxxxxx> wrote:
> 
> > Adds compatibility with the new generation of this sensor, the
> > BMP380
> > 
> > Included 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
> > 
> > Signed-off-by: Angel Iglesias <ang.iglesiasg@xxxxxxxxx>
> 
> Hi Angel,
> 
> A few comments inline, but mostly looks good to me.
> 
> Jonathan
> 
> > ---
> >  drivers/iio/pressure/bmp280-core.c   | 378
> > ++++++++++++++++++++++++++-
> >  drivers/iio/pressure/bmp280-i2c.c    |   5 +
> >  drivers/iio/pressure/bmp280-regmap.c |  55 ++++
> >  drivers/iio/pressure/bmp280-spi.c    |   5 +
> >  drivers/iio/pressure/bmp280.h        | 118 +++++++++
> >  5 files changed, 558 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/iio/pressure/bmp280-core.c
> > b/drivers/iio/pressure/bmp280-core.c
> > index bf8167f43c56..3887475a9059 100644
> > --- a/drivers/iio/pressure/bmp280-core.c
> > +++ b/drivers/iio/pressure/bmp280-core.c
> > @@ -74,6 +74,24 @@ struct bmp280_calib {
> >         s8  H6;
> >  };
> >  
> > +/* See datasheet Section 3.11.1. */
> > +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;
> > +};
> > +
> >  static const char *const bmp280_supply_names[] = {
> >         "vddd", "vdda"
> >  };
> > @@ -90,6 +108,7 @@ struct bmp280_data {
> >         union {
> >                 struct bmp180_calib bmp180;
> >                 struct bmp280_calib bmp280;
> > +               struct bmp380_calib bmp380;
> >         } calib;
> >         struct regulator_bulk_data supplies[BMP280_NUM_SUPPLIES];
> >         unsigned int start_up_time; /* in microseconds */
> > @@ -129,6 +148,25 @@ struct bmp280_chip_info {
> >  enum { T1, T2, T3 };
> >  enum { P1, P2, P3, P4, P5, P6, P7, P8, P9 };
> >  
> > +enum {
> > +       /* Temperature calib indexes */
> > +       BMP380_T1 = 0,
> > +       BMP380_T2 = 2,
> > +       BMP380_T3 = 4,
> > +       /* Pressure calib indexes */
> > +       BMP380_P1 = 5,
> > +       BMP380_P2 = 7,
> > +       BMP380_P3 = 9,
> > +       BMP380_P4 = 10,
> > +       BMP380_P5 = 11,
> > +       BMP380_P6 = 13,
> > +       BMP380_P7 = 15,
> > +       BMP380_P8 = 16,
> > +       BMP380_P9 = 17,
> > +       BMP380_P10 = 19,
> > +       BMP380_P11 = 20
> > +};
> > +
> >  static const struct iio_chan_spec bmp280_channels[] = {
> >         {
> >                 .type = IIO_PRESSURE,
> > @@ -252,6 +290,7 @@ static int bmp280_read_calib(struct bmp280_data
> > *data,
> >  
> >         return 0;
> >  }
> > +
> Stray change. It's a good one, but doesn't belong in this patch. 
> Feel free to
> do another patch tidying up whitespace in the driver.

Thanks! I'll drop this change for the next version of this series.

> >  /*
> >   * Returns humidity in percent, resolution is 0.01 percent. Output
> > value of
> >   * "47445" represents 47445/1024 = 46.333 %RH.
> > @@ -685,6 +724,302 @@ static const struct bmp280_chip_info
> > bme280_chip_info = {
> >         .read_humid = bmp280_read_humid,
> >  };
> >  
> > +/* Send a command to BMP3XX sensors */
> > +static int bmp380_cmd(struct bmp280_data *data, u8 cmd)
> > +{
> > +       int ret;
> > +       unsigned int reg;
> > +
> > +       /* check if device is ready to process a command */
> > +       ret = regmap_read(data->regmap, BMP380_REG_STATUS, &reg);
> > +       if (ret) {
> > +               dev_err(data->dev, "failed to read error
> > register\n");
> > +               goto failure;
> > +       }
> > +       if (!(cmd & BMP380_STATUS_CMD_RDY_MASK)) {
> > +               dev_err(data->dev, "device is not ready to accept
> > commands\n");
> > +               ret = -EBUSY;
> > +               goto failure;
> > +       }
> > +
> > +       /* send command to process */
> > +       ret = regmap_write(data->regmap, BMP380_REG_CMD, cmd);
> > +       if (ret) {
> > +               dev_err(data->dev, "failed to send command to
> > device\n");
> > +               goto failure;
> > +       }
> > +       /* wait for 2ms for command to be proccessed */
> > +       usleep_range(2000, 2500);
> > +       /* check for command processing error */
> > +       ret = regmap_read(data->regmap, BMP380_REG_ERROR, &reg);
> > +       if (ret) {
> > +               dev_err(data->dev, "error reading ERROR reg\n");
> > +               goto failure;
> > +       }
> > +       if (reg & BMP380_ERR_CMD_MASK) {
> > +               dev_err(data->dev, "error processing command
> > 0x%X\n", cmd);
> > +               ret = -EINVAL;
> 
> as nothing to do in failure path, direct return from here preferred.
>                 return -EINVAL; 
> Same for all similar cases.

Thanks! I'll avoid unnecessary goto's when there's no further actions
to do after a failure

> > +               goto failure;
> > +       }
> > +       dev_dbg(data->dev, "Command 0x%X proccessed
> > successfully\n", cmd);
> > +
> > +failure:
> > +       return ret;
> > +}
> > +
> > +/*
> > + * Returns temperature in DegC, resolution is 0.01 DegC.  Output
> > value of
> > + * "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
> > + */
> > +static s32 bmp380_compensate_temp(struct bmp280_data *data, u32
> > adc_temp)
> > +{
> > +       s64 var1, var2, var3, var4, var5, var6, comp_temp;
> > +       struct bmp380_calib *calib = &data->calib.bmp380;
> > +
> > +       var1 = ((s64) adc_temp) - (((s64) calib->T1) << 8);
> > +       var2 = var1 * ((s64) calib->T2);
> > +       var3 = var1 * var1;
> > +       var4 = var3 * ((s64) calib->T3);
> > +       var5 = (var2 << 18) + var4;
> > +       var6 = var5 >> 32;
> > +       data->t_fine = (s32) var6;
> > +       comp_temp = (var6 * 25) >> 14;
> > +
> > +       comp_temp = clamp_val(comp_temp, BMP380_MIN_TEMP,
> > BMP380_MAX_TEMP);
> > +       return (s32) comp_temp;
> > +}
> > +
> > +/*
> > + * Returns pressure in Pa as unsigned 32 bit integer in fractional
> > Pascal.
> > + * 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
> > + */
> > +static u32 bmp380_compensate_press(struct bmp280_data *data, u32
> > adc_press)
> > +{
> > +       s64 var1, var2, var3, var4, var5, var6, offset,
> > sensitivity;
> > +       u64 comp_press;
> > +       struct bmp380_calib *calib = &data->calib.bmp380;
> > +
> > +       var1 = ((s64)data->t_fine) * ((s64)data->t_fine);
> > +       var2 = var1 >> 6;
> > +       var3 = (var2 * ((s64) data->t_fine)) >> 8;
> > +       var4 = (((s64)calib->P8) * var3) >> 5;
> > +       var5 = (((s64) calib->P7) * var1) << 4;
> > +       var6 = (((s64) calib->P6) * ((s64)data->t_fine)) << 22;
> > +       offset = (((s64)calib->P5) << 47) + var4 + var5 + var6;
> > +       var2 = (((s64)calib->P4) * var3) >> 5;
> > +       var4 = (((s64) calib->P3) * var1) << 2;
> > +       var5 = (((s64) calib->P2) - ((s64) 1<<14)) *
> > +               (((s64)data->t_fine) << 21);
> > +       sensitivity = ((((s64) calib->P1) - ((s64) 1 << 14)) << 46)
> > +
> > +                       var2 + var4 + var5;
> > +       var1 = (sensitivity >> 24) * ((s64)adc_press);
> > +       var2 = ((s64)calib->P10) * ((s64) data->t_fine);
> > +       var3 = var2 + (((s64) calib->P9) << 16);
> > +       var4 = (var3 * ((s64)adc_press)) >> 13;
> > +
> > +       /* dividing by 10 followed by multiplying by 10
> > +        * To avoid overflow caused by (uncomp_data->pressure *
> > partial_data4)
> > +        */
> > +       var5 = (((s64)adc_press) * (var4 / 10)) >> 9;
> > +       var5 *= 10;
> > +       var6 = ((s64)adc_press) * ((s64)adc_press);
> > +       var2 = (((s64)calib->P11) * var6) >> 16;
> > +       var3 = (var2 * ((s64)adc_press)) >> 7;
> > +       var4 = (offset >> 2) + var1 + var5 + var3;
> > +       comp_press = ((u64)var4 * 25) >> 40;
> > +
> > +       comp_press = clamp_val(comp_press, BMP380_MIN_PRES,
> > BMP380_MAX_PRES);
> > +       return (u32)comp_press;
> > +}
> > +
> > +static int bmp380_read_temp(struct bmp280_data *data, int *val)
> > +{
> > +       int ret;
> > +       __le32 tmp = 0;
> 
> It's not an 32 bits, so use an array of 3 bytes instead.

Understood!

> > +       u32 adc_temp;
> > +       s32 comp_temp;
> > +
> > +       ret = regmap_bulk_read(data->regmap, BMP380_REG_TEMP_XLSB,
> > &tmp, 3);
> > +       if (ret < 0) {
> > +               dev_err(data->dev, "failed to read temperature\n");
> > +               return ret;
> > +       }
> > +
> > +       adc_temp = le32_to_cpu(tmp);
> As below, get_unaligned_le24()

Ok! Thanks, I'll fix all le32_to_cpu() missuses

> > +       if (adc_temp == BMP380_TEMP_SKIPPED) {
> > +               /* reading was skipped */
> > +               dev_err(data->dev, "reading temperature
> > skipped\n");
> > +               return -EIO;
> > +       }
> > +       comp_temp = bmp380_compensate_temp(data, adc_temp);
> > +
> > +       /*
> > +        * val might be NULL if we're called by the read_press
> > routine,
> > +        * who only cares about the carry over t_fine value.
> > +        */
> > +       if (val) {
> > +               /* IIO reports temperatures in mC */
> > +               *val = comp_temp * 10;
> > +               return IIO_VAL_INT;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int bmp380_read_press(struct bmp280_data *data, int *val,
> > int *val2)
> > +{
> > +       int ret;
> > +       __le32 tmp = 0;
> > +       u32 adc_press;
> > +       s32 comp_press;
> > +
> > +       /* Read and compensate temperature so we get a reading of
> > t_fine. */
> > +       ret = bmp380_read_temp(data, NULL);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       ret = regmap_bulk_read(data->regmap, BMP380_REG_PRESS_XLSB,
> > &tmp, 3);
> > +       if (ret < 0) {
> > +               dev_err(data->dev, "failed to read pressure\n");
> > +               return ret;
> > +       }
> > +
> > +       adc_press = le32_to_cpu(tmp);
> only 3 bytes read, so this is le24.  We have conversion functions for
> that.
> get_unaligned_le24()
> 
> 
> > +       if (adc_press == BMP380_PRESS_SKIPPED) {
> > +               /* reading was skipped */
> > +               dev_err(data->dev, "reading pressure skipped\n");
> > +               return -EIO;
> > +       }
> > +       comp_press = bmp380_compensate_press(data, adc_press);
> > +
> > +       *val = comp_press;
> > +       /* Compensated pressure is in cPa (centipascals) */
> > +       *val2 = 100000;
> > +
> > +       return IIO_VAL_FRACTIONAL;
> > +}
> > +
> > +static int bmp380_read_calib(struct bmp280_data *data,
> > +                            struct bmp380_calib *calib, unsigned
> > int chip)
> > +{
> > +       int ret;
> > +       u8 buf[BMP380_CALIB_REG_COUNT];
> > +
> > +       /* Read temperature calibration values. */
> > +       ret = regmap_bulk_read(data->regmap,
> > BMP380_REG_CALIB_TEMP_START, buf,
> > +                              BMP380_CALIB_REG_COUNT);
> > +       if (ret < 0) {
> > +               dev_err(data->dev,
> > +                       "failed to read temperature calibration
> > parameters\n");
> > +               return ret;
> > +       }
> > +
> > +       /* Toss the temperature calibration data into the entropy
> > pool */
> > +       add_device_randomness(buf, sizeof(buf));
> > +
> > +       /* Parse calibration data */
> > +       calib->T1 = le16_from_bytes(buf[BMP380_T1], buf[BMP380_T1 +
> > 1]);
> 
> This looks like a call to get_unaligned_le16() or similar. Use that
> instead.

Thanks! This macro was a bit of a mess. I wasn't aware of the
get_unaligned_* helpers

> > +       calib->T2 = le16_from_bytes(buf[BMP380_T2], buf[BMP380_T2 +
> > 1]);
> > +       calib->T3 = buf[BMP380_T3];
> > +       calib->P1 = le16_from_bytes(buf[BMP380_P1], buf[BMP380_P1 +
> > 1]);
> > +       calib->P2 = le16_from_bytes(buf[BMP380_P2], buf[BMP380_P2 +
> > 1]);
> > +       calib->P3 = buf[BMP380_P3];
> > +       calib->P4 = buf[BMP380_P4];
> > +       calib->P5 = le16_from_bytes(buf[BMP380_P5], buf[BMP380_P5 +
> > 1]);
> > +       calib->P6 = le16_from_bytes(buf[BMP380_P6], buf[BMP380_P6 +
> > 1]);
> > +       calib->P7 = buf[BMP380_P7];
> > +       calib->P8 = buf[BMP380_P8];
> > +       calib->P9 = le16_from_bytes(buf[BMP380_P9], buf[BMP380_P9 +
> > 1]);
> > +       calib->P10 = buf[BMP380_P10];
> > +       calib->P11 = buf[BMP380_P11];
> > +
> > +       return 0;
> > +}
> > +
> > +static int bmp380_chip_config(struct bmp280_data *data)
> > +{
> > +       u8 osrs;
> > +       unsigned int tmp;
> > +       int ret;
> > +
> > +       /* configure power control register */
> > +       ret = regmap_write_bits(data->regmap,
> > BMP380_REG_POWER_CONTROL,
> > +                               BMP380_CTRL_SENSORS_MASK |
> > +                               BMP380_MODE_MASK,
> > +                               BMP380_CTRL_SENSORS_PRESS_EN |
> > +                               BMP380_CTRL_SENSORS_TEMP_EN |
> > +                               BMP380_MODE_NORMAL);
> > +       if (ret < 0) {
> > +               dev_err(data->dev,
> > +                       "failed to write operation control
> > register\n");
> > +               return ret;
> > +       }
> > +
> > +       /* configure oversampling */
> > +       osrs = BMP380_OSRS_TEMP_X(data->oversampling_temp) |
> > +                               BMP380_OSRS_PRESS_X(data-
> > >oversampling_press);
> > +
> > +       ret = regmap_write_bits(data->regmap, BMP380_REG_OSR,
> > +                               BMP380_OSRS_TEMP_MASK |
> > BMP380_OSRS_PRESS_MASK,
> > +                               osrs);
> > +       if (ret < 0) {
> > +               dev_err(data->dev, "failed to write oversampling
> > register\n");
> > +               return ret;
> > +       }
> > +
> > +       /* configure output data rate */
> > +       ret = regmap_write_bits(data->regmap, BMP380_REG_ODR,
> > +                               BMP380_ODRS_MASK,
> > BMP380_ODRS_50HZ);
> > +       if (ret < 0) {
> > +               dev_err(data->dev, "failed to write ODR selection
> > register\n");
> > +               return ret;
> > +       }
> > +
> > +       /* set filter data */
> > +       ret = regmap_update_bits(data->regmap, BMP380_REG_CONFIG,
> > +                               BMP380_FILTER_MASK,
> > BMP380_FILTER_3X);
> > +       if (ret < 0) {
> > +               dev_err(data->dev, "failed to write config
> > register\n");
> > +               return ret;
> > +       }
> > +
> > +       /* check config error flag */
> > +       ret = regmap_read(data->regmap, BMP380_REG_ERROR, &tmp);
> > +       if (ret < 0) {
> > +               dev_err(data->dev,
> > +                       "failed to read error register\n");
> > +               return ret;
> > +       }
> > +       if (tmp && BMP380_ERR_CONF_MASK) {
> > +               dev_warn(data->dev,
> > +                        "sensor flagged configuration as
> > incompatible\n");
> > +               ret = -EINVAL;
>                 return -EINVAL;
> > +       }
> > +
> 
> return 0;

Ok!

> > +       return ret;
> > +}
> > +
> > +static const int bmp380_oversampling_avail[] = { 1, 2, 4, 8, 16,
> > 32 };
> > +
> > +static const struct bmp280_chip_info bmp380_chip_info = {
> > +       .oversampling_temp_avail = bmp380_oversampling_avail,
> > +       .num_oversampling_temp_avail =
> > ARRAY_SIZE(bmp380_oversampling_avail),
> > +
> > +       .oversampling_press_avail = bmp380_oversampling_avail,
> > +       .num_oversampling_press_avail =
> > ARRAY_SIZE(bmp380_oversampling_avail),
> > +
> > +       .chip_config = bmp380_chip_config,
> > +       .read_temp = bmp380_read_temp,
> > +       .read_press = bmp380_read_press,
> > +};
> > +
> >  static int bmp180_measure(struct bmp280_data *data, u8 ctrl_meas)
> >  {
> >         int ret;
> > @@ -1032,6 +1367,13 @@ int bmp280_common_probe(struct device *dev,
> >                 data->oversampling_temp = ilog2(2);
> >                 data->start_up_time = 2000;
> >                 break;
> > +       case BMP380_CHIP_ID:
> > +               indio_dev->num_channels = 2;
> > +               data->chip_info = &bmp380_chip_info;
> > +               data->oversampling_press = ilog2(4);
> > +               data->oversampling_temp = ilog2(1);
> > +               data->start_up_time = 2000;
> 
> 
> We should put the default values + num_channels into the chip_info
> structure so all this switch would do is pick the right chip_info
> structure.
> 
> Everything else is just copies from that structure done
> unconditionally
> with no need to duplicate similar lines like here.

Yes, this block would be much cleaner doing this simple change. Thanks
for the input!

> > +               break;
> >         default:
> >                 return -EINVAL;
> >         }
> > @@ -1071,7 +1413,18 @@ int bmp280_common_probe(struct device *dev,
> >         }
> >  
> >         data->regmap = regmap;
> > -       ret = regmap_read(regmap, BMP280_REG_ID, &chip_id);
> > +       switch (chip) {
> > +       case BMP180_CHIP_ID:
> 
> Why not put the address into the chip info structure?  That way
> we avoid the switch statement.

Ok!

> > +       case BMP280_CHIP_ID:
> > +       case BME280_CHIP_ID:
> > +               ret = regmap_read(regmap, BMP280_REG_ID, &chip_id);
> > +               break;
> > +       case BMP380_CHIP_ID:
> > +               ret = regmap_read(regmap, BMP380_REG_ID, &chip_id);
> > +               break;
> > +       default:
> > +               ret = -EINVAL;
> > +       }
> >         if (ret < 0)
> >                 return ret;
> >         if (chip_id != chip) {
> > @@ -1080,6 +1433,13 @@ int bmp280_common_probe(struct device *dev,
> >                 return -EINVAL;
> >         }
> >  
> > +       /* BMP3xx requires soft-reset as part of initialization */
> > +       if (chip_id == BMP380_CHIP_ID) {
> > +               ret = bmp380_cmd(data, BMP380_CMD_SOFT_RESET);
> > +               if (ret < 0)
> > +                       return ret;
> > +       }
> > +
> >         ret = data->chip_info->chip_config(data);
> >         if (ret < 0)
> >                 return ret;
> > @@ -1091,20 +1451,32 @@ int bmp280_common_probe(struct device *dev,
> >          * non-volatile memory during production". Let's read them
> > out at probe
> >          * time once. They will not change.
> >          */
> > -       if (chip_id  == BMP180_CHIP_ID) {
> > +       switch (chip_id) {
> > +       case BMP180_CHIP_ID:
> I would move these into callbacks in chip_info. No need for a switch
> statement
> here then as you just call the right one via chip_info->read_calib()

Ok!

> >                 ret = bmp180_read_calib(data, &data->calib.bmp180);
> >                 if (ret < 0) {
> >                         dev_err(data->dev,
> >                                 "failed to read calibration
> > coefficients\n");
> >                         return ret;
> >                 }
> > -       } else if (chip_id == BMP280_CHIP_ID || chip_id ==
> > BME280_CHIP_ID) {
> > +               break;
> > +       case BMP280_CHIP_ID:
> > +       case BME280_CHIP_ID:
> >                 ret = bmp280_read_calib(data, &data->calib.bmp280,
> > chip_id);
> >                 if (ret < 0) {
> >                         dev_err(data->dev,
> >                                 "failed to read calibration
> > coefficients\n");
> >                         return ret;
> >                 }
> > +               break;
> > +       case BMP380_CHIP_ID:
> > +               ret = bmp380_read_calib(data, &data->calib.bmp380,
> > chip_id);
> > +               if (ret < 0) {
> > +                       dev_err(data->dev,
> > +                               "failed to read calibration
> > coefficients\n");
> > +                       return ret;
> > +               }
> > +               break;
> >         }
> >  
> >         /*
> > diff --git a/drivers/iio/pressure/bmp280-i2c.c
> > b/drivers/iio/pressure/bmp280-i2c.c
> > index 35045bd92846..31a8a0daa39a 100644
> > --- a/drivers/iio/pressure/bmp280-i2c.c
> > +++ b/drivers/iio/pressure/bmp280-i2c.c
> > @@ -19,6 +19,9 @@ static int bmp280_i2c_probe(struct i2c_client
> > *client,
> >         case BME280_CHIP_ID:
> >                 regmap_config = &bmp280_regmap_config;
> >                 break;
> > +       case BMP380_CHIP_ID:
> > +               regmap_config = &bmp380_regmap_config;
> > +               break;
> >         default:
> >                 return -EINVAL;
> >         }
> > @@ -37,6 +40,7 @@ static int bmp280_i2c_probe(struct i2c_client
> > *client,
> >  }
> >  
> >  static const struct of_device_id bmp280_of_i2c_match[] = {
> > +       { .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 },
> > @@ -46,6 +50,7 @@ static const struct of_device_id
> > bmp280_of_i2c_match[] = {
> >  MODULE_DEVICE_TABLE(of, bmp280_of_i2c_match);
> >  
> >  static const struct i2c_device_id bmp280_i2c_id[] = {
> > +       {"bmp380", BMP380_CHIP_ID },
> >         {"bmp280", BMP280_CHIP_ID },
> >         {"bmp180", BMP180_CHIP_ID },
> >         {"bmp085", BMP180_CHIP_ID },
> > diff --git a/drivers/iio/pressure/bmp280-regmap.c
> > b/drivers/iio/pressure/bmp280-regmap.c
> > index da136dbadc8f..b440fa41bf12 100644
> > --- a/drivers/iio/pressure/bmp280-regmap.c
> > +++ b/drivers/iio/pressure/bmp280-regmap.c
> > @@ -72,6 +72,49 @@ static bool bmp280_is_volatile_reg(struct device
> > *dev, unsigned int reg)
> >         }
> >  }
> >  
> > +static bool bmp380_is_writeable_reg(struct device *dev, unsigned
> > int reg)
> > +{
> > +       switch (reg) {
> > +       case BMP380_REG_CMD:
> > +       case BMP380_REG_CONFIG:
> > +       case BMP380_REG_FIFO_CONFIG_1:
> > +       case BMP380_REG_FIFO_CONFIG_2:
> > +       case BMP380_REG_FIFO_WATERMARK_LSB:
> > +       case BMP380_REG_FIFO_WATERMARK_MSB:
> > +       case BMP380_REG_POWER_CONTROL:
> > +       case BMP380_REG_INT_CONTROL:
> > +       case BMP380_REG_IF_CONFIG:
> > +       case BMP380_REG_ODR:
> > +       case BMP380_REG_OSR:
> > +               return true;
> > +       default:
> > +               return false;
> > +       }
> > +}
> > +
> > +static bool bmp380_is_volatile_reg(struct device *dev, unsigned
> > int reg)
> > +{
> > +       switch (reg) {
> > +       case BMP380_REG_TEMP_XLSB:
> > +       case BMP380_REG_TEMP_LSB:
> > +       case BMP380_REG_TEMP_MSB:
> > +       case BMP380_REG_PRESS_XLSB:
> > +       case BMP380_REG_PRESS_LSB:
> > +       case BMP380_REG_PRESS_MSB:
> > +       case BMP380_REG_SENSOR_TIME_XLSB:
> > +       case BMP380_REG_SENSOR_TIME_LSB:
> > +       case BMP380_REG_SENSOR_TIME_MSB:
> > +       case BMP380_REG_INT_STATUS:
> > +       case BMP380_REG_FIFO_DATA:
> > +       case BMP380_REG_STATUS:
> > +       case BMP380_REG_ERROR:
> > +       case BMP380_REG_EVENT:
> > +               return true;
> > +       default:
> > +               return false;
> > +       }
> > +}
> > +
> >  const struct regmap_config bmp280_regmap_config = {
> >         .reg_bits = 8,
> >         .val_bits = 8,
> > @@ -83,3 +126,15 @@ const struct regmap_config bmp280_regmap_config
> > = {
> >         .volatile_reg = bmp280_is_volatile_reg,
> >  };
> >  EXPORT_SYMBOL(bmp280_regmap_config);
> > +
> > +const struct regmap_config bmp380_regmap_config = {
> > +       .reg_bits = 8,
> > +       .val_bits = 8,
> > +
> > +       .max_register = BMP380_REG_CMD,
> > +       .cache_type = REGCACHE_RBTREE,
> > +
> > +       .writeable_reg = bmp380_is_writeable_reg,
> > +       .volatile_reg = bmp380_is_volatile_reg,
> > +};
> > +EXPORT_SYMBOL(bmp380_regmap_config);
> > diff --git a/drivers/iio/pressure/bmp280-spi.c
> > b/drivers/iio/pressure/bmp280-spi.c
> > index 41f6cc56d229..303c41130343 100644
> > --- a/drivers/iio/pressure/bmp280-spi.c
> > +++ b/drivers/iio/pressure/bmp280-spi.c
> > @@ -66,6 +66,9 @@ static int bmp280_spi_probe(struct spi_device
> > *spi)
> >         case BME280_CHIP_ID:
> >                 regmap_config = &bmp280_regmap_config;
> >                 break;
> > +       case BMP380_CHIP_ID:
> > +               regmap_config = &bmp380_regmap_config;
> > +               break;
> >         default:
> >                 return -EINVAL;
> >         }
> > @@ -92,6 +95,7 @@ static const struct of_device_id
> > bmp280_of_spi_match[] = {
> >         { .compatible = "bosch,bmp181", },
> >         { .compatible = "bosch,bmp280", },
> >         { .compatible = "bosch,bme280", },
> > +       { .compatible = "bosch,bmp380", },
> >         { },
> >  };
> >  MODULE_DEVICE_TABLE(of, bmp280_of_spi_match);
> > @@ -101,6 +105,7 @@ static const struct spi_device_id
> > bmp280_spi_id[] = {
> >         { "bmp181", BMP180_CHIP_ID },
> >         { "bmp280", BMP280_CHIP_ID },
> >         { "bme280", BME280_CHIP_ID },
> > +       { "bmp380", BMP380_CHIP_ID },
> >         { }
> >  };
> >  MODULE_DEVICE_TABLE(spi, bmp280_spi_id);
> > diff --git a/drivers/iio/pressure/bmp280.h
> > b/drivers/iio/pressure/bmp280.h
> > index 57ba0e85db91..b4c122525ffe 100644
> > --- a/drivers/iio/pressure/bmp280.h
> > +++ b/drivers/iio/pressure/bmp280.h
> > @@ -3,6 +3,122 @@
> >  #include <linux/device.h>
> >  #include <linux/regmap.h>
> >  
> > +/* BMP380 specific registers */
> > +#define BMP380_REG_CMD                 0x7E
> > +#define BMP380_REG_CONFIG              0x1F
> > +#define BMP380_REG_ODR                 0X1D
> > +#define BMP380_REG_OSR                 0X1C
> > +#define BMP380_REG_POWER_CONTROL       0X1B
> > +#define BMP380_REG_IF_CONFIG           0X1A
> > +#define BMP380_REG_INT_CONTROL         0X19
> > +#define BMP380_REG_INT_STATUS          0X11
> > +#define BMP380_REG_EVENT               0X10
> > +#define BMP380_REG_STATUS              0X03
> > +#define BMP380_REG_ERROR               0X02
> > +#define BMP380_REG_ID                  0X00
> > +
> > +#define BMP380_REG_FIFO_CONFIG_1       0X18
> > +#define BMP380_REG_FIFO_CONFIG_2       0X17
> > +#define BMP380_REG_FIFO_WATERMARK_MSB  0X16
> > +#define BMP380_REG_FIFO_WATERMARK_LSB  0X15
> > +#define BMP380_REG_FIFO_DATA           0X14
> > +#define BMP380_REG_FIFO_LENGTH_MSB     0X13
> > +#define BMP380_REG_FIFO_LENGTH_LSB     0X12
> > +
> > +#define BMP380_REG_SENSOR_TIME_MSB     0X0E
> > +#define BMP380_REG_SENSOR_TIME_LSB     0X0D
> > +#define BMP380_REG_SENSOR_TIME_XLSB    0X0C
> > +
> > +#define BMP380_REG_TEMP_MSB            0X09
> > +#define BMP380_REG_TEMP_LSB            0X08
> > +#define BMP380_REG_TEMP_XLSB           0X07
> > +
> > +#define BMP380_REG_PRESS_MSB           0X06
> > +#define BMP380_REG_PRESS_LSB           0X05
> > +#define BMP380_REG_PRESS_XLSB          0X04
> > +
> > +#define BMP380_REG_CALIB_TEMP_START    0x31
> > +#define BMP380_CALIB_REG_COUNT         21
> > +#define le16_from_bytes(lsb, msb)      (le16_to_cpu(((((__le16)
> > msb) << 8) | \
> > +                                                       (__le16)
> > lsb)))
> 
> That doesn't look right.  (msb << 8) | lsb will be cpu endian, not
> le16.

Yes, you're right! And with the changes to get_unaligned_le16() I can
drop this macro and use the utilities already present in the kernel

> > +
> > +#define BMP380_FILTER_MASK             GENMASK(3, 1)
> > +#define BMP380_FILTER_OFF              0
> > +#define BMP380_FILTER_1X               BIT(1)
> > +#define BMP380_FILTER_3X               BIT(2)
> > +#define BMP380_FILTER_7X               (BIT(2) | BIT(1))
> > +#define BMP380_FILTER_15X              BIT(3)
> > +#define BMP380_FILTER_31X              (BIT(3) | BIT(1))
> > +#define BMP380_FILTER_63X              (BIT(3) | BIT(2))
> > +#define BMP380_FILTER_127X             (BIT(3) | BIT(2) | BIT(1))
> 
> these are values, 0,1,2,3,4,5,6,7 not a bunch of bits.
> So use with FIELD_PREP()

Should I convert the values to a enumeration or simply declare the
macros with the values? Thanks in advance

> > +
> > +#define BMP380_OSRS_TEMP_MASK          GENMASK(5, 3)
> > +#define BMP380_OSRS_TEMP_X(osrs_t)     ((osrs_t) << 3)
> > +#define BMP380_OSRS_TEMP_1X            BMP380_OSRS_TEMP_X(0)
> > +#define BMP380_OSRS_TEMP_2X            BMP380_OSRS_TEMP_X(1)
> > +#define BMP380_OSRS_TEMP_4X            BMP380_OSRS_TEMP_X(2)
> > +#define BMP380_OSRS_TEMP_8X            BMP380_OSRS_TEMP_X(3)
> > +#define BMP380_OSRS_TEMP_16X           BMP380_OSRS_TEMP_X(4)
> > +#define BMP380_OSRS_TEMP_32X           BMP380_OSRS_TEMP_X(5)
> > +
> > +#define BMP380_OSRS_PRESS_MASK         (BIT(2) | BIT(1) | BIT(0))
> 
> GENMASK unless this is made up of 3 bits with separate definitions.
> If it is, give defines for them and use them to build this full
> mask.

I forgot to change this one to GENMASK. Thanks!

> > +#define BMP380_OSRS_PRESS_X(osrs_p)    ((osrs_p) << 0)
> 
> FIELD_PREP()
> 
> > +#define BMP380_OSRS_PRESS_1X           BMP380_OSRS_PRESS_X(0)
> > +#define BMP380_OSRS_PRESS_2X           BMP380_OSRS_PRESS_X(1)
> > +#define BMP380_OSRS_PRESS_4X           BMP380_OSRS_PRESS_X(2)
> > +#define BMP380_OSRS_PRESS_8X           BMP380_OSRS_PRESS_X(3)
> > +#define BMP380_OSRS_PRESS_16X          BMP380_OSRS_PRESS_X(4)
> > +#define BMP380_OSRS_PRESS_32X          BMP380_OSRS_PRESS_X(5)
> > +
> > +#define BMP380_ODRS_MASK               GENMASK(4, 0)
> > +#define BMP380_ODRS_200HZ              0x00
> > +#define BMP380_ODRS_100HZ              0x01
> > +#define BMP380_ODRS_50HZ               0x02
> > +#define BMP380_ODRS_25HZ               0x03
> > +#define BMP380_ODRS_12_5HZ             0x04
> > +#define BMP380_ODRS_6_25HZ             0x05
> > +#define BMP380_ODRS_3_1HZ              0x06
> > +#define BMP380_ODRS_1_5HZ              0x07
> > +#define BMP380_ODRS_0_78HZ             0x08
> > +#define BMP380_ODRS_0_39HZ             0x09
> > +#define BMP380_ODRS_0_2HZ              0x0A
> > +#define BMP380_ODRS_0_1HZ              0x0B
> > +#define BMP380_ODRS_0_05HZ             0x0C
> > +#define BMP380_ODRS_0_02HZ             0x0D
> > +#define BMP380_ODRS_0_01HZ             0x0E
> > +#define BMP380_ODRS_0_006HZ            0x0F
> > +#define BMP380_ODRS_0_003HZ            0x10
> > +#define BMP380_ODRS_0_0015HZ           0x11
> > +
> > +#define BMP380_CTRL_SENSORS_MASK       GENMASK(1, 0)
> > +#define BMP380_CTRL_SENSORS_PRESS_EN   BIT(0)
> > +#define BMP380_CTRL_SENSORS_TEMP_EN    BIT(1)
> > +#define BMP380_MODE_MASK               GENMASK(5, 4)
> > +#define BMP380_MODE_SLEEP              0
> > +#define BMP380_MODE_FORCED             BIT(4)
> > +#define BMP380_MODE_NORMAL             (BIT(5) | BIT(4))
> 
> That doesn't look like either a mask or a combination of values,
> rather it looks like the number 3 in a shifted field.
> 
> Use FIELD_GET/PREP along with the mask to extract a 2 bit
> vlaue which you can then compare with 0, 1, 3

Yes, it is a three in a shifted field. Thanks for insight!

> 
> > +#define BMP380_STATUS_CMD_RDY_MASK     BIT(4)
> > +#define BMP380_STATUS_DRDY_PRESS_MASK  BIT(5)
> > +#define BMP380_STATUS_DRDY_TEMP_MASK   BIT(6)
> > +
> > +#define BMP380_ERR_FATAL_MASK          BIT(0)
> > +#define        BMP380_ERR_CMD_MASK             BIT(1)
> 
> Stray tab?
> 
> > +#define BMP380_ERR_CONF_MASK           BIT(2)
> > +
> > +#define BMP380_TEMP_SKIPPED            0x800000
> > +#define BMP380_PRESS_SKIPPED           0x800000
> > +




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux