Received-SPF: pass (google.com: domain of linux-kernel+bounces-198392-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
From: Vasileios Amoiridis <vassilisamir@gmail.com>
Date: Sun, 2 Jun 2024 21:30:23 +0200
To: Jonathan Cameron <jic23@kernel.org>
Cc: Vasileios Amoiridis <vassilisamir@gmail.com>, lars@metafoo.de,
	himanshujha199640@gmail.com, linux-iio@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 11/17] iio: chemical: bme680: Use bulk reads for
 calibration data
Message-ID: <20240602193023.GD387181@vamoiridPC>
References: <20240527183805.311501-1-vassilisamir@gmail.com>
 <20240527183805.311501-12-vassilisamir@gmail.com>
 <20240602135726.2f10fd2b@jic23-huawei>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240602135726.2f10fd2b@jic23-huawei>

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