[PATCH v3 0/7] bme680 cleanup

[PATCH v3 0/7] bme680 cleanup

[David Frey]

v3:
  - Added "MSK->MASK" patch hunk accidentally omitted from v2
  - Applied Himanshu's "iio: chemical: bme680: Add check for val2 in
    the write_raw function" patch on top of jic23/iio rev 5e45729608dd
    before rebasing this patch series on top.

v2:
  This version addresses Himanshu's feedback
  - split v1 patch 1 into 6 commits
  - substantially changed "simplify oversampling handling" after review
    identified a bug in the previous patch version and I spent some more
    time thinking about the problem

v1:
  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 (7):
  iio: chemical: bme680: use clamp macro
  iio: chemical: bme680: cleanup bme680_read_calib formatting
  iio: chemical: bme680: indent #defines consistently
  iio: chemical: bme680: change MSK->MASK in #defines
  iio: chemical: bme680: use GENMASK macro
  iio: chemical: bme680: use FIELD_GET macro
  iio: chemical: bme680: simplify oversampling handling

 drivers/iio/chemical/bme680.h      |  17 +++--
 drivers/iio/chemical/bme680_core.c | 144 +++++++++++++------------------------
 2 files changed, 59 insertions(+), 102 deletions(-)

-- 
2.11.0

[PATCH v3 1/7] iio: chemical: bme680: use clamp macro

[David Frey]

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

diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
index 36b83c851515..35cbcb16c9f9 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -408,10 +408,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 v3 2/7] iio: chemical: bme680: cleanup bme680_read_calib formatting

[David Frey]

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

diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
index 35cbcb16c9f9..cde08d57e7d5 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_MSK);
 
 	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;
-- 
2.11.0

[PATCH v3 3/7] iio: chemical: bme680: indent #defines consistently

[David Frey]

Signed-off-by: David Frey <dpfrey@gmail.com>
---
 drivers/iio/chemical/bme680.h | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/chemical/bme680.h b/drivers/iio/chemical/bme680.h
index e049323f209a..dd4247d364a0 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			0x0F
 
 #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)
@@ -42,13 +42,12 @@
 #define BME680_BIT_H1_DATA_MSK			0x0F
 
 #define BME680_REG_RES_HEAT_RANGE		0x02
-#define BME680_RHRANGE_MSK			0x30
+#define   BME680_RHRANGE_MSK			0x30
 #define BME680_REG_RES_HEAT_VAL			0x00
 #define BME680_REG_RANGE_SW_ERR			0x04
-#define BME680_RSERROR_MSK			0xF0
+#define   BME680_RSERROR_MSK			0xF0
 #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
 
-- 
2.11.0

[PATCH v3 4/7] iio: chemical: bme680: change MSK->MASK in #defines

[David Frey]

Convert all defines to use "MASK" instead of a mix of "MSK" and "MASK"

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

diff --git a/drivers/iio/chemical/bme680.h b/drivers/iio/chemical/bme680.h
index dd4247d364a0..437d75c2bad5 100644
--- a/drivers/iio/chemical/bme680.h
+++ b/drivers/iio/chemical/bme680.h
@@ -39,13 +39,13 @@
 
 #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			0x0F
 
 #define BME680_REG_RES_HEAT_RANGE		0x02
-#define   BME680_RHRANGE_MSK			0x30
+#define   BME680_RHRANGE_MASK			0x30
 #define BME680_REG_RES_HEAT_VAL			0x00
 #define BME680_REG_RANGE_SW_ERR			0x04
-#define   BME680_RSERROR_MSK			0xF0
+#define   BME680_RSERROR_MASK			0xF0
 #define BME680_REG_RES_HEAT_0			0x5A
 #define BME680_REG_GAS_WAIT_0			0x64
 #define BME680_ADC_GAS_RES_SHIFT		6
diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
index cde08d57e7d5..7c7df40b615f 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -206,7 +206,7 @@ static int bme680_read_calib(struct bme680_data *data,
 		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) {
@@ -285,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 = (tmp & BME680_RHRANGE_MASK) / 16;
 
 	ret = regmap_read(data->regmap, BME680_REG_RES_HEAT_VAL, &tmp);
 	if (ret < 0) {
@@ -299,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 = (tmp & BME680_RSERROR_MASK) / 16;
 
 	return 0;
 }
-- 
2.11.0

[PATCH v3 5/7] iio: chemical: bme680: use GENMASK macro

[David Frey]

Replace hardcoded bit masks with GENMASK macro

Signed-off-by: David Frey <dpfrey@gmail.com>
---
 drivers/iio/chemical/bme680.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/chemical/bme680.h b/drivers/iio/chemical/bme680.h
index 437d75c2bad5..a9f2a9a6abc5 100644
--- a/drivers/iio/chemical/bme680.h
+++ b/drivers/iio/chemical/bme680.h
@@ -18,7 +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			0x0F
+#define   BME680_GAS_RANGE_MASK			GENMASK(3, 0)
 
 #define BME680_REG_CTRL_HUMIDITY		0x72
 #define   BME680_OSRS_HUMIDITY_MASK		GENMASK(2, 0)
@@ -39,13 +39,13 @@
 
 #define BME680_MAX_OVERFLOW_VAL			0x40000000
 #define BME680_HUM_REG_SHIFT_VAL		4
-#define BME680_BIT_H1_DATA_MASK			0x0F
+#define BME680_BIT_H1_DATA_MASK			GENMASK(3, 0)
 
 #define BME680_REG_RES_HEAT_RANGE		0x02
-#define   BME680_RHRANGE_MASK			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_MASK			0xF0
+#define   BME680_RSERROR_MASK			GENMASK(7, 4)
 #define BME680_REG_RES_HEAT_0			0x5A
 #define BME680_REG_GAS_WAIT_0			0x64
 #define BME680_ADC_GAS_RES_SHIFT		6
-- 
2.11.0

[PATCH v3 6/7] iio: chemical: bme680: use FIELD_GET macro

[David Frey]

Use the FIELD_GET macro instead of explicit mask and shift.

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

diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
index 7c7df40b615f..46afc93041f5 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -285,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_MASK) / 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) {
@@ -299,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_MASK) / 16;
+	calib->range_sw_err = FIELD_GET(BME680_RSERROR_MASK, tmp);
 
 	return 0;
 }
-- 
2.11.0

[PATCH v3 7/7] iio: chemical: 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 | 99 ++++++++++++++------------------------
 1 file changed, 36 insertions(+), 63 deletions(-)

diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
index 46afc93041f5..30614ef043ca 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)
 {
@@ -503,12 +501,20 @@ static int bme680_set_mode(struct bme680_data *data, bool mode)
 	return ret;
 }
 
