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

[Jean-Baptiste Maneyrol]

>
>________________________________________
>From: Jonathan Cameron <jic23@xxxxxxxxxx>
>Sent: Saturday, June 14, 2025 14:53
>To: Jean-Baptiste Maneyrol via B4 Relay <devnull+jean-baptiste.maneyrol.tdk.com@xxxxxxxxxx>
>Cc: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@xxxxxxx>; Lars-Peter Clausen <lars@xxxxxxxxxx>; David Lechner <dlechner@xxxxxxxxxxxx>; Nuno Sá <nuno.sa@xxxxxxxxxx>; Andy Shevchenko <andy@xxxxxxxxxx>; linux-iio@xxxxxxxxxxxxxxx <linux-iio@xxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx <linux-kernel@xxxxxxxxxxxxxxx>
>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@xxxxxxxxxx> wrote:
>
>> From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@xxxxxxx>
>> 
>> Add WoM as accel roc rising x|y|z event.
>> 
>> Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@xxxxxxx>
>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?

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?

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

Thanks for your help here.

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

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.

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

Same here, it was decided long ago in the first series to not switch to guard()
yet but later.

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

Thanks,
JB