Re: [PATCH v4 6/6] iio: pressure: bmp280: Add triggered buffer support

[Jonathan Cameron <jic23@kernel.org>]

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
> +