+static u8 bme680_oversampling_to_reg(u8 val)
+{
+	return ilog2(val) + 1;
+}
+
 static int bme680_chip_config(struct bme680_data *data)
 {
 	struct device *dev = regmap_get_device(data->regmap);
 	int ret;
-	u8 osrs = FIELD_PREP(BME680_OSRS_HUMIDITY_MASK,
-			     data->oversampling_humid + 1);
+	u8 osrs;
+
+	osrs = FIELD_PREP(
+		BME680_OSRS_HUMIDITY_MASK,
+		bme680_oversampling_to_reg(data->oversampling_humid));
 	/*
 	 * Highly recommended to set oversampling of humidity before
 	 * temperature/pressure oversampling.
@@ -529,12 +535,12 @@ static int bme680_chip_config(struct bme680_data *data)
 		return ret;
 	}
 
-	osrs = FIELD_PREP(BME680_OSRS_TEMP_MASK, data->oversampling_temp + 1) |
-	       FIELD_PREP(BME680_OSRS_PRESS_MASK, data->oversampling_press + 1);
-
+	osrs = FIELD_PREP(BME680_OSRS_TEMP_MASK,
+			  bme680_oversampling_to_reg(data->oversampling_temp)) |
+	       FIELD_PREP(BME680_OSRS_PRESS_MASK,
+			  bme680_oversampling_to_reg(data->oversampling_press));
 	ret = regmap_write_bits(data->regmap, BME680_REG_CTRL_MEAS,
-				BME680_OSRS_TEMP_MASK |
-				BME680_OSRS_PRESS_MASK,
+				BME680_OSRS_TEMP_MASK | BME680_OSRS_PRESS_MASK,
 				osrs);
 	if (ret < 0)
 		dev_err(dev, "failed to write ctrl_meas register\n");
@@ -767,13 +773,13 @@ static int bme680_read_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
 		switch (chan->type) {
 		case IIO_TEMP:
-			*val = 1 << data->oversampling_temp;
+			*val = data->oversampling_temp;
 			return IIO_VAL_INT;
 		case IIO_PRESSURE:
-			*val = 1 << data->oversampling_press;
+			*val = data->oversampling_press;
 			return IIO_VAL_INT;
 		case IIO_HUMIDITYRELATIVE:
-			*val = 1 << data->oversampling_humid;
+			*val = data->oversampling_humid;
 			return IIO_VAL_INT;
 		default:
 			return -EINVAL;
@@ -783,52 +789,9 @@ 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 bool bme680_is_valid_oversampling(int rate)
 {
-	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);
-		}
-	}
-
-	return -EINVAL;
+	return (rate > 0 && rate <= 16 && is_power_of_2(rate));
 }
 
 static int bme680_write_raw(struct iio_dev *indio_dev,
@@ -842,16 +805,26 @@ static int bme680_write_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+	{
+		if (!bme680_is_valid_oversampling(val))
+			return -EINVAL;
+
 		switch (chan->type) {
 		case IIO_TEMP:
-			return bme680_write_oversampling_ratio_temp(data, val);
+			data->oversampling_temp = val;
+			break;
 		case IIO_PRESSURE:
-			return bme680_write_oversampling_ratio_press(data, val);
+			data->oversampling_press = val;
+			break;
 		case IIO_HUMIDITYRELATIVE:
-			return bme680_write_oversampling_ratio_humid(data, val);
+			data->oversampling_humid = val;
+			break;
 		default:
 			return -EINVAL;
 		}
+
+		return bme680_chip_config(data);
+	}
 	default:
 		return -EINVAL;
 	}
@@ -913,9 +886,9 @@ int bme680_core_probe(struct device *dev, struct regmap *regmap,
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
 	/* default values for the sensor */
-	data->oversampling_humid = ilog2(2); /* 2X oversampling rate */
-	data->oversampling_press = ilog2(4); /* 4X oversampling rate */
-	data->oversampling_temp = ilog2(8);  /* 8X oversampling rate */
+	data->oversampling_humid = 2; /* 2X oversampling rate */
+	data->oversampling_press = 4; /* 4X oversampling rate */
+	data->oversampling_temp = 8;  /* 8X oversampling rate */
 	data->heater_temp = 320; /* degree Celsius */
 	data->heater_dur = 150;  /* milliseconds */
 
-- 
2.11.0

Re: [PATCH v3 1/7] iio: chemical: bme680: use clamp macro

[Himanshu Jha]

On Fri, Aug 17, 2018 at 12:03:13PM -0700, David Frey wrote:
> Signed-off-by: David Frey <dpfrey@gmail.com>

Reviewed-by: Himanshu Jha <himanshujha199640@gmail.com>
Tested-by: Himanshu Jha <himanshujha199640@gmail.com>

Also, 0-day tested with build success!

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

Re: [PATCH v3 2/7] iio: chemical: bme680: cleanup bme680_read_calib formatting

[Himanshu Jha]

On Fri, Aug 17, 2018 at 12:03:14PM -0700, David Frey wrote:
> Signed-off-by: David Frey <dpfrey@gmail.com>

Reviewed-by: Himanshu Jha <himanshujha199640@gmail.com>
Tested-by: Himanshu Jha <himanshujha199640@gmail.com>

Also, 0-day tested with build success!

Thanks

One minor comment below.

Maybe Jonathan would fix while applying, so don't need a
new version for this.

> ---
>  drivers/iio/chemical/bme680_core.c | 36 ++++++++++++------------------------
>  1 file changed, 12 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
> index 35cbcb16c9f9..cde08d57e7d5 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_MSK);

Prefer tabs instead of spaces!
Please run checkpatch on the patch to get those warnings.

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

Same here.

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

Re: [PATCH v3 3/7] iio: chemical: bme680: indent #defines consistently

[Himanshu Jha]

On Fri, Aug 17, 2018 at 12:03:15PM -0700, David Frey wrote:
> Signed-off-by: David Frey <dpfrey@gmail.com>

Reviewed-by: Himanshu Jha <himanshujha199640@gmail.com>
Tested-by: Himanshu Jha <himanshujha199640@gmail.com>

Also, 0-day tested with build success!

Thanks


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

Re: [PATCH v3 4/7] iio: chemical: bme680: change MSK->MASK in #defines

[Himanshu Jha]

On Fri, Aug 17, 2018 at 12:03:16PM -0700, David Frey wrote:
> Convert all defines to use "MASK" instead of a mix of "MSK" and "MASK"
> 
> Signed-off-by: David Frey <dpfrey@gmail.com>

Reviewed-by: Himanshu Jha <himanshujha199640@gmail.com>
Tested-by: Himanshu Jha <himanshujha199640@gmail.com>

Also, 0-day tested with build success!

Thanks


> -	                (tmp_lsb & BME680_BIT_H1_DATA_MSK);
> +	                (tmp_lsb & BME680_BIT_H1_DATA_MASK);

