bme680 cleanup

PATCH Superseded David Frey 2 participants 5 messages Aug 7, 2018 – Aug 8, 2018 IIO

Share & tools

David Frey

author

2018-08-07 22:19 UTC #0 lore raw

cover bme680 cleanup

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

Signature · 1 line

2.11.0

David Frey

author

2018-08-07 22:19 UTC #1 lore raw

PATCH 1/2 iio: bme680: perform cosmetic cleanup

* 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 <[email protected]>

---
 drivers/iio/chemical/bme680.h      | 17 +++++++-------
 drivers/iio/chemical/bme680_core.c | 45 +++++++++++++-------------------------
 2 files changed, 23 insertions(+), 39 deletions(-)

drivers/iio/chemical/bme680.h +8 -9

index e049323f209a..a9f2a9a6abc5 100644
@@ -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

drivers/iio/chemical/bme680_core.c +15 ~~-30~~

141 lines not on this page · raw message

Signature · 1 line

2.11.0

Himanshu Jha

2018-08-08 13:14 UTC #2 lore raw

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

Quoted 10 lines · 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 <[email protected]>
---

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 <[email protected]> so that it doesn't get lost.

Thanks for making the effort, David!

Signature · 4 lines

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

David Frey

author

2018-08-07 22:19 UTC #3 lore raw

PATCH 2/2 iio: bme680: simplify oversampling handling

In reply to 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 <[email protected]>

---
 drivers/iio/chemical/bme680_core.c | 65 +++++++++++---------------------------
 1 file changed, 19 insertions(+), 46 deletions(-)

drivers/iio/chemical/bme680_core.c +19 ~~-46~~

index 0e79d03ecc40..446cf1cbef23 100644
@@ -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;
 	}

Signature · 1 line

2.11.0

Himanshu Jha

2018-08-08 13:07 UTC #4 lore raw

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

Quoted 66 lines · 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 <[email protected]>

---
 drivers/iio/chemical/bme680_core.c | 65 +++++++++++---------------------------
 1 file changed, 19 insertions(+), 46 deletions(-)

drivers/iio/chemical/bme680_core.c +1 ~~-39~~

index 0e79d03ecc40..446cf1cbef23 100644
@@ -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) {

drivers/iio/chemical/bme680_core.c static int bme680_read_raw(struct iio_dev *indio_dev, · line 792

Quoted 10 lines

drivers/iio/chemical/bme680_core.c +5 -4

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

Signature · 4 lines

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