On Sun, Jul 28, 2024 at 04:46:21PM +0100, Jonathan Cameron wrote:
> On Fri, 26 Jul 2024 01:10:33 +0200
> Vasileios Amoiridis <vassilisamir@xxxxxxxxx> wrote:
>
> > Convert individual reads to a bulk read for the humidity calibration data.
> >
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@xxxxxxxxx>
>
> One comment inline. Short version is move that complicated field start enum
> next to the code so we don't need to say so much for it to make sense.
>
> > ---
> > drivers/iio/pressure/bmp280-core.c | 62 ++++++++++--------------------
> > drivers/iio/pressure/bmp280.h | 6 +++
> > 2 files changed, 27 insertions(+), 41 deletions(-)
> >
> > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> > index 3deaa57bb3f5..d5e5eb22667a 100644
> > --- a/drivers/iio/pressure/bmp280-core.c
> > +++ b/drivers/iio/pressure/bmp280-core.c
> > @@ -118,6 +118,12 @@ enum bmp580_odr {
> > */
> > enum { T1, T2, T3, P1, P2, P3, P4, P5, P6, P7, P8, P9 };
> >
> > +/*
> > + * These enums are used for indexing into the array of humidity parameters
> > + * for BME280.
>
> I was thinking of a comment that also mentioned the overlap. Perhaps something like
> ...
>
>
> Index of the byte containing the start of each humidity parameter. Some
> parameters stretch across multiple bytes including into the start of the byte
> where another humidity parameter begins. Unaligned be/le accesses are used
> to allow fields to be extracted with FIELD_GET().
>
> Or, just refer to the field layout being complex and to see
> bme280_read_calib function.
>
> Actually come to think of it, just move this enum down there so it
> is local to the code and the usage is more obvious / comment less important.
>
Hi Jonathan,
Thanks for the feedback once again. Sorry for the late reply, I was taking
some time off for vacation :)
Indeed your comment makes sense, I can do that.
Cheers,
Vasilis
> > + */
> > +enum { H2 = 0, H3 = 2, H4 = 3, H5 = 4, H6 = 6 };
> > +
> > enum {
> > /* Temperature calib indexes */
> > BMP380_T1 = 0,
> > @@ -344,6 +350,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 +359,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 +367,24 @@ 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));
> > + data->bme280_humid_cal_buf,
> > + sizeof(data->bme280_humid_cal_buf));
> > if (ret) {
> > - dev_err(dev, "failed to read H4 comp value\n");
> > + dev_err(dev, "failed to read humidity calibration values\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));
> > - if (ret) {
> > - dev_err(dev, "failed to read H5 comp value\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_GET_MASK_UP,
> > + get_unaligned_be16(&data->bme280_humid_cal_buf[H4]));
> > + h4_upper = FIELD_PREP(BME280_COMP_H4_PREP_MASK_UP, h4_upper);
> > + h4_lower = FIELD_GET(BME280_COMP_H4_MASK_LOW,
> > + get_unaligned_be16(&data->bme280_humid_cal_buf[H4]));
> > + calib->H4 = sign_extend32(h4_upper | 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..9bea0b84d2f4 100644
> > --- a/drivers/iio/pressure/bmp280.h
> > +++ b/drivers/iio/pressure/bmp280.h
> > @@ -257,8 +257,13 @@
> > #define BME280_REG_COMP_H5 0xE5
> > #define BME280_REG_COMP_H6 0xE7
> >
> > +#define BME280_COMP_H4_GET_MASK_UP GENMASK(15, 8)
> > +#define BME280_COMP_H4_PREP_MASK_UP GENMASK(11, 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 +428,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;
>