Its here again so, perhaps better to send a new version instead.

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

Re: [PATCH v3 5/7] iio: chemical: bme680: use GENMASK macro

[Himanshu Jha]

On Fri, Aug 17, 2018 at 12:03:17PM -0700, David Frey wrote:
> Replace hardcoded bit masks with GENMASK macro
> 
> Signed-off-by: David Frey <dpfrey@gmail.com>

Reviewed-by: Himanshu Jha <himanshujha199640@gmail.com>
Tested-by: Himanshu Jha <himanshujha199640@gmail.com>

Also, 0-day tested with build success!

Thanks

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

Re: [PATCH v3 6/7] iio: chemical: bme680: use FIELD_GET macro

[Himanshu Jha]

On Fri, Aug 17, 2018 at 12:03:18PM -0700, David Frey wrote:
> Use the FIELD_GET macro instead of explicit mask and shift.
> 
> Signed-off-by: David Frey <dpfrey@gmail.com>

Reviewed-by: Himanshu Jha <himanshujha199640@gmail.com>
Tested-by: Himanshu Jha <himanshujha199640@gmail.com>

Also, 0-day tested with build success!

Thanks

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

Re: [PATCH v3 7/7] iio: chemical: bme680: simplify oversampling handling

[Himanshu Jha]

On Fri, Aug 17, 2018 at 12:03:19PM -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>

Reviewed-by: Himanshu Jha <himanshujha199640@gmail.com>
Tested-by: Himanshu Jha <himanshujha199640@gmail.com>

Also, 0-day tested with build success!

Thanks

With this I don't think my patch:
https://lore.kernel.org/lkml/20180811102636.6171-1-himanshujha199640@gmail.com/
is useful any more since bme680_is_valid_oversampling()
does the work pehaps.

The best part of this patch is supplying direct values and removes
the amiguity around ilog2(), especially at probe, when supplying
default values.

Thanks David for the series.

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

Re: [PATCH v3 1/7] iio: chemical: bme680: use clamp macro

[Jonathan Cameron]

On Sat, 18 Aug 2018 16:33:23 +0530
Himanshu Jha <himanshujha199640@gmail.com> wrote:

> On Fri, Aug 17, 2018 at 12:03:13PM -0700, David Frey wrote:
> > Signed-off-by: David Frey <dpfrey@gmail.com>  
> 
> Reviewed-by: Himanshu Jha <himanshujha199640@gmail.com>
> Tested-by: Himanshu Jha <himanshujha199640@gmail.com>
Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to play with it.

Thanks,

Jonathan

> 
> Also, 0-day tested with build success!
> 
> Thanks

Re: [PATCH v3 2/7] iio: chemical: bme680: cleanup bme680_read_calib formatting

[Jonathan Cameron]

On Sat, 18 Aug 2018 16:36:55 +0530
Himanshu Jha <himanshujha199640@gmail.com> wrote:

> On Fri, Aug 17, 2018 at 12:03:14PM -0700, David Frey wrote:
> > Signed-off-by: David Frey <dpfrey@gmail.com>  
> 
> Reviewed-by: Himanshu Jha <himanshujha199640@gmail.com>
> Tested-by: Himanshu Jha <himanshujha199640@gmail.com>
> 
> Also, 0-day tested with build success!
> 
> Thanks
> 
> One minor comment below.
> 
> Maybe Jonathan would fix while applying, so don't need a
> new version for this.
I've fixed them up.

I would have liked to see a little more detail in the description of what
was being changed and why though...

Jonathan
> 
> > ---
> >  drivers/iio/chemical/bme680_core.c | 36 ++++++++++++------------------------
> >  1 file changed, 12 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
> > index 35cbcb16c9f9..cde08d57e7d5 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_MSK);  
> 
> Prefer tabs instead of spaces!
> Please run checkpatch on the patch to get those warnings.
> 
> >  	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);  
> 
> Same here.
> 

Re: [PATCH v3 3/7] iio: chemical: bme680: indent #defines consistently

[Jonathan Cameron]

On Sat, 18 Aug 2018 16:37:25 +0530
Himanshu Jha <himanshujha199640@gmail.com> wrote:

> On Fri, Aug 17, 2018 at 12:03:15PM -0700, David Frey wrote:
> > Signed-off-by: David Frey <dpfrey@gmail.com>  
> 
> Reviewed-by: Himanshu Jha <himanshujha199640@gmail.com>
> Tested-by: Himanshu Jha <himanshujha199640@gmail.com>
> 
> Also, 0-day tested with build success!
> 
> Thanks
> 
> 

Applied,

There is one more suspicious bit of indenting in here
#define BME680_REG_CTRL_GAS_1			0x71
#define   BME680_RUN_GAS_MASK			BIT(4)
#define   BME680_NB_CONV_MASK			GENMASK(3, 0)
#define     BME680_RUN_GAS_EN_BIT		BIT(4)
#define     BME680_NB_CONV_0_VAL		0

I think RUN_GAS_EN_BIT should be one level lower?

Thanks,

Jonathan

Re: [PATCH v3 4/7] iio: chemical: bme680: change MSK->MASK in #defines

[Jonathan Cameron]

On Sat, 18 Aug 2018 16:39:18 +0530
Himanshu Jha <himanshujha199640@gmail.com> wrote:

> On Fri, Aug 17, 2018 at 12:03:16PM -0700, David Frey wrote:
> > Convert all defines to use "MASK" instead of a mix of "MSK" and "MASK"
> > 
> > Signed-off-by: David Frey <dpfrey@gmail.com>  
> 
> Reviewed-by: Himanshu Jha <himanshujha199640@gmail.com>
> Tested-by: Himanshu Jha <himanshujha199640@gmail.com>
> 
> Also, 0-day tested with build success!
> 
> Thanks
> 
> 
> > -	                (tmp_lsb & BME680_BIT_H1_DATA_MSK);
> > +	                (tmp_lsb & BME680_BIT_H1_DATA_MASK);  
> 
> Its here again so, perhaps better to send a new version instead.
> 
Applied and fixed up.  Thanks,

Jonathan

Re: [PATCH v3 5/7] iio: chemical: bme680: use GENMASK macro

[Jonathan Cameron]

On Sat, 18 Aug 2018 16:39:51 +0530
Himanshu Jha <himanshujha199640@gmail.com> wrote:

> On Fri, Aug 17, 2018 at 12:03:17PM -0700, David Frey wrote:
> > Replace hardcoded bit masks with GENMASK macro
> > 
> > Signed-off-by: David Frey <dpfrey@gmail.com>  
> 
> Reviewed-by: Himanshu Jha <himanshujha199640@gmail.com>
> Tested-by: Himanshu Jha <himanshujha199640@gmail.com>
> 
> Also, 0-day tested with build success!
> 
> Thanks
> 
Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it.

Thanks,

Jonathan

