[PATCH 0/2] bme680 cleanup

[PATCH 0/2] bme680 cleanup

[David Frey]

I have performed some minor cleanup on the bme680 driver. I don't think there's
anything controversial in my changes. Let me know what you think

David Frey (2):
  iio: bme680: perform cosmetic cleanup
  iio: bme680: simplify oversampling handling

 drivers/iio/chemical/bme680.h      |  17 +++---
 drivers/iio/chemical/bme680_core.c | 110 ++++++++++++-------------------------
 2 files changed, 42 insertions(+), 85 deletions(-)

-- 
2.11.0

[PATCH 1/2] iio: bme680: perform cosmetic cleanup

[David Frey]

 * use consistent naming for masks. _MSK -> _MASK
 * use GEN_MASK to define masks rather than hex constants
 * consistently indent masks and values under the corresponding register
   define
 * use FIELD_GET instead of explicit mask and shift
 * Join split lines in bme680_read_calib() which can fit in 80 chars
 * Make use clamp macro

Signed-off-by: David Frey <dpfrey@gmail.com>
---
 drivers/iio/chemical/bme680.h      | 17 +++++++-------
 drivers/iio/chemical/bme680_core.c | 45 +++++++++++++-------------------------
 2 files changed, 23 insertions(+), 39 deletions(-)

diff --git a/drivers/iio/chemical/bme680.h b/drivers/iio/chemical/bme680.h
index e049323f209a..a9f2a9a6abc5 100644
--- a/drivers/iio/chemical/bme680.h
+++ b/drivers/iio/chemical/bme680.h
@@ -4,10 +4,10 @@
 
 #define BME680_REG_CHIP_I2C_ID			0xD0
 #define BME680_REG_CHIP_SPI_ID			0x50
-#define BME680_CHIP_ID_VAL			0x61
+#define   BME680_CHIP_ID_VAL			0x61
 #define BME680_REG_SOFT_RESET_I2C		0xE0
 #define BME680_REG_SOFT_RESET_SPI		0x60
-#define BME680_CMD_SOFTRESET			0xB6
+#define   BME680_CMD_SOFTRESET			0xB6
 #define BME680_REG_STATUS			0x73
 #define   BME680_SPI_MEM_PAGE_BIT		BIT(4)
 #define     BME680_SPI_MEM_PAGE_1_VAL		1
@@ -18,6 +18,7 @@
 #define BME680_REG_GAS_MSB			0x2A
 #define BME680_REG_GAS_R_LSB			0x2B
 #define   BME680_GAS_STAB_BIT			BIT(4)
+#define   BME680_GAS_RANGE_MASK			GENMASK(3, 0)
 
 #define BME680_REG_CTRL_HUMIDITY		0x72
 #define   BME680_OSRS_HUMIDITY_MASK		GENMASK(2, 0)
@@ -26,9 +27,8 @@
 #define   BME680_OSRS_TEMP_MASK			GENMASK(7, 5)
 #define   BME680_OSRS_PRESS_MASK		GENMASK(4, 2)
 #define   BME680_MODE_MASK			GENMASK(1, 0)
-
-#define BME680_MODE_FORCED			1
-#define BME680_MODE_SLEEP			0
+#define     BME680_MODE_FORCED			1
+#define     BME680_MODE_SLEEP			0
 
 #define BME680_REG_CONFIG			0x75
 #define   BME680_FILTER_MASK			GENMASK(4, 2)
@@ -39,16 +39,15 @@
 
 #define BME680_MAX_OVERFLOW_VAL			0x40000000
 #define BME680_HUM_REG_SHIFT_VAL		4
-#define BME680_BIT_H1_DATA_MSK			0x0F
+#define BME680_BIT_H1_DATA_MASK			GENMASK(3, 0)
 
 #define BME680_REG_RES_HEAT_RANGE		0x02
-#define BME680_RHRANGE_MSK			0x30
+#define   BME680_RHRANGE_MASK			GENMASK(5, 4)
 #define BME680_REG_RES_HEAT_VAL			0x00
 #define BME680_REG_RANGE_SW_ERR			0x04
