lkml.org 
[lkml]   [2018]   [Jul]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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 <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
\
 
 \ /
  Last update: 2018-07-15 22:07    [W:0.080 / U:0.204 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site