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

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 10 Jun 2025 14:13:38 +0000
Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> wrote:

> Hello Andy,
> 
> sorry for the very late response, here are my answers.
> 
> Thanks,
> JB
> 
> >________________________________________
> >From: Andy Shevchenko <andy@kernel.org>
> >Sent: Saturday, April 19, 2025 17:39
> >To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>
> >Cc: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen <lars@metafoo.de>; David Lechner <dlechner@baylibre.com>; Nuno Sá <nuno.sa@analog.com>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
> >Subject: Re: [PATCH v3 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, Apr 18, 2025 at 06:19:02PM +0200, Jean-Baptiste Maneyrol via B4 Relay wrote:  
> >> From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
> >> 
> >> Add WoM as accel roc rising x|y|z event.  
> >
> >...
> >  
> >> +static unsigned int inv_icm42600_accel_convert_roc_to_wom(uint64_t roc,
> >> +							  int accel_hz, int accel_uhz)
> >> +{
> >> +	/* 1000/256mg per LSB converted in µm/s² */
> >> +	const unsigned int convert = (1000U * 9807U) / 256U;  
> >
> >Wondering if KILO (or MILLI?) is a good suit here...  
> 
> This one is a little complex, since we have gravity value in mm/s² multiplied
> by 1000 to go to µm/s².
> If you have an idea of better writing that, I will do.
 = (9807U * (MICRO / MILLI)) / 256U;

probably best way to express what you've written.  Rely on compiler working
out the constant for us.

> >> +static int inv_icm42600_accel_disable_wom(struct iio_dev *indio_dev)
> >> +{
> >> +	struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> >> +	struct device *pdev = regmap_get_device(st->map);
> >> +	struct inv_icm42600_sensor_conf conf = INV_ICM42600_SENSOR_CONF_INIT;
> >> +	unsigned int sleep_ms = 0;
> >> +	int ret;
> >> +
> >> +	scoped_guard(mutex, &st->lock) {  
> >  
> >> +		st->apex.wom.enable = false;
> >> +		st->apex.on--;  
> >
> >Hmm... Even if the below fails we consider it successful? Why?  
> 
> If it fails, there is no easy way to restore functioning. Better consider
> everything is disabled to not prevent the chip go into sleep (which will
> disable the feature anyway) and give a chance to reenable it afterward.
> 

Maybe add a comment?


J