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

[Vasileios Amoiridis <vassilisamir@gmail.com>]

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;
>