Re: [PATCH] iio: health: max30102: Add timestamp to FIFO data

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 14 May 2026 23:03:09 +0300
Md Shofiqul Islam <shofiqtest@gmail.com> wrote:

There are various things going on here - that patch title
isn't enough.


> The I2C DMA read buffer was not aligned to IIO_DMA_MINALIGN, which
> can cause issues on platforms with strict DMA alignment requirements.
> Fix this by annotating the buffer field with __aligned(IIO_DMA_MINALIGN).

Nope. Look into the requirements of i2c (basically it doesn't have
any as it always bounces the data unless you go to considerable effort
to tell it to not do so!)

I've kind of lost track but didn't I explain why timestamps and fifos
are an awful lot more complex than this!  This is an absolute non starter.
Also the max30102 is a serious weird device that runs with a custom
userspace. You can't get pulse readings without some nasty (not particularly
public) maths.

Generally don't add features to a driver you aren't using - they
end up as dead code.

Jonathan


> 
> Replace the plain processed_buffer[] array with a packed scan struct
> containing the channel data and an aligned_s64 timestamp field. This
> satisfies the alignment requirement for iio_push_to_buffers_with_timestamp()
> and ensures correct layout when the IIO core appends the timestamp.
> 
> Add IIO_CHAN_SOFT_TIMESTAMP entries to both max30102_channels[] and
> max30105_channels[] so the IIO core knows to expect a timestamp slot in
> the scan buffer.
> 
> Capture iio_get_time_ns() per FIFO sample read and pass it to
> iio_push_to_buffers_with_timestamp() so userspace consumers receive
> timing information alongside the intensity data.
> 
> Signed-off-by: Md Shofiqul Islam <shofiqtest@gmail.com>
> ---
>  drivers/iio/health/max30102.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/health/max30102.c b/drivers/iio/health/max30102.c
> index 47da44efd68b..447c4de8ff4b 100644
> --- a/drivers/iio/health/max30102.c
> +++ b/drivers/iio/health/max30102.c
> @@ -24,6 +24,7 @@
>  #include <linux/iio/iio.h>
>  #include <linux/iio/buffer.h>
>  #include <linux/iio/kfifo_buf.h>
> +#include <linux/iio/trigger_consumer.h>
>  
>  #define MAX30102_DRV_NAME	"max30102"
>  #define MAX30102_PART_NUMBER	0x15
> @@ -106,8 +107,11 @@ struct max30102_data {
>  	struct regmap *regmap;
>  	enum max30102_chip_id chip_id;
>  
> -	u8 buffer[12];
> -	__be32 processed_buffer[3]; /* 3 x 18-bit (padded to 32-bits) */
> +	u8 buffer[12] __aligned(IIO_DMA_MINALIGN);
> +	struct {
> +		__be32 channels[3]; /* 3 x 18-bit (padded to 32-bits) */
> +		aligned_s64 ts;

Where does the timestamp end up if only 1 or 2 channels are enabled?
(or indeed the device only has 2 channels).
The answer to that question will tell you why (if we were doing this)
it should be an array.


> +	} scan;
>  };

> @@ -298,11 +304,13 @@ static irqreturn_t max30102_interrupt_handler(int irq, void *private)
>  	mutex_lock(&data->lock);
>  
>  	while (cnt || (cnt = max30102_fifo_count(data)) > 0) {
> +		s64 ts = iio_get_time_ns(indio_dev);

So all the samples get timestamps near each other. Nope. They should
have the timestamps that reflect when they enter the fifo. That's hard.

> +
>  		ret = max30102_read_measurement(data, measurements);
>  		if (ret)
>  			break;
>  
> -		iio_push_to_buffers(data->indio_dev, data->processed_buffer);
> +		iio_push_to_buffers_with_timestamp(indio_dev, &data->scan, ts);
>  		cnt--;
>  	}
>