Re: [PATCH v4 1/2] iio: imu: inv_icm42600: add WoM support

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 16 Jun 2025 07:42:16 +0000
Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> wrote:

> >
> >________________________________________
> >From: Jonathan Cameron <jic23@kernel.org>
> >Sent: Saturday, June 14, 2025 14:53
> >To: Jean-Baptiste Maneyrol via B4 Relay <devnull+jean-baptiste.maneyrol.tdk.com@kernel.org>
> >Cc: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; Lars-Peter Clausen <lars@metafoo.de>; David Lechner <dlechner@baylibre.com>; Nuno Sá <nuno.sa@analog.com>; Andy Shevchenko <andy@kernel.org>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
> >Subject: Re: [PATCH v4 1/2] iio: imu: inv_icm42600: add WoM support
> > 
> >This Message Is From an External Sender
> >This message came from outside your organization.
> > 
> >On Fri, 13 Jun 2025 09:34:26 +0200
> >Jean-Baptiste Maneyrol via B4 Relay <devnull+jean-baptiste.maneyrol.tdk.com@kernel.org> wrote:
> >  
> >> From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
> >> 
> >> Add WoM as accel roc rising x|y|z event.
> >> 
> >> Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>  
> >Hi Jean-Baptiste.
> >
> >A couple of comments inline.
> >Ideally pull the movement of the timestamp struct to before the DMA safe
> >buffers to a precursor patch.   That is a bit subtle to have hiding in here.
> >
> >The guards thing can be for next time you are doing a cleanup series on this
> >driver if you prefer.
> >
> >Jonathan  
> 
> Hello Jonathan,
> 
> concerning the full driver rewrite asked by Andy to switch to uXX/sXX kernel types,
> can I put it inside this series?

Sure.

> 
> Otherwise, should it be in a separate patch and perhaps with a fixed tag so it
> can be backported to enable automatic backport of further fix patches?

It shouldn't be fixes tagged as that won't be a fix as such.
Backport wise we might need to call it out the first time something is based
on it but the stable maintainers are pretty good at spotting these sorts
of broad mechanical changes that enable a later fix so they'll probably just
pick it up when needed.

> 
> Or can it be after this series is accepted? I would prefer that.

I want the end result with the kernel types, but not that fussed on ordering.
Whilst it may seem churn heavy this stuff is usually reasonably easy to
result when fixes cross such a patch.

I'll catch up with the other thread as I see there is already such a patch
being discussed.
> >>  /**
> >>   *  struct inv_icm42600_state - driver state variables
> >>   *  @lock:		lock for serializing multiple registers access.
> >> @@ -148,9 +156,10 @@ struct inv_icm42600_suspended {
> >>   *  @suspended:		suspended sensors configuration.
> >>   *  @indio_gyro:	gyroscope IIO device.
> >>   *  @indio_accel:	accelerometer IIO device.
> >> - *  @buffer:		data transfer buffer aligned for DMA.
> >> - *  @fifo:		FIFO management structure.
> >>   *  @timestamp:		interrupt timestamps.
> >> + *  @apex:		APEX (Advanced Pedometer and Event detection) management
> >> + *  @fifo:		FIFO management structure.
> >> + *  @buffer:		data transfer buffer aligned for DMA.
> >>   */
> >>  struct inv_icm42600_state {
> >>  	struct mutex lock;
> >> @@ -164,12 +173,13 @@ struct inv_icm42600_state {
> >>  	struct inv_icm42600_suspended suspended;
> >>  	struct iio_dev *indio_gyro;
> >>  	struct iio_dev *indio_accel;
> >> -	uint8_t buffer[2] __aligned(IIO_DMA_MINALIGN);
> >> -	struct inv_icm42600_fifo fifo;
> >>  	struct {
> >>  		int64_t gyro;
> >>  		int64_t accel;
> >>  	} timestamp;  
> >This was a bit subtle and had me going for a minute.
> >The timestamp should never have been at this location in the structure because
> >it's mid way through various regions with forced alignment.  It isn't actually a bug
> >I think though (beyond unnecessary padding) because the fifo struct obeyed c spec rule
> >that anything after it must be aligned to it's largest aligned element which was
> >IIO_DMA_MINALIGN.
> >
> >Maybe move this in a precursor patch where you can talk about whether it was a problem
> >or not?  
> 
> I can move it in a separate patch at the beginning of the series. This fix was asked
> by you to avoid potential hard bugs, but it dates sorry.

yeah. I was wrong I think but definitely subtle kind of code that we don't want
if we can avoid it.  A precursor tidying it up with the reasoning would be good
from a reviewability point of view.

Jonathan