Received-SPF: pass (google.com: domain of linux-kernel+bounces-143824-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Sat, 13 Apr 2024 18:06:51 +0100
From: Jonathan Cameron <jic23@kernel.org>
To: Vasileios Amoiridis <vassilisamir@gmail.com>
Cc: lars@metafoo.de, andriy.shevchenko@linux.intel.com,
 ang.iglesiasg@gmail.com, mazziesaccount@gmail.com, ak@it-klinger.de,
 petre.rodan@subdimension.ro, phil@raspberrypi.com, 579lpy@gmail.com,
 u.kleine-koenig@pengutronix.de, biju.das.jz@bp.renesas.com,
 linus.walleij@linaro.org, semen.protsenko@linaro.org,
 linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 6/6] iio: pressure: bmp280: Add triggered buffer
 support
Message-ID: <20240413180651.1eedce99@jic23-huawei>
In-Reply-To: <20240407172920.264282-7-vassilisamir@gmail.com>
References: <20240407172920.264282-1-vassilisamir@gmail.com>
	<20240407172920.264282-7-vassilisamir@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Sun,  7 Apr 2024 19:29:20 +0200
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> BMP2xx, BME280, BMP3xx, and BMP5xx use continuous buffers for their
> temperature, pressure and humidity readings. This facilitates the
> use of burst/bulk reads in order to acquire data faster. The
> approach is different from the one used in oneshot captures.
> 
> BMP085 & BMP1xx devices use a completely different measurement
> process that is well defined and is used in their buffer_handler().
> 
> Suggested-by: Angel Iglesias <ang.iglesiasg@gmail.com>
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
Hi,

A few things inline.

Jonathan

> ---
>  drivers/iio/pressure/Kconfig       |   2 +
>  drivers/iio/pressure/bmp280-core.c | 338 +++++++++++++++++++++++++++--
>  drivers/iio/pressure/bmp280-spi.c  |   8 +-
>  drivers/iio/pressure/bmp280.h      |  21 +-
>  4 files changed, 347 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
> index 3ad38506028e..0b5406a3f85d 100644
> --- a/drivers/iio/pressure/Kconfig
> +++ b/drivers/iio/pressure/Kconfig
> @@ -31,6 +31,8 @@ config BMP280
>  	select REGMAP
>  	select BMP280_I2C if (I2C)
>  	select BMP280_SPI if (SPI_MASTER)
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
>  	help
>  	  Say yes here to build support for Bosch Sensortec BMP180, BMP280, BMP380
>  	  and BMP580 pressure and temperature sensors. Also supports the BME280 with
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index 1b894feb717b..32dd35475826 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -41,7 +41,10 @@

>
> 
>  
> +static irqreturn_t bmp580_buffer_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct bmp280_data *data = iio_priv(indio_dev);
> +	s32 adc_temp, adc_press;
> +	int ret;
> +
> +	guard(mutex)(&data->lock);
> +
> +	/* If humidity channel is enabled it means that we are called for the
> +	 * BME280 humidity sensor.
Please match IIO multiline comment syntax. Like most of the kernel.

	/*
	 * If ...

> +	 */
> +


..

> diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
> index ccba779d7a83..0373d5f9b9a8 100644
> --- a/drivers/iio/pressure/bmp280.h
> +++ b/drivers/iio/pressure/bmp280.h
> @@ -301,6 +301,16 @@

>  /* Core exported structs */
>  
>  static const char *const bmp280_supply_names[] = {
> @@ -394,13 +404,19 @@ struct bmp280_data {
>  	 */
>  	int sampling_freq;
>  
> +	/*
> +	 * Data to push to userspace triggered buffer. Up to 3 channels and
> +	 * s64 timestamp, aligned.
> +	 */
> +	s32 sensor_data[5] __aligned(8);
For the timestamp to be aligned (which is the reason this is __aligned(8))
when landing at the end of the buffer, the buffer needs to be a multiple of 8
bytes.

3 channel, 32bit padding, s64 timestamp. So [6]

On the todo list is to add runtime checking to the IIO core on these buffers
being big enough which would have caught this, but I haven't gotten started
on that project yet.

Jonathan
> +