Re: [PATCH v3] iio: chemical: Add support for Bosch BME680 sensor

[Himanshu Jha <himanshujha199640@gmail.com>]

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 <himanshujha199640@gmail.com> 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_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 <linux/bitfield.h> 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_MSB			0x2A
> > > >+#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_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			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:
> > 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.
> 
> Looks like github does the same trick kernel.org does in allowing shortened hashes.

$ git log --abbrev-commit ...

> I think
> 
> https://github.com/BoschSensortec/BME680_driver/blob/63bb5336/bme680.c#L876
> Is the same thing and under 80 chars (just) :)

Ah, yes :)
Perfect!

> > You could also notice that I haven't included datasheet link at the top
> > of this file. Well, most of the companies change the links when releasing
> > the new Revision(Rev. A,B,...) so it is likely that the link would be
> > dead/old lying at the top of source.
> 
> Even though this happens, it's better to have something than nothing
> perhaps.

Sure! Will add both links in next version.

> > Yes, we can do that but, for a while, I am following my GSoC proposal
> > timeline. And I have planned these check_bits function later, but since
> > this check_bits function was imperative in gas sensing, therefore I
> > included them now.
> > 
> > My plan in incremental changes and this patch is kindof minimal. I have
> > tested the sensor several times and found no errors in readings for
> > T P H G readings so far.
> > 
> > The problem arises that this sensor is made to work in a T->P->H->G
> > fashion and every channel is mostly dependent on the other. And IIO
> > driver design pattern isn't the best choice it. You can't just take a
> > single channel readings by running Bosch Code because it is not designed
> > like that.
> > 
> > For instance: we need t_fine values for pressure/humidity compensation
> > functions which we only get when reading temperature. So, you need to
> > run the temperaure cycle if you need pressure/humidity values. And this
> > is what I did by doing a dummy read_temp(data, NULL, NULL) to get the t_fine
> > value.
> 
> This pattern isn't that unusual in devices.   Normally you do it pretty
> much the way you have.  The sysfs interface in IIO is just interested
> in data presentation, we don't care what you have to do to get it ;)

OK. I agree, but will create a new thread for some
clarification/rectification of IIO core.

> This will fit much better when doing buffered interfaces anyway which
> naturally grab sets of channels.

Yes, I think so it work better in that way.
Also, for industrial use I think buffered interface is a better option.

> > Thanks you so much for the feedback, David! :)
> > 
> > And if at some point I have said something stupid, then please forgive me.
> > 
> > I am a 3rd year undergrad student and started with IIO few months back, and
> > not a Bosch driver developer ;)
> > 
> Going well so far ;)

Thanks Jonathan again :)

Well it is better to hint developers that you're a student, else I had a
discussion in the past where some senior developer was explaining me
about CPU profiling (I don't remember exactly) and it didn't even hit me
for the next two weeks ;)

But I would like David to review the patches in the next version too.

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