| Message ID | 1551857508-4254-2-git-send-email-mike.looijmans@topic.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/2] iio/chemical/bme680: Report temperature in millidegrees | expand |
On Wed, 6 Mar 2019 08:31:48 +0100 Mike Looijmans <mike.looijmans@topic.nl> 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 <mike.looijmans@topic.nl> 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); > } >
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 <mike.looijmans@topic.nl> > --- 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!
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 <mike.looijmans@topic.nl> >> --- > > 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.
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 <mike.looijmans@topic.nl> > >> --- > > > > 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
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); }
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 <mike.looijmans@topic.nl> --- 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(-)