Re: [PATCH v2] iio: accel: bmc150: use IIO_DECLARE_BUFFER_WITH_TS

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 9 Aug 2026 23:59:34 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Sun,  9 Aug 2026 11:24:42 +0530
> Yash Suthar <yashsuthar983@gmail.com> wrote:
> 
> > Replace bmc150_accel_data plain buffer with IIO_DECLARE_BUFFER_WITH_TS()
> > that also keep timestamp aligned.
> > 
> > Fixes: bd7fe5b71918 ("iio: accel: BMC150 accel support")
> > Signed-off-by: Yash Suthar <yashsuthar983@gmail.com>  
> 
> For future reference, please never reply to an existing thread with 
> a new version of a patch.  I'm a bit confused why this one keeps
> coming up as I'm not aware of any part of the kernel that requests
> doing it this way.  Reasons not to do this:
> 1) Confusing threads once they get sufficient numbers of replies, including
>    making it harder for tooling to work out what is going on.
> 2) Reviewers and maintainers tend to use mail clients that put replies
>    to old threads, somewhere back in history, so the chances of getting
>    a review is reduced.
> 
> Anyhow, don't resend existing patches to 'fix this' but make sure
> to do new threads, if you send out any new versions.
> 
> This looks fine to me so applied to the fixes-togreg branch of iio.git
> Note that branch will be rebased on rc1 once it is available.

Actually no. I've backed that out. 

Given the data alignment is fixed, if we were going to do this it would
be clearer as 
struct {
	s16 chans[3]; //see later, I believe this should be __le16
	aligned_s64 timestamp;
};

But that is very similar to the structure that follows immediately after
this, but that has __le16 chans[] 

So what is going on here?

bmc150_accel_trigger_handler() Does a bulk read into data->buffer (so the
array this patch is touching). That is then pushed to the buffers.
Note this should be DMA safe, so we need to ensure nothing shares cache
line with it that might be edited concurrently.

data->scan is used in __bmc150_accel_fifo_flush() by memcpying
from a local buffer into this to marshal data.

That local buffer isn't dma safe, so good to fix that as a separate issue.
The buffer is fairly small (32 x 3 x 2 bytes) so one choice would be to put an
aligned buffer at the end of the iio_priv() structure.  Anyhow, that's
a different issue.  

So we already have two very similar structures.  
The chan spec is little endian so the scan one is more correct.

So I think the fix for this issue is move the scan element to the end of
struct bmc150_accel_data and mark it with __aligned(IIO_DMA_MINALIGN);

Then use that for both the bmc150_accel_trigger_handler() and __bmc150_accel_fifo_flush()
paths.  A second fix will resolve the local buffer in __bmc150_accel_fifo_flush()
that is being used for a bulk regmap transfer that may need a dma safe buffer.

I'd place that after the moved scan element in struct bmc150_accel_data.
Do that change as a separate fix.  As we have two dma buffer issues in this
driver take a look for any other bulk regmap accesses that may have similar
problems!

Please combine these two fixes (if you agree with my analysis) with a
follow up to do the iio_push_to_buffers_with_ts() all in one series
as they will be touching the same code.

Jonathan



> 
> Thanks,
> 
> Jonathan
> 
> 
> 
> > ---
> > v2:
> > - Rewrap commit message.
> > - Add Fixes tag.
> > 
> >  drivers/iio/accel/bmc150-accel.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/accel/bmc150-accel.h b/drivers/iio/accel/bmc150-accel.h
> > index e8f26198359f..e0773533efeb 100644
> > --- a/drivers/iio/accel/bmc150-accel.h
> > +++ b/drivers/iio/accel/bmc150-accel.h
> > @@ -64,7 +64,7 @@ struct bmc150_accel_data {
> >  	struct bmc150_accel_trigger triggers[BMC150_ACCEL_TRIGGERS];
> >  	struct mutex mutex;
> >  	u8 fifo_mode, watermark;
> > -	s16 buffer[8];
> > +	IIO_DECLARE_BUFFER_WITH_TS(s16, buffer, 3);
> >  	/*
> >  	 * Ensure there is sufficient space and correct alignment for
> >  	 * the timestamp if enabled  
> 
>