Received-SPF: pass (google.com: domain of linux-kernel+bounces-198244-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Sun, 2 Jun 2024 13:57:26 +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: <20240602135726.2f10fd2b@jic23-huawei>
In-Reply-To: <20240527183805.311501-12-vassilisamir@gmail.com>
References: <20240527183805.311501-1-vassilisamir@gmail.com>
	<20240527183805.311501-12-vassilisamir@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

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