Re: [PATCH] iio/chemical/bme680: Fix SPI read interface
On Mon, 18 Feb 2019 07:03:10 +
Mike Looijmans wrote:
> On 16-02-19 12:27, Himanshu Jha wrote:
> > Hello Mike,
> >
> > On Fri, Feb 15, 2019 at 02:47:55PM +0100, Mike Looijmans wrote:
> >> The SPI interface implementation was completely broken.
> >
> > My apologies!
> >
> > SPI interface caused me a lot of trouble in my project timeline. I tried
> > many different ways to try playing with acpi dsl, printks etc. One
> > of the problem was also that we don't have any facility such `i2cdetect`
> > or instantiate a `new_device` from userspace.
> >
> > Are there any methods that one should try to debug SPIs ?
> > -- excluding the use of Oscilloscope
>
> Just persisting usually makes it work... Once you're able to read a single
> register, the rest is peanuts...
Agreed on that assessment. It's the first read that sometimes takes ours
of pulling your hair out.
A few comments inline, on the discussion rather than the patch.
Nice work Mike. We'll definitely want to push this to stable. I'll do
that for v2 if it looks good. Nothing much will go upstream until
after the merge window closes now anyway which will be about 3 weeks
time.
Jonathan
>
> > With all these problems, I referred other drivers from the Bosch family,
> > particularly BME280 which has exactly the same behavior and decided to
> > do the same. I believed that it would also work cleanly for BME680 and
> > now I see the page selection fuzz was the main problem.
> >
> > I tried to test it on Beaglebone but everytime something new pops-up
> > with beagle setup such as they moved from capemgr -> uboot overlays.
> > Lack of latest documentation on "What new in this release?" troubled me
> > + most of blogs on setup are outdated.
> >
> >> When using the SPI interface, there are only 7 address bits, the upper bit
> >> is controlled by a page select register.
> >
> > Isn't it that upper bit is controlled R/W ?
>
> I meant the upper 'address' bit here. Bit 7 of the first byte indeed controls
> read/write like on many register-controlled SPI devices (see regmap's
> standard
> SPI implementation).
>
> > And one question that I have in mind:
> >
> > Initially after poweron we are in page 0, and hence we can't access
> > Page 1 which actually has the `spi_mem_page` bit. So, how can we switch
> > to Page 1 eventually since Page 1 covers all the relevant data/config
> > registers ?
>
> My approach was to just assume that the register would work in either page,
> and that turned out to be correct. Most 'paging' chips work that way. Though
> they typically use register 0 or 255 for that.
>
> > I assume if you're in Page 0, then you can't access Page 1 registers or
> > simply put those registers are not visible.
> >
> > Some inconsistency in datasheet:
> >
> > Page 24/50:
> >
> > "After power-on, spi_mem_page is in its reset state and page 0 (0x80 to
> > 0xFF)
> > will be active. Page 1 (0x00 to 0x7F) will be
> > active on setting spi_mem_page to 1."
> >
> > OTOH,
> >
> > Page 26/50:
> >
> > "5.3.1.4 SPI memory map page selection – spi_mem_page
> >
> > In SPI mode complete memory page is accessed using page 0 & page 1.
> > Register spi_mem_page is used for page selection.
> > After power-on, spi_mem_page is in its reset state and
> > page 0(0x00 to 0x7F) will be active."
> >
> >
> > That's contradicting!
>
> They fell into their own trap of calling the high range "page 0" I guess.
>
> In this context, it's also meaningless. Upon probe, you cannot really be sure
> which page is active anyway, and the chip doesn't have a reset GPIO to
> enforce
> a known startup state. So "assume nothing" is the way forward.
>
>
> > page 0 (0x80 to 0xFF) Vs page 0(0x00 to 0x7F) ?
> >
> >> The core needs access to both
> >> ranges, so implement register read/write for both regions. The regmap
> >> paging functionality didn't agree with a register that needs to be read
> >> and modified, so I implemented a custom paging algorithm.
> >>
> >> This fixes that the device wouldn't even probe in SPI mode.
> >
> > Most importantly: Does it now work for you ?
>
> Yes, with these changes the chip works (I have it connected to SPI only).
>
> > As I tried you patch and it is not working for me in my QEMU setup.
> >
> > /sys/bus/iio/devices # lsmod
> > Module Size Used byTainted: G
> > bme680_spi 16384 0
> > bme680_core24576 1 bme680_spi
> > /sys/bus/iio/devices # ls
> >
> > SPI didn't probe and no logs for any errors/warnings as well.
>
> Probably it isn't in the devicetree then. Mine looks like this:
>
> + {
> + status = "okay";
> + num-cs = <2>;
> +
> + /* Environmental Sensor */
> + bme680_env: bme680_env@0 {
> + compatible = "bme680";
> + reg = <0>;
> + spi-max-frequency = <1000>;
> + };
>
>
> > Maybe I'll give it try again later on a different board or a native
> >
Re: [PATCH] iio/chemical/bme680: Fix SPI read interface
.. > > Report temperature in millidegrees Celcius instead of degrees. > > Jonathan, I didn't know about this but is there any rationale to report > in millidegress ? Misguided attempt in the early days of IIO to match hwmon units. Sadly it is now the ABI and hence 'unfixable'. Same odd units for voltage and current + a few others that date back to those days. A big oops. There are some other non obvious bits and pieces so should always check the ABI docs which should be near complete. > > I was under the impression that degC is the standard and you would > always want you Fitbit/Smart watch to report in degC. Ofcourse, > userspace program can convert millideg -> degC
Re: [PATCH] iio/chemical/bme680: Fix SPI read interface
On 16-02-19 12:27, Himanshu Jha wrote:
> Hello Mike,
>
> On Fri, Feb 15, 2019 at 02:47:55PM +0100, Mike Looijmans wrote:
>> The SPI interface implementation was completely broken.
>
> My apologies!
>
> SPI interface caused me a lot of trouble in my project timeline. I tried
> many different ways to try playing with acpi dsl, printks etc. One
> of the problem was also that we don't have any facility such `i2cdetect`
> or instantiate a `new_device` from userspace.
>
> Are there any methods that one should try to debug SPIs ?
> -- excluding the use of Oscilloscope
Just persisting usually makes it work... Once you're able to read a single
register, the rest is peanuts...
> With all these problems, I referred other drivers from the Bosch family,
> particularly BME280 which has exactly the same behavior and decided to
> do the same. I believed that it would also work cleanly for BME680 and
> now I see the page selection fuzz was the main problem.
>
> I tried to test it on Beaglebone but everytime something new pops-up
> with beagle setup such as they moved from capemgr -> uboot overlays.
> Lack of latest documentation on "What new in this release?" troubled me
> + most of blogs on setup are outdated.
>
>> When using the SPI interface, there are only 7 address bits, the upper bit
>> is controlled by a page select register.
>
> Isn't it that upper bit is controlled R/W ?
I meant the upper 'address' bit here. Bit 7 of the first byte indeed controls
read/write like on many register-controlled SPI devices (see regmap's standard
SPI implementation).
> And one question that I have in mind:
>
> Initially after poweron we are in page 0, and hence we can't access
> Page 1 which actually has the `spi_mem_page` bit. So, how can we switch
> to Page 1 eventually since Page 1 covers all the relevant data/config
> registers ?
My approach was to just assume that the register would work in either page,
and that turned out to be correct. Most 'paging' chips work that way. Though
they typically use register 0 or 255 for that.
> I assume if you're in Page 0, then you can't access Page 1 registers or
> simply put those registers are not visible.
>
> Some inconsistency in datasheet:
>
> Page 24/50:
>
> "After power-on, spi_mem_page is in its reset state and page 0 (0x80 to 0xFF)
> will be active. Page 1 (0x00 to 0x7F) will be
> active on setting spi_mem_page to 1."
>
> OTOH,
>
> Page 26/50:
>
> "5.3.1.4 SPI memory map page selection – spi_mem_page
>
> In SPI mode complete memory page is accessed using page 0 & page 1.
> Register spi_mem_page is used for page selection.
> After power-on, spi_mem_page is in its reset state and
> page 0(0x00 to 0x7F) will be active."
>
>
> That's contradicting!
They fell into their own trap of calling the high range "page 0" I guess.
In this context, it's also meaningless. Upon probe, you cannot really be sure
which page is active anyway, and the chip doesn't have a reset GPIO to enforce
a known startup state. So "assume nothing" is the way forward.
> page 0 (0x80 to 0xFF) Vs page 0(0x00 to 0x7F) ?
>
>> The core needs access to both
>> ranges, so implement register read/write for both regions. The regmap
>> paging functionality didn't agree with a register that needs to be read
>> and modified, so I implemented a custom paging algorithm.
>>
>> This fixes that the device wouldn't even probe in SPI mode.
>
> Most importantly: Does it now work for you ?
Yes, with these changes the chip works (I have it connected to SPI only).
> As I tried you patch and it is not working for me in my QEMU setup.
>
> /sys/bus/iio/devices # lsmod
> Module Size Used byTainted: G
> bme680_spi 16384 0
> bme680_core24576 1 bme680_spi
> /sys/bus/iio/devices # ls
>
> SPI didn't probe and no logs for any errors/warnings as well.
Probably it isn't in the devicetree then. Mine looks like this:
+ {
+ status = "okay";
+ num-cs = <2>;
+
+ /* Environmental Sensor */
+ bme680_env: bme680_env@0 {
+ compatible = "bme680";
+ reg = <0>;
+ spi-max-frequency = <1000>;
+ };
> Maybe I'll give it try again later on a different board or a native
> build+boot on the current board.
>
> Anyway, does anyone has any idea/clues for writing a dt-overlay for
> am335x-boneblack-wireless.dts to add just a compatible property to
> probe bme680_spi driver ?
>
>> The SPI interface then isn't different from I2C, merged them into the core,
>> and the I2C/SPI named registers are no longer needed.
>>
>> Implemented register value caching for the registers to reduce the I2C/SPI
>> data transfers considerably.
>>
>> The calibration set reads as all zeroes until some undefined point in time,
>> and I couldn't determine what makes it valid. The datasheet mentions these
>> registers but does not provide any hints on when they become valid, and they
>> aren't even
Re: [PATCH] iio/chemical/bme680: Fix SPI read interface
Hello Mike, On Fri, Feb 15, 2019 at 02:47:55PM +0100, Mike Looijmans wrote: > The SPI interface implementation was completely broken. My apologies! SPI interface caused me a lot of trouble in my project timeline. I tried many different ways to try playing with acpi dsl, printks etc. One of the problem was also that we don't have any facility such `i2cdetect` or instantiate a `new_device` from userspace. Are there any methods that one should try to debug SPIs ? -- excluding the use of Oscilloscope With all these problems, I referred other drivers from the Bosch family, particularly BME280 which has exactly the same behavior and decided to do the same. I believed that it would also work cleanly for BME680 and now I see the page selection fuzz was the main problem. I tried to test it on Beaglebone but everytime something new pops-up with beagle setup such as they moved from capemgr -> uboot overlays. Lack of latest documentation on "What new in this release?" troubled me + most of blogs on setup are outdated. > When using the SPI interface, there are only 7 address bits, the upper bit > is controlled by a page select register. Isn't it that upper bit is controlled R/W ? And one question that I have in mind: Initially after poweron we are in page 0, and hence we can't access Page 1 which actually has the `spi_mem_page` bit. So, how can we switch to Page 1 eventually since Page 1 covers all the relevant data/config registers ? I assume if you're in Page 0, then you can't access Page 1 registers or simply put those registers are not visible. Some inconsistency in datasheet: Page 24/50: "After power-on, spi_mem_page is in its reset state and page 0 (0x80 to 0xFF) will be active. Page 1 (0x00 to 0x7F) will be active on setting spi_mem_page to 1." OTOH, Page 26/50: "5.3.1.4 SPI memory map page selection – spi_mem_page In SPI mode complete memory page is accessed using page 0 & page 1. Register spi_mem_page is used for page selection. After power-on, spi_mem_page is in its reset state and page 0(0x00 to 0x7F) will be active." That's contradicting! page 0 (0x80 to 0xFF) Vs page 0(0x00 to 0x7F) ? > The core needs access to both > ranges, so implement register read/write for both regions. The regmap > paging functionality didn't agree with a register that needs to be read > and modified, so I implemented a custom paging algorithm. > > This fixes that the device wouldn't even probe in SPI mode. Most importantly: Does it now work for you ? As I tried you patch and it is not working for me in my QEMU setup. /sys/bus/iio/devices # lsmod Module Size Used byTainted: G bme680_spi 16384 0 bme680_core24576 1 bme680_spi /sys/bus/iio/devices # ls SPI didn't probe and no logs for any errors/warnings as well. Maybe I'll give it try again later on a different board or a native build+boot on the current board. Anyway, does anyone has any idea/clues for writing a dt-overlay for am335x-boneblack-wireless.dts to add just a compatible property to probe bme680_spi driver ? > The SPI interface then isn't different from I2C, merged them into the core, > and the I2C/SPI named registers are no longer needed. > > Implemented register value caching for the registers to reduce the I2C/SPI > data transfers considerably. > > The calibration set reads as all zeroes until some undefined point in time, > and I couldn't determine what makes it valid. The datasheet mentions these > registers but does not provide any hints on when they become valid, and they > aren't even enumerated in the memory map. So check the calibration and > retry reading it from the device after each measurement until it provides > something valid. Hmm. One point I would point out here is that: if calib constants are invalid then without any doubt readings would wrong/disastrous. But I never saw this issue when testing I2C interface. As for missing info from datasheet, I mean Bosch doesn't even care to reply or involve in the discussions. This driver was also ported to Zephyr project and Pull request lying stale since November. In the end, developers from Nordic Semiconductor who majorly work on BLE and some other stuff took over their work and are now trying to complete it. Also, look around BSEC(https://www.bosch-sensortec.com/bst/products/all_products/bsecreveals) for more info that you may want to know. And go ahead reverse engineering[1] their code to know more OR work around and rely on the heuristics[2] that you observe while testing. [1] I saw a code snippet in BSEC which says after measurement is completed in FORCED mode and data is ready to read, we need to wait for current mode go in SLEEP mode before actually reading the data. [2] One fellow developer few moths ago explained to play with those calib constants as well: https://lore.kernel.org/linux-iio/20181215191743.GA1263@himanshu-Vostro-3559/ Then there are many
[PATCH] iio/chemical/bme680: Fix SPI read interface
The SPI interface implementation was completely broken.
When using the SPI interface, there are only 7 address bits, the upper bit
is controlled by a page select register. The core needs access to both
ranges, so implement register read/write for both regions. The regmap
paging functionality didn't agree with a register that needs to be read
and modified, so I implemented a custom paging algorithm.
This fixes that the device wouldn't even probe in SPI mode.
The SPI interface then isn't different from I2C, merged them into the core,
and the I2C/SPI named registers are no longer needed.
Implemented register value caching for the registers to reduce the I2C/SPI
data transfers considerably.
The calibration set reads as all zeroes until some undefined point in time,
and I couldn't determine what makes it valid. The datasheet mentions these
registers but does not provide any hints on when they become valid, and they
aren't even enumerated in the memory map. So check the calibration and
retry reading it from the device after each measurement until it provides
something valid.
Report temperature in millidegrees Celcius instead of degrees.
Signed-off-by: Mike Looijmans
---
drivers/iio/chemical/bme680.h | 6 +-
drivers/iio/chemical/bme680_core.c | 54 ++---
drivers/iio/chemical/bme680_i2c.c | 21 ---
drivers/iio/chemical/bme680_spi.c | 116 +
4 files changed, 126 insertions(+), 71 deletions(-)
diff --git a/drivers/iio/chemical/bme680.h b/drivers/iio/chemical/bme680.h
index 0ae89b87..4edc5d21 100644
--- a/drivers/iio/chemical/bme680.h
+++ b/drivers/iio/chemical/bme680.h
@@ -2,11 +2,9 @@
#ifndef BME680_H_
#define BME680_H_
-#define BME680_REG_CHIP_I2C_ID 0xD0
-#define BME680_REG_CHIP_SPI_ID 0x50
+#define BME680_REG_CHIP_ID 0xD0
#define BME680_CHIP_ID_VAL 0x61
-#define BME680_REG_SOFT_RESET_I2C 0xE0
-#define BME680_REG_SOFT_RESET_SPI 0x60
+#define BME680_REG_SOFT_RESET 0xE0
#define BME680_CMD_SOFTRESET 0xB6
#define BME680_REG_STATUS 0x73
#define BME680_SPI_MEM_PAGE_BIT BIT(4)
diff --git a/drivers/iio/chemical/bme680_core.c
b/drivers/iio/chemical/bme680_core.c
index 70c1fe4..ccde4c6 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -63,9 +63,23 @@ struct bme680_data {
s32 t_fine;
};
+static const struct regmap_range bme680_volatile_ranges[] = {
+ regmap_reg_range(BME680_REG_MEAS_STAT_0, BME680_REG_GAS_R_LSB),
+ regmap_reg_range(BME680_REG_STATUS, BME680_REG_STATUS),
+ regmap_reg_range(BME680_T2_LSB_REG, BME680_GH3_REG),
+};
+
+static const struct regmap_access_table bme680_volatile_table = {
+ .yes_ranges = bme680_volatile_ranges,
+ .n_yes_ranges = ARRAY_SIZE(bme680_volatile_ranges),
+};
+
const struct regmap_config bme680_regmap_config = {
.reg_bits = 8,
.val_bits = 8,
+ .max_register = 0xef,
+ .volatile_table = _volatile_table,
+ .cache_type = REGCACHE_RBTREE,
};
EXPORT_SYMBOL(bme680_regmap_config);
@@ -316,6 +330,10 @@ static s16 bme680_compensate_temp(struct bme680_data *data,
s64 var1, var2, var3;
s16 calc_temp;
+ /* If the calibration is invalid, attempt to reload it */
+ if (!calib->par_t2)
+ bme680_read_calib(data, calib);
+
var1 = (adc_temp >> 3) - (calib->par_t1 << 1);
var2 = (var1 * calib->par_t2) >> 11;
var3 = ((var1 >> 1) * (var1 >> 1)) >> 12;
@@ -583,8 +601,7 @@ static int bme680_gas_config(struct bme680_data *data)
return ret;
}
-static int bme680_read_temp(struct bme680_data *data,
- int *val, int *val2)
+static int bme680_read_temp(struct bme680_data *data, int *val)
{
struct device *dev = regmap_get_device(data->regmap);
int ret;
@@ -617,10 +634,9 @@ static int bme680_read_temp(struct bme680_data *data,
* compensate_press/compensate_humid to get compensated
* pressure/humidity readings.
*/
- if (val && val2) {
- *val = comp_temp;
- *val2 = 100;
- return IIO_VAL_FRACTIONAL;
+ if (val) {
+ *val = comp_temp * 10; /* Centidegrees to millidegrees */
+ return IIO_VAL_INT;
}
return ret;
@@ -635,7 +651,7 @@ static int bme680_read_press(struct bme680_data *data,
s32 adc_press;
/* Read and compensate temperature to get a reading of t_fine */
- ret = bme680_read_temp(data, NULL, NULL);
+ ret = bme680_read_temp(data, NULL);
if (ret < 0)
return ret;
@@ -668,7 +684,7 @@ static int bme680_read_humid(struct bme680_data *data,
u32 comp_humidity;
/* Read and compensate temperature to get a reading of t_fine */
- ret =