Re: [PATCH v3 6/7] iio: chemical: bme680: use FIELD_GET macro

[Jonathan Cameron]

On Sat, 18 Aug 2018 16:40:35 +0530
Himanshu Jha <himanshujha199640@gmail.com> wrote:

> On Fri, Aug 17, 2018 at 12:03:18PM -0700, David Frey wrote:
> > Use the FIELD_GET macro instead of explicit mask and shift.
> > 
> > Signed-off-by: David Frey <dpfrey@gmail.com>  
> 
> Reviewed-by: Himanshu Jha <himanshujha199640@gmail.com>
> Tested-by: Himanshu Jha <himanshujha199640@gmail.com>
> 
> Also, 0-day tested with build success!
> 
> Thanks
> 
Applied,

Thanks,

Jonathan

Re: [PATCH v3 7/7] iio: chemical: bme680: simplify oversampling handling

[Jonathan Cameron]

On Sat, 18 Aug 2018 16:47:50 +0530
Himanshu Jha <himanshujha199640@gmail.com> wrote:

> On Fri, Aug 17, 2018 at 12:03:19PM -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>  
> 
> Reviewed-by: Himanshu Jha <himanshujha199640@gmail.com>
> Tested-by: Himanshu Jha <himanshujha199640@gmail.com>
> 
> Also, 0-day tested with build success!
> 
> Thanks
> 
> With this I don't think my patch:
> https://lore.kernel.org/lkml/20180811102636.6171-1-himanshujha199640@gmail.com/
> is useful any more since bme680_is_valid_oversampling()
> does the work pehaps.
I don't think it does unfortunately as it doesn't check val2 at all.
So we need a new version of your patch unless I'm missing something.

> 
> The best part of this patch is supplying direct values and removes
> the amiguity around ilog2(), especially at probe, when supplying
> default values.
> 
> Thanks David for the series.

Indeed.  Some nice tidying up.  Thanks,

Jonathan
> 

Re: [PATCH v3 2/7] iio: chemical: bme680: cleanup bme680_read_calib formatting

[Himanshu Jha]

On Sun, Aug 19, 2018 at 04:54:21PM +0100, Jonathan Cameron wrote:
> On Sat, 18 Aug 2018 16:36:55 +0530
> Himanshu Jha <himanshujha199640@gmail.com> wrote:
> 
> > On Fri, Aug 17, 2018 at 12:03:14PM -0700, David Frey wrote:
> > > Signed-off-by: David Frey <dpfrey@gmail.com>  
> > 
> > Reviewed-by: Himanshu Jha <himanshujha199640@gmail.com>
> > Tested-by: Himanshu Jha <himanshujha199640@gmail.com>
> > 
> > Also, 0-day tested with build success!
> > 
> > Thanks
> > 
> > One minor comment below.
> > 
> > Maybe Jonathan would fix while applying, so don't need a
> > new version for this.
> I've fixed them up.
> 
> I would have liked to see a little more detail in the description of what
> was being changed and why though...

Indeed!
AFAIK Greg never takes any patch with empty commit log
but I've seen patches with empty log in IIO.

Point noted!
Now, I'll poke each and everyone if I find such a case
in future.

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

Re: [PATCH v3 3/7] iio: chemical: bme680: indent #defines consistently

[Himanshu Jha]

On Sun, Aug 19, 2018 at 05:02:15PM +0100, Jonathan Cameron wrote:
> On Sat, 18 Aug 2018 16:37:25 +0530
> Himanshu Jha <himanshujha199640@gmail.com> wrote:
> 
> > On Fri, Aug 17, 2018 at 12:03:15PM -0700, David Frey wrote:
> > > Signed-off-by: David Frey <dpfrey@gmail.com>  
> > 
> > Reviewed-by: Himanshu Jha <himanshujha199640@gmail.com>
> > Tested-by: Himanshu Jha <himanshujha199640@gmail.com>
> > 
> > Also, 0-day tested with build success!
> > 
> > Thanks
> > 
> > 
> 
> Applied,
> 
> There is one more suspicious bit of indenting in here
> #define BME680_REG_CTRL_GAS_1			0x71
> #define   BME680_RUN_GAS_MASK			BIT(4)
> #define   BME680_NB_CONV_MASK			GENMASK(3, 0)
> #define     BME680_RUN_GAS_EN_BIT		BIT(4)
> #define     BME680_NB_CONV_0_VAL		0
> 
> I think RUN_GAS_EN_BIT should be one level lower?

No, its fine. It is a value and third level indent is expected.
It should be set to 1 to initiate measurement. And
the total mask value is
	0b00001000 == BME680_RUN_GAS_EN_BIT | BME680_NB_CONV_0_VAL


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

Re: [PATCH v3 3/7] iio: chemical: bme680: indent #defines consistently

[Jonathan Cameron]

On Sun, 19 Aug 2018 22:58:44 +0530
Himanshu Jha <himanshujha199640@gmail.com> wrote:

> On Sun, Aug 19, 2018 at 05:02:15PM +0100, Jonathan Cameron wrote:
> > On Sat, 18 Aug 2018 16:37:25 +0530
> > Himanshu Jha <himanshujha199640@gmail.com> wrote:
> >   
> > > On Fri, Aug 17, 2018 at 12:03:15PM -0700, David Frey wrote:  
> > > > Signed-off-by: David Frey <dpfrey@gmail.com>    
> > > 
> > > Reviewed-by: Himanshu Jha <himanshujha199640@gmail.com>
> > > Tested-by: Himanshu Jha <himanshujha199640@gmail.com>
> > > 
> > > Also, 0-day tested with build success!
> > > 
> > > Thanks
> > > 
> > >   
> > 
> > Applied,
> > 
> > There is one more suspicious bit of indenting in here
> > #define BME680_REG_CTRL_GAS_1			0x71
> > #define   BME680_RUN_GAS_MASK			BIT(4)
> > #define   BME680_NB_CONV_MASK			GENMASK(3, 0)
> > #define     BME680_RUN_GAS_EN_BIT		BIT(4)
> > #define     BME680_NB_CONV_0_VAL		0
> > 
> > I think RUN_GAS_EN_BIT should be one level lower?  
> 
> No, its fine. It is a value and third level indent is expected.
> It should be set to 1 to initiate measurement. And
> the total mask value is
> 	0b00001000 == BME680_RUN_GAS_EN_BIT | BME680_NB_CONV_0_VAL
> 
Ah, then I'd argue it's name is wrong.  BIT elsewhere has been used
to indicate a field in the register, not a value.

Should probably just be BME680_RUN_GAS_EN?

Jonathan

> 

Re: [PATCH v3 3/7] iio: chemical: bme680: indent #defines consistently

[Himanshu Jha]

