2019-02-15 16:19:12

by Mike Looijmans

Subject: [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 <[email protected]>
---
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 = &bme680_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 = bme680_read_temp(data, NULL, NULL);
+ ret = bme680_read_temp(data, NULL);
if (ret < 0)
return ret;

@@ -761,7 +777,7 @@ static int bme680_read_raw(struct iio_dev *indio_dev,
case IIO_CHAN_INFO_PROCESSED:
switch (chan->type) {
case IIO_TEMP:
- return bme680_read_temp(data, val, val2);
+ return bme680_read_temp(data, val);
case IIO_PRESSURE:
return bme680_read_press(data, val, val2);
case IIO_HUMIDITYRELATIVE:
@@ -867,8 +883,28 @@ int bme680_core_probe(struct device *dev, struct regmap *regmap,
{
struct iio_dev *indio_dev;
struct bme680_data *data;
+ unsigned int val;
int ret;

+ ret = regmap_write(regmap, BME680_REG_SOFT_RESET,
+ BME680_CMD_SOFTRESET);
+ if (ret < 0) {
+ dev_err(dev, "Failed to reset chip\n");
+ return ret;
+ }
+
+ ret = regmap_read(regmap, BME680_REG_CHIP_ID, &val);
+ if (ret < 0) {
+ dev_err(dev, "Error reading chip ID\n");
+ return ret;
+ }
+
+ if (val != BME680_CHIP_ID_VAL) {
+ dev_err(dev, "Wrong chip ID, got %x expected %x\n",
+ val, BME680_CHIP_ID_VAL);
+ return -ENODEV;
+ }
+
indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
if (!indio_dev)
return -ENOMEM;
diff --git a/drivers/iio/chemical/bme680_i2c.c b/drivers/iio/chemical/bme680_i2c.c
index 06d4be5..cfc4449 100644
--- a/drivers/iio/chemical/bme680_i2c.c
+++ b/drivers/iio/chemical/bme680_i2c.c
@@ -23,8 +23,6 @@ static int bme680_i2c_probe(struct i2c_client *client,
{
struct regmap *regmap;
const char *name = NULL;
- unsigned int val;
- int ret;

regmap = devm_regmap_init_i2c(client, &bme680_regmap_config);
if (IS_ERR(regmap)) {
@@ -33,25 +31,6 @@ static int bme680_i2c_probe(struct i2c_client *client,
return PTR_ERR(regmap);
}

- ret = regmap_write(regmap, BME680_REG_SOFT_RESET_I2C,
- BME680_CMD_SOFTRESET);
- if (ret < 0) {
- dev_err(&client->dev, "Failed to reset chip\n");
- return ret;
- }
-
- ret = regmap_read(regmap, BME680_REG_CHIP_I2C_ID, &val);
- if (ret < 0) {
- dev_err(&client->dev, "Error reading I2C chip ID\n");
- return ret;
- }
-
- if (val != BME680_CHIP_ID_VAL) {
- dev_err(&client->dev, "Wrong chip ID, got %x expected %x\n",
- val, BME680_CHIP_ID_VAL);
- return -ENODEV;
- }
-
if (id)
name = id->name;

diff --git a/drivers/iio/chemical/bme680_spi.c b/drivers/iio/chemical/bme680_spi.c
index c9fb05e..e130715 100644
--- a/drivers/iio/chemical/bme680_spi.c
+++ b/drivers/iio/chemical/bme680_spi.c
@@ -11,28 +11,94 @@

#include "bme680.h"

+struct bme680_spi_bus_context {
+ struct spi_device *spi;
+ u8 current_page;
+};
+
+/*
+ * In SPI mode there are only 7 address bits, a "page" register determines
+ * which part of the 8-bit range is active. This function looks at the address
+ * and writes the page selection bit if needed
+ */
+static int bme680_regmap_spi_select_page(
+ struct bme680_spi_bus_context *ctx, u8 reg)
+{
+ struct spi_device *spi = ctx->spi;
+ int ret;
+ u8 buf[2];
+ u8 page = (reg & 0x80) ? 0 : 1; /* Page "1" is low range */
+
+ if (page == ctx->current_page)
+ return 0;
+
+ /*
+ * Data sheet claims we're only allowed to change bit 4, so we must do
+ * a read-modify-write on each and every page select
+ */
+ buf[0] = BME680_REG_STATUS;
+ ret = spi_write_then_read(spi, buf, 1, buf + 1, 1);
+ if (ret < 0) {
+ dev_err(&spi->dev, "failed to set page %u\n", page);
+ return ret;
+ }
+
+ buf[0] = BME680_REG_STATUS;
+ if (page)
+ buf[1] |= BME680_SPI_MEM_PAGE_BIT;
+ else
+ buf[1] &= ~BME680_SPI_MEM_PAGE_BIT;
+
+ ret = spi_write(spi, buf, 2);
+ if (ret < 0) {
+ dev_err(&spi->dev, "failed to set page %u\n", page);
+ return ret;
+ }
+
+ ctx->current_page = page;
+
+ return 0;
+}
+
static int bme680_regmap_spi_write(void *context, const void *data,
size_t count)
{
- struct spi_device *spi = context;
+ struct bme680_spi_bus_context *ctx = context;
+ struct spi_device *spi = ctx->spi;
+ int ret;
u8 buf[2];

memcpy(buf, data, 2);
+
+ ret = bme680_regmap_spi_select_page(ctx, buf[0]);
+ if (ret)
+ return ret;
+
/*
* The SPI register address (= full register address without bit 7)
* and the write command (bit7 = RW = '0')
*/
buf[0] &= ~0x80;

- return spi_write_then_read(spi, buf, 2, NULL, 0);
+ return spi_write(spi, buf, 2);
}

static int bme680_regmap_spi_read(void *context, const void *reg,
size_t reg_size, void *val, size_t val_size)
{
- struct spi_device *spi = context;
+ struct bme680_spi_bus_context *ctx = context;
+ struct spi_device *spi = ctx->spi;
+ int ret;
+ u8 addr = *(const u8 *)reg;
+ u8 addr7;
+
+ ret = bme680_regmap_spi_select_page(ctx, addr);
+ if (ret)
+ return ret;

- return spi_write_then_read(spi, reg, reg_size, val, val_size);
+ addr |= 0x80; /* bit7 = RW = '1' */
+
+ return spi_write_then_read(spi, &addr, 1, val, val_size);
}

static struct regmap_bus bme680_regmap_bus = {
@@ -45,8 +111,8 @@ static int bme680_regmap_spi_read(void *context, const void *reg,
static int bme680_spi_probe(struct spi_device *spi)
{
const struct spi_device_id *id = spi_get_device_id(spi);
+ struct bme680_spi_bus_context *bus_context;
struct regmap *regmap;
- unsigned int val;
int ret;

spi->bits_per_word = 8;
@@ -56,45 +122,21 @@ static int bme680_spi_probe(struct spi_device *spi)
return ret;
}

+ bus_context = devm_kzalloc(&spi->dev, sizeof(*bus_context), GFP_KERNEL);
+ if (!bus_context)
+ return -ENOMEM;
+
+ bus_context->spi = spi;
+ bus_context->current_page = 0xff; /* Undefined on warm boot */
+
regmap = devm_regmap_init(&spi->dev, &bme680_regmap_bus,
- &spi->dev, &bme680_regmap_config);
+ bus_context, &bme680_regmap_config);
if (IS_ERR(regmap)) {
dev_err(&spi->dev, "Failed to register spi regmap %d\n",
(int)PTR_ERR(regmap));
return PTR_ERR(regmap);
}

- ret = regmap_write(regmap, BME680_REG_SOFT_RESET_SPI,
- BME680_CMD_SOFTRESET);
- if (ret < 0) {
- dev_err(&spi->dev, "Failed to reset chip\n");
- return ret;
- }
-
- /* after power-on reset, Page 0(0x80-0xFF) of spi_mem_page is active */
- ret = regmap_read(regmap, BME680_REG_CHIP_SPI_ID, &val);
- if (ret < 0) {
- dev_err(&spi->dev, "Error reading SPI chip ID\n");
- return ret;
- }
-
- if (val != BME680_CHIP_ID_VAL) {
- dev_err(&spi->dev, "Wrong chip ID, got %x expected %x\n",
- val, BME680_CHIP_ID_VAL);
- return -ENODEV;
- }
- /*
- * select Page 1 of spi_mem_page to enable access to
- * to registers from address 0x00 to 0x7F.
- */
- ret = regmap_write_bits(regmap, BME680_REG_STATUS,
- BME680_SPI_MEM_PAGE_BIT,
- BME680_SPI_MEM_PAGE_1_VAL);
- if (ret < 0) {
- dev_err(&spi->dev, "failed to set page 1 of spi_mem_page\n");
- return ret;
- }
-
return bme680_core_probe(&spi->dev, regmap, id->name);
}

--
1.9.1

2019-02-16 13:14:50

by Himanshu Jha

Subject: 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 by Tainted: G
bme680_spi 16384 0
bme680_core 24576 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 things to rant about. But currently I'm busy with
university exams, so I might not be able to work on this till 10th March.

> Report temperature in millidegrees Celcius instead of degrees.

Jonathan, I didn't know about this but is there any rationale to report
in millidegress ?

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

> Signed-off-by: Mike Looijmans <[email protected]>
> ---
> 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,

Any reason for 0xEF ?

> + .volatile_table = &bme680_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);

How did you observe this behavior ? Was the readings not correct before ?

> 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 = bme680_read_temp(data, NULL, NULL);
> + ret = bme680_read_temp(data, NULL);
> if (ret < 0)
> return ret;
>
> @@ -761,7 +777,7 @@ static int bme680_read_raw(struct iio_dev *indio_dev,
> case IIO_CHAN_INFO_PROCESSED:
> switch (chan->type) {
> case IIO_TEMP:
> - return bme680_read_temp(data, val, val2);
> + return bme680_read_temp(data, val);
> case IIO_PRESSURE:
> return bme680_read_press(data, val, val2);
> case IIO_HUMIDITYRELATIVE:
> @@ -867,8 +883,28 @@ int bme680_core_probe(struct device *dev, struct regmap *regmap,
> {
> struct iio_dev *indio_dev;
> struct bme680_data *data;
> + unsigned int val;
> int ret;
>
> + ret = regmap_write(regmap, BME680_REG_SOFT_RESET,
> + BME680_CMD_SOFTRESET);
> + if (ret < 0) {
> + dev_err(dev, "Failed to reset chip\n");
> + return ret;
> + }

FYI Bosch commented to me earlier:

"It is recommended but not mandatory. The idea behind the soft
reset is to get the driver in sync with the sensor and the
fastest way to do so is via a soft reset."

But its good to keep it, nevermind ..

> + ret = regmap_read(regmap, BME680_REG_CHIP_ID, &val);
> + if (ret < 0) {
> + dev_err(dev, "Error reading chip ID\n");
> + return ret;
> + }
> +
> + if (val != BME680_CHIP_ID_VAL) {
> + dev_err(dev, "Wrong chip ID, got %x expected %x\n",
> + val, BME680_CHIP_ID_VAL);
> + return -ENODEV;
> + }
> +
> indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> if (!indio_dev)
> return -ENOMEM;
> diff --git a/drivers/iio/chemical/bme680_i2c.c b/drivers/iio/chemical/bme680_i2c.c
> index 06d4be5..cfc4449 100644
> --- a/drivers/iio/chemical/bme680_i2c.c
> +++ b/drivers/iio/chemical/bme680_i2c.c
> @@ -23,8 +23,6 @@ static int bme680_i2c_probe(struct i2c_client *client,
> {
> struct regmap *regmap;
> const char *name = NULL;
> - unsigned int val;
> - int ret;
>
> regmap = devm_regmap_init_i2c(client, &bme680_regmap_config);
> if (IS_ERR(regmap)) {
> @@ -33,25 +31,6 @@ static int bme680_i2c_probe(struct i2c_client *client,
> return PTR_ERR(regmap);
> }
>
> - ret = regmap_write(regmap, BME680_REG_SOFT_RESET_I2C,
> - BME680_CMD_SOFTRESET);
> - if (ret < 0) {
> - dev_err(&client->dev, "Failed to reset chip\n");
> - return ret;
> - }
> -
> - ret = regmap_read(regmap, BME680_REG_CHIP_I2C_ID, &val);
> - if (ret < 0) {
> - dev_err(&client->dev, "Error reading I2C chip ID\n");
> - return ret;
> - }
> -
> - if (val != BME680_CHIP_ID_VAL) {
> - dev_err(&client->dev, "Wrong chip ID, got %x expected %x\n",
> - val, BME680_CHIP_ID_VAL);
> - return -ENODEV;
> - }
> -
> if (id)
> name = id->name;
>
> diff --git a/drivers/iio/chemical/bme680_spi.c b/drivers/iio/chemical/bme680_spi.c
> index c9fb05e..e130715 100644
> --- a/drivers/iio/chemical/bme680_spi.c
> +++ b/drivers/iio/chemical/bme680_spi.c
> @@ -11,28 +11,94 @@
>
> #include "bme680.h"
>
> +struct bme680_spi_bus_context {
> + struct spi_device *spi;
> + u8 current_page;
> +};
> +
> +/*
> + * In SPI mode there are only 7 address bits, a "page" register determines
> + * which part of the 8-bit range is active. This function looks at the address
> + * and writes the page selection bit if needed
> + */
> +static int bme680_regmap_spi_select_page(
> + struct bme680_spi_bus_context *ctx, u8 reg)
> +{
> + struct spi_device *spi = ctx->spi;
> + int ret;
> + u8 buf[2];
> + u8 page = (reg & 0x80) ? 0 : 1; /* Page "1" is low range */
> +
> + if (page == ctx->current_page)
> + return 0;
> +
> + /*
> + * Data sheet claims we're only allowed to change bit 4, so we must do
> + * a read-modify-write on each and every page select
> + */
> + buf[0] = BME680_REG_STATUS;
> + ret = spi_write_then_read(spi, buf, 1, buf + 1, 1);
> + if (ret < 0) {
> + dev_err(&spi->dev, "failed to set page %u\n", page);
> + return ret;
> + }
> +
> + buf[0] = BME680_REG_STATUS;
> + if (page)
> + buf[1] |= BME680_SPI_MEM_PAGE_BIT;
> + else
> + buf[1] &= ~BME680_SPI_MEM_PAGE_BIT;
> +
> + ret = spi_write(spi, buf, 2);
> + if (ret < 0) {
> + dev_err(&spi->dev, "failed to set page %u\n", page);
> + return ret;
> + }
> +
> + ctx->current_page = page;
> +
> + return 0;
> +}

I will take another look at this later as my head is spinning now ...

> static int bme680_regmap_spi_write(void *context, const void *data,
> size_t count)
> {
> - struct spi_device *spi = context;
> + struct bme680_spi_bus_context *ctx = context;
> + struct spi_device *spi = ctx->spi;
> + int ret;
> u8 buf[2];
>
> memcpy(buf, data, 2);
> +
> + ret = bme680_regmap_spi_select_page(ctx, buf[0]);
> + if (ret)
> + return ret;
> +
> /*
> * The SPI register address (= full register address without bit 7)
> * and the write command (bit7 = RW = '0')
> */
> buf[0] &= ~0x80;
>
> - return spi_write_then_read(spi, buf, 2, NULL, 0);
> + return spi_write(spi, buf, 2);
> }
>
> static int bme680_regmap_spi_read(void *context, const void *reg,
> size_t reg_size, void *val, size_t val_size)
> {
> - struct spi_device *spi = context;
> + struct bme680_spi_bus_context *ctx = context;
> + struct spi_device *spi = ctx->spi;
> + int ret;
> + u8 addr = *(const u8 *)reg;
> + u8 addr7;

Unused variable.

> + ret = bme680_regmap_spi_select_page(ctx, addr);
> + if (ret)
> + return ret;
>
> - return spi_write_then_read(spi, reg, reg_size, val, val_size);
> + addr |= 0x80; /* bit7 = RW = '1' */
> +
> + return spi_write_then_read(spi, &addr, 1, val, val_size);
> }
>
> static struct regmap_bus bme680_regmap_bus = {
> @@ -45,8 +111,8 @@ static int bme680_regmap_spi_read(void *context, const void *reg,
> static int bme680_spi_probe(struct spi_device *spi)
> {
> const struct spi_device_id *id = spi_get_device_id(spi);
> + struct bme680_spi_bus_context *bus_context;
> struct regmap *regmap;
> - unsigned int val;
> int ret;
>
> spi->bits_per_word = 8;
> @@ -56,45 +122,21 @@ static int bme680_spi_probe(struct spi_device *spi)
> return ret;
> }
>
> + bus_context = devm_kzalloc(&spi->dev, sizeof(*bus_context), GFP_KERNEL);
> + if (!bus_context)
> + return -ENOMEM;
> +
> + bus_context->spi = spi;
> + bus_context->current_page = 0xff; /* Undefined on warm boot */

OK. This is what you observed.

If this patch works as expected then I think a "Fixes:" tag should be
added while marking it for stable ?

Thanks for the patch Mike and apologies again for the whole mess.

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

2019-02-18 07:06:46

by Mike Looijmans

Subject: 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 by Tainted: G
> bme680_spi 16384 0
> bme680_core 24576 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:

+&spi1 {
+ status = "okay";
+ num-cs = <2>;
+
+ /* Environmental Sensor */
+ bme680_env: bme680_env@0 {
+ compatible = "bme680";
+ reg = <0>;
+ spi-max-frequency = <10000000>;
+ };


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

It might be timing related. The SPI bus runs at 10MHz, and my platform is a
ZynqMP.

My initial idea was to just read everything from page 0 (only chip-id, reset,
and calibration data is there) and then switch to page 1 and remain there. But
the calibration data always read zero at probe time. Reading the registers
again at a later time made them valid.

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

Yeah, there are only few manufacturers who care about drivers. Most seem to
think that a bare-metal app or a dedicated 3.1 kernel is all the world will
ever need.

Most of our hardware developers have learned that software support is often
more important than price or features.

> 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 things to rant about. But currently I'm busy with
> university exams, so I might not be able to work on this till 10th March.
>
>> Report temperature in millidegrees Celcius instead of degrees.
>
> Jonathan, I didn't know about this but is there any rationale to report
> in millidegress ?
>
> 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

All IIO drivers report millidegrees. It's probably related to hwmon.


>> Signed-off-by: Mike Looijmans <[email protected]>
>> ---
>> 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,
>
> Any reason for 0xEF ?

It's what I calculated the highest possibly address to be. It mostly affects
debugging (/sys/kernel/debug/regmap).


>> + .volatile_table = &bme680_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);
>
> How did you observe this behavior ? Was the readings not correct before ?

I always read back either zeroes or valid data from the calibration data. If
par_t2 is zero, it nulls the temperature readout, so it must have a non-zero
value to be valid.

I think the chips needs at least a 'forced' mode before it actually loads
these, or maybe they're just in a memory region that so slow that it takes the
chip seconds to read them into its register space.

The datasheet doesn't mention much about these registers at all, so this is
the best I could come up with.


>
>> 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 = bme680_read_temp(data, NULL, NULL);
>> + ret = bme680_read_temp(data, NULL);
>> if (ret < 0)
>> return ret;
>>
>> @@ -761,7 +777,7 @@ static int bme680_read_raw(struct iio_dev *indio_dev,
>> case IIO_CHAN_INFO_PROCESSED:
>> switch (chan->type) {
>> case IIO_TEMP:
>> - return bme680_read_temp(data, val, val2);
>> + return bme680_read_temp(data, val);
>> case IIO_PRESSURE:
>> return bme680_read_press(data, val, val2);
>> case IIO_HUMIDITYRELATIVE:
>> @@ -867,8 +883,28 @@ int bme680_core_probe(struct device *dev, struct regmap *regmap,
>> {
>> struct iio_dev *indio_dev;
>> struct bme680_data *data;
>> + unsigned int val;
>> int ret;
>>
>> + ret = regmap_write(regmap, BME680_REG_SOFT_RESET,
>> + BME680_CMD_SOFTRESET);
>> + if (ret < 0) {
>> + dev_err(dev, "Failed to reset chip\n");
>> + return ret;
>> + }
>
> FYI Bosch commented to me earlier:
>
> "It is recommended but not mandatory. The idea behind the soft
> reset is to get the driver in sync with the sensor and the
> fastest way to do so is via a soft reset."
>
> But its good to keep it, nevermind ..

This initialization block was just moved from the SPI and I2C parts into the
core, no change in behaviour.

>
>> + ret = regmap_read(regmap, BME680_REG_CHIP_ID, &val);
>> + if (ret < 0) {
>> + dev_err(dev, "Error reading chip ID\n");
>> + return ret;
>> + }
>> +
>> + if (val != BME680_CHIP_ID_VAL) {
>> + dev_err(dev, "Wrong chip ID, got %x expected %x\n",
>> + val, BME680_CHIP_ID_VAL);
>> + return -ENODEV;
>> + }
>> +
>> indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
>> if (!indio_dev)
>> return -ENOMEM;
>> diff --git a/drivers/iio/chemical/bme680_i2c.c b/drivers/iio/chemical/bme680_i2c.c
>> index 06d4be5..cfc4449 100644
>> --- a/drivers/iio/chemical/bme680_i2c.c
>> +++ b/drivers/iio/chemical/bme680_i2c.c
>> @@ -23,8 +23,6 @@ static int bme680_i2c_probe(struct i2c_client *client,
>> {
>> struct regmap *regmap;
>> const char *name = NULL;
>> - unsigned int val;
>> - int ret;
>>
>> regmap = devm_regmap_init_i2c(client, &bme680_regmap_config);
>> if (IS_ERR(regmap)) {
>> @@ -33,25 +31,6 @@ static int bme680_i2c_probe(struct i2c_client *client,
>> return PTR_ERR(regmap);
>> }
>>
>> - ret = regmap_write(regmap, BME680_REG_SOFT_RESET_I2C,
>> - BME680_CMD_SOFTRESET);
>> - if (ret < 0) {
>> - dev_err(&client->dev, "Failed to reset chip\n");
>> - return ret;
>> - }
>> -
>> - ret = regmap_read(regmap, BME680_REG_CHIP_I2C_ID, &val);
>> - if (ret < 0) {
>> - dev_err(&client->dev, "Error reading I2C chip ID\n");
>> - return ret;
>> - }
>> -
>> - if (val != BME680_CHIP_ID_VAL) {
>> - dev_err(&client->dev, "Wrong chip ID, got %x expected %x\n",
>> - val, BME680_CHIP_ID_VAL);
>> - return -ENODEV;
>> - }
>> -
>> if (id)
>> name = id->name;
>>
>> diff --git a/drivers/iio/chemical/bme680_spi.c b/drivers/iio/chemical/bme680_spi.c
>> index c9fb05e..e130715 100644
>> --- a/drivers/iio/chemical/bme680_spi.c
>> +++ b/drivers/iio/chemical/bme680_spi.c
>> @@ -11,28 +11,94 @@
>>
>> #include "bme680.h"
>>
>> +struct bme680_spi_bus_context {
>> + struct spi_device *spi;
>> + u8 current_page;
>> +};
>> +
>> +/*
>> + * In SPI mode there are only 7 address bits, a "page" register determines
>> + * which part of the 8-bit range is active. This function looks at the address
>> + * and writes the page selection bit if needed
>> + */
>> +static int bme680_regmap_spi_select_page(
>> + struct bme680_spi_bus_context *ctx, u8 reg)
>> +{
>> + struct spi_device *spi = ctx->spi;
>> + int ret;
>> + u8 buf[2];
>> + u8 page = (reg & 0x80) ? 0 : 1; /* Page "1" is low range */
>> +
>> + if (page == ctx->current_page)
>> + return 0;
>> +
>> + /*
>> + * Data sheet claims we're only allowed to change bit 4, so we must do
>> + * a read-modify-write on each and every page select
>> + */
>> + buf[0] = BME680_REG_STATUS;
>> + ret = spi_write_then_read(spi, buf, 1, buf + 1, 1);
>> + if (ret < 0) {
>> + dev_err(&spi->dev, "failed to set page %u\n", page);
>> + return ret;
>> + }
>> +
>> + buf[0] = BME680_REG_STATUS;
>> + if (page)
>> + buf[1] |= BME680_SPI_MEM_PAGE_BIT;
>> + else
>> + buf[1] &= ~BME680_SPI_MEM_PAGE_BIT;
>> +
>> + ret = spi_write(spi, buf, 2);
>> + if (ret < 0) {
>> + dev_err(&spi->dev, "failed to set page %u\n", page);
>> + return ret;
>> + }
>> +
>> + ctx->current_page = page;
>> +
>> + return 0;
>> +}
>
> I will take another look at this later as my head is spinning now ...
>
>> static int bme680_regmap_spi_write(void *context, const void *data,
>> size_t count)
>> {
>> - struct spi_device *spi = context;
>> + struct bme680_spi_bus_context *ctx = context;
>> + struct spi_device *spi = ctx->spi;
>> + int ret;
>> u8 buf[2];
>>
>> memcpy(buf, data, 2);
>> +
>> + ret = bme680_regmap_spi_select_page(ctx, buf[0]);
>> + if (ret)
>> + return ret;
>> +
>> /*
>> * The SPI register address (= full register address without bit 7)
>> * and the write command (bit7 = RW = '0')
>> */
>> buf[0] &= ~0x80;
>>
>> - return spi_write_then_read(spi, buf, 2, NULL, 0);
>> + return spi_write(spi, buf, 2);
>> }
>>
>> static int bme680_regmap_spi_read(void *context, const void *reg,
>> size_t reg_size, void *val, size_t val_size)
>> {
>> - struct spi_device *spi = context;
>> + struct bme680_spi_bus_context *ctx = context;
>> + struct spi_device *spi = ctx->spi;
>> + int ret;
>> + u8 addr = *(const u8 *)reg;
>> + u8 addr7;
>
> Unused variable.

Indeed. Will need a v2.

>
>> + ret = bme680_regmap_spi_select_page(ctx, addr);
>> + if (ret)
>> + return ret;
>>
>> - return spi_write_then_read(spi, reg, reg_size, val, val_size);
>> + addr |= 0x80; /* bit7 = RW = '1' */
>> +
>> + return spi_write_then_read(spi, &addr, 1, val, val_size);
>> }
>>
>> static struct regmap_bus bme680_regmap_bus = {
>> @@ -45,8 +111,8 @@ static int bme680_regmap_spi_read(void *context, const void *reg,
>> static int bme680_spi_probe(struct spi_device *spi)
>> {
>> const struct spi_device_id *id = spi_get_device_id(spi);
>> + struct bme680_spi_bus_context *bus_context;
>> struct regmap *regmap;
>> - unsigned int val;
>> int ret;
>>
>> spi->bits_per_word = 8;
>> @@ -56,45 +122,21 @@ static int bme680_spi_probe(struct spi_device *spi)
>> return ret;
>> }
>>
>> + bus_context = devm_kzalloc(&spi->dev, sizeof(*bus_context), GFP_KERNEL);
>> + if (!bus_context)
>> + return -ENOMEM;
>> +
>> + bus_context->spi = spi;
>> + bus_context->current_page = 0xff; /* Undefined on warm boot */
>
> OK. This is what you observed.
>
> If this patch works as expected then I think a "Fixes:" tag should be
> added while marking it for stable ?
>
> Thanks for the patch Mike and apologies again for the whole mess.
>

You're welcome.

2019-02-20 11:23:51

by Jonathan Cameron

Subject: 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

2019-02-20 11:34:51

by Jonathan Cameron

Subject: Re: [PATCH] iio/chemical/bme680: Fix SPI read interface

On Mon, 18 Feb 2019 07:03:10 +0000
Mike Looijmans <[email protected]> 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 by Tainted: G
> > bme680_spi 16384 0
> > bme680_core 24576 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:
>
> +&spi1 {
> + status = "okay";
> + num-cs = <2>;
> +
> + /* Environmental Sensor */
> + bme680_env: bme680_env@0 {
> + compatible = "bme680";
> + reg = <0>;
> + spi-max-frequency = <10000000>;
> + };
>
>
> > 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.
>
> It might be timing related. The SPI bus runs at 10MHz, and my platform is a
> ZynqMP.
>
> My initial idea was to just read everything from page 0 (only chip-id, reset,
> and calibration data is there) and then switch to page 1 and remain there. But
> the calibration data always read zero at probe time. Reading the registers
> again at a later time made them valid.
>
> > 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.
>
> Yeah, there are only few manufacturers who care about drivers. Most seem to
> think that a bare-metal app or a dedicated 3.1 kernel is all the world will
> ever need.
>
> Most of our hardware developers have learned that software support is often
> more important than price or features.

Absolutely!

>
> > 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 things to rant about. But currently I'm busy with
> > university exams, so I might not be able to work on this till 10th March.
> >
> >> Report temperature in millidegrees Celcius instead of degrees.
> >
> > Jonathan, I didn't know about this but is there any rationale to report
> > in millidegress ?
> >
> > 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
>
> All IIO drivers report millidegrees. It's probably related to hwmon.

Indeed. Good guess :)

>
>
> >> Signed-off-by: Mike Looijmans <[email protected]>
> >> ---
> >> 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,
> >
> > Any reason for 0xEF ?
>
> It's what I calculated the highest possibly address to be. It mostly affects
> debugging (/sys/kernel/debug/regmap).
>
>
> >> + .volatile_table = &bme680_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);
> >
> > How did you observe this behavior ? Was the readings not correct before ?
>
> I always read back either zeroes or valid data from the calibration data. If
> par_t2 is zero, it nulls the temperature readout, so it must have a non-zero
> value to be valid.
>
> I think the chips needs at least a 'forced' mode before it actually loads
> these, or maybe they're just in a memory region that so slow that it takes the
> chip seconds to read them into its register space.
>
> The datasheet doesn't mention much about these registers at all, so this is
> the best I could come up with.
>
>
> >
> >> 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 = bme680_read_temp(data, NULL, NULL);
> >> + ret = bme680_read_temp(data, NULL);
> >> if (ret < 0)
> >> return ret;
> >>
> >> @@ -761,7 +777,7 @@ static int bme680_read_raw(struct iio_dev *indio_dev,
> >> case IIO_CHAN_INFO_PROCESSED:
> >> switch (chan->type) {
> >> case IIO_TEMP:
> >> - return bme680_read_temp(data, val, val2);
> >> + return bme680_read_temp(data, val);
> >> case IIO_PRESSURE:
> >> return bme680_read_press(data, val, val2);
> >> case IIO_HUMIDITYRELATIVE:
> >> @@ -867,8 +883,28 @@ int bme680_core_probe(struct device *dev, struct regmap *regmap,
> >> {
> >> struct iio_dev *indio_dev;
> >> struct bme680_data *data;
> >> + unsigned int val;
> >> int ret;
> >>
> >> + ret = regmap_write(regmap, BME680_REG_SOFT_RESET,
> >> + BME680_CMD_SOFTRESET);
> >> + if (ret < 0) {
> >> + dev_err(dev, "Failed to reset chip\n");
> >> + return ret;
> >> + }
> >
> > FYI Bosch commented to me earlier:
> >
> > "It is recommended but not mandatory. The idea behind the soft
> > reset is to get the driver in sync with the sensor and the
> > fastest way to do so is via a soft reset."
> >
> > But its good to keep it, nevermind ..
>
> This initialization block was just moved from the SPI and I2C parts into the
> core, no change in behaviour.
>
> >
> >> + ret = regmap_read(regmap, BME680_REG_CHIP_ID, &val);
> >> + if (ret < 0) {
> >> + dev_err(dev, "Error reading chip ID\n");
> >> + return ret;
> >> + }
> >> +
> >> + if (val != BME680_CHIP_ID_VAL) {
> >> + dev_err(dev, "Wrong chip ID, got %x expected %x\n",
> >> + val, BME680_CHIP_ID_VAL);
> >> + return -ENODEV;
> >> + }
> >> +
> >> indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> >> if (!indio_dev)
> >> return -ENOMEM;
> >> diff --git a/drivers/iio/chemical/bme680_i2c.c b/drivers/iio/chemical/bme680_i2c.c
> >> index 06d4be5..cfc4449 100644
> >> --- a/drivers/iio/chemical/bme680_i2c.c
> >> +++ b/drivers/iio/chemical/bme680_i2c.c
> >> @@ -23,8 +23,6 @@ static int bme680_i2c_probe(struct i2c_client *client,
> >> {
> >> struct regmap *regmap;
> >> const char *name = NULL;
> >> - unsigned int val;
> >> - int ret;
> >>
> >> regmap = devm_regmap_init_i2c(client, &bme680_regmap_config);
> >> if (IS_ERR(regmap)) {
> >> @@ -33,25 +31,6 @@ static int bme680_i2c_probe(struct i2c_client *client,
> >> return PTR_ERR(regmap);
> >> }
> >>
> >> - ret = regmap_write(regmap, BME680_REG_SOFT_RESET_I2C,
> >> - BME680_CMD_SOFTRESET);
> >> - if (ret < 0) {
> >> - dev_err(&client->dev, "Failed to reset chip\n");
> >> - return ret;
> >> - }
> >> -
> >> - ret = regmap_read(regmap, BME680_REG_CHIP_I2C_ID, &val);
> >> - if (ret < 0) {
> >> - dev_err(&client->dev, "Error reading I2C chip ID\n");
> >> - return ret;
> >> - }
> >> -
> >> - if (val != BME680_CHIP_ID_VAL) {
> >> - dev_err(&client->dev, "Wrong chip ID, got %x expected %x\n",
> >> - val, BME680_CHIP_ID_VAL);
> >> - return -ENODEV;
> >> - }
> >> -
> >> if (id)
> >> name = id->name;
> >>
> >> diff --git a/drivers/iio/chemical/bme680_spi.c b/drivers/iio/chemical/bme680_spi.c
> >> index c9fb05e..e130715 100644
> >> --- a/drivers/iio/chemical/bme680_spi.c
> >> +++ b/drivers/iio/chemical/bme680_spi.c
> >> @@ -11,28 +11,94 @@
> >>
> >> #include "bme680.h"
> >>
> >> +struct bme680_spi_bus_context {
> >> + struct spi_device *spi;
> >> + u8 current_page;
> >> +};
> >> +
> >> +/*
> >> + * In SPI mode there are only 7 address bits, a "page" register determines
> >> + * which part of the 8-bit range is active. This function looks at the address
> >> + * and writes the page selection bit if needed
> >> + */
> >> +static int bme680_regmap_spi_select_page(
> >> + struct bme680_spi_bus_context *ctx, u8 reg)
> >> +{
> >> + struct spi_device *spi = ctx->spi;
> >> + int ret;
> >> + u8 buf[2];
> >> + u8 page = (reg & 0x80) ? 0 : 1; /* Page "1" is low range */
> >> +
> >> + if (page == ctx->current_page)
> >> + return 0;
> >> +
> >> + /*
> >> + * Data sheet claims we're only allowed to change bit 4, so we must do
> >> + * a read-modify-write on each and every page select
> >> + */
> >> + buf[0] = BME680_REG_STATUS;
> >> + ret = spi_write_then_read(spi, buf, 1, buf + 1, 1);
> >> + if (ret < 0) {
> >> + dev_err(&spi->dev, "failed to set page %u\n", page);
> >> + return ret;
> >> + }
> >> +
> >> + buf[0] = BME680_REG_STATUS;
> >> + if (page)
> >> + buf[1] |= BME680_SPI_MEM_PAGE_BIT;
> >> + else
> >> + buf[1] &= ~BME680_SPI_MEM_PAGE_BIT;
> >> +
> >> + ret = spi_write(spi, buf, 2);
> >> + if (ret < 0) {
> >> + dev_err(&spi->dev, "failed to set page %u\n", page);
> >> + return ret;
> >> + }
> >> +
> >> + ctx->current_page = page;
> >> +
> >> + return 0;
> >> +}
> >
> > I will take another look at this later as my head is spinning now ...
> >
> >> static int bme680_regmap_spi_write(void *context, const void *data,
> >> size_t count)
> >> {
> >> - struct spi_device *spi = context;
> >> + struct bme680_spi_bus_context *ctx = context;
> >> + struct spi_device *spi = ctx->spi;
> >> + int ret;
> >> u8 buf[2];
> >>
> >> memcpy(buf, data, 2);
> >> +
> >> + ret = bme680_regmap_spi_select_page(ctx, buf[0]);
> >> + if (ret)
> >> + return ret;
> >> +
> >> /*
> >> * The SPI register address (= full register address without bit 7)
> >> * and the write command (bit7 = RW = '0')
> >> */
> >> buf[0] &= ~0x80;
> >>
> >> - return spi_write_then_read(spi, buf, 2, NULL, 0);
> >> + return spi_write(spi, buf, 2);
> >> }
> >>
> >> static int bme680_regmap_spi_read(void *context, const void *reg,
> >> size_t reg_size, void *val, size_t val_size)
> >> {
> >> - struct spi_device *spi = context;
> >> + struct bme680_spi_bus_context *ctx = context;
> >> + struct spi_device *spi = ctx->spi;
> >> + int ret;
> >> + u8 addr = *(const u8 *)reg;
> >> + u8 addr7;
> >
> > Unused variable.
>
> Indeed. Will need a v2.
>
> >
> >> + ret = bme680_regmap_spi_select_page(ctx, addr);
> >> + if (ret)
> >> + return ret;
> >>
> >> - return spi_write_then_read(spi, reg, reg_size, val, val_size);
> >> + addr |= 0x80; /* bit7 = RW = '1' */
> >> +
> >> + return spi_write_then_read(spi, &addr, 1, val, val_size);
> >> }
> >>
> >> static struct regmap_bus bme680_regmap_bus = {
> >> @@ -45,8 +111,8 @@ static int bme680_regmap_spi_read(void *context, const void *reg,
> >> static int bme680_spi_probe(struct spi_device *spi)
> >> {
> >> const struct spi_device_id *id = spi_get_device_id(spi);
> >> + struct bme680_spi_bus_context *bus_context;
> >> struct regmap *regmap;
> >> - unsigned int val;
> >> int ret;
> >>
> >> spi->bits_per_word = 8;
> >> @@ -56,45 +122,21 @@ static int bme680_spi_probe(struct spi_device *spi)
> >> return ret;
> >> }
> >>
> >> + bus_context = devm_kzalloc(&spi->dev, sizeof(*bus_context), GFP_KERNEL);
> >> + if (!bus_context)
> >> + return -ENOMEM;
> >> +
> >> + bus_context->spi = spi;
> >> + bus_context->current_page = 0xff; /* Undefined on warm boot */
> >
> > OK. This is what you observed.
> >
> > If this patch works as expected then I think a "Fixes:" tag should be
> > added while marking it for stable ?
> >
> > Thanks for the patch Mike and apologies again for the whole mess.
> >
>
> You're welcome.

Nice work Mike.

Jonathan

2019-02-21 09:22:36

by Mike Looijmans

Subject: [PATCH v2] 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 <[email protected]>
---
v2: Remove unused 'addr7' variable

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 | 115 +++++++++++++++++++++++++------------
4 files changed, 125 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 = &bme680_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 = bme680_read_temp(data, NULL, NULL);
+ ret = bme680_read_temp(data, NULL);
if (ret < 0)
return ret;

@@ -761,7 +777,7 @@ static int bme680_read_raw(struct iio_dev *indio_dev,
case IIO_CHAN_INFO_PROCESSED:
switch (chan->type) {
case IIO_TEMP:
- return bme680_read_temp(data, val, val2);
+ return bme680_read_temp(data, val);
case IIO_PRESSURE:
return bme680_read_press(data, val, val2);
case IIO_HUMIDITYRELATIVE:
@@ -867,8 +883,28 @@ int bme680_core_probe(struct device *dev, struct regmap *regmap,
{
struct iio_dev *indio_dev;
struct bme680_data *data;
+ unsigned int val;
int ret;

+ ret = regmap_write(regmap, BME680_REG_SOFT_RESET,
+ BME680_CMD_SOFTRESET);
+ if (ret < 0) {
+ dev_err(dev, "Failed to reset chip\n");
+ return ret;
+ }
+
+ ret = regmap_read(regmap, BME680_REG_CHIP_ID, &val);
+ if (ret < 0) {
+ dev_err(dev, "Error reading chip ID\n");
+ return ret;
+ }
+
+ if (val != BME680_CHIP_ID_VAL) {
+ dev_err(dev, "Wrong chip ID, got %x expected %x\n",
+ val, BME680_CHIP_ID_VAL);
+ return -ENODEV;
+ }
+
indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
if (!indio_dev)
return -ENOMEM;
diff --git a/drivers/iio/chemical/bme680_i2c.c b/drivers/iio/chemical/bme680_i2c.c
index 06d4be5..cfc4449 100644
--- a/drivers/iio/chemical/bme680_i2c.c
+++ b/drivers/iio/chemical/bme680_i2c.c
@@ -23,8 +23,6 @@ static int bme680_i2c_probe(struct i2c_client *client,
{
struct regmap *regmap;
const char *name = NULL;
- unsigned int val;
- int ret;

regmap = devm_regmap_init_i2c(client, &bme680_regmap_config);
if (IS_ERR(regmap)) {
@@ -33,25 +31,6 @@ static int bme680_i2c_probe(struct i2c_client *client,
return PTR_ERR(regmap);
}

- ret = regmap_write(regmap, BME680_REG_SOFT_RESET_I2C,
- BME680_CMD_SOFTRESET);
- if (ret < 0) {
- dev_err(&client->dev, "Failed to reset chip\n");
- return ret;
- }
-
- ret = regmap_read(regmap, BME680_REG_CHIP_I2C_ID, &val);
- if (ret < 0) {
- dev_err(&client->dev, "Error reading I2C chip ID\n");
- return ret;
- }
-
- if (val != BME680_CHIP_ID_VAL) {
- dev_err(&client->dev, "Wrong chip ID, got %x expected %x\n",
- val, BME680_CHIP_ID_VAL);
- return -ENODEV;
- }
-
if (id)
name = id->name;

diff --git a/drivers/iio/chemical/bme680_spi.c b/drivers/iio/chemical/bme680_spi.c
index c9fb05e..881778e 100644
--- a/drivers/iio/chemical/bme680_spi.c
+++ b/drivers/iio/chemical/bme680_spi.c
@@ -11,28 +11,93 @@

#include "bme680.h"

+struct bme680_spi_bus_context {
+ struct spi_device *spi;
+ u8 current_page;
+};
+
+/*
+ * In SPI mode there are only 7 address bits, a "page" register determines
+ * which part of the 8-bit range is active. This function looks at the address
+ * and writes the page selection bit if needed
+ */
+static int bme680_regmap_spi_select_page(
+ struct bme680_spi_bus_context *ctx, u8 reg)
+{
+ struct spi_device *spi = ctx->spi;
+ int ret;
+ u8 buf[2];
+ u8 page = (reg & 0x80) ? 0 : 1; /* Page "1" is low range */
+
+ if (page == ctx->current_page)
+ return 0;
+
+ /*
+ * Data sheet claims we're only allowed to change bit 4, so we must do
+ * a read-modify-write on each and every page select
+ */
+ buf[0] = BME680_REG_STATUS;
+ ret = spi_write_then_read(spi, buf, 1, buf + 1, 1);
+ if (ret < 0) {
+ dev_err(&spi->dev, "failed to set page %u\n", page);
+ return ret;
+ }
+
+ buf[0] = BME680_REG_STATUS;
+ if (page)
+ buf[1] |= BME680_SPI_MEM_PAGE_BIT;
+ else
+ buf[1] &= ~BME680_SPI_MEM_PAGE_BIT;
+
+ ret = spi_write(spi, buf, 2);
+ if (ret < 0) {
+ dev_err(&spi->dev, "failed to set page %u\n", page);
+ return ret;
+ }
+
+ ctx->current_page = page;
+
+ return 0;
+}
+
static int bme680_regmap_spi_write(void *context, const void *data,
size_t count)
{
- struct spi_device *spi = context;
+ struct bme680_spi_bus_context *ctx = context;
+ struct spi_device *spi = ctx->spi;
+ int ret;
u8 buf[2];

memcpy(buf, data, 2);
+
+ ret = bme680_regmap_spi_select_page(ctx, buf[0]);
+ if (ret)
+ return ret;
+
/*
* The SPI register address (= full register address without bit 7)
* and the write command (bit7 = RW = '0')
*/
buf[0] &= ~0x80;

- return spi_write_then_read(spi, buf, 2, NULL, 0);
+ return spi_write(spi, buf, 2);
}

static int bme680_regmap_spi_read(void *context, const void *reg,
size_t reg_size, void *val, size_t val_size)
{
- struct spi_device *spi = context;
+ struct bme680_spi_bus_context *ctx = context;
+ struct spi_device *spi = ctx->spi;
+ int ret;
+ u8 addr = *(const u8 *)reg;
+
+ ret = bme680_regmap_spi_select_page(ctx, addr);
+ if (ret)
+ return ret;

- return spi_write_then_read(spi, reg, reg_size, val, val_size);
+ addr |= 0x80; /* bit7 = RW = '1' */
+
+ return spi_write_then_read(spi, &addr, 1, val, val_size);
}

static struct regmap_bus bme680_regmap_bus = {
@@ -45,8 +110,8 @@ static int bme680_regmap_spi_read(void *context, const void *reg,
static int bme680_spi_probe(struct spi_device *spi)
{
const struct spi_device_id *id = spi_get_device_id(spi);
+ struct bme680_spi_bus_context *bus_context;
struct regmap *regmap;
- unsigned int val;
int ret;

spi->bits_per_word = 8;
@@ -56,45 +121,21 @@ static int bme680_spi_probe(struct spi_device *spi)
return ret;
}

+ bus_context = devm_kzalloc(&spi->dev, sizeof(*bus_context), GFP_KERNEL);
+ if (!bus_context)
+ return -ENOMEM;
+
+ bus_context->spi = spi;
+ bus_context->current_page = 0xff; /* Undefined on warm boot */
+
regmap = devm_regmap_init(&spi->dev, &bme680_regmap_bus,
- &spi->dev, &bme680_regmap_config);
+ bus_context, &bme680_regmap_config);
if (IS_ERR(regmap)) {
dev_err(&spi->dev, "Failed to register spi regmap %d\n",
(int)PTR_ERR(regmap));
return PTR_ERR(regmap);
}

- ret = regmap_write(regmap, BME680_REG_SOFT_RESET_SPI,
- BME680_CMD_SOFTRESET);
- if (ret < 0) {
- dev_err(&spi->dev, "Failed to reset chip\n");
- return ret;
- }
-
- /* after power-on reset, Page 0(0x80-0xFF) of spi_mem_page is active */
- ret = regmap_read(regmap, BME680_REG_CHIP_SPI_ID, &val);
- if (ret < 0) {
- dev_err(&spi->dev, "Error reading SPI chip ID\n");
- return ret;
- }
-
- if (val != BME680_CHIP_ID_VAL) {
- dev_err(&spi->dev, "Wrong chip ID, got %x expected %x\n",
- val, BME680_CHIP_ID_VAL);
- return -ENODEV;
- }
- /*
- * select Page 1 of spi_mem_page to enable access to
- * to registers from address 0x00 to 0x7F.
- */
- ret = regmap_write_bits(regmap, BME680_REG_STATUS,
- BME680_SPI_MEM_PAGE_BIT,
- BME680_SPI_MEM_PAGE_1_VAL);
- if (ret < 0) {
- dev_err(&spi->dev, "failed to set page 1 of spi_mem_page\n");
- return ret;
- }
-
return bme680_core_probe(&spi->dev, regmap, id->name);
}

--
1.9.1

2019-03-03 16:59:12

by Jonathan Cameron

Subject: Re: [PATCH v2] iio/chemical/bme680: Fix SPI read interface

On Thu, 21 Feb 2019 10:20:49 +0100
Mike Looijmans <[email protected]> wrote:

> 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.
Hi Mike,

This last bit is an unrelated issue. Would you mind splitting the patch into two?
Please put the temperature one first as that is definitely a stable
worthy patch. The larger one is more debatable as it seems that it
never worked and is a fairly large patch.

I'll probably mark them both for stable, but it is possible not all
the stable branches will pick them both up.

Thanks,

Jonathan

>
> Signed-off-by: Mike Looijmans <[email protected]>
> ---
> v2: Remove unused 'addr7' variable
>
> 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 | 115 +++++++++++++++++++++++++------------
> 4 files changed, 125 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 = &bme680_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 = bme680_read_temp(data, NULL, NULL);
> + ret = bme680_read_temp(data, NULL);
> if (ret < 0)
> return ret;
>
> @@ -761,7 +777,7 @@ static int bme680_read_raw(struct iio_dev *indio_dev,
> case IIO_CHAN_INFO_PROCESSED:
> switch (chan->type) {
> case IIO_TEMP:
> - return bme680_read_temp(data, val, val2);
> + return bme680_read_temp(data, val);
> case IIO_PRESSURE:
> return bme680_read_press(data, val, val2);
> case IIO_HUMIDITYRELATIVE:
> @@ -867,8 +883,28 @@ int bme680_core_probe(struct device *dev, struct regmap *regmap,
> {
> struct iio_dev *indio_dev;
> struct bme680_data *data;
> + unsigned int val;
> int ret;
>
> + ret = regmap_write(regmap, BME680_REG_SOFT_RESET,
> + BME680_CMD_SOFTRESET);
> + if (ret < 0) {
> + dev_err(dev, "Failed to reset chip\n");
> + return ret;
> + }
> +
> + ret = regmap_read(regmap, BME680_REG_CHIP_ID, &val);
> + if (ret < 0) {
> + dev_err(dev, "Error reading chip ID\n");
> + return ret;
> + }
> +
> + if (val != BME680_CHIP_ID_VAL) {
> + dev_err(dev, "Wrong chip ID, got %x expected %x\n",
> + val, BME680_CHIP_ID_VAL);
> + return -ENODEV;
> + }
> +
> indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> if (!indio_dev)
> return -ENOMEM;
> diff --git a/drivers/iio/chemical/bme680_i2c.c b/drivers/iio/chemical/bme680_i2c.c
> index 06d4be5..cfc4449 100644
> --- a/drivers/iio/chemical/bme680_i2c.c
> +++ b/drivers/iio/chemical/bme680_i2c.c
> @@ -23,8 +23,6 @@ static int bme680_i2c_probe(struct i2c_client *client,
> {
> struct regmap *regmap;
> const char *name = NULL;
> - unsigned int val;
> - int ret;
>
> regmap = devm_regmap_init_i2c(client, &bme680_regmap_config);
> if (IS_ERR(regmap)) {
> @@ -33,25 +31,6 @@ static int bme680_i2c_probe(struct i2c_client *client,
> return PTR_ERR(regmap);
> }
>
> - ret = regmap_write(regmap, BME680_REG_SOFT_RESET_I2C,
> - BME680_CMD_SOFTRESET);
> - if (ret < 0) {
> - dev_err(&client->dev, "Failed to reset chip\n");
> - return ret;
> - }
> -
> - ret = regmap_read(regmap, BME680_REG_CHIP_I2C_ID, &val);
> - if (ret < 0) {
> - dev_err(&client->dev, "Error reading I2C chip ID\n");
> - return ret;
> - }
> -
> - if (val != BME680_CHIP_ID_VAL) {
> - dev_err(&client->dev, "Wrong chip ID, got %x expected %x\n",
> - val, BME680_CHIP_ID_VAL);
> - return -ENODEV;
> - }
> -
> if (id)
> name = id->name;
>
> diff --git a/drivers/iio/chemical/bme680_spi.c b/drivers/iio/chemical/bme680_spi.c
> index c9fb05e..881778e 100644
> --- a/drivers/iio/chemical/bme680_spi.c
> +++ b/drivers/iio/chemical/bme680_spi.c
> @@ -11,28 +11,93 @@
>
> #include "bme680.h"
>
> +struct bme680_spi_bus_context {
> + struct spi_device *spi;
> + u8 current_page;
> +};
> +
> +/*
> + * In SPI mode there are only 7 address bits, a "page" register determines
> + * which part of the 8-bit range is active. This function looks at the address
> + * and writes the page selection bit if needed
> + */
> +static int bme680_regmap_spi_select_page(
> + struct bme680_spi_bus_context *ctx, u8 reg)
> +{
> + struct spi_device *spi = ctx->spi;
> + int ret;
> + u8 buf[2];
> + u8 page = (reg & 0x80) ? 0 : 1; /* Page "1" is low range */
> +
> + if (page == ctx->current_page)
> + return 0;
> +
> + /*
> + * Data sheet claims we're only allowed to change bit 4, so we must do
> + * a read-modify-write on each and every page select
> + */
> + buf[0] = BME680_REG_STATUS;
> + ret = spi_write_then_read(spi, buf, 1, buf + 1, 1);
> + if (ret < 0) {
> + dev_err(&spi->dev, "failed to set page %u\n", page);
> + return ret;
> + }
> +
> + buf[0] = BME680_REG_STATUS;
> + if (page)
> + buf[1] |= BME680_SPI_MEM_PAGE_BIT;
> + else
> + buf[1] &= ~BME680_SPI_MEM_PAGE_BIT;
> +
> + ret = spi_write(spi, buf, 2);
> + if (ret < 0) {
> + dev_err(&spi->dev, "failed to set page %u\n", page);
> + return ret;
> + }
> +
> + ctx->current_page = page;
> +
> + return 0;
> +}
> +
> static int bme680_regmap_spi_write(void *context, const void *data,
> size_t count)
> {
> - struct spi_device *spi = context;
> + struct bme680_spi_bus_context *ctx = context;
> + struct spi_device *spi = ctx->spi;
> + int ret;
> u8 buf[2];
>
> memcpy(buf, data, 2);
> +
> + ret = bme680_regmap_spi_select_page(ctx, buf[0]);
> + if (ret)
> + return ret;
> +
> /*
> * The SPI register address (= full register address without bit 7)
> * and the write command (bit7 = RW = '0')
> */
> buf[0] &= ~0x80;
>
> - return spi_write_then_read(spi, buf, 2, NULL, 0);
> + return spi_write(spi, buf, 2);
> }
>
> static int bme680_regmap_spi_read(void *context, const void *reg,
> size_t reg_size, void *val, size_t val_size)
> {
> - struct spi_device *spi = context;
> + struct bme680_spi_bus_context *ctx = context;
> + struct spi_device *spi = ctx->spi;
> + int ret;
> + u8 addr = *(const u8 *)reg;
> +
> + ret = bme680_regmap_spi_select_page(ctx, addr);
> + if (ret)
> + return ret;
>
> - return spi_write_then_read(spi, reg, reg_size, val, val_size);
> + addr |= 0x80; /* bit7 = RW = '1' */
> +
> + return spi_write_then_read(spi, &addr, 1, val, val_size);
> }
>
> static struct regmap_bus bme680_regmap_bus = {
> @@ -45,8 +110,8 @@ static int bme680_regmap_spi_read(void *context, const void *reg,
> static int bme680_spi_probe(struct spi_device *spi)
> {
> const struct spi_device_id *id = spi_get_device_id(spi);
> + struct bme680_spi_bus_context *bus_context;
> struct regmap *regmap;
> - unsigned int val;
> int ret;
>
> spi->bits_per_word = 8;
> @@ -56,45 +121,21 @@ static int bme680_spi_probe(struct spi_device *spi)
> return ret;
> }
>
> + bus_context = devm_kzalloc(&spi->dev, sizeof(*bus_context), GFP_KERNEL);
> + if (!bus_context)
> + return -ENOMEM;
> +
> + bus_context->spi = spi;
> + bus_context->current_page = 0xff; /* Undefined on warm boot */
> +
> regmap = devm_regmap_init(&spi->dev, &bme680_regmap_bus,
> - &spi->dev, &bme680_regmap_config);
> + bus_context, &bme680_regmap_config);
> if (IS_ERR(regmap)) {
> dev_err(&spi->dev, "Failed to register spi regmap %d\n",
> (int)PTR_ERR(regmap));
> return PTR_ERR(regmap);
> }
>
> - ret = regmap_write(regmap, BME680_REG_SOFT_RESET_SPI,
> - BME680_CMD_SOFTRESET);
> - if (ret < 0) {
> - dev_err(&spi->dev, "Failed to reset chip\n");
> - return ret;
> - }
> -
> - /* after power-on reset, Page 0(0x80-0xFF) of spi_mem_page is active */
> - ret = regmap_read(regmap, BME680_REG_CHIP_SPI_ID, &val);
> - if (ret < 0) {
> - dev_err(&spi->dev, "Error reading SPI chip ID\n");
> - return ret;
> - }
> -
> - if (val != BME680_CHIP_ID_VAL) {
> - dev_err(&spi->dev, "Wrong chip ID, got %x expected %x\n",
> - val, BME680_CHIP_ID_VAL);
> - return -ENODEV;
> - }
> - /*
> - * select Page 1 of spi_mem_page to enable access to
> - * to registers from address 0x00 to 0x7F.
> - */
> - ret = regmap_write_bits(regmap, BME680_REG_STATUS,
> - BME680_SPI_MEM_PAGE_BIT,
> - BME680_SPI_MEM_PAGE_1_VAL);
> - if (ret < 0) {
> - dev_err(&spi->dev, "failed to set page 1 of spi_mem_page\n");
> - return ret;
> - }
> -
> return bme680_core_probe(&spi->dev, regmap, id->name);
> }
>

2019-03-06 08:16:21

by Mike Looijmans

Subject: [PATCH v3 2/2] 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.

Signed-off-by: Mike Looijmans <[email protected]>
---
v2: Remove unused 'addr7' variable
v3: Split patch into temperature and SPI

drivers/iio/chemical/bme680.h | 6 +-
drivers/iio/chemical/bme680_core.c | 38 ++++++++++++
drivers/iio/chemical/bme680_i2c.c | 21 -------
drivers/iio/chemical/bme680_spi.c | 115 +++++++++++++++++++++++++------------
4 files changed, 118 insertions(+), 62 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 fefe32b..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 = &bme680_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;
@@ -865,8 +883,28 @@ int bme680_core_probe(struct device *dev, struct regmap *regmap,
{
struct iio_dev *indio_dev;
struct bme680_data *data;
+ unsigned int val;
int ret;

+ ret = regmap_write(regmap, BME680_REG_SOFT_RESET,
+ BME680_CMD_SOFTRESET);
+ if (ret < 0) {
+ dev_err(dev, "Failed to reset chip\n");
+ return ret;
+ }
+
+ ret = regmap_read(regmap, BME680_REG_CHIP_ID, &val);
+ if (ret < 0) {
+ dev_err(dev, "Error reading chip ID\n");
+ return ret;
+ }
+
+ if (val != BME680_CHIP_ID_VAL) {
+ dev_err(dev, "Wrong chip ID, got %x expected %x\n",
+ val, BME680_CHIP_ID_VAL);
+ return -ENODEV;
+ }
+
indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
if (!indio_dev)
return -ENOMEM;
diff --git a/drivers/iio/chemical/bme680_i2c.c b/drivers/iio/chemical/bme680_i2c.c
index 06d4be5..cfc4449 100644
--- a/drivers/iio/chemical/bme680_i2c.c
+++ b/drivers/iio/chemical/bme680_i2c.c
@@ -23,8 +23,6 @@ static int bme680_i2c_probe(struct i2c_client *client,
{
struct regmap *regmap;
const char *name = NULL;
- unsigned int val;
- int ret;

regmap = devm_regmap_init_i2c(client, &bme680_regmap_config);
if (IS_ERR(regmap)) {
@@ -33,25 +31,6 @@ static int bme680_i2c_probe(struct i2c_client *client,
return PTR_ERR(regmap);
}

- ret = regmap_write(regmap, BME680_REG_SOFT_RESET_I2C,
- BME680_CMD_SOFTRESET);
- if (ret < 0) {
- dev_err(&client->dev, "Failed to reset chip\n");
- return ret;
- }
-
- ret = regmap_read(regmap, BME680_REG_CHIP_I2C_ID, &val);
- if (ret < 0) {
- dev_err(&client->dev, "Error reading I2C chip ID\n");
- return ret;
- }
-
- if (val != BME680_CHIP_ID_VAL) {
- dev_err(&client->dev, "Wrong chip ID, got %x expected %x\n",
- val, BME680_CHIP_ID_VAL);
- return -ENODEV;
- }
-
if (id)
name = id->name;

diff --git a/drivers/iio/chemical/bme680_spi.c b/drivers/iio/chemical/bme680_spi.c
index c9fb05e..881778e 100644
--- a/drivers/iio/chemical/bme680_spi.c
+++ b/drivers/iio/chemical/bme680_spi.c
@@ -11,28 +11,93 @@

#include "bme680.h"

+struct bme680_spi_bus_context {
+ struct spi_device *spi;
+ u8 current_page;
+};
+
+/*
+ * In SPI mode there are only 7 address bits, a "page" register determines
+ * which part of the 8-bit range is active. This function looks at the address
+ * and writes the page selection bit if needed
+ */
+static int bme680_regmap_spi_select_page(
+ struct bme680_spi_bus_context *ctx, u8 reg)
+{
+ struct spi_device *spi = ctx->spi;
+ int ret;
+ u8 buf[2];
+ u8 page = (reg & 0x80) ? 0 : 1; /* Page "1" is low range */
+
+ if (page == ctx->current_page)
+ return 0;
+
+ /*
+ * Data sheet claims we're only allowed to change bit 4, so we must do
+ * a read-modify-write on each and every page select
+ */
+ buf[0] = BME680_REG_STATUS;
+ ret = spi_write_then_read(spi, buf, 1, buf + 1, 1);
+ if (ret < 0) {
+ dev_err(&spi->dev, "failed to set page %u\n", page);
+ return ret;
+ }
+
+ buf[0] = BME680_REG_STATUS;
+ if (page)
+ buf[1] |= BME680_SPI_MEM_PAGE_BIT;
+ else
+ buf[1] &= ~BME680_SPI_MEM_PAGE_BIT;
+
+ ret = spi_write(spi, buf, 2);
+ if (ret < 0) {
+ dev_err(&spi->dev, "failed to set page %u\n", page);
+ return ret;
+ }
+
+ ctx->current_page = page;
+
+ return 0;
+}
+
static int bme680_regmap_spi_write(void *context, const void *data,
size_t count)
{
- struct spi_device *spi = context;
+ struct bme680_spi_bus_context *ctx = context;
+ struct spi_device *spi = ctx->spi;
+ int ret;
u8 buf[2];

memcpy(buf, data, 2);
+
+ ret = bme680_regmap_spi_select_page(ctx, buf[0]);
+ if (ret)
+ return ret;
+
/*
* The SPI register address (= full register address without bit 7)
* and the write command (bit7 = RW = '0')
*/
buf[0] &= ~0x80;

- return spi_write_then_read(spi, buf, 2, NULL, 0);
+ return spi_write(spi, buf, 2);
}

static int bme680_regmap_spi_read(void *context, const void *reg,
size_t reg_size, void *val, size_t val_size)
{
- struct spi_device *spi = context;
+ struct bme680_spi_bus_context *ctx = context;
+ struct spi_device *spi = ctx->spi;
+ int ret;
+ u8 addr = *(const u8 *)reg;
+
+ ret = bme680_regmap_spi_select_page(ctx, addr);
+ if (ret)
+ return ret;

- return spi_write_then_read(spi, reg, reg_size, val, val_size);
+ addr |= 0x80; /* bit7 = RW = '1' */
+
+ return spi_write_then_read(spi, &addr, 1, val, val_size);
}

static struct regmap_bus bme680_regmap_bus = {
@@ -45,8 +110,8 @@ static int bme680_regmap_spi_read(void *context, const void *reg,
static int bme680_spi_probe(struct spi_device *spi)
{
const struct spi_device_id *id = spi_get_device_id(spi);
+ struct bme680_spi_bus_context *bus_context;
struct regmap *regmap;
- unsigned int val;
int ret;

spi->bits_per_word = 8;
@@ -56,45 +121,21 @@ static int bme680_spi_probe(struct spi_device *spi)
return ret;
}

+ bus_context = devm_kzalloc(&spi->dev, sizeof(*bus_context), GFP_KERNEL);
+ if (!bus_context)
+ return -ENOMEM;
+
+ bus_context->spi = spi;
+ bus_context->current_page = 0xff; /* Undefined on warm boot */
+
regmap = devm_regmap_init(&spi->dev, &bme680_regmap_bus,
- &spi->dev, &bme680_regmap_config);
+ bus_context, &bme680_regmap_config);
if (IS_ERR(regmap)) {
dev_err(&spi->dev, "Failed to register spi regmap %d\n",
(int)PTR_ERR(regmap));
return PTR_ERR(regmap);
}

- ret = regmap_write(regmap, BME680_REG_SOFT_RESET_SPI,
- BME680_CMD_SOFTRESET);
- if (ret < 0) {
- dev_err(&spi->dev, "Failed to reset chip\n");
- return ret;
- }
-
- /* after power-on reset, Page 0(0x80-0xFF) of spi_mem_page is active */
- ret = regmap_read(regmap, BME680_REG_CHIP_SPI_ID, &val);
- if (ret < 0) {
- dev_err(&spi->dev, "Error reading SPI chip ID\n");
- return ret;
- }
-
- if (val != BME680_CHIP_ID_VAL) {
- dev_err(&spi->dev, "Wrong chip ID, got %x expected %x\n",
- val, BME680_CHIP_ID_VAL);
- return -ENODEV;
- }
- /*
- * select Page 1 of spi_mem_page to enable access to
- * to registers from address 0x00 to 0x7F.
- */
- ret = regmap_write_bits(regmap, BME680_REG_STATUS,
- BME680_SPI_MEM_PAGE_BIT,
- BME680_SPI_MEM_PAGE_1_VAL);
- if (ret < 0) {
- dev_err(&spi->dev, "failed to set page 1 of spi_mem_page\n");
- return ret;
- }
-
return bme680_core_probe(&spi->dev, regmap, id->name);
}

--
1.9.1

2019-03-06 08:16:37

by Mike Looijmans

Subject: Re: [PATCH v2] iio/chemical/bme680: Fix SPI read interface

On 03-03-19 17:57, Jonathan Cameron wrote:
> On Thu, 21 Feb 2019 10:20:49 +0100
> Mike Looijmans <[email protected]> wrote:
>
>> 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.
> Hi Mike,
>
> This last bit is an unrelated issue. Would you mind splitting the patch into two?
> Please put the temperature one first as that is definitely a stable
> worthy patch. The larger one is more debatable as it seems that it
> never worked and is a fairly large patch.
>
> I'll probably mark them both for stable, but it is possible not all
> the stable branches will pick them both up.

Splitting the patch was easy enough, the're independent, I'll post a v3 with
both patches.


>
> Thanks,
>
> Jonathan
>
>>
>> Signed-off-by: Mike Looijmans <[email protected]>
>> ---
>> v2: Remove unused 'addr7' variable
>>
>> 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 | 115 +++++++++++++++++++++++++------------
>> 4 files changed, 125 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 = &bme680_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 = bme680_read_temp(data, NULL, NULL);
>> + ret = bme680_read_temp(data, NULL);
>> if (ret < 0)
>> return ret;
>>
>> @@ -761,7 +777,7 @@ static int bme680_read_raw(struct iio_dev *indio_dev,
>> case IIO_CHAN_INFO_PROCESSED:
>> switch (chan->type) {
>> case IIO_TEMP:
>> - return bme680_read_temp(data, val, val2);
>> + return bme680_read_temp(data, val);
>> case IIO_PRESSURE:
>> return bme680_read_press(data, val, val2);
>> case IIO_HUMIDITYRELATIVE:
>> @@ -867,8 +883,28 @@ int bme680_core_probe(struct device *dev, struct regmap *regmap,
>> {
>> struct iio_dev *indio_dev;
>> struct bme680_data *data;
>> + unsigned int val;
>> int ret;
>>
>> + ret = regmap_write(regmap, BME680_REG_SOFT_RESET,
>> + BME680_CMD_SOFTRESET);
>> + if (ret < 0) {
>> + dev_err(dev, "Failed to reset chip\n");
>> + return ret;
>> + }
>> +
>> + ret = regmap_read(regmap, BME680_REG_CHIP_ID, &val);
>> + if (ret < 0) {
>> + dev_err(dev, "Error reading chip ID\n");
>> + return ret;
>> + }
>> +
>> + if (val != BME680_CHIP_ID_VAL) {
>> + dev_err(dev, "Wrong chip ID, got %x expected %x\n",
>> + val, BME680_CHIP_ID_VAL);
>> + return -ENODEV;
>> + }
>> +
>> indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
>> if (!indio_dev)
>> return -ENOMEM;
>> diff --git a/drivers/iio/chemical/bme680_i2c.c b/drivers/iio/chemical/bme680_i2c.c
>> index 06d4be5..cfc4449 100644
>> --- a/drivers/iio/chemical/bme680_i2c.c
>> +++ b/drivers/iio/chemical/bme680_i2c.c
>> @@ -23,8 +23,6 @@ static int bme680_i2c_probe(struct i2c_client *client,
>> {
>> struct regmap *regmap;
>> const char *name = NULL;
>> - unsigned int val;
>> - int ret;
>>
>> regmap = devm_regmap_init_i2c(client, &bme680_regmap_config);
>> if (IS_ERR(regmap)) {
>> @@ -33,25 +31,6 @@ static int bme680_i2c_probe(struct i2c_client *client,
>> return PTR_ERR(regmap);
>> }
>>
>> - ret = regmap_write(regmap, BME680_REG_SOFT_RESET_I2C,
>> - BME680_CMD_SOFTRESET);
>> - if (ret < 0) {
>> - dev_err(&client->dev, "Failed to reset chip\n");
>> - return ret;
>> - }
>> -
>> - ret = regmap_read(regmap, BME680_REG_CHIP_I2C_ID, &val);
>> - if (ret < 0) {
>> - dev_err(&client->dev, "Error reading I2C chip ID\n");
>> - return ret;
>> - }
>> -
>> - if (val != BME680_CHIP_ID_VAL) {
>> - dev_err(&client->dev, "Wrong chip ID, got %x expected %x\n",
>> - val, BME680_CHIP_ID_VAL);
>> - return -ENODEV;
>> - }
>> -
>> if (id)
>> name = id->name;
>>
>> diff --git a/drivers/iio/chemical/bme680_spi.c b/drivers/iio/chemical/bme680_spi.c
>> index c9fb05e..881778e 100644
>> --- a/drivers/iio/chemical/bme680_spi.c
>> +++ b/drivers/iio/chemical/bme680_spi.c
>> @@ -11,28 +11,93 @@
>>
>> #include "bme680.h"
>>
>> +struct bme680_spi_bus_context {
>> + struct spi_device *spi;
>> + u8 current_page;
>> +};
>> +
>> +/*
>> + * In SPI mode there are only 7 address bits, a "page" register determines
>> + * which part of the 8-bit range is active. This function looks at the address
>> + * and writes the page selection bit if needed
>> + */
>> +static int bme680_regmap_spi_select_page(
>> + struct bme680_spi_bus_context *ctx, u8 reg)
>> +{
>> + struct spi_device *spi = ctx->spi;
>> + int ret;
>> + u8 buf[2];
>> + u8 page = (reg & 0x80) ? 0 : 1; /* Page "1" is low range */
>> +
>> + if (page == ctx->current_page)
>> + return 0;
>> +
>> + /*
>> + * Data sheet claims we're only allowed to change bit 4, so we must do
>> + * a read-modify-write on each and every page select
>> + */
>> + buf[0] = BME680_REG_STATUS;
>> + ret = spi_write_then_read(spi, buf, 1, buf + 1, 1);
>> + if (ret < 0) {
>> + dev_err(&spi->dev, "failed to set page %u\n", page);
>> + return ret;
>> + }
>> +
>> + buf[0] = BME680_REG_STATUS;
>> + if (page)
>> + buf[1] |= BME680_SPI_MEM_PAGE_BIT;
>> + else
>> + buf[1] &= ~BME680_SPI_MEM_PAGE_BIT;
>> +
>> + ret = spi_write(spi, buf, 2);
>> + if (ret < 0) {
>> + dev_err(&spi->dev, "failed to set page %u\n", page);
>> + return ret;
>> + }
>> +
>> + ctx->current_page = page;
>> +
>> + return 0;
>> +}
>> +
>> static int bme680_regmap_spi_write(void *context, const void *data,
>> size_t count)
>> {
>> - struct spi_device *spi = context;
>> + struct bme680_spi_bus_context *ctx = context;
>> + struct spi_device *spi = ctx->spi;
>> + int ret;
>> u8 buf[2];
>>
>> memcpy(buf, data, 2);
>> +
>> + ret = bme680_regmap_spi_select_page(ctx, buf[0]);
>> + if (ret)
>> + return ret;
>> +
>> /*
>> * The SPI register address (= full register address without bit 7)
>> * and the write command (bit7 = RW = '0')
>> */
>> buf[0] &= ~0x80;
>>
>> - return spi_write_then_read(spi, buf, 2, NULL, 0);
>> + return spi_write(spi, buf, 2);
>> }
>>
>> static int bme680_regmap_spi_read(void *context, const void *reg,
>> size_t reg_size, void *val, size_t val_size)
>> {
>> - struct spi_device *spi = context;
>> + struct bme680_spi_bus_context *ctx = context;
>> + struct spi_device *spi = ctx->spi;
>> + int ret;
>> + u8 addr = *(const u8 *)reg;
>> +
>> + ret = bme680_regmap_spi_select_page(ctx, addr);
>> + if (ret)
>> + return ret;
>>
>> - return spi_write_then_read(spi, reg, reg_size, val, val_size);
>> + addr |= 0x80; /* bit7 = RW = '1' */
>> +
>> + return spi_write_then_read(spi, &addr, 1, val, val_size);
>> }
>>
>> static struct regmap_bus bme680_regmap_bus = {
>> @@ -45,8 +110,8 @@ static int bme680_regmap_spi_read(void *context, const void *reg,
>> static int bme680_spi_probe(struct spi_device *spi)
>> {
>> const struct spi_device_id *id = spi_get_device_id(spi);
>> + struct bme680_spi_bus_context *bus_context;
>> struct regmap *regmap;
>> - unsigned int val;
>> int ret;
>>
>> spi->bits_per_word = 8;
>> @@ -56,45 +121,21 @@ static int bme680_spi_probe(struct spi_device *spi)
>> return ret;
>> }
>>
>> + bus_context = devm_kzalloc(&spi->dev, sizeof(*bus_context), GFP_KERNEL);
>> + if (!bus_context)
>> + return -ENOMEM;
>> +
>> + bus_context->spi = spi;
>> + bus_context->current_page = 0xff; /* Undefined on warm boot */
>> +
>> regmap = devm_regmap_init(&spi->dev, &bme680_regmap_bus,
>> - &spi->dev, &bme680_regmap_config);
>> + bus_context, &bme680_regmap_config);
>> if (IS_ERR(regmap)) {
>> dev_err(&spi->dev, "Failed to register spi regmap %d\n",
>> (int)PTR_ERR(regmap));
>> return PTR_ERR(regmap);
>> }
>>
>> - ret = regmap_write(regmap, BME680_REG_SOFT_RESET_SPI,
>> - BME680_CMD_SOFTRESET);
>> - if (ret < 0) {
>> - dev_err(&spi->dev, "Failed to reset chip\n");
>> - return ret;
>> - }
>> -
>> - /* after power-on reset, Page 0(0x80-0xFF) of spi_mem_page is active */
>> - ret = regmap_read(regmap, BME680_REG_CHIP_SPI_ID, &val);
>> - if (ret < 0) {
>> - dev_err(&spi->dev, "Error reading SPI chip ID\n");
>> - return ret;
>> - }
>> -
>> - if (val != BME680_CHIP_ID_VAL) {
>> - dev_err(&spi->dev, "Wrong chip ID, got %x expected %x\n",
>> - val, BME680_CHIP_ID_VAL);
>> - return -ENODEV;
>> - }
>> - /*
>> - * select Page 1 of spi_mem_page to enable access to
>> - * to registers from address 0x00 to 0x7F.
>> - */
>> - ret = regmap_write_bits(regmap, BME680_REG_STATUS,
>> - BME680_SPI_MEM_PAGE_BIT,
>> - BME680_SPI_MEM_PAGE_1_VAL);
>> - if (ret < 0) {
>> - dev_err(&spi->dev, "failed to set page 1 of spi_mem_page\n");
>> - return ret;
>> - }
>> -
>> return bme680_core_probe(&spi->dev, regmap, id->name);
>> }
>>
>

2019-03-06 08:16:57

by Mike Looijmans

Subject: [PATCH v3 1/2] iio/chemical/bme680: Report temperature in millidegrees

The standard unit for temperature is millidegrees Celcius. Adapt the
driver to report in millidegrees instead of degrees.

Signed-off-by: Mike Looijmans <[email protected]>
---
v2: Remove unused 'addr7' variable
v3: Split patch into temperature and SPI

drivers/iio/chemical/bme680_core.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
index 70c1fe4..fefe32b 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -583,8 +583,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 +616,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 +633,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 +666,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 = bme680_read_temp(data, NULL, NULL);
+ ret = bme680_read_temp(data, NULL);
if (ret < 0)
return ret;

@@ -761,7 +759,7 @@ static int bme680_read_raw(struct iio_dev *indio_dev,
case IIO_CHAN_INFO_PROCESSED:
switch (chan->type) {
case IIO_TEMP:
- return bme680_read_temp(data, val, val2);
+ return bme680_read_temp(data, val);
case IIO_PRESSURE:
return bme680_read_press(data, val, val2);
case IIO_HUMIDITYRELATIVE:
--
1.9.1

2019-03-09 17:06:51

by Jonathan Cameron

Subject: Re: [PATCH v3 1/2] iio/chemical/bme680: Report temperature in millidegrees

On Wed, 6 Mar 2019 08:31:47 +0100
Mike Looijmans <[email protected]> wrote:

> The standard unit for temperature is millidegrees Celcius. Adapt the
> driver to report in millidegrees instead of degrees.
>
> Signed-off-by: Mike Looijmans <[email protected]>
I tweaked the patch title to make it obvious this was a fix
(added the word fix ;)

Applied to the fixes-togreg branch of iio.git and marked for stable.

Thanks,

Jonathan

> ---
> v2: Remove unused 'addr7' variable
> v3: Split patch into temperature and SPI
>
> drivers/iio/chemical/bme680_core.c | 16 +++++++---------
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
> index 70c1fe4..fefe32b 100644
> --- a/drivers/iio/chemical/bme680_core.c
> +++ b/drivers/iio/chemical/bme680_core.c
> @@ -583,8 +583,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 +616,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 +633,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 +666,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 = bme680_read_temp(data, NULL, NULL);
> + ret = bme680_read_temp(data, NULL);
> if (ret < 0)
> return ret;
>
> @@ -761,7 +759,7 @@ static int bme680_read_raw(struct iio_dev *indio_dev,
> case IIO_CHAN_INFO_PROCESSED:
> switch (chan->type) {
> case IIO_TEMP:
> - return bme680_read_temp(data, val, val2);
> + return bme680_read_temp(data, val);
> case IIO_PRESSURE:
> return bme680_read_press(data, val, val2);
> case IIO_HUMIDITYRELATIVE:

2019-03-09 17:29:48

by Jonathan Cameron

Subject: Re: [PATCH v3 2/2] iio/chemical/bme680: Fix SPI read interface

On Wed, 6 Mar 2019 08:31:48 +0100
Mike Looijmans <[email protected]> wrote:

> 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.
>
> Signed-off-by: Mike Looijmans <[email protected]>
Applied to the fixes-togreg branch of iio.git and marked for
stable with a fixes tag added for the original patch introducing
the driver. It might not apply cleanly all that far back,
but at least this makes people aware this isn't a new problem.

Thanks,

Jonathan

> ---
> v2: Remove unused 'addr7' variable
> v3: Split patch into temperature and SPI
>
> drivers/iio/chemical/bme680.h | 6 +-
> drivers/iio/chemical/bme680_core.c | 38 ++++++++++++
> drivers/iio/chemical/bme680_i2c.c | 21 -------
> drivers/iio/chemical/bme680_spi.c | 115 +++++++++++++++++++++++++------------
> 4 files changed, 118 insertions(+), 62 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 fefe32b..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 = &bme680_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;
> @@ -865,8 +883,28 @@ int bme680_core_probe(struct device *dev, struct regmap *regmap,
> {
> struct iio_dev *indio_dev;
> struct bme680_data *data;
> + unsigned int val;
> int ret;
>
> + ret = regmap_write(regmap, BME680_REG_SOFT_RESET,
> + BME680_CMD_SOFTRESET);
> + if (ret < 0) {
> + dev_err(dev, "Failed to reset chip\n");
> + return ret;
> + }
> +
> + ret = regmap_read(regmap, BME680_REG_CHIP_ID, &val);
> + if (ret < 0) {
> + dev_err(dev, "Error reading chip ID\n");
> + return ret;
> + }
> +
> + if (val != BME680_CHIP_ID_VAL) {
> + dev_err(dev, "Wrong chip ID, got %x expected %x\n",
> + val, BME680_CHIP_ID_VAL);
> + return -ENODEV;
> + }
> +
> indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> if (!indio_dev)
> return -ENOMEM;
> diff --git a/drivers/iio/chemical/bme680_i2c.c b/drivers/iio/chemical/bme680_i2c.c
> index 06d4be5..cfc4449 100644
> --- a/drivers/iio/chemical/bme680_i2c.c
> +++ b/drivers/iio/chemical/bme680_i2c.c
> @@ -23,8 +23,6 @@ static int bme680_i2c_probe(struct i2c_client *client,
> {
> struct regmap *regmap;
> const char *name = NULL;
> - unsigned int val;
> - int ret;
>
> regmap = devm_regmap_init_i2c(client, &bme680_regmap_config);
> if (IS_ERR(regmap)) {
> @@ -33,25 +31,6 @@ static int bme680_i2c_probe(struct i2c_client *client,
> return PTR_ERR(regmap);
> }
>
> - ret = regmap_write(regmap, BME680_REG_SOFT_RESET_I2C,
> - BME680_CMD_SOFTRESET);
> - if (ret < 0) {
> - dev_err(&client->dev, "Failed to reset chip\n");
> - return ret;
> - }
> -
> - ret = regmap_read(regmap, BME680_REG_CHIP_I2C_ID, &val);
> - if (ret < 0) {
> - dev_err(&client->dev, "Error reading I2C chip ID\n");
> - return ret;
> - }
> -
> - if (val != BME680_CHIP_ID_VAL) {
> - dev_err(&client->dev, "Wrong chip ID, got %x expected %x\n",
> - val, BME680_CHIP_ID_VAL);
> - return -ENODEV;
> - }
> -
> if (id)
> name = id->name;
>
> diff --git a/drivers/iio/chemical/bme680_spi.c b/drivers/iio/chemical/bme680_spi.c
> index c9fb05e..881778e 100644
> --- a/drivers/iio/chemical/bme680_spi.c
> +++ b/drivers/iio/chemical/bme680_spi.c
> @@ -11,28 +11,93 @@
>
> #include "bme680.h"
>
> +struct bme680_spi_bus_context {
> + struct spi_device *spi;
> + u8 current_page;
> +};
> +
> +/*
> + * In SPI mode there are only 7 address bits, a "page" register determines
> + * which part of the 8-bit range is active. This function looks at the address
> + * and writes the page selection bit if needed
> + */
> +static int bme680_regmap_spi_select_page(
> + struct bme680_spi_bus_context *ctx, u8 reg)
> +{
> + struct spi_device *spi = ctx->spi;
> + int ret;
> + u8 buf[2];
> + u8 page = (reg & 0x80) ? 0 : 1; /* Page "1" is low range */
> +
> + if (page == ctx->current_page)
> + return 0;
> +
> + /*
> + * Data sheet claims we're only allowed to change bit 4, so we must do
> + * a read-modify-write on each and every page select
> + */
> + buf[0] = BME680_REG_STATUS;
> + ret = spi_write_then_read(spi, buf, 1, buf + 1, 1);
> + if (ret < 0) {
> + dev_err(&spi->dev, "failed to set page %u\n", page);
> + return ret;
> + }
> +
> + buf[0] = BME680_REG_STATUS;
> + if (page)
> + buf[1] |= BME680_SPI_MEM_PAGE_BIT;
> + else
> + buf[1] &= ~BME680_SPI_MEM_PAGE_BIT;
> +
> + ret = spi_write(spi, buf, 2);
> + if (ret < 0) {
> + dev_err(&spi->dev, "failed to set page %u\n", page);
> + return ret;
> + }
> +
> + ctx->current_page = page;
> +
> + return 0;
> +}
> +
> static int bme680_regmap_spi_write(void *context, const void *data,
> size_t count)
> {
> - struct spi_device *spi = context;
> + struct bme680_spi_bus_context *ctx = context;
> + struct spi_device *spi = ctx->spi;
> + int ret;
> u8 buf[2];
>
> memcpy(buf, data, 2);
> +
> + ret = bme680_regmap_spi_select_page(ctx, buf[0]);
> + if (ret)
> + return ret;
> +
> /*
> * The SPI register address (= full register address without bit 7)
> * and the write command (bit7 = RW = '0')
> */
> buf[0] &= ~0x80;
>
> - return spi_write_then_read(spi, buf, 2, NULL, 0);
> + return spi_write(spi, buf, 2);
> }
>
> static int bme680_regmap_spi_read(void *context, const void *reg,
> size_t reg_size, void *val, size_t val_size)
> {
> - struct spi_device *spi = context;
> + struct bme680_spi_bus_context *ctx = context;
> + struct spi_device *spi = ctx->spi;
> + int ret;
> + u8 addr = *(const u8 *)reg;
> +
> + ret = bme680_regmap_spi_select_page(ctx, addr);
> + if (ret)
> + return ret;
>
> - return spi_write_then_read(spi, reg, reg_size, val, val_size);
> + addr |= 0x80; /* bit7 = RW = '1' */
> +
> + return spi_write_then_read(spi, &addr, 1, val, val_size);
> }
>
> static struct regmap_bus bme680_regmap_bus = {
> @@ -45,8 +110,8 @@ static int bme680_regmap_spi_read(void *context, const void *reg,
> static int bme680_spi_probe(struct spi_device *spi)
> {
> const struct spi_device_id *id = spi_get_device_id(spi);
> + struct bme680_spi_bus_context *bus_context;
> struct regmap *regmap;
> - unsigned int val;
> int ret;
>
> spi->bits_per_word = 8;
> @@ -56,45 +121,21 @@ static int bme680_spi_probe(struct spi_device *spi)
> return ret;
> }
>
> + bus_context = devm_kzalloc(&spi->dev, sizeof(*bus_context), GFP_KERNEL);
> + if (!bus_context)
> + return -ENOMEM;
> +
> + bus_context->spi = spi;
> + bus_context->current_page = 0xff; /* Undefined on warm boot */
> +
> regmap = devm_regmap_init(&spi->dev, &bme680_regmap_bus,
> - &spi->dev, &bme680_regmap_config);
> + bus_context, &bme680_regmap_config);
> if (IS_ERR(regmap)) {
> dev_err(&spi->dev, "Failed to register spi regmap %d\n",
> (int)PTR_ERR(regmap));
> return PTR_ERR(regmap);
> }
>
> - ret = regmap_write(regmap, BME680_REG_SOFT_RESET_SPI,
> - BME680_CMD_SOFTRESET);
> - if (ret < 0) {
> - dev_err(&spi->dev, "Failed to reset chip\n");
> - return ret;
> - }
> -
> - /* after power-on reset, Page 0(0x80-0xFF) of spi_mem_page is active */
> - ret = regmap_read(regmap, BME680_REG_CHIP_SPI_ID, &val);
> - if (ret < 0) {
> - dev_err(&spi->dev, "Error reading SPI chip ID\n");
> - return ret;
> - }
> -
> - if (val != BME680_CHIP_ID_VAL) {
> - dev_err(&spi->dev, "Wrong chip ID, got %x expected %x\n",
> - val, BME680_CHIP_ID_VAL);
> - return -ENODEV;
> - }
> - /*
> - * select Page 1 of spi_mem_page to enable access to
> - * to registers from address 0x00 to 0x7F.
> - */
> - ret = regmap_write_bits(regmap, BME680_REG_STATUS,
> - BME680_SPI_MEM_PAGE_BIT,
> - BME680_SPI_MEM_PAGE_1_VAL);
> - if (ret < 0) {
> - dev_err(&spi->dev, "failed to set page 1 of spi_mem_page\n");
> - return ret;
> - }
> -
> return bme680_core_probe(&spi->dev, regmap, id->name);
> }
>

2019-03-16 10:25:28

by Himanshu Jha

Subject: Re: [PATCH v3 2/2] iio/chemical/bme680: Fix SPI read interface

On Wed, Mar 06, 2019 at 08:31:48AM +0100, Mike Looijmans wrote:
> 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.
>
> Signed-off-by: Mike Looijmans <[email protected]>
> ---

I have been trying to test this patch in the past week and still it
failed everytime.

First I used ACPI to enumerate the device in QEMU setup:

Added some printks for debugging:

[ 14.510198] bme680_spi spi-BME0680:00: Jumping to core driver now ...
[ 14.544528] bme680_spi spi-BME0680:00: Page setting done, on Page :0 now
[ 14.554363] bme680_spi spi-BME0680:00: bme680_regmap_spi_write: on Page :0 now
[ 14.556151] bme680_spi spi-BME0680:00: bme680_regmap_spi_read: on Page :0 now
[ 14.567815] bme680_spi spi-BME0680:00: Wrong chip ID, got ff expected 61

I also tried bypassing this by removing the following snippet and force
registration to see what happens next:

> + ret = regmap_write(regmap, BME680_REG_SOFT_RESET,
> + BME680_CMD_SOFTRESET);
> + if (ret < 0) {
> + dev_err(dev, "Failed to reset chip\n");
> + return ret;
> + }
> +
> + ret = regmap_read(regmap, BME680_REG_CHIP_ID, &val);
> + if (ret < 0) {
> + dev_err(dev, "Error reading chip ID\n");
> + return ret;
> + }
> +
> + if (val != BME680_CHIP_ID_VAL) {
> + dev_err(dev, "Wrong chip ID, got %x expected %x\n",
> + val, BME680_CHIP_ID_VAL);
> + return -ENODEV;
> + }
> +

And it registered successfully, but all the bme680 attributes were
giving wrong values like temp was constant to 0.0000007, resistance
was resource busy due to insuffient target temperature error.
Pretty eveything was messed up at this stage.

Then I build and booted the kernel on BeagleBone Black Wireless with
DT matching this time:

debian@beaglebone:~$ uname -a
Linux beaglebone 4.19.5-ti-r5 #1xross SMP PREEMPT Sat Mar 16 12:11:50 IST 2019 armv7l GNU/Linux
debian@beaglebone:~$ dmesg | grep 'bme680'
[ 30.269207] bme680_spi spi0.0: Wrong chip ID, got ff expected 61
[ 361.867410] bme680_core: disagrees about version of symbol module_layout
debian@beaglebone:~$ lsmod | grep 'bme'
bme680_spi 16384 0
bme680_core 20480 1 bme680_spi
debian@beaglebone:~$ cat /sys/bus/spi/devices/spi0.0/
modalias of_node/ power/ statistics/ subsystem/ uevent
debian@beaglebone:~$ cat /sys/bus/spi/devices/spi0.0/modalias
spi:bme680
debian@beaglebone:~$ cat /sys/bus/spi/devices/spi0.0/of_node/
compatible name reg spi-max-frequency
debian@beaglebone:~$ cat /sys/bus/spi/devices/spi0.0/of_node/compatible
bme680
debian@beaglebone:~$ cat /sys/bus/spi/devices/spi0.0/of_node/name
bme680
debian@beaglebone:~$ dtc -f -I fs /proc/device-tree | grep -A3 'bme680'
bme680@0 {
compatible = "bme680";
reg = <0x0>;
spi-max-frequency = <0x989680>;
};

Same error again!

I really don't know where the problem is and how to rectify ?

OTOH, I2C works like a charm as it used to work before:

root@beaglebone:/sys/bus/iio/devices/iio:device1# cat name in_temp_input \
in_pressure_input in_humidityrelative_input in_resistance_input

bme680
26860 --> w/o your patch it used to be 26.86000 degC
990.870000000
55.265000000
10091


I'm still assuming that there is some problem on my side, as it works
flawless for you. But it is really difficult for me to figure out
exactly where the problem could be!


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

2019-03-16 13:06:08

by Mike Looijmans

Subject: Re: [PATCH v3 2/2] iio/chemical/bme680: Fix SPI read interface

On 16-03-19 11:24, Himanshu Jha wrote:
> On Wed, Mar 06, 2019 at 08:31:48AM +0100, Mike Looijmans wrote:
>> 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.
>>
>> Signed-off-by: Mike Looijmans <[email protected]>
>> ---
>
> I have been trying to test this patch in the past week and still it
> failed everytime.
>
> First I used ACPI to enumerate the device in QEMU setup:
>
> Added some printks for debugging:
>
> [ 14.510198] bme680_spi spi-BME0680:00: Jumping to core driver now ...
> [ 14.544528] bme680_spi spi-BME0680:00: Page setting done, on Page :0 now
> [ 14.554363] bme680_spi spi-BME0680:00: bme680_regmap_spi_write: on Page :0 now
> [ 14.556151] bme680_spi spi-BME0680:00: bme680_regmap_spi_read: on Page :0 now
> [ 14.567815] bme680_spi spi-BME0680:00: Wrong chip ID, got ff expected 61
>

Looks like the SPI communication isn't working. At this point I'd check
the wires using an osciloscope or analyzer or something.


> I also tried bypassing this by removing the following snippet and force
> registration to see what happens next:
>
> > + ret = regmap_write(regmap, BME680_REG_SOFT_RESET,
> > + BME680_CMD_SOFTRESET);
> > + if (ret < 0) {
> > + dev_err(dev, "Failed to reset chip\n");
> > + return ret;
> > + }
> > +
> > + ret = regmap_read(regmap, BME680_REG_CHIP_ID, &val);
> > + if (ret < 0) {
> > + dev_err(dev, "Error reading chip ID\n");
> > + return ret;
> > + }
> > +
> > + if (val != BME680_CHIP_ID_VAL) {
> > + dev_err(dev, "Wrong chip ID, got %x expected %x\n",
> > + val, BME680_CHIP_ID_VAL);
> > + return -ENODEV;
> > + }
> > +
>
> And it registered successfully, but all the bme680 attributes were
> giving wrong values like temp was constant to 0.0000007, resistance
> was resource busy due to insuffient target temperature error.
> Pretty eveything was messed up at this stage.

Makes perfect sense if it's unable to read the registers.

If you cannot read the chip ID, nothing will work, no point skipping that.

> Then I build and booted the kernel on BeagleBone Black Wireless with
> DT matching this time:
>
> debian@beaglebone:~$ uname -a
> Linux beaglebone 4.19.5-ti-r5 #1xross SMP PREEMPT Sat Mar 16 12:11:50 IST 2019 armv7l GNU/Linux
> debian@beaglebone:~$ dmesg | grep 'bme680'
> [ 30.269207] bme680_spi spi0.0: Wrong chip ID, got ff expected 61
> [ 361.867410] bme680_core: disagrees about version of symbol module_layout

Looks like a compilation problem with your kernel module?

> debian@beaglebone:~$ lsmod | grep 'bme'
> bme680_spi 16384 0
> bme680_core 20480 1 bme680_spi
> debian@beaglebone:~$ cat /sys/bus/spi/devices/spi0.0/
> modalias of_node/ power/ statistics/ subsystem/ uevent
> debian@beaglebone:~$ cat /sys/bus/spi/devices/spi0.0/modalias
> spi:bme680
> debian@beaglebone:~$ cat /sys/bus/spi/devices/spi0.0/of_node/
> compatible name reg spi-max-frequency
> debian@beaglebone:~$ cat /sys/bus/spi/devices/spi0.0/of_node/compatible
> bme680
> debian@beaglebone:~$ cat /sys/bus/spi/devices/spi0.0/of_node/name
> bme680
> debian@beaglebone:~$ dtc -f -I fs /proc/device-tree | grep -A3 'bme680'
> bme680@0 {
> compatible = "bme680";
> reg = <0x0>;
> spi-max-frequency = <0x989680>;
> };
>
> Same error again!
>
> I really don't know where the problem is and how to rectify ?
>
> OTOH, I2C works like a charm as it used to work before:

That's reassuring, good to know i didn't kill that part.

>
> root@beaglebone:/sys/bus/iio/devices/iio:device1# cat name in_temp_input \
> in_pressure_input in_humidityrelative_input in_resistance_input
>
> bme680
> 26860 --> w/o your patch it used to be 26.86000 degC
> 990.870000000
> 55.265000000
> 10091
>
>
> I'm still assuming that there is some problem on my side, as it works
> flawless for you. But it is really difficult for me to figure out
> exactly where the problem could be!

I would not go as far as calling it "flawless". The "resistance"
measurement usually fails a few times, and the calibration values aren't
available at the first read apparently. After reading the values
multiple times (hint: "grep . *" is really nice for showing file
contents in a sysfs directory) the chip appears to function okay. That's
what my last commit paragraph was explaining.

It's a big improvement over the previous version where the SPI interface
wouldn't work at all.

--
Mike Looijmans

2019-03-17 09:51:44

by Himanshu Jha

Subject: Re: [PATCH v3 2/2] iio/chemical/bme680: Fix SPI read interface

On Sat, Mar 16, 2019 at 01:00:39PM +0000, Mike Looijmans wrote:
> On 16-03-19 11:24, Himanshu Jha wrote:
> > On Wed, Mar 06, 2019 at 08:31:48AM +0100, Mike Looijmans wrote:
> >> 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.
> >>
> >> Signed-off-by: Mike Looijmans <[email protected]>
> >> ---
> >
> > I have been trying to test this patch in the past week and still it
> > failed everytime.
> >
> > First I used ACPI to enumerate the device in QEMU setup:
> >
> > Added some printks for debugging:
> >
> > [ 14.510198] bme680_spi spi-BME0680:00: Jumping to core driver now ...
> > [ 14.544528] bme680_spi spi-BME0680:00: Page setting done, on Page :0 now
> > [ 14.554363] bme680_spi spi-BME0680:00: bme680_regmap_spi_write: on Page :0 now
> > [ 14.556151] bme680_spi spi-BME0680:00: bme680_regmap_spi_read: on Page :0 now
> > [ 14.567815] bme680_spi spi-BME0680:00: Wrong chip ID, got ff expected 61
> >
>
> Looks like the SPI communication isn't working. At this point I'd check
> the wires using an osciloscope or analyzer or something.

OK. Will give it a try at university lab.

> > I also tried bypassing this by removing the following snippet and force
> > registration to see what happens next:
> >
> > > + ret = regmap_write(regmap, BME680_REG_SOFT_RESET,
> > > + BME680_CMD_SOFTRESET);
> > > + if (ret < 0) {
> > > + dev_err(dev, "Failed to reset chip\n");
> > > + return ret;
> > > + }
> > > +
> > > + ret = regmap_read(regmap, BME680_REG_CHIP_ID, &val);
> > > + if (ret < 0) {
> > > + dev_err(dev, "Error reading chip ID\n");
> > > + return ret;
> > > + }
> > > +
> > > + if (val != BME680_CHIP_ID_VAL) {
> > > + dev_err(dev, "Wrong chip ID, got %x expected %x\n",
> > > + val, BME680_CHIP_ID_VAL);
> > > + return -ENODEV;
> > > + }
> > > +
> >
> > And it registered successfully, but all the bme680 attributes were
> > giving wrong values like temp was constant to 0.0000007, resistance
> > was resource busy due to insuffient target temperature error.
> > Pretty eveything was messed up at this stage.
>
> Makes perfect sense if it's unable to read the registers.
>
> If you cannot read the chip ID, nothing will work, no point skipping that.

Agreed! I was just trying to triage the issue.

> > Then I build and booted the kernel on BeagleBone Black Wireless with
> > DT matching this time:
> >
> > debian@beaglebone:~$ uname -a
> > Linux beaglebone 4.19.5-ti-r5 #1xross SMP PREEMPT Sat Mar 16 12:11:50 IST 2019 armv7l GNU/Linux
> > debian@beaglebone:~$ dmesg | grep 'bme680'
> > [ 30.269207] bme680_spi spi0.0: Wrong chip ID, got ff expected 61
> > [ 361.867410] bme680_core: disagrees about version of symbol module_layout
>
> Looks like a compilation problem with your kernel module?

No. I doesn't appear now.
I also build a more latest kernel but same error:

[ 519.741364] bme680_spi spi0.0: Wrong chip ID, got ff expected 61

> > debian@beaglebone:~$ lsmod | grep 'bme'
> > bme680_spi 16384 0
> > bme680_core 20480 1 bme680_spi
> > debian@beaglebone:~$ cat /sys/bus/spi/devices/spi0.0/
> > modalias of_node/ power/ statistics/ subsystem/ uevent
> > debian@beaglebone:~$ cat /sys/bus/spi/devices/spi0.0/modalias
> > spi:bme680
> > debian@beaglebone:~$ cat /sys/bus/spi/devices/spi0.0/of_node/
> > compatible name reg spi-max-frequency
> > debian@beaglebone:~$ cat /sys/bus/spi/devices/spi0.0/of_node/compatible
> > bme680
> > debian@beaglebone:~$ cat /sys/bus/spi/devices/spi0.0/of_node/name
> > bme680
> > debian@beaglebone:~$ dtc -f -I fs /proc/device-tree | grep -A3 'bme680'
> > bme680@0 {
> > compatible = "bme680";
> > reg = <0x0>;
> > spi-max-frequency = <0x989680>;
> > };
> >
> > Same error again!
> >
> > I really don't know where the problem is and how to rectify ?
> >
> > OTOH, I2C works like a charm as it used to work before:
>
> That's reassuring, good to know i didn't kill that part.

Yes!

> > root@beaglebone:/sys/bus/iio/devices/iio:device1# cat name in_temp_input \
> > in_pressure_input in_humidityrelative_input in_resistance_input
> >
> > bme680
> > 26860 --> w/o your patch it used to be 26.86000 degC
> > 990.870000000
> > 55.265000000
> > 10091
> >
> >
> > I'm still assuming that there is some problem on my side, as it works
> > flawless for you. But it is really difficult for me to figure out
> > exactly where the problem could be!
>
> I would not go as far as calling it "flawless". The "resistance"
> measurement usually fails a few times, and the calibration values aren't
> available at the first read apparently.

I know about resistance measurement failing intially "sometimes"
and it depends on environment factors as well. If there is sufficient
heat, then less chances of failure on initial reading.

> After reading the values
> multiple times (hint: "grep . *" is really nice for showing file
> contents in a sysfs directory) the chip appears to function okay. That's
> what my last commit paragraph was explaining.

I use watch command instead.

> It's a big improvement over the previous version where the SPI interface
> wouldn't work at all.

Agreed, Sir _/\_

> --
> Mike Looijmans


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