Received-SPF: pass (google.com: domain of linux-kernel+bounces-199638-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 3 Jun 2024 20:25:37 +0100
From: Jonathan Cameron <jic23@kernel.org>
To: Vasileios Amoiridis <vassilisamir@gmail.com>
Cc: 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: <20240603202537.4b40c80a@jic23-huawei>
In-Reply-To: <20240602193023.GD387181@vamoiridPC>
References: <20240527183805.311501-1-vassilisamir@gmail.com>
	<20240527183805.311501-12-vassilisamir@gmail.com>
	<20240602135726.2f10fd2b@jic23-huawei>
	<20240602193023.GD387181@vamoiridPC>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

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