On Sun, Aug 19, 2018 at 08:14:39PM +0100, Jonathan Cameron wrote:
> On Sun, 19 Aug 2018 22:58:44 +0530
> Himanshu Jha <himanshujha199640@gmail.com> wrote:
> 
> > On Sun, Aug 19, 2018 at 05:02:15PM +0100, Jonathan Cameron wrote:
> > > On Sat, 18 Aug 2018 16:37:25 +0530
> > > Himanshu Jha <himanshujha199640@gmail.com> wrote:
> > >   
> > > > On Fri, Aug 17, 2018 at 12:03:15PM -0700, David Frey wrote:  
> > > > > Signed-off-by: David Frey <dpfrey@gmail.com>    
> > > > 
> > > > Reviewed-by: Himanshu Jha <himanshujha199640@gmail.com>
> > > > Tested-by: Himanshu Jha <himanshujha199640@gmail.com>
> > > > 
> > > > Also, 0-day tested with build success!
> > > > 
> > > > Thanks
> > > > 
> > > >   
> > > 
> > > Applied,
> > > 
> > > There is one more suspicious bit of indenting in here
> > > #define BME680_REG_CTRL_GAS_1			0x71
> > > #define   BME680_RUN_GAS_MASK			BIT(4)
> > > #define   BME680_NB_CONV_MASK			GENMASK(3, 0)
> > > #define     BME680_RUN_GAS_EN_BIT		BIT(4)
> > > #define     BME680_NB_CONV_0_VAL		0
> > > 
> > > I think RUN_GAS_EN_BIT should be one level lower?  
> > 
> > No, its fine. It is a value and third level indent is expected.
> > It should be set to 1 to initiate measurement. And
> > the total mask value is
> > 	0b00001000 == BME680_RUN_GAS_EN_BIT | BME680_NB_CONV_0_VAL
> > 
> Ah, then I'd argue it's name is wrong.  BIT elsewhere has been used
> to indicate a field in the register, not a value.
> 
> Should probably just be BME680_RUN_GAS_EN?

Sure.
I will send a patch soon.

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

Re: [PATCH v3 1/7] iio: chemical: bme680: use clamp macro

[David Frey]

On 8/19/2018 8:47 AM, Jonathan Cameron wrote:
> On Sat, 18 Aug 2018 16:33:23 +0530
> Himanshu Jha <himanshujha199640@gmail.com> wrote:
> 
>> On Fri, Aug 17, 2018 at 12:03:13PM -0700, David Frey wrote:
>>> Signed-off-by: David Frey <dpfrey@gmail.com>  
>>
>> Reviewed-by: Himanshu Jha <himanshujha199640@gmail.com>
>> Tested-by: Himanshu Jha <himanshujha199640@gmail.com>
> Applied to the togreg branch of iio.git and pushed out as testing
> for the autobuilders to play with it.

Hi,

I'm trying to understand the linux-iio mailing list workflow a bit
better.  I have
git://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git as my "iio"
remote in my Linux kernel repository.  From that remote, I can see
remotes/iio/togreg.  When I do a "git fetch", the togreg branch hasn't
been updated.  Did Jonathan mean that he applied the patch to the togreg
branch in his local repository, but hasn't yet pushed it to the one that
I have listed above?  Are the "autobuilders" specific to IIO?  Is this
infrastructure publicly accessible or is it only available to specific
users?  If it's public, where can I view it/learn more about it?

Thanks,
David

[PATCH] iio: chemical: bme680: Remove field value defines

[David Frey]

Remove BME680_RUN_GAS_EN_BIT and BME680_NB_CONV_0_VAL field value
definitions because the fields are simply boolean and integer
respectively.

Signed-off-by: David Frey <dpfrey@gmail.com>
---
This patch applies on top of my "indent #defines consistently" v3 patch.
Appologies if I should have submitted this patch in a different way.  If
I should have submitted this differently, I would appreciate a pointer
on what I should have done in this case.

BME680_RUN_GAS_EN_BIT was indeed somewhat wrongly formatted, but the
issue was not the indentation level, but rather that I should have
followed immediately after BME680_RUN_GAS_MASK.  Once I moved it there,
I realized that neither this definition nor BME680_NB_CONV_0_VAL really
added any value and hence I removed both in this patch.

 drivers/iio/chemical/bme680.h      | 2 --
 drivers/iio/chemical/bme680_core.c | 5 +++--
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/chemical/bme680.h b/drivers/iio/chemical/bme680.h
index dd4247d364a0..48dc9e50e017 100644
--- a/drivers/iio/chemical/bme680.h
+++ b/drivers/iio/chemical/bme680.h
@@ -54,8 +54,6 @@
 #define BME680_REG_CTRL_GAS_1			0x71
 #define   BME680_RUN_GAS_MASK			BIT(4)
 #define   BME680_NB_CONV_MASK			GENMASK(3, 0)
-#define     BME680_RUN_GAS_EN_BIT		BIT(4)
-#define     BME680_NB_CONV_0_VAL		0
 
 #define BME680_REG_MEAS_STAT_0			0x1D
 #define   BME680_GAS_MEAS_BIT			BIT(6)
diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
index cde08d57e7d5..01ca7ba64ea0 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -566,10 +566,11 @@ static int bme680_gas_config(struct bme680_data *data)
 		return ret;
 	}
 
-	/* Selecting the runGas and NB conversion settings for the sensor */
+	/* Enable the gas sensor and select heater profile set-point 0 */
 	ret = regmap_update_bits(data->regmap, BME680_REG_CTRL_GAS_1,
 				 BME680_RUN_GAS_MASK | BME680_NB_CONV_MASK,
-				 BME680_RUN_GAS_EN_BIT | BME680_NB_CONV_0_VAL);
+				 FIELD_PREP(BME680_RUN_GAS_MASK, 1) |
+				 FIELD_PREP(BME680_NB_CONV_MASK, 0));
 	if (ret < 0)
 		dev_err(dev, "failed to write ctrl_gas_1 register\n");
 
-- 
2.11.0

Re: [PATCH v3 1/7] iio: chemical: bme680: use clamp macro

[Himanshu Jha]

On Mon, Aug 20, 2018 at 10:18:54AM -0700, David Frey wrote:
> On 8/19/2018 8:47 AM, Jonathan Cameron wrote:
> > On Sat, 18 Aug 2018 16:33:23 +0530
> > Himanshu Jha <himanshujha199640@gmail.com> wrote:
> > 
> >> On Fri, Aug 17, 2018 at 12:03:13PM -0700, David Frey wrote:
> >>> Signed-off-by: David Frey <dpfrey@gmail.com>  
> >>
> >> Reviewed-by: Himanshu Jha <himanshujha199640@gmail.com>
> >> Tested-by: Himanshu Jha <himanshujha199640@gmail.com>
> > Applied to the togreg branch of iio.git and pushed out as testing
> > for the autobuilders to play with it.
> 
> Hi,
> 
> I'm trying to understand the linux-iio mailing list workflow a bit
> better.  I have
> git://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git as my "iio"
> remote in my Linux kernel repository.  From that remote, I can see
> remotes/iio/togreg.  When I do a "git fetch", the togreg branch hasn't
> been updated.  Did Jonathan mean that he applied the patch to the togreg
> branch in his local repository, but hasn't yet pushed it to the one that
> I have listed above?  Are the "autobuilders" specific to IIO?  Is this
> infrastructure publicly accessible or is it only available to specific
> users?  If it's public, where can I view it/learn more about it?

The patches are currently in "testing" branch of iio.git repo.

The testing branch link is tracked by kbuild test robot at

0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Now, this testing service tracks *maintainers* trees and whenever they
push anything, the bot starts testing them in more than 200 different
kernel configuration and reports them with build reggression or success.
If there is a regression then the bot also bisects the bad commit and
sends it to the relevant people.

Also, apart from building/booting kernel it also tests through
sparse, smatch, coccinelle and others.

Smatch bugs are sent to Dan Carpenter(inventor of Smatch)
Coccinelle bugs are sent to Julia Lawall(inventor of Coccinelle)

AFAIK an year ago it used to do power management benchmarks and lots
of others but kernel community didn't want those. I got one of those
intimidating reports once with all those benchmarks.

Also, these services are *mostly* only available to maintainers since it
is most useful to them. Saves a lot of their time for compile testing and
you know how much would time it would take to do a `make allyesconfig'

I was luckly enough to get one of these services last year while working
with Luis R. Rodriguez.

But you can test on your own if you want, the code is freely available:
https://github.com/fengguang/lkp-tests

So, I expect Jonathan does the following:

1. apply and push patches to 'testing' branch.
2. As soon it is pushed, bot starts testing them and reports back.
3. If build success, then applies to 'togreg' branch.
4. If failure, then reports to the patch submitter.

Other than that I don't know what 'autobuilders' scripts he posses !?

See Fenggung Wu's reply:
https://lore.kernel.org/lkml/20180119014803.n75l5vrxlpifm3sc@wfg-t540p.sh.intel.com/

" Here are the complete list of 800+ trees we monitored:
https://git.kernel.org/pub/scm/linux/kernel/git/wfg/lkp-tests.git/tree/repo/linux "

Goddamn 800+ tress *__*

Sample report:
---------------------------------------------------------------------
Date: Sat, 18 Aug 2018 16:00:43 +0800
From: kbuild test robot <lkp@intel.com>
To: Himanshu Jha <himanshujha199640@gmail.com>
Subject: [himanshujha199640:20180817-dpfrey-cleanups-bme680] BUILD SUCCESS 5aad04740ef119704edf29f84714a7ae5b96f683
Message-ID: <5b77d22b.QBiWNTiXoXy+Kc/2%lkp@intel.com>
User-Agent: Heirloom mailx 12.5 6/20/10
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit

tree/branch: https://github.com/himanshujha199640/linux-next  20180817-dpfrey-cleanups-bme680
branch HEAD: 5aad04740ef119704edf29f84714a7ae5b96f683  iio: chemical: bme680: simplify oversampling handling

elapsed time: 65m

configs tested: 121

The following configs have been built successfully.
More configs may be tested in the coming days.

alpha                               defconfig
parisc                            allnoconfig
parisc                         b180_defconfig
parisc                        c3000_defconfig
parisc                              defconfig
x86_64                             acpi-redef
x86_64                           allyesdebian
x86_64                                nfsroot
i386                               tinyconfig
i386                   randconfig-x018-201832
i386                   randconfig-x017-201832
i386                   randconfig-x010-201832
i386                   randconfig-x016-201832
i386                   randconfig-x013-201832
i386                   randconfig-x011-201832
i386                   randconfig-x015-201832
i386                   randconfig-x019-201832
i386                   randconfig-x014-201832
i386                   randconfig-x012-201832
x86_64                              fedora-25
x86_64                                    lkp
x86_64                                   rhel
x86_64                               rhel-7.2
i386                     randconfig-n0-201832
x86_64                 randconfig-x009-201832
x86_64                 randconfig-x000-201832
x86_64                 randconfig-x004-201832
x86_64                 randconfig-x001-201832
x86_64                 randconfig-x003-201832
x86_64                 randconfig-x007-201832
x86_64                 randconfig-x008-201832
x86_64                 randconfig-x005-201832
x86_64                 randconfig-x002-201832
x86_64                 randconfig-x006-201832
ia64                             alldefconfig
ia64                              allnoconfig
ia64                                defconfig
i386                     randconfig-i0-201832
i386                     randconfig-i1-201832
i386                     randconfig-a0-201832
i386                     randconfig-a1-201832
x86_64                 randconfig-x019-201832
x86_64                 randconfig-x013-201832
x86_64                 randconfig-x017-201832
x86_64                 randconfig-x012-201832
x86_64                 randconfig-x018-201832
x86_64                 randconfig-x010-201832
x86_64                 randconfig-x014-201832
x86_64                 randconfig-x016-201832
x86_64                 randconfig-x011-201832
x86_64                 randconfig-x015-201832
i386                             alldefconfig
i386                              allnoconfig
i386                                defconfig
i386                     randconfig-s0-201832
i386                     randconfig-s1-201832
x86_64                           allmodconfig
i386                             allmodconfig
powerpc                           allnoconfig
powerpc                             defconfig
powerpc                       ppc64_defconfig
s390                        default_defconfig
x86_64                   randconfig-i0-201832
sparc                               defconfig
sparc64                           allnoconfig
sparc64                             defconfig
microblaze                      mmu_defconfig
microblaze                    nommu_defconfig
i386                             allyesconfig
i386                   randconfig-x077-201832
i386                   randconfig-x076-201832
i386                   randconfig-x070-201832
i386                   randconfig-x075-201832
i386                   randconfig-x072-201832
i386                   randconfig-x074-201832
i386                   randconfig-x071-201832
i386                   randconfig-x078-201832
i386                   randconfig-x079-201832
i386                   randconfig-x073-201832
sh                                allnoconfig
sh                          rsk7269_defconfig
sh                  sh7785lcr_32bit_defconfig
sh                            titan_defconfig
i386                   randconfig-x002-201832
i386                   randconfig-x003-201832
i386                   randconfig-x005-201832
i386                   randconfig-x008-201832
i386                   randconfig-x000-201832
i386                   randconfig-x007-201832
i386                   randconfig-x001-201832
i386                   randconfig-x004-201832
i386                   randconfig-x009-201832
i386                   randconfig-x006-201832
m68k                       m5475evb_defconfig
m68k                          multi_defconfig
m68k                           sun3_defconfig
c6x                        evmc6678_defconfig
h8300                    h8300h-sim_defconfig
nios2                         10m50_defconfig
xtensa                       common_defconfig
xtensa                          iss_defconfig
arm                               allnoconfig
arm                         at91_dt_defconfig
arm                           efm32_defconfig
arm                          exynos_defconfig
arm                        multi_v5_defconfig
arm                        multi_v7_defconfig
arm                        shmobile_defconfig
arm                           sunxi_defconfig
arm64                             allnoconfig
arm64                               defconfig
openrisc                    or1ksim_defconfig
um                             i386_defconfig
um                           x86_64_defconfig
mips                           32r2_defconfig
mips                         64r6el_defconfig
mips                              allnoconfig
mips                      fuloong2e_defconfig
mips                                   jz4740
mips                      malta_kvm_defconfig
mips                                     txx9

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
---------------------------------------------------------------------

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

Re: [PATCH v3 1/7] iio: chemical: bme680: use clamp macro

[Jonathan Cameron]

On 20 August 2018 18:18:54 BST, David Frey <dpfrey@gmail.com> wrote:
>On 8/19/2018 8:47 AM, Jonathan Cameron wrote:
>> On Sat, 18 Aug 2018 16:33:23 +0530
>> Himanshu Jha <himanshujha199640@gmail.com> wrote:
>> 
>>> On Fri, Aug 17, 2018 at 12:03:13PM -0700, David Frey wrote:
>>>> Signed-off-by: David Frey <dpfrey@gmail.com>  
>>>
>>> Reviewed-by: Himanshu Jha <himanshujha199640@gmail.com>
>>> Tested-by: Himanshu Jha <himanshujha199640@gmail.com>
>> Applied to the togreg branch of iio.git and pushed out as testing
>> for the autobuilders to play with it.
>
>Hi,

Hi brief as I am on my phone.
>
>I'm trying to understand the linux-iio mailing list workflow a bit
>better.  I have
>git://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git as my "iio"
>remote in my Linux kernel repository.  From that remote, I can see
>remotes/iio/togreg.  When I do a "git fetch", the togreg branch hasn't
>been updated.  Did Jonathan mean that he applied the patch to the
>togreg
>branch in his local repository, but hasn't yet pushed it to the one
>that
>I have listed above?

Exactly, the point of that is to distinguish between patches that are taking a different path.
Mostly this is just those applied to the fixes-togreg branch.

>  Are the "autobuilders" specific to IIO?  

Nope. This is referring mainly to the 0day builders intel provide to the community.

https://01.org/lkp/documentation/0-day-test-service

>Is this
>infrastructure publicly accessible or is it only available to specific
>users?  If it's public, where can I view it/learn more about it?

As above. It will sometimes pick directly from the mailing lists bit not always.  

You can also send patches to it for testing as that page describes.

For trees that feed into mainline, you can request the whole tree is monitored so 
for example I get a report of what tests have run and potential issues a few hours after I push
 things to the testing branch.  Typically builds a few hundred configs in that time and keeps going afterwards if resources allow.

The move to pushing out a togreg branch publicly is a manual step that I can only do when
on my development machine and which I forget sometimes.  Given reports were good when I got
 them today, I will probably push togreg out soonish.

Oday is great.  Other services such as kernel ci run on smaller numbers of trees and do boot
 tests as well. Mostly not so relevant for IIO though as we tend to need a bit more than a
 boot.  I always meant to do a city test rig for a few boards and devices specific to IIO but it
 would only be of limited use due to the huge number of devices we have supported.

Jonathan

>
>Thanks,
>David

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH v3 2/7] iio: chemical: bme680: cleanup bme680_read_calib formatting

[David Frey]

On 8/18/2018 4:06 AM, Himanshu Jha wrote:
> On Fri, Aug 17, 2018 at 12:03:14PM -0700, David Frey wrote:
>> Signed-off-by: David Frey <dpfrey@gmail.com>
> 
> One minor comment below.

<snip>

>> diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
>> index 35cbcb16c9f9..cde08d57e7d5 100644
>> --- a/drivers/iio/chemical/bme680_core.c
>> +++ b/drivers/iio/chemical/bme680_core.c
<snip>
>> @@ -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_MSK);
> 
> Prefer tabs instead of spaces!
> Please run checkpatch on the patch to get those warnings.

Thanks for telling me about the existence of checkpatch.pl.  The
official docs
(https://www.kernel.org/doc/html/v4.18/process/coding-style.html) are
pretty light on information about indentation/alignment, but there is
this relevant quote: "Outside of comments, documentation and except in
Kconfig, spaces are never used for indentation, and the above example is
deliberately broken."

I thought that the way I indented it was optimal because I used spaces a
tab for indentation and then spaces for alignment, but checkpatch.pl
seems to disagree.  Based on a quick read of the code, it seems that it
will complain about more than 7 spaces in a row.

I guess you could achieve visually identical alignment (where "------->"
is tab and "_" is space) and this would still pass checkpatch.pl.  Note
that I changed the identifier name slightly to demonstrate a mix of tabs
and spaces.

------->calib->par_h123 = (tmp_msb << BME680_HUM_REG_SHIFT_VAL) |
------->------->------->__(tmp_lsb & BME680_BIT_H1_DATA_MSK);

What I don't understand is the logic used to justify the original
indentation:
------->calib->par_h1 = (tmp_msb << BME680_HUM_REG_SHIFT_VAL) |
------->------->------->------->(tmp_lsb & BME680_BIT_H1_DATA_MSK);

Why not?
------->calib->par_h1 = (tmp_msb << BME680_HUM_REG_SHIFT_VAL) |
------->------->------->(tmp_lsb & BME680_BIT_H1_DATA_MSK);

Or?
------->calib->par_h1 = (tmp_msb << BME680_HUM_REG_SHIFT_VAL) |
------->------->(tmp_lsb & BME680_BIT_H1_DATA_MSK);

I'm not trying to turn this into a crazy bike shedding thread.  I just
want to understand the rules a bit better.

Thanks,
David

Re: [PATCH v3 2/7] iio: chemical: bme680: cleanup bme680_read_calib formatting

[Himanshu Jha]

CC'ing Joe

On Mon, Aug 20, 2018 at 12:24:43PM -0700, David Frey wrote:
> On 8/18/2018 4:06 AM, Himanshu Jha wrote:
> > On Fri, Aug 17, 2018 at 12:03:14PM -0700, David Frey wrote:
> >> Signed-off-by: David Frey <dpfrey@gmail.com>
> > 
> > One minor comment below.
> 
> <snip>
> 
> >> diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
> >> index 35cbcb16c9f9..cde08d57e7d5 100644
> >> --- a/drivers/iio/chemical/bme680_core.c
> >> +++ b/drivers/iio/chemical/bme680_core.c
> <snip>
> >> @@ -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_MSK);
> > 
> > Prefer tabs instead of spaces!
> > Please run checkpatch on the patch to get those warnings.
> 
> Thanks for telling me about the existence of checkpatch.pl.  The
> official docs
> (https://www.kernel.org/doc/html/v4.18/process/coding-style.html) are
> pretty light on information about indentation/alignment, but there is
> this relevant quote: "Outside of comments, documentation and except in
> Kconfig, spaces are never used for indentation, and the above example is
> deliberately broken."
> 
> I thought that the way I indented it was optimal because I used spaces a
> tab for indentation and then spaces for alignment, but checkpatch.pl
> seems to disagree.  Based on a quick read of the code, it seems that it
> will complain about more than 7 spaces in a row.
> 
> I guess you could achieve visually identical alignment (where "------->"
> is tab and "_" is space) and this would still pass checkpatch.pl.  Note
> that I changed the identifier name slightly to demonstrate a mix of tabs
> and spaces.
> 
> ------->calib->par_h123 = (tmp_msb << BME680_HUM_REG_SHIFT_VAL) |
> ------->------->------->__(tmp_lsb & BME680_BIT_H1_DATA_MSK);
> 
> What I don't understand is the logic used to justify the original
> indentation:
> ------->calib->par_h1 = (tmp_msb << BME680_HUM_REG_SHIFT_VAL) |
> ------->------->------->------->(tmp_lsb & BME680_BIT_H1_DATA_MSK);

Hmm.
Which looks more readable to you ?

This:
        var1 = (calib->par_p9 * (((press_comp >> 3) *
                        (press_comp >> 3)) >> 13)) >> 12;
        var2 = ((press_comp >> 2) * calib->par_p8) >> 13;
        var3 = ((press_comp >> 8) * (press_comp >> 8) *
                        (press_comp >> 8) * calib->par_p10) >> 17;

Or this:
        var1 = (calib->par_p9 * (((press_comp >> 3) *
                (press_comp >> 3)) >> 13)) >> 12;
        var2 = ((press_comp >> 2) * calib->par_p8) >> 13;
        var3 = ((press_comp >> 8) * (press_comp >> 8) *
                (press_comp >> 8) * calib->par_p10) >> 17;

Not sure if my rationale of "readablilty" applies to you and others.
But first case looks more appropriate to me and I don't know why!

> Why not?
> ------->calib->par_h1 = (tmp_msb << BME680_HUM_REG_SHIFT_VAL) |
> ------->------->------->(tmp_lsb & BME680_BIT_H1_DATA_MSK);

This looks better too.
Can't decide.

> Or?
> ------->calib->par_h1 = (tmp_msb << BME680_HUM_REG_SHIFT_VAL) |
> ------->------->(tmp_lsb & BME680_BIT_H1_DATA_MSK);

No.
This is fairly simple, why would anyone do it like that ?
It should be atleast to the right of assignment operator with
a space.

Rationale is: Use tabs as far as possible and if tabs make it
look ugly then use spacaes instead of "a" tab and align properly.

Mix of tabs + few spaces:
static int bme680_read_raw(struct iio_dev *indio_dev,
			   struct iio_chan_spec const *chan,
			   int *val, int *val2, long mask)
Use of only tabs:
static int bme680_read_raw(struct iio_dev *indio_dev,
				struct iio_chan_spec const *chan,
				int *val, int *val2, long mask)

> I'm not trying to turn this into a crazy bike shedding thread.  I just
> want to understand the rules a bit better.

Joe will provide a much better answer for checkpatch and rules.

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

Re: [PATCH] iio: chemical: bme680: Remove field value defines

[Himanshu Jha]

On Mon, Aug 20, 2018 at 10:39:59AM -0700, David Frey wrote:
> Remove BME680_RUN_GAS_EN_BIT and BME680_NB_CONV_0_VAL field value
> definitions because the fields are simply boolean and integer
> respectively.
> 
> Signed-off-by: David Frey <dpfrey@gmail.com>
Reviewed-by: Himanshu Jha <himanshujha199640@gmail.com>
Tested-by: Himanshu Jha <himanshujha199640@gmail.com>

> ---
> This patch applies on top of my "indent #defines consistently" v3 patch.
> Appologies if I should have submitted this patch in a different way.  If
> I should have submitted this differently, I would appreciate a pointer
> on what I should have done in this case.

This applies cleanly so no worries I guess.
Would have been better to send this patch as a separate thread since
thread becomes complex and its hard to find the new patch in the
nested series of replies.

> BME680_RUN_GAS_EN_BIT was indeed somewhat wrongly formatted, but the
> issue was not the indentation level, but rather that I should have
> followed immediately after BME680_RUN_GAS_MASK.  Once I moved it there,
> I realized that neither this definition nor BME680_NB_CONV_0_VAL really
> added any value and hence I removed both in this patch.

My main intention was to make it explicit that we are selected NB_CONV_0
set point, and I didn't knew about FIELD_PREP helper macro until you
pointed me out in the early review cycle.

Now, it is much appropriate.

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

Re: [PATCH] iio: chemical: bme680: Remove field value defines

[Jonathan Cameron]

On Wed, 22 Aug 2018 16:14:05 +0530
Himanshu Jha <himanshujha199640@gmail.com> wrote:

> On Mon, Aug 20, 2018 at 10:39:59AM -0700, David Frey wrote:
> > Remove BME680_RUN_GAS_EN_BIT and BME680_NB_CONV_0_VAL field value
> > definitions because the fields are simply boolean and integer
> > respectively.
> > 
> > Signed-off-by: David Frey <dpfrey@gmail.com>  
> Reviewed-by: Himanshu Jha <himanshujha199640@gmail.com>
> Tested-by: Himanshu Jha <himanshujha199640@gmail.com>

Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to play with it.

Thanks,

Jonathan

> 
> > ---
> > This patch applies on top of my "indent #defines consistently" v3 patch.
> > Appologies if I should have submitted this patch in a different way.  If
> > I should have submitted this differently, I would appreciate a pointer
> > on what I should have done in this case.  
> 
> This applies cleanly so no worries I guess.
> Would have been better to send this patch as a separate thread since
> thread becomes complex and its hard to find the new patch in the
> nested series of replies.
> 
> > BME680_RUN_GAS_EN_BIT was indeed somewhat wrongly formatted, but the
> > issue was not the indentation level, but rather that I should have
> > followed immediately after BME680_RUN_GAS_MASK.  Once I moved it there,
> > I realized that neither this definition nor BME680_NB_CONV_0_VAL really
> > added any value and hence I removed both in this patch.  
> 
> My main intention was to make it explicit that we are selected NB_CONV_0
> set point, and I didn't knew about FIELD_PREP helper macro until you
> pointed me out in the early review cycle.
> 
> Now, it is much appropriate.
> 
> Thanks