Re: [PATCH v3] iio: chemical: Add support for Bosch BME680 sensor
Hi Jonathan, On Sun, Jul 15, 2018 at 10:10:36AM +0100, Jonathan Cameron wrote: > On Sat, 14 Jul 2018 13:03:42 +0530 > Himanshu Jha wrote: > > > Hi David, > > > > On Fri, Jul 13, 2018 at 01:42:35PM -0700, David Frey wrote: > > > Hi Himanshu Jha, > > > > > > First a bit of background. I'm working on a device which will contain a > > > bme680 sensor. A colleague of mine started work on a Linux kernel driver > > > for the chip a little while ago. The (WIP) driver can be found here: > > > https://github.com/mangOH/mangOH/tree/master/linux_kernel_modules/bme680 > > > > Great! > > > > > This driver is written targeting an older kernel (3.18.x) because that's > > > the > > > kernel we're stuck on for now. Rather than writing the driver from > > > scratch, > > > what we did was write the Linux kernel driver as a wrapper around the > > > Bosch > > > code. My theory at the time was that Bosch made the chip, so they > > > probably > > > know what they're doing when it comes to writing a driver library. After > > > having looked at the code in more detail, I'm less confident that our > > > approach was the best one. I'm not attempting to upstream the driver > > > built > > > by my colleague and I'm not trying to request review of this code either. > > > I > > > simply want to make you aware of it so that you can look at it to get some > > > ideas. > > > > Thanks for taking your time to review. > > > > > I have included a number of comments on your driver below. Keep up the > > > good > > > work! > > > > > > >+++ b/drivers/iio/chemical/bme680.h > > > >@@ -0,0 +1,99 @@ > > > >+/* SPDX-License-Identifier: GPL-2.0 */ > > > >+#ifndef BME680_H_ > > > >+#define BME680_H_ > > > >+ > > > >+#define BME680_REG_CHIP_I2C_ID 0xD0 > > > >+#define BME680_REG_CHIP_SPI_ID 0x50 > > > >+#define BME680_CHIP_ID_VAL 0x61 > > > Try to be consistent with the indenting of the defines. I think this > > > would > > > be clearest: > > > #define BME680_REG_X 0x00 > > > #define BME680_X_FOO_EN_MASKBIT(0) > > > #define BME680_X_BAR_MASK GENMASK(3, 1) > > > #define BME680_BAR_VAL1 3 > > > #define BME680_BAR_VAL2 7 > > > > > > This way the register, field definition and field values are all visually > > > distinctive. > > > > I have used this pattern everywhere where applicable. But not applied > > for *_VAL, would definitely follow this up. > > > > > >+#define BME680_REG_SOFT_RESET 0xE0 > > > The datasheet says that the soft reset register differs for I2C and SPI. > > > For I2C it is 0xE0 and for SPI it is 0x60 when page 0 is selected. > > > > That's really a stupid mistake :( > > I have exported these individual initialization code in the I2C & SPI > > drivers respectively but it slipped my mind somehow. This device has > > peculiar characteristics in register addressing. > > > > I will correct this in next version. > > > > > >+#define BME680_CMD_SOFTRESET0xB6 > > > >+#define BME680_REG_STATUS 0x73 > > > >+#define BME680_SPI_MEM_PAGE_BIT BIT(4) > > > >+#define BME680_SPI_MEM_PAGE_1_VAL 1 > > > >+ > > > >+#define BME680_OSRS_TEMP_X(osrs_t) ((osrs_t) << 5) > > > >+#define BME680_OSRS_PRESS_X(osrs_p) ((osrs_p) << 2) > > > >+#define BME680_OSRS_HUMID_X(osrs_h) ((osrs_h) << 0) > > > You could use the FIELD_PREP macro from to eliminate > > > the > > > need for these macros. For example: > > > ctrl_meas_reg = FIELD_PREP(BME680_OSRS_TEMP_MASK, temp_val) | > > > FIELD_PREP(BME680_OSRS_PRESS_MASK, press_val) | > > > FIELD_PREP(BME880_MODE_MASK, mode_val); > > > > Ah, yes! I didn't knew about these magic macros. It will remove some > > log2() computation hacks from my code. > > > > > >+ > > > >+#define BME680_REG_TEMP_MSB 0x22 > > > >+#define BME680_REG_PRESS_MSB0x1F > > > >+#define BM6880_REG_HUMIDITY_MSB 0x25 > > > >+#define BME680_REG_GAS_MSB 0x2A > > > >+#define BME680_REG_GAS_R_LSB0x2B > > > >+#define BME680_GAS_STAB_BIT BIT(4) > > > >+ > > > >+#define BME680_REG_CTRL_HUMIDITY0x72 > > > >+#define BME680_OSRS_HUMIDITY_MASK GENMASK(2, 0) > > > >+ > > > >+#define BME680_REG_CTRL_MEAS0x74 > > > >+#define BME680_OSRS_TEMP_MASK GENMASK(7, 5) > > > >+#define BME680_OSRS_PRESS_MASKGENMASK(4, 2) > > > >+#define BME680_MODE_MASK GENMASK(1, 0) > > > >+ > > > >+#define BME680_MODE_FORCED BIT(0) > > > >+#define BME680_MODE_SLEEP 0 > > > This should be: > > > #define BME680_MODE_SLEEP 0 > > > #define BME680_MODE_FORCED
Re: [PATCH v3] iio: chemical: Add support for Bosch BME680 sensor
Hi Jonathan, On Sun, Jul 15, 2018 at 10:10:36AM +0100, Jonathan Cameron wrote: > On Sat, 14 Jul 2018 13:03:42 +0530 > Himanshu Jha wrote: > > > Hi David, > > > > On Fri, Jul 13, 2018 at 01:42:35PM -0700, David Frey wrote: > > > Hi Himanshu Jha, > > > > > > First a bit of background. I'm working on a device which will contain a > > > bme680 sensor. A colleague of mine started work on a Linux kernel driver > > > for the chip a little while ago. The (WIP) driver can be found here: > > > https://github.com/mangOH/mangOH/tree/master/linux_kernel_modules/bme680 > > > > Great! > > > > > This driver is written targeting an older kernel (3.18.x) because that's > > > the > > > kernel we're stuck on for now. Rather than writing the driver from > > > scratch, > > > what we did was write the Linux kernel driver as a wrapper around the > > > Bosch > > > code. My theory at the time was that Bosch made the chip, so they > > > probably > > > know what they're doing when it comes to writing a driver library. After > > > having looked at the code in more detail, I'm less confident that our > > > approach was the best one. I'm not attempting to upstream the driver > > > built > > > by my colleague and I'm not trying to request review of this code either. > > > I > > > simply want to make you aware of it so that you can look at it to get some > > > ideas. > > > > Thanks for taking your time to review. > > > > > I have included a number of comments on your driver below. Keep up the > > > good > > > work! > > > > > > >+++ b/drivers/iio/chemical/bme680.h > > > >@@ -0,0 +1,99 @@ > > > >+/* SPDX-License-Identifier: GPL-2.0 */ > > > >+#ifndef BME680_H_ > > > >+#define BME680_H_ > > > >+ > > > >+#define BME680_REG_CHIP_I2C_ID 0xD0 > > > >+#define BME680_REG_CHIP_SPI_ID 0x50 > > > >+#define BME680_CHIP_ID_VAL 0x61 > > > Try to be consistent with the indenting of the defines. I think this > > > would > > > be clearest: > > > #define BME680_REG_X 0x00 > > > #define BME680_X_FOO_EN_MASKBIT(0) > > > #define BME680_X_BAR_MASK GENMASK(3, 1) > > > #define BME680_BAR_VAL1 3 > > > #define BME680_BAR_VAL2 7 > > > > > > This way the register, field definition and field values are all visually > > > distinctive. > > > > I have used this pattern everywhere where applicable. But not applied > > for *_VAL, would definitely follow this up. > > > > > >+#define BME680_REG_SOFT_RESET 0xE0 > > > The datasheet says that the soft reset register differs for I2C and SPI. > > > For I2C it is 0xE0 and for SPI it is 0x60 when page 0 is selected. > > > > That's really a stupid mistake :( > > I have exported these individual initialization code in the I2C & SPI > > drivers respectively but it slipped my mind somehow. This device has > > peculiar characteristics in register addressing. > > > > I will correct this in next version. > > > > > >+#define BME680_CMD_SOFTRESET0xB6 > > > >+#define BME680_REG_STATUS 0x73 > > > >+#define BME680_SPI_MEM_PAGE_BIT BIT(4) > > > >+#define BME680_SPI_MEM_PAGE_1_VAL 1 > > > >+ > > > >+#define BME680_OSRS_TEMP_X(osrs_t) ((osrs_t) << 5) > > > >+#define BME680_OSRS_PRESS_X(osrs_p) ((osrs_p) << 2) > > > >+#define BME680_OSRS_HUMID_X(osrs_h) ((osrs_h) << 0) > > > You could use the FIELD_PREP macro from to eliminate > > > the > > > need for these macros. For example: > > > ctrl_meas_reg = FIELD_PREP(BME680_OSRS_TEMP_MASK, temp_val) | > > > FIELD_PREP(BME680_OSRS_PRESS_MASK, press_val) | > > > FIELD_PREP(BME880_MODE_MASK, mode_val); > > > > Ah, yes! I didn't knew about these magic macros. It will remove some > > log2() computation hacks from my code. > > > > > >+ > > > >+#define BME680_REG_TEMP_MSB 0x22 > > > >+#define BME680_REG_PRESS_MSB0x1F > > > >+#define BM6880_REG_HUMIDITY_MSB 0x25 > > > >+#define BME680_REG_GAS_MSB 0x2A > > > >+#define BME680_REG_GAS_R_LSB0x2B > > > >+#define BME680_GAS_STAB_BIT BIT(4) > > > >+ > > > >+#define BME680_REG_CTRL_HUMIDITY0x72 > > > >+#define BME680_OSRS_HUMIDITY_MASK GENMASK(2, 0) > > > >+ > > > >+#define BME680_REG_CTRL_MEAS0x74 > > > >+#define BME680_OSRS_TEMP_MASK GENMASK(7, 5) > > > >+#define BME680_OSRS_PRESS_MASKGENMASK(4, 2) > > > >+#define BME680_MODE_MASK GENMASK(1, 0) > > > >+ > > > >+#define BME680_MODE_FORCED BIT(0) > > > >+#define BME680_MODE_SLEEP 0 > > > This should be: > > > #define BME680_MODE_SLEEP 0 > > > #define BME680_MODE_FORCED
Re: [PATCH v3] iio: chemical: Add support for Bosch BME680 sensor
On Sat, 14 Jul 2018 13:03:42 +0530 Himanshu Jha wrote: > Hi David, > > On Fri, Jul 13, 2018 at 01:42:35PM -0700, David Frey wrote: > > Hi Himanshu Jha, > > > > First a bit of background. I'm working on a device which will contain a > > bme680 sensor. A colleague of mine started work on a Linux kernel driver > > for the chip a little while ago. The (WIP) driver can be found here: > > https://github.com/mangOH/mangOH/tree/master/linux_kernel_modules/bme680 > > Great! > > > This driver is written targeting an older kernel (3.18.x) because that's the > > kernel we're stuck on for now. Rather than writing the driver from scratch, > > what we did was write the Linux kernel driver as a wrapper around the Bosch > > code. My theory at the time was that Bosch made the chip, so they probably > > know what they're doing when it comes to writing a driver library. After > > having looked at the code in more detail, I'm less confident that our > > approach was the best one. I'm not attempting to upstream the driver built > > by my colleague and I'm not trying to request review of this code either. I > > simply want to make you aware of it so that you can look at it to get some > > ideas. > > Thanks for taking your time to review. > > > I have included a number of comments on your driver below. Keep up the good > > work! > > > > >+++ b/drivers/iio/chemical/bme680.h > > >@@ -0,0 +1,99 @@ > > >+/* SPDX-License-Identifier: GPL-2.0 */ > > >+#ifndef BME680_H_ > > >+#define BME680_H_ > > >+ > > >+#define BME680_REG_CHIP_I2C_ID0xD0 > > >+#define BME680_REG_CHIP_SPI_ID0x50 > > >+#define BME680_CHIP_ID_VAL0x61 > > Try to be consistent with the indenting of the defines. I think this would > > be clearest: > > #define BME680_REG_X0x00 > > #define BME680_X_FOO_EN_MASK BIT(0) > > #define BME680_X_BAR_MASK GENMASK(3, 1) > > #define BME680_BAR_VAL1 3 > > #define BME680_BAR_VAL2 7 > > > > This way the register, field definition and field values are all visually > > distinctive. > > I have used this pattern everywhere where applicable. But not applied > for *_VAL, would definitely follow this up. > > > >+#define BME680_REG_SOFT_RESET 0xE0 > > The datasheet says that the soft reset register differs for I2C and SPI. > > For I2C it is 0xE0 and for SPI it is 0x60 when page 0 is selected. > > That's really a stupid mistake :( > I have exported these individual initialization code in the I2C & SPI > drivers respectively but it slipped my mind somehow. This device has > peculiar characteristics in register addressing. > > I will correct this in next version. > > > >+#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 > > >+ > > >+#define BME680_OSRS_TEMP_X(osrs_t)((osrs_t) << 5) > > >+#define BME680_OSRS_PRESS_X(osrs_p) ((osrs_p) << 2) > > >+#define BME680_OSRS_HUMID_X(osrs_h) ((osrs_h) << 0) > > You could use the FIELD_PREP macro from to eliminate the > > need for these macros. For example: > > ctrl_meas_reg = FIELD_PREP(BME680_OSRS_TEMP_MASK, temp_val) | > > FIELD_PREP(BME680_OSRS_PRESS_MASK, press_val) | > > FIELD_PREP(BME880_MODE_MASK, mode_val); > > Ah, yes! I didn't knew about these magic macros. It will remove some > log2() computation hacks from my code. > > > >+ > > >+#define BME680_REG_TEMP_MSB 0x22 > > >+#define BME680_REG_PRESS_MSB 0x1F > > >+#define BM6880_REG_HUMIDITY_MSB 0x25 > > >+#define BME680_REG_GAS_MSB0x2A > > >+#define BME680_REG_GAS_R_LSB 0x2B > > >+#define BME680_GAS_STAB_BIT BIT(4) > > >+ > > >+#define BME680_REG_CTRL_HUMIDITY 0x72 > > >+#define BME680_OSRS_HUMIDITY_MASK GENMASK(2, 0) > > >+ > > >+#define BME680_REG_CTRL_MEAS 0x74 > > >+#define BME680_OSRS_TEMP_MASK GENMASK(7, 5) > > >+#define BME680_OSRS_PRESS_MASK GENMASK(4, 2) > > >+#define BME680_MODE_MASKGENMASK(1, 0) > > >+ > > >+#define BME680_MODE_FORCEDBIT(0) > > >+#define BME680_MODE_SLEEP 0 > > This should be: > > #define BME680_MODE_SLEEP 0 > > #define BME680_MODE_FORCED 1 > > Yes, this is much clearer and removes ambiguity. > > > >+/* Taken from Bosch BME680 API */ > > > > I think there should be a link to the Bosch code > > (https://github.com/BoschSensortec/BME680_driver/) somewhere within the > > comments of this file. Maybe it belongs at the top of this file? > > I planned to add: >
Re: [PATCH v3] iio: chemical: Add support for Bosch BME680 sensor
On Sat, 14 Jul 2018 13:03:42 +0530 Himanshu Jha wrote: > Hi David, > > On Fri, Jul 13, 2018 at 01:42:35PM -0700, David Frey wrote: > > Hi Himanshu Jha, > > > > First a bit of background. I'm working on a device which will contain a > > bme680 sensor. A colleague of mine started work on a Linux kernel driver > > for the chip a little while ago. The (WIP) driver can be found here: > > https://github.com/mangOH/mangOH/tree/master/linux_kernel_modules/bme680 > > Great! > > > This driver is written targeting an older kernel (3.18.x) because that's the > > kernel we're stuck on for now. Rather than writing the driver from scratch, > > what we did was write the Linux kernel driver as a wrapper around the Bosch > > code. My theory at the time was that Bosch made the chip, so they probably > > know what they're doing when it comes to writing a driver library. After > > having looked at the code in more detail, I'm less confident that our > > approach was the best one. I'm not attempting to upstream the driver built > > by my colleague and I'm not trying to request review of this code either. I > > simply want to make you aware of it so that you can look at it to get some > > ideas. > > Thanks for taking your time to review. > > > I have included a number of comments on your driver below. Keep up the good > > work! > > > > >+++ b/drivers/iio/chemical/bme680.h > > >@@ -0,0 +1,99 @@ > > >+/* SPDX-License-Identifier: GPL-2.0 */ > > >+#ifndef BME680_H_ > > >+#define BME680_H_ > > >+ > > >+#define BME680_REG_CHIP_I2C_ID0xD0 > > >+#define BME680_REG_CHIP_SPI_ID0x50 > > >+#define BME680_CHIP_ID_VAL0x61 > > Try to be consistent with the indenting of the defines. I think this would > > be clearest: > > #define BME680_REG_X0x00 > > #define BME680_X_FOO_EN_MASK BIT(0) > > #define BME680_X_BAR_MASK GENMASK(3, 1) > > #define BME680_BAR_VAL1 3 > > #define BME680_BAR_VAL2 7 > > > > This way the register, field definition and field values are all visually > > distinctive. > > I have used this pattern everywhere where applicable. But not applied > for *_VAL, would definitely follow this up. > > > >+#define BME680_REG_SOFT_RESET 0xE0 > > The datasheet says that the soft reset register differs for I2C and SPI. > > For I2C it is 0xE0 and for SPI it is 0x60 when page 0 is selected. > > That's really a stupid mistake :( > I have exported these individual initialization code in the I2C & SPI > drivers respectively but it slipped my mind somehow. This device has > peculiar characteristics in register addressing. > > I will correct this in next version. > > > >+#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 > > >+ > > >+#define BME680_OSRS_TEMP_X(osrs_t)((osrs_t) << 5) > > >+#define BME680_OSRS_PRESS_X(osrs_p) ((osrs_p) << 2) > > >+#define BME680_OSRS_HUMID_X(osrs_h) ((osrs_h) << 0) > > You could use the FIELD_PREP macro from to eliminate the > > need for these macros. For example: > > ctrl_meas_reg = FIELD_PREP(BME680_OSRS_TEMP_MASK, temp_val) | > > FIELD_PREP(BME680_OSRS_PRESS_MASK, press_val) | > > FIELD_PREP(BME880_MODE_MASK, mode_val); > > Ah, yes! I didn't knew about these magic macros. It will remove some > log2() computation hacks from my code. > > > >+ > > >+#define BME680_REG_TEMP_MSB 0x22 > > >+#define BME680_REG_PRESS_MSB 0x1F > > >+#define BM6880_REG_HUMIDITY_MSB 0x25 > > >+#define BME680_REG_GAS_MSB0x2A > > >+#define BME680_REG_GAS_R_LSB 0x2B > > >+#define BME680_GAS_STAB_BIT BIT(4) > > >+ > > >+#define BME680_REG_CTRL_HUMIDITY 0x72 > > >+#define BME680_OSRS_HUMIDITY_MASK GENMASK(2, 0) > > >+ > > >+#define BME680_REG_CTRL_MEAS 0x74 > > >+#define BME680_OSRS_TEMP_MASK GENMASK(7, 5) > > >+#define BME680_OSRS_PRESS_MASK GENMASK(4, 2) > > >+#define BME680_MODE_MASKGENMASK(1, 0) > > >+ > > >+#define BME680_MODE_FORCEDBIT(0) > > >+#define BME680_MODE_SLEEP 0 > > This should be: > > #define BME680_MODE_SLEEP 0 > > #define BME680_MODE_FORCED 1 > > Yes, this is much clearer and removes ambiguity. > > > >+/* Taken from Bosch BME680 API */ > > > > I think there should be a link to the Bosch code > > (https://github.com/BoschSensortec/BME680_driver/) somewhere within the > > comments of this file. Maybe it belongs at the top of this file? > > I planned to add: >
Re: [PATCH v3] iio: chemical: Add support for Bosch BME680 sensor
Hi David, On Fri, Jul 13, 2018 at 01:42:35PM -0700, David Frey wrote: > Hi Himanshu Jha, > > First a bit of background. I'm working on a device which will contain a > bme680 sensor. A colleague of mine started work on a Linux kernel driver > for the chip a little while ago. The (WIP) driver can be found here: > https://github.com/mangOH/mangOH/tree/master/linux_kernel_modules/bme680 Great! > This driver is written targeting an older kernel (3.18.x) because that's the > kernel we're stuck on for now. Rather than writing the driver from scratch, > what we did was write the Linux kernel driver as a wrapper around the Bosch > code. My theory at the time was that Bosch made the chip, so they probably > know what they're doing when it comes to writing a driver library. After > having looked at the code in more detail, I'm less confident that our > approach was the best one. I'm not attempting to upstream the driver built > by my colleague and I'm not trying to request review of this code either. I > simply want to make you aware of it so that you can look at it to get some > ideas. Thanks for taking your time to review. > I have included a number of comments on your driver below. Keep up the good > work! > > >+++ b/drivers/iio/chemical/bme680.h > >@@ -0,0 +1,99 @@ > >+/* SPDX-License-Identifier: GPL-2.0 */ > >+#ifndef BME680_H_ > >+#define BME680_H_ > >+ > >+#define BME680_REG_CHIP_I2C_ID 0xD0 > >+#define BME680_REG_CHIP_SPI_ID 0x50 > >+#define BME680_CHIP_ID_VAL 0x61 > Try to be consistent with the indenting of the defines. I think this would > be clearest: > #define BME680_REG_X 0x00 > #define BME680_X_FOO_EN_MASKBIT(0) > #define BME680_X_BAR_MASK GENMASK(3, 1) > #define BME680_BAR_VAL1 3 > #define BME680_BAR_VAL2 7 > > This way the register, field definition and field values are all visually > distinctive. I have used this pattern everywhere where applicable. But not applied for *_VAL, would definitely follow this up. > >+#define BME680_REG_SOFT_RESET 0xE0 > The datasheet says that the soft reset register differs for I2C and SPI. > For I2C it is 0xE0 and for SPI it is 0x60 when page 0 is selected. That's really a stupid mistake :( I have exported these individual initialization code in the I2C & SPI drivers respectively but it slipped my mind somehow. This device has peculiar characteristics in register addressing. I will correct this in next version. > >+#define BME680_CMD_SOFTRESET0xB6 > >+#define BME680_REG_STATUS 0x73 > >+#define BME680_SPI_MEM_PAGE_BIT BIT(4) > >+#define BME680_SPI_MEM_PAGE_1_VAL 1 > >+ > >+#define BME680_OSRS_TEMP_X(osrs_t) ((osrs_t) << 5) > >+#define BME680_OSRS_PRESS_X(osrs_p) ((osrs_p) << 2) > >+#define BME680_OSRS_HUMID_X(osrs_h) ((osrs_h) << 0) > You could use the FIELD_PREP macro from to eliminate the > need for these macros. For example: > ctrl_meas_reg = FIELD_PREP(BME680_OSRS_TEMP_MASK, temp_val) | > FIELD_PREP(BME680_OSRS_PRESS_MASK, press_val) | > FIELD_PREP(BME880_MODE_MASK, mode_val); Ah, yes! I didn't knew about these magic macros. It will remove some log2() computation hacks from my code. > >+ > >+#define BME680_REG_TEMP_MSB 0x22 > >+#define BME680_REG_PRESS_MSB0x1F > >+#define BM6880_REG_HUMIDITY_MSB 0x25 > >+#define BME680_REG_GAS_MSB 0x2A > >+#define BME680_REG_GAS_R_LSB0x2B > >+#define BME680_GAS_STAB_BIT BIT(4) > >+ > >+#define BME680_REG_CTRL_HUMIDITY0x72 > >+#define BME680_OSRS_HUMIDITY_MASK GENMASK(2, 0) > >+ > >+#define BME680_REG_CTRL_MEAS0x74 > >+#define BME680_OSRS_TEMP_MASK GENMASK(7, 5) > >+#define BME680_OSRS_PRESS_MASKGENMASK(4, 2) > >+#define BME680_MODE_MASK GENMASK(1, 0) > >+ > >+#define BME680_MODE_FORCED BIT(0) > >+#define BME680_MODE_SLEEP 0 > This should be: > #define BME680_MODE_SLEEP 0 > #define BME680_MODE_FORCED1 Yes, this is much clearer and removes ambiguity. > >+/* Taken from Bosch BME680 API */ > > I think there should be a link to the Bosch code > (https://github.com/BoschSensortec/BME680_driver/) somewhere within the > comments of this file. Maybe it belongs at the top of this file? I planned to add: https://github.com/BoschSensortec/BME680_driver/blob/63bb5336db4659519860832be2738c685133aa33/bme680.c#L876 to here and likewise to other compensate functions. But these links may change(if somehow they plan to migrate to Gitlab), long lines are not welcomed. You could also notice that I haven't included datasheet link at the top of this
Re: [PATCH v3] iio: chemical: Add support for Bosch BME680 sensor
Hi David, On Fri, Jul 13, 2018 at 01:42:35PM -0700, David Frey wrote: > Hi Himanshu Jha, > > First a bit of background. I'm working on a device which will contain a > bme680 sensor. A colleague of mine started work on a Linux kernel driver > for the chip a little while ago. The (WIP) driver can be found here: > https://github.com/mangOH/mangOH/tree/master/linux_kernel_modules/bme680 Great! > This driver is written targeting an older kernel (3.18.x) because that's the > kernel we're stuck on for now. Rather than writing the driver from scratch, > what we did was write the Linux kernel driver as a wrapper around the Bosch > code. My theory at the time was that Bosch made the chip, so they probably > know what they're doing when it comes to writing a driver library. After > having looked at the code in more detail, I'm less confident that our > approach was the best one. I'm not attempting to upstream the driver built > by my colleague and I'm not trying to request review of this code either. I > simply want to make you aware of it so that you can look at it to get some > ideas. Thanks for taking your time to review. > I have included a number of comments on your driver below. Keep up the good > work! > > >+++ b/drivers/iio/chemical/bme680.h > >@@ -0,0 +1,99 @@ > >+/* SPDX-License-Identifier: GPL-2.0 */ > >+#ifndef BME680_H_ > >+#define BME680_H_ > >+ > >+#define BME680_REG_CHIP_I2C_ID 0xD0 > >+#define BME680_REG_CHIP_SPI_ID 0x50 > >+#define BME680_CHIP_ID_VAL 0x61 > Try to be consistent with the indenting of the defines. I think this would > be clearest: > #define BME680_REG_X 0x00 > #define BME680_X_FOO_EN_MASKBIT(0) > #define BME680_X_BAR_MASK GENMASK(3, 1) > #define BME680_BAR_VAL1 3 > #define BME680_BAR_VAL2 7 > > This way the register, field definition and field values are all visually > distinctive. I have used this pattern everywhere where applicable. But not applied for *_VAL, would definitely follow this up. > >+#define BME680_REG_SOFT_RESET 0xE0 > The datasheet says that the soft reset register differs for I2C and SPI. > For I2C it is 0xE0 and for SPI it is 0x60 when page 0 is selected. That's really a stupid mistake :( I have exported these individual initialization code in the I2C & SPI drivers respectively but it slipped my mind somehow. This device has peculiar characteristics in register addressing. I will correct this in next version. > >+#define BME680_CMD_SOFTRESET0xB6 > >+#define BME680_REG_STATUS 0x73 > >+#define BME680_SPI_MEM_PAGE_BIT BIT(4) > >+#define BME680_SPI_MEM_PAGE_1_VAL 1 > >+ > >+#define BME680_OSRS_TEMP_X(osrs_t) ((osrs_t) << 5) > >+#define BME680_OSRS_PRESS_X(osrs_p) ((osrs_p) << 2) > >+#define BME680_OSRS_HUMID_X(osrs_h) ((osrs_h) << 0) > You could use the FIELD_PREP macro from to eliminate the > need for these macros. For example: > ctrl_meas_reg = FIELD_PREP(BME680_OSRS_TEMP_MASK, temp_val) | > FIELD_PREP(BME680_OSRS_PRESS_MASK, press_val) | > FIELD_PREP(BME880_MODE_MASK, mode_val); Ah, yes! I didn't knew about these magic macros. It will remove some log2() computation hacks from my code. > >+ > >+#define BME680_REG_TEMP_MSB 0x22 > >+#define BME680_REG_PRESS_MSB0x1F > >+#define BM6880_REG_HUMIDITY_MSB 0x25 > >+#define BME680_REG_GAS_MSB 0x2A > >+#define BME680_REG_GAS_R_LSB0x2B > >+#define BME680_GAS_STAB_BIT BIT(4) > >+ > >+#define BME680_REG_CTRL_HUMIDITY0x72 > >+#define BME680_OSRS_HUMIDITY_MASK GENMASK(2, 0) > >+ > >+#define BME680_REG_CTRL_MEAS0x74 > >+#define BME680_OSRS_TEMP_MASK GENMASK(7, 5) > >+#define BME680_OSRS_PRESS_MASKGENMASK(4, 2) > >+#define BME680_MODE_MASK GENMASK(1, 0) > >+ > >+#define BME680_MODE_FORCED BIT(0) > >+#define BME680_MODE_SLEEP 0 > This should be: > #define BME680_MODE_SLEEP 0 > #define BME680_MODE_FORCED1 Yes, this is much clearer and removes ambiguity. > >+/* Taken from Bosch BME680 API */ > > I think there should be a link to the Bosch code > (https://github.com/BoschSensortec/BME680_driver/) somewhere within the > comments of this file. Maybe it belongs at the top of this file? I planned to add: https://github.com/BoschSensortec/BME680_driver/blob/63bb5336db4659519860832be2738c685133aa33/bme680.c#L876 to here and likewise to other compensate functions. But these links may change(if somehow they plan to migrate to Gitlab), long lines are not welcomed. You could also notice that I haven't included datasheet link at the top of this
Re: [PATCH v3] iio: chemical: Add support for Bosch BME680 sensor
Hi Himanshu Jha, First a bit of background. I'm working on a device which will contain a bme680 sensor. A colleague of mine started work on a Linux kernel driver for the chip a little while ago. The (WIP) driver can be found here: https://github.com/mangOH/mangOH/tree/master/linux_kernel_modules/bme680 This driver is written targeting an older kernel (3.18.x) because that's the kernel we're stuck on for now. Rather than writing the driver from scratch, what we did was write the Linux kernel driver as a wrapper around the Bosch code. My theory at the time was that Bosch made the chip, so they probably know what they're doing when it comes to writing a driver library. After having looked at the code in more detail, I'm less confident that our approach was the best one. I'm not attempting to upstream the driver built by my colleague and I'm not trying to request review of this code either. I simply want to make you aware of it so that you can look at it to get some ideas. I have included a number of comments on your driver below. Keep up the good work! On 7/11/2018 5:13 AM, Himanshu Jha wrote: Bosch BME680 is a 4-in-1 sensor with temperature, pressure, humidity and gas sensing capability. It supports both I2C and SPI communication protocol for effective data communication. The device supports two modes: 1. Sleep mode 2. Forced mode The measurements only takes place when forced mode is triggered and a single TPHG cycle is performed by the sensor. The sensor automatically goes to sleep after afterwards. The device has various calibration constants/parameters programmed into devices' non-volatile memory(NVM) during production and can't be altered by the user. These constants are used in the compensation functions to get the required compensated readings along with the raw data. The compensation functions/algorithms are provided by Bosch Sensortec GmbH via their API[1]. As these don't change during the measurement cycle, therefore we read and store them at the probe. The default configs supplied by Bosch are also set at probe. 0-day tested with build success. GSoC-2018: https://summerofcode.withgoogle.com/projects/#6691473790074880 Mentor: Daniel Baluta [1] https://github.com/BoschSensortec/BME680_driver Datasheet: https://ae-bst.resource.bosch.com/media/_tech/media/datasheets/BST-BME680-DS001-00.pdf Cc: Daniel Baluta Signed-off-by: Himanshu Jha --- v3: -moved files to chemical directory instead of a dedicated directory. -read calibration parameters serially with endian conversions. -drop some return ret. -removed few unnecessary casts safely. -added 'u' suffix to explicitly specify unsigned for large values and thereby fixing comiler warning. -left aligned all comments. -added a comment explaining heater stability failure. v2: -Used devm_add_action() to add a generic remove method for both I2C & SPI driver. -Introduction of compensation functions. -chip initialisation routines moved to respective I2C and SPI driver. -Introduction of gas sensing rountines. -Simplified Kconfig to reduce various options. drivers/iio/chemical/Kconfig | 25 + drivers/iio/chemical/Makefile | 3 + drivers/iio/chemical/bme680.h | 99 drivers/iio/chemical/bme680_core.c | 946 + drivers/iio/chemical/bme680_i2c.c | 83 drivers/iio/chemical/bme680_spi.c | 123 + 6 files changed, 1279 insertions(+) create mode 100644 drivers/iio/chemical/bme680.h create mode 100644 drivers/iio/chemical/bme680_core.c create mode 100644 drivers/iio/chemical/bme680_i2c.c create mode 100644 drivers/iio/chemical/bme680_spi.c diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig index 5cb5be7..24790a8 100644 --- a/drivers/iio/chemical/Kconfig +++ b/drivers/iio/chemical/Kconfig @@ -21,6 +21,31 @@ config ATLAS_PH_SENSOR To compile this driver as module, choose M here: the module will be called atlas-ph-sensor. +config BME680 + tristate "Bosch Sensortec BME680 sensor driver" + depends on (I2C || SPI) + select REGMAP + select BME680_I2C if (I2C) + select BME680_SPI if (SPI) + help + Say yes here to build support for Bosch Sensortec BME680 sensor with + temperature, pressure, humidity and gas sensing capability. + + This driver can also be built as a module. If so, the module for I2C + would be called bme680_i2c and bme680_spi for SPI support. + +config BME680_I2C + tristate + depends on BME680 + depends on I2C + select REGMAP_I2C + +config BME680_SPI + tristate + depends on BME680 + depends on SPI + select REGMAP_SPI + config CCS811 tristate "AMS CCS811 VOC sensor" depends on I2C diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile index a629b29..2f4c4ba 100644 ---
Re: [PATCH v3] iio: chemical: Add support for Bosch BME680 sensor
Hi Himanshu Jha, First a bit of background. I'm working on a device which will contain a bme680 sensor. A colleague of mine started work on a Linux kernel driver for the chip a little while ago. The (WIP) driver can be found here: https://github.com/mangOH/mangOH/tree/master/linux_kernel_modules/bme680 This driver is written targeting an older kernel (3.18.x) because that's the kernel we're stuck on for now. Rather than writing the driver from scratch, what we did was write the Linux kernel driver as a wrapper around the Bosch code. My theory at the time was that Bosch made the chip, so they probably know what they're doing when it comes to writing a driver library. After having looked at the code in more detail, I'm less confident that our approach was the best one. I'm not attempting to upstream the driver built by my colleague and I'm not trying to request review of this code either. I simply want to make you aware of it so that you can look at it to get some ideas. I have included a number of comments on your driver below. Keep up the good work! On 7/11/2018 5:13 AM, Himanshu Jha wrote: Bosch BME680 is a 4-in-1 sensor with temperature, pressure, humidity and gas sensing capability. It supports both I2C and SPI communication protocol for effective data communication. The device supports two modes: 1. Sleep mode 2. Forced mode The measurements only takes place when forced mode is triggered and a single TPHG cycle is performed by the sensor. The sensor automatically goes to sleep after afterwards. The device has various calibration constants/parameters programmed into devices' non-volatile memory(NVM) during production and can't be altered by the user. These constants are used in the compensation functions to get the required compensated readings along with the raw data. The compensation functions/algorithms are provided by Bosch Sensortec GmbH via their API[1]. As these don't change during the measurement cycle, therefore we read and store them at the probe. The default configs supplied by Bosch are also set at probe. 0-day tested with build success. GSoC-2018: https://summerofcode.withgoogle.com/projects/#6691473790074880 Mentor: Daniel Baluta [1] https://github.com/BoschSensortec/BME680_driver Datasheet: https://ae-bst.resource.bosch.com/media/_tech/media/datasheets/BST-BME680-DS001-00.pdf Cc: Daniel Baluta Signed-off-by: Himanshu Jha --- v3: -moved files to chemical directory instead of a dedicated directory. -read calibration parameters serially with endian conversions. -drop some return ret. -removed few unnecessary casts safely. -added 'u' suffix to explicitly specify unsigned for large values and thereby fixing comiler warning. -left aligned all comments. -added a comment explaining heater stability failure. v2: -Used devm_add_action() to add a generic remove method for both I2C & SPI driver. -Introduction of compensation functions. -chip initialisation routines moved to respective I2C and SPI driver. -Introduction of gas sensing rountines. -Simplified Kconfig to reduce various options. drivers/iio/chemical/Kconfig | 25 + drivers/iio/chemical/Makefile | 3 + drivers/iio/chemical/bme680.h | 99 drivers/iio/chemical/bme680_core.c | 946 + drivers/iio/chemical/bme680_i2c.c | 83 drivers/iio/chemical/bme680_spi.c | 123 + 6 files changed, 1279 insertions(+) create mode 100644 drivers/iio/chemical/bme680.h create mode 100644 drivers/iio/chemical/bme680_core.c create mode 100644 drivers/iio/chemical/bme680_i2c.c create mode 100644 drivers/iio/chemical/bme680_spi.c diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig index 5cb5be7..24790a8 100644 --- a/drivers/iio/chemical/Kconfig +++ b/drivers/iio/chemical/Kconfig @@ -21,6 +21,31 @@ config ATLAS_PH_SENSOR To compile this driver as module, choose M here: the module will be called atlas-ph-sensor. +config BME680 + tristate "Bosch Sensortec BME680 sensor driver" + depends on (I2C || SPI) + select REGMAP + select BME680_I2C if (I2C) + select BME680_SPI if (SPI) + help + Say yes here to build support for Bosch Sensortec BME680 sensor with + temperature, pressure, humidity and gas sensing capability. + + This driver can also be built as a module. If so, the module for I2C + would be called bme680_i2c and bme680_spi for SPI support. + +config BME680_I2C + tristate + depends on BME680 + depends on I2C + select REGMAP_I2C + +config BME680_SPI + tristate + depends on BME680 + depends on SPI + select REGMAP_SPI + config CCS811 tristate "AMS CCS811 VOC sensor" depends on I2C diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile index a629b29..2f4c4ba 100644 ---
Re: [PATCH v3] iio: chemical: Add support for Bosch BME680 sensor
On Wed, 11 Jul 2018 17:40:07 -0700 Matt Ranostay wrote: > On Wed, Jul 11, 2018 at 5:13 AM, Himanshu Jha > wrote: > > Bosch BME680 is a 4-in-1 sensor with temperature, pressure, humidity > > and gas sensing capability. It supports both I2C and SPI communication > > protocol for effective data communication. > > > > The device supports two modes: > > > > 1. Sleep mode > > 2. Forced mode > > > > The measurements only takes place when forced mode is triggered and a > > single TPHG cycle is performed by the sensor. The sensor automatically > > goes to sleep after afterwards. > > > > The device has various calibration constants/parameters programmed into > > devices' non-volatile memory(NVM) during production and can't be altered > > by the user. These constants are used in the compensation functions to > > get the required compensated readings along with the raw data. The > > compensation functions/algorithms are provided by Bosch Sensortec GmbH > > via their API[1]. As these don't change during the measurement cycle, > > therefore we read and store them at the probe. The default configs > > supplied by Bosch are also set at probe. > > > > 0-day tested with build success. > > > > GSoC-2018: https://summerofcode.withgoogle.com/projects/#6691473790074880 > > Mentor: Daniel Baluta > > [1] https://github.com/BoschSensortec/BME680_driver > > Datasheet: > > https://ae-bst.resource.bosch.com/media/_tech/media/datasheets/BST-BME680-DS001-00.pdf > > > > Cc: Daniel Baluta > > Signed-off-by: Himanshu Jha > > --- > > > > v3: > >-moved files to chemical directory instead of a dedicated directory. > >-read calibration parameters serially with endian conversions. > >-drop some return ret. > >-removed few unnecessary casts safely. > >-added 'u' suffix to explicitly specify unsigned for large values > > and thereby fixing comiler warning. > >-left aligned all comments. > >-added a comment explaining heater stability failure. > > > > v2: > >-Used devm_add_action() to add a generic remove method for > > both I2C & SPI driver. > >-Introduction of compensation functions. > >-chip initialisation routines moved to respective I2C and SPI > > driver. > >-Introduction of gas sensing rountines. > >-Simplified Kconfig to reduce various options. > > > > drivers/iio/chemical/Kconfig | 25 + > > drivers/iio/chemical/Makefile | 3 + > > drivers/iio/chemical/bme680.h | 99 > > drivers/iio/chemical/bme680_core.c | 946 > > + > > drivers/iio/chemical/bme680_i2c.c | 83 > > drivers/iio/chemical/bme680_spi.c | 123 + > > 6 files changed, 1279 insertions(+) > > create mode 100644 drivers/iio/chemical/bme680.h > > create mode 100644 drivers/iio/chemical/bme680_core.c > > create mode 100644 drivers/iio/chemical/bme680_i2c.c > > create mode 100644 drivers/iio/chemical/bme680_spi.c > > > > diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig > > index 5cb5be7..24790a8 100644 > > --- a/drivers/iio/chemical/Kconfig > > +++ b/drivers/iio/chemical/Kconfig > > @@ -21,6 +21,31 @@ config ATLAS_PH_SENSOR > > To compile this driver as module, choose M here: the > > module will be called atlas-ph-sensor. > > > > +config BME680 > > + tristate "Bosch Sensortec BME680 sensor driver" > > + depends on (I2C || SPI) > > + select REGMAP > > + select BME680_I2C if (I2C) > > + select BME680_SPI if (SPI) > > Don't think you actually need parentheses around any of these, but of > course then again doesn't hurt Nice to tidy this one up, though if it is all there is I can do that whilst applying. So don't bother sending a v4 for this until other reviews are in. > > > + help > > + Say yes here to build support for Bosch Sensortec BME680 sensor > > with > > + temperature, pressure, humidity and gas sensing capability. > > + > > + This driver can also be built as a module. If so, the module for > > I2C > > + would be called bme680_i2c and bme680_spi for SPI support. > > + > > +config BME680_I2C > > + tristate > > + depends on BME680 > > + depends on I2C > > Wouldn't "depends on I2C && BME680" be cleaner? Maybe someone else > here can tell me if I'm too nit-picky :) You said it ;) Can't say I care either way on these. > > > + select REGMAP_I2C > > + > > +config BME680_SPI > > + tristate > > + depends on BME680 > > + depends on SPI > > Same only with SPI > > > + select REGMAP_SPI > > + > > config CCS811 > > tristate "AMS CCS811 VOC sensor" > > depends on I2C ... Matt, please give us indication if you don't have anything else to say :) Saves on a lot of scrolling and wondering if we missed something (which of course I might have done!)
Re: [PATCH v3] iio: chemical: Add support for Bosch BME680 sensor
On Wed, 11 Jul 2018 17:40:07 -0700 Matt Ranostay wrote: > On Wed, Jul 11, 2018 at 5:13 AM, Himanshu Jha > wrote: > > Bosch BME680 is a 4-in-1 sensor with temperature, pressure, humidity > > and gas sensing capability. It supports both I2C and SPI communication > > protocol for effective data communication. > > > > The device supports two modes: > > > > 1. Sleep mode > > 2. Forced mode > > > > The measurements only takes place when forced mode is triggered and a > > single TPHG cycle is performed by the sensor. The sensor automatically > > goes to sleep after afterwards. > > > > The device has various calibration constants/parameters programmed into > > devices' non-volatile memory(NVM) during production and can't be altered > > by the user. These constants are used in the compensation functions to > > get the required compensated readings along with the raw data. The > > compensation functions/algorithms are provided by Bosch Sensortec GmbH > > via their API[1]. As these don't change during the measurement cycle, > > therefore we read and store them at the probe. The default configs > > supplied by Bosch are also set at probe. > > > > 0-day tested with build success. > > > > GSoC-2018: https://summerofcode.withgoogle.com/projects/#6691473790074880 > > Mentor: Daniel Baluta > > [1] https://github.com/BoschSensortec/BME680_driver > > Datasheet: > > https://ae-bst.resource.bosch.com/media/_tech/media/datasheets/BST-BME680-DS001-00.pdf > > > > Cc: Daniel Baluta > > Signed-off-by: Himanshu Jha > > --- > > > > v3: > >-moved files to chemical directory instead of a dedicated directory. > >-read calibration parameters serially with endian conversions. > >-drop some return ret. > >-removed few unnecessary casts safely. > >-added 'u' suffix to explicitly specify unsigned for large values > > and thereby fixing comiler warning. > >-left aligned all comments. > >-added a comment explaining heater stability failure. > > > > v2: > >-Used devm_add_action() to add a generic remove method for > > both I2C & SPI driver. > >-Introduction of compensation functions. > >-chip initialisation routines moved to respective I2C and SPI > > driver. > >-Introduction of gas sensing rountines. > >-Simplified Kconfig to reduce various options. > > > > drivers/iio/chemical/Kconfig | 25 + > > drivers/iio/chemical/Makefile | 3 + > > drivers/iio/chemical/bme680.h | 99 > > drivers/iio/chemical/bme680_core.c | 946 > > + > > drivers/iio/chemical/bme680_i2c.c | 83 > > drivers/iio/chemical/bme680_spi.c | 123 + > > 6 files changed, 1279 insertions(+) > > create mode 100644 drivers/iio/chemical/bme680.h > > create mode 100644 drivers/iio/chemical/bme680_core.c > > create mode 100644 drivers/iio/chemical/bme680_i2c.c > > create mode 100644 drivers/iio/chemical/bme680_spi.c > > > > diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig > > index 5cb5be7..24790a8 100644 > > --- a/drivers/iio/chemical/Kconfig > > +++ b/drivers/iio/chemical/Kconfig > > @@ -21,6 +21,31 @@ config ATLAS_PH_SENSOR > > To compile this driver as module, choose M here: the > > module will be called atlas-ph-sensor. > > > > +config BME680 > > + tristate "Bosch Sensortec BME680 sensor driver" > > + depends on (I2C || SPI) > > + select REGMAP > > + select BME680_I2C if (I2C) > > + select BME680_SPI if (SPI) > > Don't think you actually need parentheses around any of these, but of > course then again doesn't hurt Nice to tidy this one up, though if it is all there is I can do that whilst applying. So don't bother sending a v4 for this until other reviews are in. > > > + help > > + Say yes here to build support for Bosch Sensortec BME680 sensor > > with > > + temperature, pressure, humidity and gas sensing capability. > > + > > + This driver can also be built as a module. If so, the module for > > I2C > > + would be called bme680_i2c and bme680_spi for SPI support. > > + > > +config BME680_I2C > > + tristate > > + depends on BME680 > > + depends on I2C > > Wouldn't "depends on I2C && BME680" be cleaner? Maybe someone else > here can tell me if I'm too nit-picky :) You said it ;) Can't say I care either way on these. > > > + select REGMAP_I2C > > + > > +config BME680_SPI > > + tristate > > + depends on BME680 > > + depends on SPI > > Same only with SPI > > > + select REGMAP_SPI > > + > > config CCS811 > > tristate "AMS CCS811 VOC sensor" > > depends on I2C ... Matt, please give us indication if you don't have anything else to say :) Saves on a lot of scrolling and wondering if we missed something (which of course I might have done!)
Re: [PATCH v3] iio: chemical: Add support for Bosch BME680 sensor
On Wed, Jul 11, 2018 at 5:13 AM, Himanshu Jha wrote: > Bosch BME680 is a 4-in-1 sensor with temperature, pressure, humidity > and gas sensing capability. It supports both I2C and SPI communication > protocol for effective data communication. > > The device supports two modes: > > 1. Sleep mode > 2. Forced mode > > The measurements only takes place when forced mode is triggered and a > single TPHG cycle is performed by the sensor. The sensor automatically > goes to sleep after afterwards. > > The device has various calibration constants/parameters programmed into > devices' non-volatile memory(NVM) during production and can't be altered > by the user. These constants are used in the compensation functions to > get the required compensated readings along with the raw data. The > compensation functions/algorithms are provided by Bosch Sensortec GmbH > via their API[1]. As these don't change during the measurement cycle, > therefore we read and store them at the probe. The default configs > supplied by Bosch are also set at probe. > > 0-day tested with build success. > > GSoC-2018: https://summerofcode.withgoogle.com/projects/#6691473790074880 > Mentor: Daniel Baluta > [1] https://github.com/BoschSensortec/BME680_driver > Datasheet: > https://ae-bst.resource.bosch.com/media/_tech/media/datasheets/BST-BME680-DS001-00.pdf > > Cc: Daniel Baluta > Signed-off-by: Himanshu Jha > --- > > v3: >-moved files to chemical directory instead of a dedicated directory. >-read calibration parameters serially with endian conversions. >-drop some return ret. >-removed few unnecessary casts safely. >-added 'u' suffix to explicitly specify unsigned for large values > and thereby fixing comiler warning. >-left aligned all comments. >-added a comment explaining heater stability failure. > > v2: >-Used devm_add_action() to add a generic remove method for > both I2C & SPI driver. >-Introduction of compensation functions. >-chip initialisation routines moved to respective I2C and SPI > driver. >-Introduction of gas sensing rountines. >-Simplified Kconfig to reduce various options. > > drivers/iio/chemical/Kconfig | 25 + > drivers/iio/chemical/Makefile | 3 + > drivers/iio/chemical/bme680.h | 99 > drivers/iio/chemical/bme680_core.c | 946 > + > drivers/iio/chemical/bme680_i2c.c | 83 > drivers/iio/chemical/bme680_spi.c | 123 + > 6 files changed, 1279 insertions(+) > create mode 100644 drivers/iio/chemical/bme680.h > create mode 100644 drivers/iio/chemical/bme680_core.c > create mode 100644 drivers/iio/chemical/bme680_i2c.c > create mode 100644 drivers/iio/chemical/bme680_spi.c > > diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig > index 5cb5be7..24790a8 100644 > --- a/drivers/iio/chemical/Kconfig > +++ b/drivers/iio/chemical/Kconfig > @@ -21,6 +21,31 @@ config ATLAS_PH_SENSOR > To compile this driver as module, choose M here: the > module will be called atlas-ph-sensor. > > +config BME680 > + tristate "Bosch Sensortec BME680 sensor driver" > + depends on (I2C || SPI) > + select REGMAP > + select BME680_I2C if (I2C) > + select BME680_SPI if (SPI) Don't think you actually need parentheses around any of these, but of course then again doesn't hurt > + help > + Say yes here to build support for Bosch Sensortec BME680 sensor with > + temperature, pressure, humidity and gas sensing capability. > + > + This driver can also be built as a module. If so, the module for I2C > + would be called bme680_i2c and bme680_spi for SPI support. > + > +config BME680_I2C > + tristate > + depends on BME680 > + depends on I2C Wouldn't "depends on I2C && BME680" be cleaner? Maybe someone else here can tell me if I'm too nit-picky :) > + select REGMAP_I2C > + > +config BME680_SPI > + tristate > + depends on BME680 > + depends on SPI Same only with SPI > + select REGMAP_SPI > + > config CCS811 > tristate "AMS CCS811 VOC sensor" > depends on I2C > diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile > index a629b29..2f4c4ba 100644 > --- a/drivers/iio/chemical/Makefile > +++ b/drivers/iio/chemical/Makefile > @@ -4,6 +4,9 @@ > > # When adding new entries keep the list in alphabetical order > obj-$(CONFIG_ATLAS_PH_SENSOR) += atlas-ph-sensor.o > +obj-$(CONFIG_BME680) += bme680_core.o > +obj-$(CONFIG_BME680_I2C) += bme680_i2c.o > +obj-$(CONFIG_BME680_SPI) += bme680_spi.o > obj-$(CONFIG_CCS811) += ccs811.o > obj-$(CONFIG_IAQCORE) += ams-iaq-core.o > obj-$(CONFIG_VZ89X)+= vz89x.o > diff --git a/drivers/iio/chemical/bme680.h b/drivers/iio/chemical/bme680.h > new file mode 100644 > index 000..80c4190 > --- /dev/null > +++ b/drivers/iio/chemical/bme680.h > @@ -0,0 +1,99 @@ > +/*
Re: [PATCH v3] iio: chemical: Add support for Bosch BME680 sensor
On Wed, Jul 11, 2018 at 5:13 AM, Himanshu Jha wrote: > Bosch BME680 is a 4-in-1 sensor with temperature, pressure, humidity > and gas sensing capability. It supports both I2C and SPI communication > protocol for effective data communication. > > The device supports two modes: > > 1. Sleep mode > 2. Forced mode > > The measurements only takes place when forced mode is triggered and a > single TPHG cycle is performed by the sensor. The sensor automatically > goes to sleep after afterwards. > > The device has various calibration constants/parameters programmed into > devices' non-volatile memory(NVM) during production and can't be altered > by the user. These constants are used in the compensation functions to > get the required compensated readings along with the raw data. The > compensation functions/algorithms are provided by Bosch Sensortec GmbH > via their API[1]. As these don't change during the measurement cycle, > therefore we read and store them at the probe. The default configs > supplied by Bosch are also set at probe. > > 0-day tested with build success. > > GSoC-2018: https://summerofcode.withgoogle.com/projects/#6691473790074880 > Mentor: Daniel Baluta > [1] https://github.com/BoschSensortec/BME680_driver > Datasheet: > https://ae-bst.resource.bosch.com/media/_tech/media/datasheets/BST-BME680-DS001-00.pdf > > Cc: Daniel Baluta > Signed-off-by: Himanshu Jha > --- > > v3: >-moved files to chemical directory instead of a dedicated directory. >-read calibration parameters serially with endian conversions. >-drop some return ret. >-removed few unnecessary casts safely. >-added 'u' suffix to explicitly specify unsigned for large values > and thereby fixing comiler warning. >-left aligned all comments. >-added a comment explaining heater stability failure. > > v2: >-Used devm_add_action() to add a generic remove method for > both I2C & SPI driver. >-Introduction of compensation functions. >-chip initialisation routines moved to respective I2C and SPI > driver. >-Introduction of gas sensing rountines. >-Simplified Kconfig to reduce various options. > > drivers/iio/chemical/Kconfig | 25 + > drivers/iio/chemical/Makefile | 3 + > drivers/iio/chemical/bme680.h | 99 > drivers/iio/chemical/bme680_core.c | 946 > + > drivers/iio/chemical/bme680_i2c.c | 83 > drivers/iio/chemical/bme680_spi.c | 123 + > 6 files changed, 1279 insertions(+) > create mode 100644 drivers/iio/chemical/bme680.h > create mode 100644 drivers/iio/chemical/bme680_core.c > create mode 100644 drivers/iio/chemical/bme680_i2c.c > create mode 100644 drivers/iio/chemical/bme680_spi.c > > diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig > index 5cb5be7..24790a8 100644 > --- a/drivers/iio/chemical/Kconfig > +++ b/drivers/iio/chemical/Kconfig > @@ -21,6 +21,31 @@ config ATLAS_PH_SENSOR > To compile this driver as module, choose M here: the > module will be called atlas-ph-sensor. > > +config BME680 > + tristate "Bosch Sensortec BME680 sensor driver" > + depends on (I2C || SPI) > + select REGMAP > + select BME680_I2C if (I2C) > + select BME680_SPI if (SPI) Don't think you actually need parentheses around any of these, but of course then again doesn't hurt > + help > + Say yes here to build support for Bosch Sensortec BME680 sensor with > + temperature, pressure, humidity and gas sensing capability. > + > + This driver can also be built as a module. If so, the module for I2C > + would be called bme680_i2c and bme680_spi for SPI support. > + > +config BME680_I2C > + tristate > + depends on BME680 > + depends on I2C Wouldn't "depends on I2C && BME680" be cleaner? Maybe someone else here can tell me if I'm too nit-picky :) > + select REGMAP_I2C > + > +config BME680_SPI > + tristate > + depends on BME680 > + depends on SPI Same only with SPI > + select REGMAP_SPI > + > config CCS811 > tristate "AMS CCS811 VOC sensor" > depends on I2C > diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile > index a629b29..2f4c4ba 100644 > --- a/drivers/iio/chemical/Makefile > +++ b/drivers/iio/chemical/Makefile > @@ -4,6 +4,9 @@ > > # When adding new entries keep the list in alphabetical order > obj-$(CONFIG_ATLAS_PH_SENSOR) += atlas-ph-sensor.o > +obj-$(CONFIG_BME680) += bme680_core.o > +obj-$(CONFIG_BME680_I2C) += bme680_i2c.o > +obj-$(CONFIG_BME680_SPI) += bme680_spi.o > obj-$(CONFIG_CCS811) += ccs811.o > obj-$(CONFIG_IAQCORE) += ams-iaq-core.o > obj-$(CONFIG_VZ89X)+= vz89x.o > diff --git a/drivers/iio/chemical/bme680.h b/drivers/iio/chemical/bme680.h > new file mode 100644 > index 000..80c4190 > --- /dev/null > +++ b/drivers/iio/chemical/bme680.h > @@ -0,0 +1,99 @@ > +/*
