| Message ID | 20240711211558.106327-5-vassilisamir@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | BMP280: Minor cleanup and interrupt support | expand |
On Thu, 11 Jul 2024 23:15:52 +0200 Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > Convert individual reads to a bulk read for the humidity calibration data. > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> > --- > drivers/iio/pressure/bmp280-core.c | 57 +++++++++--------------------- > drivers/iio/pressure/bmp280.h | 5 +++ > 2 files changed, 21 insertions(+), 41 deletions(-) > > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c > index 3deaa57bb3f5..9c32266403bd 100644 > --- a/drivers/iio/pressure/bmp280-core.c > +++ b/drivers/iio/pressure/bmp280-core.c > @@ -118,6 +118,8 @@ enum bmp580_odr { > */ > enum { T1, T2, T3, P1, P2, P3, P4, P5, P6, P7, P8, P9 }; > > +enum { H2 = 0, H3 = 2, H4 = 3, H5 = 4, H6 = 6 }; Maybe add a comment to this that these are the locations where the field 'starts' and that some overlap such as H5 and H6. > + > enum { > /* Temperature calib indexes */ > BMP380_T1 = 0, > @@ -344,6 +346,7 @@ static int bme280_read_calib(struct bmp280_data *data) > { > struct bmp280_calib *calib = &data->calib.bmp280; > struct device *dev = data->dev; > + s16 h4_upper, h4_lower; > unsigned int tmp; > int ret; > > @@ -352,14 +355,6 @@ static int bme280_read_calib(struct bmp280_data *data) > if (ret) > return ret; > > - /* > - * Read humidity calibration values. > - * Due to some odd register addressing we cannot just > - * do a big bulk read. Instead, we have to read each Hx > - * value separately and sometimes do some bit shifting... > - * Humidity data is only available on BME280. > - */ > - > ret = regmap_read(data->regmap, BME280_REG_COMP_H1, &tmp); > if (ret) { > dev_err(dev, "failed to read H1 comp value\n"); > @@ -368,43 +363,23 @@ static int bme280_read_calib(struct bmp280_data *data) > calib->H1 = tmp; > > ret = regmap_bulk_read(data->regmap, BME280_REG_COMP_H2, > - &data->le16, sizeof(data->le16)); > - if (ret) { > - dev_err(dev, "failed to read H2 comp value\n"); > - return ret; > - } > - calib->H2 = sign_extend32(le16_to_cpu(data->le16), 15); > - > - ret = regmap_read(data->regmap, BME280_REG_COMP_H3, &tmp); > - if (ret) { > - dev_err(dev, "failed to read H3 comp value\n"); > - return ret; > - } > - calib->H3 = tmp; > - > - ret = regmap_bulk_read(data->regmap, BME280_REG_COMP_H4, > - &data->be16, sizeof(data->be16)); > - if (ret) { > - dev_err(dev, "failed to read H4 comp value\n"); > - return ret; > - } > - calib->H4 = sign_extend32(((be16_to_cpu(data->be16) >> 4) & 0xff0) | > - (be16_to_cpu(data->be16) & 0xf), 11); > - > - ret = regmap_bulk_read(data->regmap, BME280_REG_COMP_H5, > - &data->le16, sizeof(data->le16)); > + data->bme280_humid_cal_buf, > + sizeof(data->bme280_humid_cal_buf)); > if (ret) { > - dev_err(dev, "failed to read H5 comp value\n"); > + dev_err(dev, "failed to read humidity calibration values\n"); > return ret; > } > - calib->H5 = sign_extend32(FIELD_GET(BME280_COMP_H5_MASK, le16_to_cpu(data->le16)), 11); > > - ret = regmap_read(data->regmap, BME280_REG_COMP_H6, &tmp); > - if (ret) { > - dev_err(dev, "failed to read H6 comp value\n"); > - return ret; > - } > - calib->H6 = sign_extend32(tmp, 7); > + calib->H2 = get_unaligned_le16(&data->bme280_humid_cal_buf[H2]); > + calib->H3 = data->bme280_humid_cal_buf[H3]; > + h4_upper = FIELD_GET(BME280_COMP_H4_MASK_UP, > + get_unaligned_be16(&data->bme280_humid_cal_buf[H4])); > + h4_lower = FIELD_GET(BME280_COMP_H4_MASK_LOW, > + get_unaligned_be16(&data->bme280_humid_cal_buf[H4])); > + calib->H4 = sign_extend32((h4_upper & ~BME280_COMP_H4_MASK_LOW) | h4_lower, 11); This looks unusual. Why mask with MASK_LOW? The field_get above already drops the bottom 4 bits an this is dropping more. Should that H4_MASK_UP actually be GENMASK(15, 8) and then you shift it left 4 to make space for the lower part? Original code was messing with values inline so there is less need for it to be explicit in how it does the masks. Here you imply a 12 bit field but only use 8 bits of it which isn't good. > + calib->H5 = sign_extend32(FIELD_GET(BME280_COMP_H5_MASK, > + get_unaligned_le16(&data->bme280_humid_cal_buf[H5])), 11); > + calib->H6 = data->bme280_humid_cal_buf[H6]; > > return 0; > } > diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h > index ccacc67c1473..56c01f224728 100644 > --- a/drivers/iio/pressure/bmp280.h > +++ b/drivers/iio/pressure/bmp280.h > @@ -257,8 +257,12 @@ > #define BME280_REG_COMP_H5 0xE5 > #define BME280_REG_COMP_H6 0xE7 > > +#define BME280_COMP_H4_MASK_UP GENMASK(15, 4) > +#define BME280_COMP_H4_MASK_LOW GENMASK(3, 0) > #define BME280_COMP_H5_MASK GENMASK(15, 4) > > +#define BME280_CONTIGUOUS_CALIB_REGS 7 > + > #define BME280_OSRS_HUMIDITY_MASK GENMASK(2, 0) > #define BME280_OSRS_HUMIDITY_SKIP 0 > #define BME280_OSRS_HUMIDITY_1X 1 > @@ -423,6 +427,7 @@ struct bmp280_data { > /* Calibration data buffers */ > __le16 bmp280_cal_buf[BMP280_CONTIGUOUS_CALIB_REGS / 2]; > __be16 bmp180_cal_buf[BMP180_REG_CALIB_COUNT / 2]; > + u8 bme280_humid_cal_buf[BME280_CONTIGUOUS_CALIB_REGS]; > u8 bmp380_cal_buf[BMP380_CALIB_REG_COUNT]; > /* Miscellaneous, endianness-aware data buffers */ > __le16 le16;
On Sat, Jul 20, 2024 at 12:16:04PM +0100, Jonathan Cameron wrote: > On Thu, 11 Jul 2024 23:15:52 +0200 > Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > > > Convert individual reads to a bulk read for the humidity calibration data. > > > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> > > --- > > drivers/iio/pressure/bmp280-core.c | 57 +++++++++--------------------- > > drivers/iio/pressure/bmp280.h | 5 +++ > > 2 files changed, 21 insertions(+), 41 deletions(-) > > > > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c > > index 3deaa57bb3f5..9c32266403bd 100644 > > --- a/drivers/iio/pressure/bmp280-core.c > > +++ b/drivers/iio/pressure/bmp280-core.c > > @@ -118,6 +118,8 @@ enum bmp580_odr { > > */ > > enum { T1, T2, T3, P1, P2, P3, P4, P5, P6, P7, P8, P9 }; > > > > +enum { H2 = 0, H3 = 2, H4 = 3, H5 = 4, H6 = 6 }; > Maybe add a comment to this that these are the locations where > the field 'starts' and that some overlap such as H5 and H6. > True, thanks! > > + > > enum { > > /* Temperature calib indexes */ > > BMP380_T1 = 0, > > @@ -344,6 +346,7 @@ static int bme280_read_calib(struct bmp280_data *data) > > { > > struct bmp280_calib *calib = &data->calib.bmp280; > > struct device *dev = data->dev; > > + s16 h4_upper, h4_lower; > > unsigned int tmp; > > int ret; > > > > @@ -352,14 +355,6 @@ static int bme280_read_calib(struct bmp280_data *data) > > if (ret) > > return ret; > > > > - /* > > - * Read humidity calibration values. > > - * Due to some odd register addressing we cannot just > > - * do a big bulk read. Instead, we have to read each Hx > > - * value separately and sometimes do some bit shifting... > > - * Humidity data is only available on BME280. > > - */ > > - > > ret = regmap_read(data->regmap, BME280_REG_COMP_H1, &tmp); > > if (ret) { > > dev_err(dev, "failed to read H1 comp value\n"); > > @@ -368,43 +363,23 @@ static int bme280_read_calib(struct bmp280_data *data) > > calib->H1 = tmp; > > > > ret = regmap_bulk_read(data->regmap, BME280_REG_COMP_H2, > > - &data->le16, sizeof(data->le16)); > > - if (ret) { > > - dev_err(dev, "failed to read H2 comp value\n"); > > - return ret; > > - } > > - calib->H2 = sign_extend32(le16_to_cpu(data->le16), 15); > > - > > - ret = regmap_read(data->regmap, BME280_REG_COMP_H3, &tmp); > > - if (ret) { > > - dev_err(dev, "failed to read H3 comp value\n"); > > - return ret; > > - } > > - calib->H3 = tmp; > > - > > - ret = regmap_bulk_read(data->regmap, BME280_REG_COMP_H4, > > - &data->be16, sizeof(data->be16)); > > - if (ret) { > > - dev_err(dev, "failed to read H4 comp value\n"); > > - return ret; > > - } > > - calib->H4 = sign_extend32(((be16_to_cpu(data->be16) >> 4) & 0xff0) | > > - (be16_to_cpu(data->be16) & 0xf), 11); > > - > > - ret = regmap_bulk_read(data->regmap, BME280_REG_COMP_H5, > > - &data->le16, sizeof(data->le16)); > > + data->bme280_humid_cal_buf, > > + sizeof(data->bme280_humid_cal_buf)); > > if (ret) { > > - dev_err(dev, "failed to read H5 comp value\n"); > > + dev_err(dev, "failed to read humidity calibration values\n"); > > return ret; > > } > > - calib->H5 = sign_extend32(FIELD_GET(BME280_COMP_H5_MASK, le16_to_cpu(data->le16)), 11); > > > > - ret = regmap_read(data->regmap, BME280_REG_COMP_H6, &tmp); > > - if (ret) { > > - dev_err(dev, "failed to read H6 comp value\n"); > > - return ret; > > - } > > - calib->H6 = sign_extend32(tmp, 7); > > + calib->H2 = get_unaligned_le16(&data->bme280_humid_cal_buf[H2]); > > + calib->H3 = data->bme280_humid_cal_buf[H3]; > > + h4_upper = FIELD_GET(BME280_COMP_H4_MASK_UP, > > + get_unaligned_be16(&data->bme280_humid_cal_buf[H4])); > > + h4_lower = FIELD_GET(BME280_COMP_H4_MASK_LOW, > > + get_unaligned_be16(&data->bme280_humid_cal_buf[H4])); > > + calib->H4 = sign_extend32((h4_upper & ~BME280_COMP_H4_MASK_LOW) | h4_lower, 11); > > This looks unusual. Why mask with MASK_LOW? The field_get above already drops the bottom > 4 bits an this is dropping more. Should that H4_MASK_UP actually be GENMASK(15, 8) > and then you shift it left 4 to make space for the lower part? > > Original code was messing with values inline so there is less need for it > to be explicit in how it does the masks. Here you imply a 12 bit field but only use > 8 bits of it which isn't good. > > You are right it is a bit confusing. This endianness "fun" made me probably write much more complex code. Indeed, it doesn't look good what I do even though it works. As you said, that H4_MASK_UP should be GENMASK(15,8) and then I should find a better way. Cheers, Vasilis > > + calib->H5 = sign_extend32(FIELD_GET(BME280_COMP_H5_MASK, > > + get_unaligned_le16(&data->bme280_humid_cal_buf[H5])), 11); > > + calib->H6 = data->bme280_humid_cal_buf[H6]; > > > > return 0; > > } > > diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h > > index ccacc67c1473..56c01f224728 100644 > > --- a/drivers/iio/pressure/bmp280.h > > +++ b/drivers/iio/pressure/bmp280.h > > @@ -257,8 +257,12 @@ > > #define BME280_REG_COMP_H5 0xE5 > > #define BME280_REG_COMP_H6 0xE7 > > > > +#define BME280_COMP_H4_MASK_UP GENMASK(15, 4) > > +#define BME280_COMP_H4_MASK_LOW GENMASK(3, 0) > > #define BME280_COMP_H5_MASK GENMASK(15, 4) > > > > +#define BME280_CONTIGUOUS_CALIB_REGS 7 > > + > > #define BME280_OSRS_HUMIDITY_MASK GENMASK(2, 0) > > #define BME280_OSRS_HUMIDITY_SKIP 0 > > #define BME280_OSRS_HUMIDITY_1X 1 > > @@ -423,6 +427,7 @@ struct bmp280_data { > > /* Calibration data buffers */ > > __le16 bmp280_cal_buf[BMP280_CONTIGUOUS_CALIB_REGS / 2]; > > __be16 bmp180_cal_buf[BMP180_REG_CALIB_COUNT / 2]; > > + u8 bme280_humid_cal_buf[BME280_CONTIGUOUS_CALIB_REGS]; > > u8 bmp380_cal_buf[BMP380_CALIB_REG_COUNT]; > > /* Miscellaneous, endianness-aware data buffers */ > > __le16 le16; >
diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c index 3deaa57bb3f5..9c32266403bd 100644 --- a/drivers/iio/pressure/bmp280-core.c +++ b/drivers/iio/pressure/bmp280-core.c @@ -118,6 +118,8 @@ enum bmp580_odr { */ enum { T1, T2, T3, P1, P2, P3, P4, P5, P6, P7, P8, P9 }; +enum { H2 = 0, H3 = 2, H4 = 3, H5 = 4, H6 = 6 }; + enum { /* Temperature calib indexes */ BMP380_T1 = 0, @@ -344,6 +346,7 @@ static int bme280_read_calib(struct bmp280_data *data) { struct bmp280_calib *calib = &data->calib.bmp280; struct device *dev = data->dev; + s16 h4_upper, h4_lower; unsigned int tmp; int ret; @@ -352,14 +355,6 @@ static int bme280_read_calib(struct bmp280_data *data) if (ret) return ret; - /* - * Read humidity calibration values. - * Due to some odd register addressing we cannot just - * do a big bulk read. Instead, we have to read each Hx - * value separately and sometimes do some bit shifting... - * Humidity data is only available on BME280. - */ - ret = regmap_read(data->regmap, BME280_REG_COMP_H1, &tmp); if (ret) { dev_err(dev, "failed to read H1 comp value\n"); @@ -368,43 +363,23 @@ static int bme280_read_calib(struct bmp280_data *data) calib->H1 = tmp; ret = regmap_bulk_read(data->regmap, BME280_REG_COMP_H2, - &data->le16, sizeof(data->le16)); - if (ret) { - dev_err(dev, "failed to read H2 comp value\n"); - return ret; - } - calib->H2 = sign_extend32(le16_to_cpu(data->le16), 15); - - ret = regmap_read(data->regmap, BME280_REG_COMP_H3, &tmp); - if (ret) { - dev_err(dev, "failed to read H3 comp value\n"); - return ret; - } - calib->H3 = tmp; - - ret = regmap_bulk_read(data->regmap, BME280_REG_COMP_H4, - &data->be16, sizeof(data->be16)); - if (ret) { - dev_err(dev, "failed to read H4 comp value\n"); - return ret; - } - calib->H4 = sign_extend32(((be16_to_cpu(data->be16) >> 4) & 0xff0) | - (be16_to_cpu(data->be16) & 0xf), 11); - - ret = regmap_bulk_read(data->regmap, BME280_REG_COMP_H5, - &data->le16, sizeof(data->le16)); + data->bme280_humid_cal_buf, + sizeof(data->bme280_humid_cal_buf)); if (ret) { - dev_err(dev, "failed to read H5 comp value\n"); + dev_err(dev, "failed to read humidity calibration values\n"); return ret; } - calib->H5 = sign_extend32(FIELD_GET(BME280_COMP_H5_MASK, le16_to_cpu(data->le16)), 11); - ret = regmap_read(data->regmap, BME280_REG_COMP_H6, &tmp); - if (ret) { - dev_err(dev, "failed to read H6 comp value\n"); - return ret; - } - calib->H6 = sign_extend32(tmp, 7); + calib->H2 = get_unaligned_le16(&data->bme280_humid_cal_buf[H2]); + calib->H3 = data->bme280_humid_cal_buf[H3]; + h4_upper = FIELD_GET(BME280_COMP_H4_MASK_UP, + get_unaligned_be16(&data->bme280_humid_cal_buf[H4])); + h4_lower = FIELD_GET(BME280_COMP_H4_MASK_LOW, + get_unaligned_be16(&data->bme280_humid_cal_buf[H4])); + calib->H4 = sign_extend32((h4_upper & ~BME280_COMP_H4_MASK_LOW) | h4_lower, 11); + calib->H5 = sign_extend32(FIELD_GET(BME280_COMP_H5_MASK, + get_unaligned_le16(&data->bme280_humid_cal_buf[H5])), 11); + calib->H6 = data->bme280_humid_cal_buf[H6]; return 0; } diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h index ccacc67c1473..56c01f224728 100644 --- a/drivers/iio/pressure/bmp280.h +++ b/drivers/iio/pressure/bmp280.h @@ -257,8 +257,12 @@ #define BME280_REG_COMP_H5 0xE5 #define BME280_REG_COMP_H6 0xE7 +#define BME280_COMP_H4_MASK_UP GENMASK(15, 4) +#define BME280_COMP_H4_MASK_LOW GENMASK(3, 0) #define BME280_COMP_H5_MASK GENMASK(15, 4) +#define BME280_CONTIGUOUS_CALIB_REGS 7 + #define BME280_OSRS_HUMIDITY_MASK GENMASK(2, 0) #define BME280_OSRS_HUMIDITY_SKIP 0 #define BME280_OSRS_HUMIDITY_1X 1 @@ -423,6 +427,7 @@ struct bmp280_data { /* Calibration data buffers */ __le16 bmp280_cal_buf[BMP280_CONTIGUOUS_CALIB_REGS / 2]; __be16 bmp180_cal_buf[BMP180_REG_CALIB_COUNT / 2]; + u8 bme280_humid_cal_buf[BME280_CONTIGUOUS_CALIB_REGS]; u8 bmp380_cal_buf[BMP380_CALIB_REG_COUNT]; /* Miscellaneous, endianness-aware data buffers */ __le16 le16;
Convert individual reads to a bulk read for the humidity calibration data. Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> --- drivers/iio/pressure/bmp280-core.c | 57 +++++++++--------------------- drivers/iio/pressure/bmp280.h | 5 +++ 2 files changed, 21 insertions(+), 41 deletions(-)