Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sat, 9 Mar 2019 17:28:34 +0000
From:   Jonathan Cameron <jic23@kernel.org>
To:     Mike Looijmans <mike.looijmans@topic.nl>
Cc:     linux-iio@vger.kernel.org, himanshujha199640@gmail.com,
        linux-kernel@vger.kernel.org, knaack.h@gmx.de, lars@metafoo.de,
        pmeerw@pmeerw.net, dpfrey@gmail.com, colin.king@canonical.com
Subject: Re: [PATCH v3 2/2] iio/chemical/bme680: Fix SPI read interface
Message-ID: <20190309172834.56edeaac@archlinux>
In-Reply-To: <1551857508-4254-2-git-send-email-mike.looijmans@topic.nl>
References: <1551857508-4254-1-git-send-email-mike.looijmans@topic.nl>
        <1551857508-4254-2-git-send-email-mike.looijmans@topic.nl>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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);
>  }
>