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

[Jonathan Cameron <jic23@kernel.org>]

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

> ---
>  drivers/iio/imu/inv_icm42600/inv_icm42600.h        |  54 +++-
>  drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c  | 289 ++++++++++++++++++++-
>  drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c |   2 +-
>  drivers/iio/imu/inv_icm42600/inv_icm42600_core.c   |  58 +++++
>  4 files changed, 395 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> index f893dbe6996506a33eb5d3be47e6765a923665c9..413a15493bcb880dc00b20da3b3168d5addd32a9 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> @@ -135,6 +135,14 @@ struct inv_icm42600_suspended {
>  	bool temp;
>  };
>  
> +struct inv_icm42600_apex {
> +	unsigned int on;
> +	struct {
> +		uint64_t value;
> +		bool enable;
> +	} wom;
> +};
> +
>  /**
>   *  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?

> +	struct inv_icm42600_apex apex;
> +	struct inv_icm42600_fifo fifo;
> +	uint8_t buffer[3] __aligned(IIO_DMA_MINALIGN);
>  };
>
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> index e6cd9dcb0687d19554e63a69dc60f065c58d70ee..9a2089527a9426b70eb796d4e9c234d8804c508b 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c



> @@ -860,6 +911,13 @@ static int inv_icm42600_resume(struct device *dev)
>  	if (ret)
>  		goto out_unlock;
>  
> +	/* restore APEX features */
> +	if (st->apex.wom.enable) {
> +		ret = inv_icm42600_enable_wom(st);
> +		if (ret)
> +			goto out_unlock;

One for another day, but this would definitely benefit from some guard() magic
and there are a few other bits of existing code that would as well.


> +	}
> +
>  	/* restore FIFO data streaming */
>  	if (st->fifo.on) {
>  		inv_sensors_timestamp_reset(&gyro_st->ts);
>