-#define BME680_RSERROR_MSK			0xF0
+#define   BME680_RSERROR_MASK			GENMASK(7, 4)
 #define BME680_REG_RES_HEAT_0			0x5A
 #define BME680_REG_GAS_WAIT_0			0x64
-#define BME680_GAS_RANGE_MASK			0x0F
 #define BME680_ADC_GAS_RES_SHIFT		6
 #define BME680_AMB_TEMP				25
 
diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
index 7d9bb62baa3f..0e79d03ecc40 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -102,16 +102,14 @@ static int bme680_read_calib(struct bme680_data *data,
 	__le16 buf;
 
 	/* Temperature related coefficients */
-	ret = regmap_bulk_read(data->regmap, BME680_T1_LSB_REG,
-			       (u8 *) &buf, 2);
+	ret = regmap_bulk_read(data->regmap, BME680_T1_LSB_REG, (u8 *) &buf, 2);
 	if (ret < 0) {
 		dev_err(dev, "failed to read BME680_T1_LSB_REG\n");
 		return ret;
 	}
 	calib->par_t1 = le16_to_cpu(buf);
 
-	ret = regmap_bulk_read(data->regmap, BME680_T2_LSB_REG,
-			       (u8 *) &buf, 2);
+	ret = regmap_bulk_read(data->regmap, BME680_T2_LSB_REG, (u8 *) &buf, 2);
 	if (ret < 0) {
 		dev_err(dev, "failed to read BME680_T2_LSB_REG\n");
 		return ret;
@@ -126,16 +124,14 @@ static int bme680_read_calib(struct bme680_data *data,
 	calib->par_t3 = tmp;
 
 	/* Pressure related coefficients */
-	ret = regmap_bulk_read(data->regmap, BME680_P1_LSB_REG,
-			       (u8 *) &buf, 2);
+	ret = regmap_bulk_read(data->regmap, BME680_P1_LSB_REG, (u8 *) &buf, 2);
 	if (ret < 0) {
 		dev_err(dev, "failed to read BME680_P1_LSB_REG\n");
 		return ret;
 	}
 	calib->par_p1 = le16_to_cpu(buf);
 
-	ret = regmap_bulk_read(data->regmap, BME680_P2_LSB_REG,
-			       (u8 *) &buf, 2);
+	ret = regmap_bulk_read(data->regmap, BME680_P2_LSB_REG, (u8 *) &buf, 2);
 	if (ret < 0) {
 		dev_err(dev, "failed to read BME680_P2_LSB_REG\n");
 		return ret;
@@ -149,16 +145,14 @@ static int bme680_read_calib(struct bme680_data *data,
 	}
 	calib->par_p3 = tmp;
 
-	ret = regmap_bulk_read(data->regmap, BME680_P4_LSB_REG,
-			       (u8 *) &buf, 2);
+	ret = regmap_bulk_read(data->regmap, BME680_P4_LSB_REG, (u8 *) &buf, 2);
 	if (ret < 0) {
 		dev_err(dev, "failed to read BME680_P4_LSB_REG\n");
 		return ret;
 	}
 	calib->par_p4 = le16_to_cpu(buf);
 
-	ret = regmap_bulk_read(data->regmap, BME680_P5_LSB_REG,
-			       (u8 *) &buf, 2);
+	ret = regmap_bulk_read(data->regmap, BME680_P5_LSB_REG, (u8 *) &buf, 2);
 	if (ret < 0) {
 		dev_err(dev, "failed to read BME680_P5_LSB_REG\n");
 		return ret;
@@ -179,16 +173,14 @@ static int bme680_read_calib(struct bme680_data *data,
 	}
 	calib->par_p7 = tmp;
 
-	ret = regmap_bulk_read(data->regmap, BME680_P8_LSB_REG,
-			       (u8 *) &buf, 2);
+	ret = regmap_bulk_read(data->regmap, BME680_P8_LSB_REG, (u8 *) &buf, 2);
 	if (ret < 0) {
 		dev_err(dev, "failed to read BME680_P8_LSB_REG\n");
 		return ret;
 	}
 	calib->par_p8 = le16_to_cpu(buf);
 
-	ret = regmap_bulk_read(data->regmap, BME680_P9_LSB_REG,
-			       (u8 *) &buf, 2);
+	ret = regmap_bulk_read(data->regmap, BME680_P9_LSB_REG, (u8 *) &buf, 2);
 	if (ret < 0) {
 		dev_err(dev, "failed to read BME680_P9_LSB_REG\n");
 		return ret;
@@ -208,30 +200,26 @@ static int bme680_read_calib(struct bme680_data *data,
 		dev_err(dev, "failed to read BME680_H1_MSB_REG\n");
 		return ret;
 	}
-
 	ret = regmap_read(data->regmap, BME680_H1_LSB_REG, &tmp_lsb);
 	if (ret < 0) {
 		dev_err(dev, "failed to read BME680_H1_LSB_REG\n");
 		return ret;
 	}
-
 	calib->par_h1 = (tmp_msb << BME680_HUM_REG_SHIFT_VAL) |
-				(tmp_lsb & BME680_BIT_H1_DATA_MSK);
+	                (tmp_lsb & BME680_BIT_H1_DATA_MASK);
 
 	ret = regmap_read(data->regmap, BME680_H2_MSB_REG, &tmp_msb);
 	if (ret < 0) {
 		dev_err(dev, "failed to read BME680_H2_MSB_REG\n");
 		return ret;
 	}
-
 	ret = regmap_read(data->regmap, BME680_H2_LSB_REG, &tmp_lsb);
 	if (ret < 0) {
 		dev_err(dev, "failed to read BME680_H2_LSB_REG\n");
 		return ret;
 	}
-
 	calib->par_h2 = (tmp_msb << BME680_HUM_REG_SHIFT_VAL) |
-				(tmp_lsb >> BME680_HUM_REG_SHIFT_VAL);
+	                (tmp_lsb >> BME680_HUM_REG_SHIFT_VAL);
 
 	ret = regmap_read(data->regmap, BME680_H3_REG, &tmp);
 	if (ret < 0) {
@@ -276,8 +264,8 @@ static int bme680_read_calib(struct bme680_data *data,
 	}
 	calib->par_gh1 = tmp;
 
-	ret = regmap_bulk_read(data->regmap, BME680_GH2_LSB_REG,
-			       (u8 *) &buf, 2);
+	ret = regmap_bulk_read(data->regmap, BME680_GH2_LSB_REG, (u8 *) &buf,
+	                       2);
 	if (ret < 0) {
 		dev_err(dev, "failed to read BME680_GH2_LSB_REG\n");
 		return ret;
@@ -297,7 +285,7 @@ static int bme680_read_calib(struct bme680_data *data,
 		dev_err(dev, "failed to read resistance heat range\n");
 		return ret;
 	}
-	calib->res_heat_range = (tmp & BME680_RHRANGE_MSK) / 16;
+	calib->res_heat_range = FIELD_GET(BME680_RHRANGE_MASK, tmp);
 
 	ret = regmap_read(data->regmap, BME680_REG_RES_HEAT_VAL, &tmp);
 	if (ret < 0) {
@@ -311,7 +299,7 @@ static int bme680_read_calib(struct bme680_data *data,
 		dev_err(dev, "failed to read range software error\n");
 		return ret;
 	}
-	calib->range_sw_err = (tmp & BME680_RSERROR_MSK) / 16;
+	calib->range_sw_err = FIELD_GET(BME680_RSERROR_MASK, tmp);
 
 	return 0;
 }
@@ -408,10 +396,7 @@ static u32 bme680_compensate_humid(struct bme680_data *data,
 	var6 = (var4 * var5) >> 1;
 	calc_hum = (((var3 + var6) >> 10) * 1000) >> 12;
 
-	if (calc_hum > 100000) /* Cap at 100%rH */
-		calc_hum = 100000;
-	else if (calc_hum < 0)
-		calc_hum = 0;
+	calc_hum = clamp(calc_hum, 0, 100000); /* clamp between 0-100 %rH */
 
 	return calc_hum;
 }
-- 
2.11.0

[PATCH 2/2] iio: bme680: simplify oversampling handling

[David Frey]

Temperature, pressure and humidity all expose and oversampling setting
that works in the same way.  Provide common handling for the
oversampling sysfs attributes.

Signed-off-by: David Frey <dpfrey@gmail.com>
---
 drivers/iio/chemical/bme680_core.c | 65 +++++++++++---------------------------
 1 file changed, 19 insertions(+), 46 deletions(-)

diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
index 0e79d03ecc40..446cf1cbef23 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -91,8 +91,6 @@ static const struct iio_chan_spec bme680_channels[] = {
 	},
 };
 
-static const int bme680_oversampling_avail[] = { 1, 2, 4, 8, 16 };
-
 static int bme680_read_calib(struct bme680_data *data,
 			     struct bme680_calib *calib)
 {
@@ -783,49 +781,14 @@ static int bme680_read_raw(struct iio_dev *indio_dev,
 	}
 }
 
-static int bme680_write_oversampling_ratio_temp(struct bme680_data *data,
-						int val)
-{
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(bme680_oversampling_avail); i++) {
-		if (bme680_oversampling_avail[i] == val) {
-			data->oversampling_temp = ilog2(val);
-
-			return bme680_chip_config(data);
-		}
-	}
-
-	return -EINVAL;
-}
-
-static int bme680_write_oversampling_ratio_press(struct bme680_data *data,
-						 int val)
-{
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(bme680_oversampling_avail); i++) {
-		if (bme680_oversampling_avail[i] == val) {
-			data->oversampling_press = ilog2(val);
-
-			return bme680_chip_config(data);
-		}
-	}
-
-	return -EINVAL;
-}
-
-static int bme680_write_oversampling_ratio_humid(struct bme680_data *data,
-						 int val)
+static int bme680_oversampling_value_to_setting(int value)
 {
 	int i;
-
-	for (i = 0; i < ARRAY_SIZE(bme680_oversampling_avail); i++) {
-		if (bme680_oversampling_avail[i] == val) {
-			data->oversampling_humid = ilog2(val);
-
-			return bme680_chip_config(data);
-		}
+	/* valid values are 2^n where n >=0 && n <= 4 */
+	for (i = 0; i <= 4; i++) {
+		u8 setting = (1 << i);
+		if (setting == value)
+			return setting;
 	}
 
 	return -EINVAL;
@@ -839,16 +802,26 @@ static int bme680_write_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+	{
+		int os_setting = bme680_oversampling_value_to_setting(val);
+		if (os_setting < 0)
+			return -EINVAL;
+
 		switch (chan->type) {
 		case IIO_TEMP:
-			return bme680_write_oversampling_ratio_temp(data, val);
+			data->oversampling_temp = os_setting;
+			break;
 		case IIO_PRESSURE:
-			return bme680_write_oversampling_ratio_press(data, val);
+			data->oversampling_press = os_setting;
+			break;
 		case IIO_HUMIDITYRELATIVE:
-			return bme680_write_oversampling_ratio_humid(data, val);
+			data->oversampling_humid = os_setting;
+			break;
 		default:
 			return -EINVAL;
 		}
+		return bme680_chip_config(data);
+	}
 	default:
 		return -EINVAL;
 	}
-- 
2.11.0

Re: [PATCH 2/2] iio: bme680: simplify oversampling handling

[Himanshu Jha]

On Tue, Aug 07, 2018 at 01:07:21PM -0700, David Frey wrote:
> Temperature, pressure and humidity all expose and oversampling setting
> that works in the same way.  Provide common handling for the
> oversampling sysfs attributes.
> 
> Signed-off-by: David Frey <dpfrey@gmail.com>
> ---
>  drivers/iio/chemical/bme680_core.c | 65 +++++++++++---------------------------
>  1 file changed, 19 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
> index 0e79d03ecc40..446cf1cbef23 100644
> --- a/drivers/iio/chemical/bme680_core.c
> +++ b/drivers/iio/chemical/bme680_core.c
> @@ -91,8 +91,6 @@ static const struct iio_chan_spec bme680_channels[] = {
>  	},
>  };
>  
> -static const int bme680_oversampling_avail[] = { 1, 2, 4, 8, 16 };
> -
>  static int bme680_read_calib(struct bme680_data *data,
>  			     struct bme680_calib *calib)
>  {
> @@ -783,49 +781,14 @@ static int bme680_read_raw(struct iio_dev *indio_dev,
>  	}
>  }
>  
> -static int bme680_write_oversampling_ratio_temp(struct bme680_data *data,
> -						int val)
> -{
> -	int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(bme680_oversampling_avail); i++) {
> -		if (bme680_oversampling_avail[i] == val) {
> -			data->oversampling_temp = ilog2(val);
> -
> -			return bme680_chip_config(data);
> -		}
> -	}
> -
> -	return -EINVAL;
> -}
> -
> -static int bme680_write_oversampling_ratio_press(struct bme680_data *data,
> -						 int val)
> -{
> -	int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(bme680_oversampling_avail); i++) {
> -		if (bme680_oversampling_avail[i] == val) {
> -			data->oversampling_press = ilog2(val);
> -
> -			return bme680_chip_config(data);
> -		}
> -	}
> -
> -	return -EINVAL;
> -}
> -
> -static int bme680_write_oversampling_ratio_humid(struct bme680_data *data,
> -						 int val)
> +static int bme680_oversampling_value_to_setting(int value)
>  {
>  	int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(bme680_oversampling_avail); i++) {
> -		if (bme680_oversampling_avail[i] == val) {
> -			data->oversampling_humid = ilog2(val);
> -
> -			return bme680_chip_config(data);
> -		}
> +	/* valid values are 2^n where n >=0 && n <= 4 */
> +	for (i = 0; i <= 4; i++) {
> +		u8 setting = (1 << i);
> +		if (setting == value)
> +			return setting;
>  	}

No, this is not correct!

Take example of:

osrs_h<2:0>		Humidity oversampling
011			oversampling * 4

And you will understand the ilog2() magic and also
look bme680_chip_config() how that value is written.

In your case, 

u8 osrs = FIELD_PREP(BME680_OSRS_HUMIDITY_MASK,
		     data->oversampling_humid + 1);

4 + 1 = 5(101) will be written instead of 4(101).

But I like the refactoring of using a generic oversampling
function. 

Just use:
while making a generic function bme680_write_oversampling_ratio()
and everything is fine as is using the ilog2 magic.
...
                switch (chan->type) {
                case IIO_TEMP:
                        return bme680_write_oversampling_ratio(data, val);
                case IIO_PRESSURE:
                        return bme680_write_oversampling_ratio(data, val);
                case IIO_HUMIDITYRELATIVE:
                        return bme680_write_oversampling_ratio(data, val);
                default:
                        return -EINVAL;
...


Thanks.
-- 
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology

Re: [PATCH 1/2] iio: bme680: perform cosmetic cleanup

[Himanshu Jha]

On Tue, Aug 07, 2018 at 01:07:20PM -0700, David Frey wrote:
>  * use consistent naming for masks. _MSK -> _MASK
>  * use GEN_MASK to define masks rather than hex constants
>  * consistently indent masks and values under the corresponding register
>    define
>  * use FIELD_GET instead of explicit mask and shift
>  * Join split lines in bme680_read_calib() which can fit in 80 chars
>  * Make use clamp macro
> 
> Signed-off-by: David Frey <dpfrey@gmail.com>
> ---

This patch is fine but ideally it should be split for each
change. "One patch per change policy!"

Dan is on vacation I guess otherwise he must have poked you
before me ;)

Also, please consider making subject:

"iio: chemical: bme680: .... .. "

Once you roll a v2, I will get it tested on my sensor.

And yes, please also CC Jonathan <jic23@kernel.org> so that
it doesn't get lost.

Thanks for making the effort, David!
-- 
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology