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

From: Jonathan Cameron
Date: Sat Mar 09 2019 - 12:28:44 EST

Next message: Jonathan Cameron: "Re: [PATCH v3] io: accel: kxcjk1013: restore the range after resume."
Previous message: Christian Brauner: "Re: [PATCH RESEND v5 2/5] namei: O_BENEATH-style path resolution flags"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, 6 Mar 2019 08:31:48 +0100
Mike Looijmans <mike.looijmans@xxxxxxxx> 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@xxxxxxxx>
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);
> }
>

Next message: Jonathan Cameron: "Re: [PATCH v3] io: accel: kxcjk1013: restore the range after resume."
Previous message: Christian Brauner: "Re: [PATCH RESEND v5 2/5] namei: O_BENEATH-style path resolution flags"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]