[PATCH] iio: accel: bmc150: use IIO_DECLARE_BUFFER_WITH_TS

[PATCH] iio: accel: bmc150: use IIO_DECLARE_BUFFER_WITH_TS

[Yash Suthar]

Replace bmc150_accel_data plain buffer with
IIO_DECLARE_BUFFER_WITH_TS() that also keep
timestamp aligned.

Signed-off-by: Yash Suthar <yashsuthar983@gmail.com>
---
 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
-- 
2.43.0

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

[David Lechner]

On 8/8/26 5:02 PM, Yash Suthar wrote:
> Replace bmc150_accel_data plain buffer with
> IIO_DECLARE_BUFFER_WITH_TS() that also keep
> timestamp aligned.

Please don't wrap the lines so short. In git commits we usually
aim for 76 columns, IIRC.

> 
> Signed-off-by: Yash Suthar <yashsuthar983@gmail.com>
> ---
>  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

Probably also needs fixes tag since the old array was not properly aligned.


And as a follow-up task (in a separate patch), we can look at replacing
iio_push_to_buffers_with_timestamp() with iio_push_to_buffers_with_ts()
in this driver. Should be trivial in this case.

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

[Yash Suthar]

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

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

[Jonathan Cameron]

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.

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

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

[Jonathan Cameron]

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

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

[Andy Shevchenko]

On Mon, Aug 10, 2026 at 12:18:52AM +0100, Jonathan Cameron wrote:
> 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:

...

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

AI slop?

-- 
With Best Regards,
Andy Shevchenko

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

[Yash Suthar]

On Mon, Aug 10, 2026 at 12:00:05PM +0300, Andy Shevchenko wrote:
> > So what is going on here?
>
> AI slop?

No. I only intended to convert buffer to IIO_DECLARE_BUFFER_WITH_TS() and dont
want to mix it with the existing scan member. Really sorry for that i should 
have asked.

On Mon, Aug 10, 2026 at 12:18:52AM +0100, Jonathan Cameron wrote:
> 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?
> ...
> 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.
>
> 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.

Thanks, Jonathan.
I agree with your analysis.

On type, kept s16 because that was the originally used.
I'll keep __le16 and will merge buffer and scan together as you suggested.
Will move scan to the end with __aligned(IIO_DMA_MINALIGN) and use it for both
trigger handler and fifo flush.
A separate patch for the FIFO stack buffer and keep the
iio_push_to_buffers_with_ts() change in the same.

Yash Suthar