Re: [PATCH v1 04/10] iio: pressure: bmp280: Use bulk read for humidity calibration data

[Jonathan Cameron <jic23@kernel.org>]

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;