| Message ID | 20240527183805.311501-12-vassilisamir@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: chemical: bme680: Driver cleanup | expand |
On Mon, 27 May 2024 20:37:59 +0200 Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > Calibration data are located in contiguous-ish registers > inside the chip. For that reason we can use bulk reads as is > done as well in the BME68x Sensor API [1]. > > The arrays that are used for reading the data out of the sensor > are located inside DMA safe buffer. See below. I think in this case that isn't necessary. However it's a quirk of how the custom regmap works. Whilst we can't rely on regmap core spi implementations continuing to bounce buffer, we can rely on one local to our particular driver. > > [1]: https://github.com/boschsensortec/BME68x_SensorAPI/blob/v4.4.8/bme68x.c#L1769 > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> > diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c > index 681f271f9b06..ed4cdb4d64af 100644 > --- a/drivers/iio/chemical/bme680_core.c > +++ b/drivers/iio/chemical/bme680_core.c > + > struct bme680_calib { > u16 par_t1; > s16 par_t2; > @@ -64,6 +109,16 @@ struct bme680_data { > * and humidity compensation calculations. > */ > s32 t_fine; > + > + /* > + * DMA (thus cache coherency maintenance) may require the > + * transfer buffers to live in their own cache lines. > + */ > + union { > + u8 bme680_cal_buf_1[BME680_CALIB_RANGE_1_LEN]; > + u8 bme680_cal_buf_2[BME680_CALIB_RANGE_2_LEN]; > + u8 bme680_cal_buf_3[BME680_CALIB_RANGE_3_LEN]; > + } __aligned(IIO_DMA_MINALIGN); Ah! I should have read ahead. I don't think you need this alignment forcing because bme680_regmap_spi_read uses spi_write_then_read() which always bounces the data. > }; > > static const struct regmap_range bme680_volatile_ranges[] = { > @@ -112,217 +167,73 @@ static int bme680_read_calib(struct bme680_data *data, > struct bme680_calib *calib) > { > + calib->par_h3 = data->bme680_cal_buf_2[H3]; > + calib->par_h4 = data->bme680_cal_buf_2[H4]; > + calib->par_h5 = data->bme680_cal_buf_2[H5]; > + calib->par_h6 = data->bme680_cal_buf_2[H6]; > + calib->par_h7 = data->bme680_cal_buf_2[H7]; > + calib->par_t1 = get_unaligned_le16(&data->bme680_cal_buf_2[T1_LSB]); > + calib->par_gh2 = get_unaligned_le16(&data->bme680_cal_buf_2[GH2_LSB]); > + calib->par_gh1 = data->bme680_cal_buf_2[GH1]; > + calib->par_gh3 = data->bme680_cal_buf_2[GH3]; > > - ret = regmap_read(data->regmap, BME680_H7_REG, &tmp); > + ret = regmap_bulk_read(data->regmap, BME680_REG_RES_HEAT_VAL, > + &data->bme680_cal_buf_3[0], This one is always debated, but personally I'd prefer data->bme680_cal_buf_3, for cases like this. Up to you though. > + sizeof(data->bme680_cal_buf_3)); > if (ret < 0) { > - dev_err(dev, "failed to read BME680_H7_REG\n"); > + dev_err(dev, "failed to read 3rd set of calib data;\n"); > return ret; > }
On Sun, Jun 02, 2024 at 01:57:26PM +0100, Jonathan Cameron wrote: > On Mon, 27 May 2024 20:37:59 +0200 > Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > > > Calibration data are located in contiguous-ish registers > > inside the chip. For that reason we can use bulk reads as is > > done as well in the BME68x Sensor API [1]. > > > > The arrays that are used for reading the data out of the sensor > > are located inside DMA safe buffer. > > See below. I think in this case that isn't necessary. > However it's a quirk of how the custom regmap works. Whilst > we can't rely on regmap core spi implementations continuing to > bounce buffer, we can rely on one local to our particular driver. > What about the I2C implementation though? I watched recently a video from Wolfram Sang [1] and as far as I understood, the buffers are not provided by the I2C API, but you have to provide them. In any case, I should maybe check both SPI and I2C reads to understand the internals. [1]: https://www.youtube.com/watch?v=JDwaMClvV-s > > > > [1]: https://github.com/boschsensortec/BME68x_SensorAPI/blob/v4.4.8/bme68x.c#L1769 > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> > > > > diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c > > index 681f271f9b06..ed4cdb4d64af 100644 > > --- a/drivers/iio/chemical/bme680_core.c > > +++ b/drivers/iio/chemical/bme680_core.c > > > + > > struct bme680_calib { > > u16 par_t1; > > s16 par_t2; > > @@ -64,6 +109,16 @@ struct bme680_data { > > * and humidity compensation calculations. > > */ > > s32 t_fine; > > + > > + /* > > + * DMA (thus cache coherency maintenance) may require the > > + * transfer buffers to live in their own cache lines. > > + */ > > + union { > > + u8 bme680_cal_buf_1[BME680_CALIB_RANGE_1_LEN]; > > + u8 bme680_cal_buf_2[BME680_CALIB_RANGE_2_LEN]; > > + u8 bme680_cal_buf_3[BME680_CALIB_RANGE_3_LEN]; > > + } __aligned(IIO_DMA_MINALIGN); > Ah! I should have read ahead. I don't think you need this alignment forcing > because bme680_regmap_spi_read uses spi_write_then_read() which always > bounces the data. > Same comment. What about I2C? > > }; > > > > static const struct regmap_range bme680_volatile_ranges[] = { > > @@ -112,217 +167,73 @@ static int bme680_read_calib(struct bme680_data *data, > > struct bme680_calib *calib) > > { > > > > + calib->par_h3 = data->bme680_cal_buf_2[H3]; > > + calib->par_h4 = data->bme680_cal_buf_2[H4]; > > + calib->par_h5 = data->bme680_cal_buf_2[H5]; > > + calib->par_h6 = data->bme680_cal_buf_2[H6]; > > + calib->par_h7 = data->bme680_cal_buf_2[H7]; > > + calib->par_t1 = get_unaligned_le16(&data->bme680_cal_buf_2[T1_LSB]); > > + calib->par_gh2 = get_unaligned_le16(&data->bme680_cal_buf_2[GH2_LSB]); > > + calib->par_gh1 = data->bme680_cal_buf_2[GH1]; > > + calib->par_gh3 = data->bme680_cal_buf_2[GH3]; > > > > - ret = regmap_read(data->regmap, BME680_H7_REG, &tmp); > > + ret = regmap_bulk_read(data->regmap, BME680_REG_RES_HEAT_VAL, > > + &data->bme680_cal_buf_3[0], > This one is always debated, but personally I'd prefer > data->bme680_cal_buf_3, > For me it's the same, I could change it to what you proposed, no problem! Cheers, Vasilis > for cases like this. Up to you though. > > + sizeof(data->bme680_cal_buf_3)); > > if (ret < 0) { > > - dev_err(dev, "failed to read BME680_H7_REG\n"); > > + dev_err(dev, "failed to read 3rd set of calib data;\n"); > > return ret; > > } >
On Sun, 2 Jun 2024 21:30:23 +0200 Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > On Sun, Jun 02, 2024 at 01:57:26PM +0100, Jonathan Cameron wrote: > > On Mon, 27 May 2024 20:37:59 +0200 > > Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > > > > > Calibration data are located in contiguous-ish registers > > > inside the chip. For that reason we can use bulk reads as is > > > done as well in the BME68x Sensor API [1]. > > > > > > The arrays that are used for reading the data out of the sensor > > > are located inside DMA safe buffer. > > > > See below. I think in this case that isn't necessary. > > However it's a quirk of how the custom regmap works. Whilst > > we can't rely on regmap core spi implementations continuing to > > bounce buffer, we can rely on one local to our particular driver. > > > > What about the I2C implementation though? I watched recently a video > from Wolfram Sang [1] and as far as I understood, the buffers are not > provided by the I2C API, but you have to provide them. In any case, I > should maybe check both SPI and I2C reads to understand the internals. > > [1]: https://www.youtube.com/watch?v=JDwaMClvV-s > I'm not sure Wolfram got far with his desire for generally avoiding the bounce buffers for i2c. I think it's strictly opt in only so don't opt in unless your code is safe for it and regmap never will by default as too many drivers will be subtly broken. > > > > > > [1]: https://github.com/boschsensortec/BME68x_SensorAPI/blob/v4.4.8/bme68x.c#L1769 > > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> > > > > > > > diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c > > > index 681f271f9b06..ed4cdb4d64af 100644 > > > --- a/drivers/iio/chemical/bme680_core.c > > > +++ b/drivers/iio/chemical/bme680_core.c > > > > > + > > > struct bme680_calib { > > > u16 par_t1; > > > s16 par_t2; > > > @@ -64,6 +109,16 @@ struct bme680_data { > > > * and humidity compensation calculations. > > > */ > > > s32 t_fine; > > > + > > > + /* > > > + * DMA (thus cache coherency maintenance) may require the > > > + * transfer buffers to live in their own cache lines. > > > + */ > > > + union { > > > + u8 bme680_cal_buf_1[BME680_CALIB_RANGE_1_LEN]; > > > + u8 bme680_cal_buf_2[BME680_CALIB_RANGE_2_LEN]; > > > + u8 bme680_cal_buf_3[BME680_CALIB_RANGE_3_LEN]; > > > + } __aligned(IIO_DMA_MINALIGN); > > Ah! I should have read ahead. I don't think you need this alignment forcing > > because bme680_regmap_spi_read uses spi_write_then_read() which always > > bounces the data. > > > > Same comment. What about I2C? > > > > }; > > > > > > static const struct regmap_range bme680_volatile_ranges[] = { > > > @@ -112,217 +167,73 @@ static int bme680_read_calib(struct bme680_data *data, > > > struct bme680_calib *calib) > > > { > > > > > > > + calib->par_h3 = data->bme680_cal_buf_2[H3]; > > > + calib->par_h4 = data->bme680_cal_buf_2[H4]; > > > + calib->par_h5 = data->bme680_cal_buf_2[H5]; > > > + calib->par_h6 = data->bme680_cal_buf_2[H6]; > > > + calib->par_h7 = data->bme680_cal_buf_2[H7]; > > > + calib->par_t1 = get_unaligned_le16(&data->bme680_cal_buf_2[T1_LSB]); > > > + calib->par_gh2 = get_unaligned_le16(&data->bme680_cal_buf_2[GH2_LSB]); > > > + calib->par_gh1 = data->bme680_cal_buf_2[GH1]; > > > + calib->par_gh3 = data->bme680_cal_buf_2[GH3]; > > > > > > - ret = regmap_read(data->regmap, BME680_H7_REG, &tmp); > > > + ret = regmap_bulk_read(data->regmap, BME680_REG_RES_HEAT_VAL, > > > + &data->bme680_cal_buf_3[0], > > This one is always debated, but personally I'd prefer > > data->bme680_cal_buf_3, > > > > For me it's the same, I could change it to what you proposed, no problem! > > Cheers, > Vasilis > > > for cases like this. Up to you though. > > > + sizeof(data->bme680_cal_buf_3)); > > > if (ret < 0) { > > > - dev_err(dev, "failed to read BME680_H7_REG\n"); > > > + dev_err(dev, "failed to read 3rd set of calib data;\n"); > > > return ret; > > > } > >
On Mon, Jun 03, 2024 at 08:25:37PM +0100, Jonathan Cameron wrote: > On Sun, 2 Jun 2024 21:30:23 +0200 > Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > > > On Sun, Jun 02, 2024 at 01:57:26PM +0100, Jonathan Cameron wrote: > > > On Mon, 27 May 2024 20:37:59 +0200 > > > Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > > > > > > > Calibration data are located in contiguous-ish registers > > > > inside the chip. For that reason we can use bulk reads as is > > > > done as well in the BME68x Sensor API [1]. > > > > > > > > The arrays that are used for reading the data out of the sensor > > > > are located inside DMA safe buffer. > > > > > > See below. I think in this case that isn't necessary. > > > However it's a quirk of how the custom regmap works. Whilst > > > we can't rely on regmap core spi implementations continuing to > > > bounce buffer, we can rely on one local to our particular driver. > > > > > > > What about the I2C implementation though? I watched recently a video > > from Wolfram Sang [1] and as far as I understood, the buffers are not > > provided by the I2C API, but you have to provide them. In any case, I > > should maybe check both SPI and I2C reads to understand the internals. > > > > [1]: https://www.youtube.com/watch?v=JDwaMClvV-s > > > > I'm not sure Wolfram got far with his desire for generally avoiding the > bounce buffers for i2c. I think it's strictly opt in only so don't opt in > unless your code is safe for it and regmap never will by default as too > many drivers will be subtly broken. > The things that I found about DMA "safety" in I2C are [1] and [2] so I think that the IIO_DMA_MINALIGN should remain because in the future, in case it's needed for triggered buffers to do buffer reads from the volatile registers of the device, then it might be a problem for I2C. [1]: https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-core-base.c#L2627 [2]: https://elixir.bootlin.com/linux/latest/source/include/linux/i2c.h#L92 > > > > > > > > > [1]: https://github.com/boschsensortec/BME68x_SensorAPI/blob/v4.4.8/bme68x.c#L1769 > > > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> > > > > > > > > > > diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c > > > > index 681f271f9b06..ed4cdb4d64af 100644 > > > > --- a/drivers/iio/chemical/bme680_core.c > > > > +++ b/drivers/iio/chemical/bme680_core.c > > > > > > > + > > > > struct bme680_calib { > > > > u16 par_t1; > > > > s16 par_t2; > > > > @@ -64,6 +109,16 @@ struct bme680_data { > > > > * and humidity compensation calculations. > > > > */ > > > > s32 t_fine; > > > > + > > > > + /* > > > > + * DMA (thus cache coherency maintenance) may require the > > > > + * transfer buffers to live in their own cache lines. > > > > + */ > > > > + union { > > > > + u8 bme680_cal_buf_1[BME680_CALIB_RANGE_1_LEN]; > > > > + u8 bme680_cal_buf_2[BME680_CALIB_RANGE_2_LEN]; > > > > + u8 bme680_cal_buf_3[BME680_CALIB_RANGE_3_LEN]; > > > > + } __aligned(IIO_DMA_MINALIGN); > > > Ah! I should have read ahead. I don't think you need this alignment forcing > > > because bme680_regmap_spi_read uses spi_write_then_read() which always > > > bounces the data. > > > > > > > Same comment. What about I2C? > > > > > > }; > > > > > > > > static const struct regmap_range bme680_volatile_ranges[] = { > > > > @@ -112,217 +167,73 @@ static int bme680_read_calib(struct bme680_data *data, > > > > struct bme680_calib *calib) > > > > { > > > > > > > > > > + calib->par_h3 = data->bme680_cal_buf_2[H3]; > > > > + calib->par_h4 = data->bme680_cal_buf_2[H4]; > > > > + calib->par_h5 = data->bme680_cal_buf_2[H5]; > > > > + calib->par_h6 = data->bme680_cal_buf_2[H6]; > > > > + calib->par_h7 = data->bme680_cal_buf_2[H7]; > > > > + calib->par_t1 = get_unaligned_le16(&data->bme680_cal_buf_2[T1_LSB]); > > > > + calib->par_gh2 = get_unaligned_le16(&data->bme680_cal_buf_2[GH2_LSB]); > > > > + calib->par_gh1 = data->bme680_cal_buf_2[GH1]; > > > > + calib->par_gh3 = data->bme680_cal_buf_2[GH3]; > > > > > > > > - ret = regmap_read(data->regmap, BME680_H7_REG, &tmp); > > > > + ret = regmap_bulk_read(data->regmap, BME680_REG_RES_HEAT_VAL, > > > > + &data->bme680_cal_buf_3[0], > > > This one is always debated, but personally I'd prefer > > > data->bme680_cal_buf_3, > > > > > > > For me it's the same, I could change it to what you proposed, no problem! > > > > Cheers, > > Vasilis > > > > > for cases like this. Up to you though. > > > > + sizeof(data->bme680_cal_buf_3)); > > > > if (ret < 0) { > > > > - dev_err(dev, "failed to read BME680_H7_REG\n"); > > > > + dev_err(dev, "failed to read 3rd set of calib data;\n"); > > > > return ret; > > > > } > > > >
On Mon, 3 Jun 2024 22:30:03 +0200 Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > On Mon, Jun 03, 2024 at 08:25:37PM +0100, Jonathan Cameron wrote: > > On Sun, 2 Jun 2024 21:30:23 +0200 > > Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > > > > > On Sun, Jun 02, 2024 at 01:57:26PM +0100, Jonathan Cameron wrote: > > > > On Mon, 27 May 2024 20:37:59 +0200 > > > > Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > > > > > > > > > Calibration data are located in contiguous-ish registers > > > > > inside the chip. For that reason we can use bulk reads as is > > > > > done as well in the BME68x Sensor API [1]. > > > > > > > > > > The arrays that are used for reading the data out of the sensor > > > > > are located inside DMA safe buffer. > > > > > > > > See below. I think in this case that isn't necessary. > > > > However it's a quirk of how the custom regmap works. Whilst > > > > we can't rely on regmap core spi implementations continuing to > > > > bounce buffer, we can rely on one local to our particular driver. > > > > > > > > > > What about the I2C implementation though? I watched recently a video > > > from Wolfram Sang [1] and as far as I understood, the buffers are not > > > provided by the I2C API, but you have to provide them. In any case, I > > > should maybe check both SPI and I2C reads to understand the internals. > > > > > > [1]: https://www.youtube.com/watch?v=JDwaMClvV-s > > > > > > > I'm not sure Wolfram got far with his desire for generally avoiding the > > bounce buffers for i2c. I think it's strictly opt in only so don't opt in > > unless your code is safe for it and regmap never will by default as too > > many drivers will be subtly broken. > > > > The things that I found about DMA "safety" in I2C are [1] and [2] so I think > that the IIO_DMA_MINALIGN should remain because in the future, in case it's > needed for triggered buffers to do buffer reads from the volatile registers > of the device, then it might be a problem for I2C. > > [1]: https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-core-base.c#L2627 > [2]: https://elixir.bootlin.com/linux/latest/source/include/linux/i2c.h#L92 > Those will remain opt in for a very long time if not for ever. I suspect they will actually go away as a result of Catalin's https://lore.kernel.org/linux-arm-kernel/20230612153201.554742-15-catalin.marinas@arm.com/ which bounces small or unaligned buffers and will apply to an annoying number of IIO buffers even though they are actually safe because it's a heuristic on the size part. Today only a few architectures that do non coherent DMA use it though. Will take the others opting in to the point where the config variable goes away before we can stop this dance. I don't know if anyone has yet looked at the impact on performance of that change on the many drivers that have to work whether that is in place or not and do small dma mappings. Maybe we need to teach bus drivers when they need to map padding around an actually safe buffer that doesn't look like it. Jonathan > > > > > > > > > > > > [1]: https://github.com/boschsensortec/BME68x_SensorAPI/blob/v4.4.8/bme68x.c#L1769 > > > > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> > > > > > > > > > > > > > diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c > > > > > index 681f271f9b06..ed4cdb4d64af 100644 > > > > > --- a/drivers/iio/chemical/bme680_core.c > > > > > +++ b/drivers/iio/chemical/bme680_core.c > > > > > > > > > + > > > > > struct bme680_calib { > > > > > u16 par_t1; > > > > > s16 par_t2; > > > > > @@ -64,6 +109,16 @@ struct bme680_data { > > > > > * and humidity compensation calculations. > > > > > */ > > > > > s32 t_fine; > > > > > + > > > > > + /* > > > > > + * DMA (thus cache coherency maintenance) may require the > > > > > + * transfer buffers to live in their own cache lines. > > > > > + */ > > > > > + union { > > > > > + u8 bme680_cal_buf_1[BME680_CALIB_RANGE_1_LEN]; > > > > > + u8 bme680_cal_buf_2[BME680_CALIB_RANGE_2_LEN]; > > > > > + u8 bme680_cal_buf_3[BME680_CALIB_RANGE_3_LEN]; > > > > > + } __aligned(IIO_DMA_MINALIGN); > > > > Ah! I should have read ahead. I don't think you need this alignment forcing > > > > because bme680_regmap_spi_read uses spi_write_then_read() which always > > > > bounces the data. > > > > > > > > > > Same comment. What about I2C? > > > > > > > > }; > > > > > > > > > > static const struct regmap_range bme680_volatile_ranges[] = { > > > > > @@ -112,217 +167,73 @@ static int bme680_read_calib(struct bme680_data *data, > > > > > struct bme680_calib *calib) > > > > > { > > > > > > > > > > > > > + calib->par_h3 = data->bme680_cal_buf_2[H3]; > > > > > + calib->par_h4 = data->bme680_cal_buf_2[H4]; > > > > > + calib->par_h5 = data->bme680_cal_buf_2[H5]; > > > > > + calib->par_h6 = data->bme680_cal_buf_2[H6]; > > > > > + calib->par_h7 = data->bme680_cal_buf_2[H7]; > > > > > + calib->par_t1 = get_unaligned_le16(&data->bme680_cal_buf_2[T1_LSB]); > > > > > + calib->par_gh2 = get_unaligned_le16(&data->bme680_cal_buf_2[GH2_LSB]); > > > > > + calib->par_gh1 = data->bme680_cal_buf_2[GH1]; > > > > > + calib->par_gh3 = data->bme680_cal_buf_2[GH3]; > > > > > > > > > > - ret = regmap_read(data->regmap, BME680_H7_REG, &tmp); > > > > > + ret = regmap_bulk_read(data->regmap, BME680_REG_RES_HEAT_VAL, > > > > > + &data->bme680_cal_buf_3[0], > > > > This one is always debated, but personally I'd prefer > > > > data->bme680_cal_buf_3, > > > > > > > > > > For me it's the same, I could change it to what you proposed, no problem! > > > > > > Cheers, > > > Vasilis > > > > > > > for cases like this. Up to you though. > > > > > + sizeof(data->bme680_cal_buf_3)); > > > > > if (ret < 0) { > > > > > - dev_err(dev, "failed to read BME680_H7_REG\n"); > > > > > + dev_err(dev, "failed to read 3rd set of calib data;\n"); > > > > > return ret; > > > > > } > > > > > >
diff --git a/drivers/iio/chemical/bme680.h b/drivers/iio/chemical/bme680.h index 3133d624270a..5f602170a3af 100644 --- a/drivers/iio/chemical/bme680.h +++ b/drivers/iio/chemical/bme680.h @@ -39,10 +39,8 @@ #define BME680_HUM_REG_SHIFT_VAL 4 #define BME680_BIT_H1_DATA_MASK GENMASK(3, 0) -#define BME680_REG_RES_HEAT_RANGE 0x02 #define BME680_RHRANGE_MASK GENMASK(5, 4) #define BME680_REG_RES_HEAT_VAL 0x00 -#define BME680_REG_RANGE_SW_ERR 0x04 #define BME680_RSERROR_MASK GENMASK(7, 4) #define BME680_REG_RES_HEAT_0 0x5A #define BME680_REG_GAS_WAIT_0 0x64 @@ -58,31 +56,13 @@ /* Calibration Parameters */ #define BME680_T2_LSB_REG 0x8A -#define BME680_T3_REG 0x8C -#define BME680_P1_LSB_REG 0x8E -#define BME680_P2_LSB_REG 0x90 -#define BME680_P3_REG 0x92 -#define BME680_P4_LSB_REG 0x94 -#define BME680_P5_LSB_REG 0x96 -#define BME680_P7_REG 0x98 -#define BME680_P6_REG 0x99 -#define BME680_P8_LSB_REG 0x9C -#define BME680_P9_LSB_REG 0x9E -#define BME680_P10_REG 0xA0 -#define BME680_H2_LSB_REG 0xE2 #define BME680_H2_MSB_REG 0xE1 -#define BME680_H1_MSB_REG 0xE3 -#define BME680_H1_LSB_REG 0xE2 -#define BME680_H3_REG 0xE4 -#define BME680_H4_REG 0xE5 -#define BME680_H5_REG 0xE6 -#define BME680_H6_REG 0xE7 -#define BME680_H7_REG 0xE8 -#define BME680_T1_LSB_REG 0xE9 -#define BME680_GH2_LSB_REG 0xEB -#define BME680_GH1_REG 0xED #define BME680_GH3_REG 0xEE +#define BME680_CALIB_RANGE_1_LEN 23 +#define BME680_CALIB_RANGE_2_LEN 14 +#define BME680_CALIB_RANGE_3_LEN 5 + extern const struct regmap_config bme680_regmap_config; int bme680_core_probe(struct device *dev, struct regmap *regmap, diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c index 681f271f9b06..ed4cdb4d64af 100644 --- a/drivers/iio/chemical/bme680_core.c +++ b/drivers/iio/chemical/bme680_core.c @@ -19,8 +19,53 @@ #include <linux/iio/iio.h> #include <linux/iio/sysfs.h> +#include <asm/unaligned.h> + #include "bme680.h" +/* 1st set of calibration data */ +enum { + /* Temperature calib indexes */ + T2_LSB = 0, + T3 = 2, + /* Pressure calib indexes */ + P1_LSB = 4, + P2_LSB = 6, + P3 = 8, + P4_LSB = 10, + P5_LSB = 12, + P7 = 14, + P6 = 15, + P8_LSB = 18, + P9_LSB = 20, + P10 = 22, +}; + +/* 2nd set of calibration data */ +enum { + /* Humidity calib indexes */ + H2_MSB = 0, + H1_LSB = 1, + H3 = 3, + H4 = 4, + H5 = 5, + H6 = 6, + H7 = 7, + /* Stray T1 calib index */ + T1_LSB = 8, + /* Gas heater calib indexes */ + GH2_LSB = 10, + GH1 = 12, + GH3 = 13, +}; + +/* 3rd set of calibration data */ +enum { + RES_HEAT_VAL = 0, + RES_HEAT_RANGE = 2, + RANGE_SW_ERR = 4, +}; + struct bme680_calib { u16 par_t1; s16 par_t2; @@ -64,6 +109,16 @@ struct bme680_data { * and humidity compensation calculations. */ s32 t_fine; + + /* + * DMA (thus cache coherency maintenance) may require the + * transfer buffers to live in their own cache lines. + */ + union { + u8 bme680_cal_buf_1[BME680_CALIB_RANGE_1_LEN]; + u8 bme680_cal_buf_2[BME680_CALIB_RANGE_2_LEN]; + u8 bme680_cal_buf_3[BME680_CALIB_RANGE_3_LEN]; + } __aligned(IIO_DMA_MINALIGN); }; static const struct regmap_range bme680_volatile_ranges[] = { @@ -112,217 +167,73 @@ static int bme680_read_calib(struct bme680_data *data, struct bme680_calib *calib) { struct device *dev = regmap_get_device(data->regmap); - unsigned int tmp, tmp_msb, tmp_lsb; + unsigned int tmp_msb, tmp_lsb; int ret; - __le16 buf; - - /* Temperature related coefficients */ - ret = regmap_bulk_read(data->regmap, BME680_T1_LSB_REG, - &buf, sizeof(buf)); - if (ret < 0) { - dev_err(dev, "failed to read BME680_T1_LSB_REG\n"); - return ret; - } - calib->par_t1 = le16_to_cpu(buf); ret = regmap_bulk_read(data->regmap, BME680_T2_LSB_REG, - &buf, sizeof(buf)); + &data->bme680_cal_buf_1[0], + sizeof(data->bme680_cal_buf_1)); if (ret < 0) { - dev_err(dev, "failed to read BME680_T2_LSB_REG\n"); + dev_err(dev, "failed to read 1st set of calib data;\n"); return ret; } - calib->par_t2 = le16_to_cpu(buf); - ret = regmap_read(data->regmap, BME680_T3_REG, &tmp); - if (ret < 0) { - dev_err(dev, "failed to read BME680_T3_REG\n"); - return ret; - } - calib->par_t3 = tmp; + calib->par_t2 = get_unaligned_le16(&data->bme680_cal_buf_1[T2_LSB]); + calib->par_t3 = data->bme680_cal_buf_1[T3]; + calib->par_p1 = get_unaligned_le16(&data->bme680_cal_buf_1[P1_LSB]); + calib->par_p2 = get_unaligned_le16(&data->bme680_cal_buf_1[P2_LSB]); + calib->par_p3 = data->bme680_cal_buf_1[P3]; + calib->par_p4 = get_unaligned_le16(&data->bme680_cal_buf_1[P4_LSB]); + calib->par_p5 = get_unaligned_le16(&data->bme680_cal_buf_1[P5_LSB]); + calib->par_p7 = data->bme680_cal_buf_1[P7]; + calib->par_p6 = data->bme680_cal_buf_1[P6]; + calib->par_p8 = get_unaligned_le16(&data->bme680_cal_buf_1[P8_LSB]); + calib->par_p9 = get_unaligned_le16(&data->bme680_cal_buf_1[P9_LSB]); + calib->par_p10 = data->bme680_cal_buf_1[P10]; - /* Pressure related coefficients */ - ret = regmap_bulk_read(data->regmap, BME680_P1_LSB_REG, - &buf, sizeof(buf)); + ret = regmap_bulk_read(data->regmap, BME680_H2_MSB_REG, + &data->bme680_cal_buf_2[0], + sizeof(data->bme680_cal_buf_2)); if (ret < 0) { - dev_err(dev, "failed to read BME680_P1_LSB_REG\n"); + dev_err(dev, "failed to read 2nd set of calib data;\n"); return ret; } - calib->par_p1 = le16_to_cpu(buf); - ret = regmap_bulk_read(data->regmap, BME680_P2_LSB_REG, - &buf, sizeof(buf)); - if (ret < 0) { - dev_err(dev, "failed to read BME680_P2_LSB_REG\n"); - return ret; - } - calib->par_p2 = le16_to_cpu(buf); - - ret = regmap_read(data->regmap, BME680_P3_REG, &tmp); - if (ret < 0) { - dev_err(dev, "failed to read BME680_P3_REG\n"); - return ret; - } - calib->par_p3 = tmp; - - ret = regmap_bulk_read(data->regmap, BME680_P4_LSB_REG, - &buf, sizeof(buf)); - if (ret < 0) { - dev_err(dev, "failed to read BME680_P4_LSB_REG\n"); - return ret; - } - calib->par_p4 = le16_to_cpu(buf); - - ret = regmap_bulk_read(data->regmap, BME680_P5_LSB_REG, - &buf, sizeof(buf)); - if (ret < 0) { - dev_err(dev, "failed to read BME680_P5_LSB_REG\n"); - return ret; - } - calib->par_p5 = le16_to_cpu(buf); - - ret = regmap_read(data->regmap, BME680_P6_REG, &tmp); - if (ret < 0) { - dev_err(dev, "failed to read BME680_P6_REG\n"); - return ret; - } - calib->par_p6 = tmp; - - ret = regmap_read(data->regmap, BME680_P7_REG, &tmp); - if (ret < 0) { - dev_err(dev, "failed to read BME680_P7_REG\n"); - return ret; - } - calib->par_p7 = tmp; - - ret = regmap_bulk_read(data->regmap, BME680_P8_LSB_REG, - &buf, sizeof(buf)); - if (ret < 0) { - dev_err(dev, "failed to read BME680_P8_LSB_REG\n"); - return ret; - } - calib->par_p8 = le16_to_cpu(buf); - - ret = regmap_bulk_read(data->regmap, BME680_P9_LSB_REG, - &buf, sizeof(buf)); - if (ret < 0) { - dev_err(dev, "failed to read BME680_P9_LSB_REG\n"); - return ret; - } - calib->par_p9 = le16_to_cpu(buf); - - ret = regmap_read(data->regmap, BME680_P10_REG, &tmp); - if (ret < 0) { - dev_err(dev, "failed to read BME680_P10_REG\n"); - return ret; - } - calib->par_p10 = tmp; - - /* Humidity related coefficients */ - ret = regmap_read(data->regmap, BME680_H1_MSB_REG, &tmp_msb); - if (ret < 0) { - dev_err(dev, "failed to read BME680_H1_MSB_REG\n"); - return ret; - } - ret = regmap_read(data->regmap, BME680_H1_LSB_REG, &tmp_lsb); - if (ret < 0) { - dev_err(dev, "failed to read BME680_H1_LSB_REG\n"); - return ret; - } + tmp_lsb = data->bme680_cal_buf_2[H1_LSB]; + tmp_msb = data->bme680_cal_buf_2[H1_LSB + 1]; calib->par_h1 = (tmp_msb << BME680_HUM_REG_SHIFT_VAL) | (tmp_lsb & BME680_BIT_H1_DATA_MASK); - ret = regmap_read(data->regmap, BME680_H2_MSB_REG, &tmp_msb); - if (ret < 0) { - dev_err(dev, "failed to read BME680_H2_MSB_REG\n"); - return ret; - } - ret = regmap_read(data->regmap, BME680_H2_LSB_REG, &tmp_lsb); - if (ret < 0) { - dev_err(dev, "failed to read BME680_H2_LSB_REG\n"); - return ret; - } + tmp_msb = data->bme680_cal_buf_2[H2_MSB]; + tmp_lsb = data->bme680_cal_buf_2[H2_MSB + 1]; calib->par_h2 = (tmp_msb << BME680_HUM_REG_SHIFT_VAL) | (tmp_lsb >> BME680_HUM_REG_SHIFT_VAL); - ret = regmap_read(data->regmap, BME680_H3_REG, &tmp); - if (ret < 0) { - dev_err(dev, "failed to read BME680_H3_REG\n"); - return ret; - } - calib->par_h3 = tmp; - - ret = regmap_read(data->regmap, BME680_H4_REG, &tmp); - if (ret < 0) { - dev_err(dev, "failed to read BME680_H4_REG\n"); - return ret; - } - calib->par_h4 = tmp; - - ret = regmap_read(data->regmap, BME680_H5_REG, &tmp); - if (ret < 0) { - dev_err(dev, "failed to read BME680_H5_REG\n"); - return ret; - } - calib->par_h5 = tmp; - - ret = regmap_read(data->regmap, BME680_H6_REG, &tmp); - if (ret < 0) { - dev_err(dev, "failed to read BME680_H6_REG\n"); - return ret; - } - calib->par_h6 = tmp; + calib->par_h3 = data->bme680_cal_buf_2[H3]; + calib->par_h4 = data->bme680_cal_buf_2[H4]; + calib->par_h5 = data->bme680_cal_buf_2[H5]; + calib->par_h6 = data->bme680_cal_buf_2[H6]; + calib->par_h7 = data->bme680_cal_buf_2[H7]; + calib->par_t1 = get_unaligned_le16(&data->bme680_cal_buf_2[T1_LSB]); + calib->par_gh2 = get_unaligned_le16(&data->bme680_cal_buf_2[GH2_LSB]); + calib->par_gh1 = data->bme680_cal_buf_2[GH1]; + calib->par_gh3 = data->bme680_cal_buf_2[GH3]; - ret = regmap_read(data->regmap, BME680_H7_REG, &tmp); + ret = regmap_bulk_read(data->regmap, BME680_REG_RES_HEAT_VAL, + &data->bme680_cal_buf_3[0], + sizeof(data->bme680_cal_buf_3)); if (ret < 0) { - dev_err(dev, "failed to read BME680_H7_REG\n"); + dev_err(dev, "failed to read 3rd set of calib data;\n"); return ret; } - calib->par_h7 = tmp; - /* Gas heater related coefficients */ - ret = regmap_read(data->regmap, BME680_GH1_REG, &tmp); - if (ret < 0) { - dev_err(dev, "failed to read BME680_GH1_REG\n"); - return ret; - } - calib->par_gh1 = tmp; + calib->res_heat_val = data->bme680_cal_buf_3[RES_HEAT_VAL]; - ret = regmap_bulk_read(data->regmap, BME680_GH2_LSB_REG, - &buf, sizeof(buf)); - if (ret < 0) { - dev_err(dev, "failed to read BME680_GH2_LSB_REG\n"); - return ret; - } - calib->par_gh2 = le16_to_cpu(buf); + calib->res_heat_range = FIELD_GET(BME680_RHRANGE_MASK, + data->bme680_cal_buf_3[RES_HEAT_RANGE]); - ret = regmap_read(data->regmap, BME680_GH3_REG, &tmp); - if (ret < 0) { - dev_err(dev, "failed to read BME680_GH3_REG\n"); - return ret; - } - calib->par_gh3 = tmp; - - /* Other coefficients */ - ret = regmap_read(data->regmap, BME680_REG_RES_HEAT_RANGE, &tmp); - if (ret < 0) { - dev_err(dev, "failed to read resistance heat range\n"); - return ret; - } - calib->res_heat_range = FIELD_GET(BME680_RHRANGE_MASK, tmp); - - ret = regmap_read(data->regmap, BME680_REG_RES_HEAT_VAL, &tmp); - if (ret < 0) { - dev_err(dev, "failed to read resistance heat value\n"); - return ret; - } - calib->res_heat_val = tmp; - - ret = regmap_read(data->regmap, BME680_REG_RANGE_SW_ERR, &tmp); - if (ret < 0) { - dev_err(dev, "failed to read range software error\n"); - return ret; - } - calib->range_sw_err = FIELD_GET(BME680_RSERROR_MASK, tmp); + calib->range_sw_err = FIELD_GET(BME680_RSERROR_MASK, + data->bme680_cal_buf_3[RANGE_SW_ERR]); return 0; }
Calibration data are located in contiguous-ish registers inside the chip. For that reason we can use bulk reads as is done as well in the BME68x Sensor API [1]. The arrays that are used for reading the data out of the sensor are located inside DMA safe buffer. [1]: https://github.com/boschsensortec/BME68x_SensorAPI/blob/v4.4.8/bme68x.c#L1769 Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> --- drivers/iio/chemical/bme680.h | 28 +-- drivers/iio/chemical/bme680_core.c | 283 ++++++++++------------------- 2 files changed, 101 insertions(+), 210 deletions(-)