Re: [PATCH v2 1/2] iio: imu: inv_icm42600: add WoM support
From: Andy Shevchenko
Date: Tue Apr 15 2025 - 14:19:26 EST
On Tue, Apr 15, 2025 at 5:47 PM 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.
...
> +struct inv_icm42600_apex {
> + unsigned int on;
> + struct {
> + bool enable;
> + uint64_t value;
Why not kernel types? It seems in many places in the driver the
*intXX_t are used instead of simple uXX/sXX.
> + } wom;
Have you run `pahole`? To me it seems swapping the members in the
outer struct will save some memory at run time.
> +};
...
> struct {
> int64_t gyro;
> int64_t accel;
s64 ?
> } timestamp;
...
> + uint8_t buffer[3] __aligned(IIO_DMA_MINALIGN);
u8 ?
...
> +static unsigned int inv_icm42600_accel_convert_roc_to_wom(uint64_t roc,
u64
> + int accel_hz, int accel_uhz)
> +{
> + /* 1000/256mg per LSB converted in m/s² in micro (1000000) */
That 1000000 is redundant, just properly spell the units.
> + const unsigned int convert = (1000U * 9807U) / 256U;
> + uint64_t value;
> + uint64_t freq_uhz;
u64
> + /* return 0 only if roc is 0 */
> + if (roc == 0)
> + return 0;
> +
> + freq_uhz = (uint64_t)accel_hz * 1000000U + (uint64_t)accel_uhz;
u64
MICRO?
> + value = div64_u64(roc * 1000000U, freq_uhz * (uint64_t)convert);
MICRO
> + /* limit value to 8 bits and prevent 0 */
> + return min(255, max(1, value));
Reinvention of the clamp() ?
> +}
...
> +static uint64_t inv_icm42600_accel_convert_wom_to_roc(unsigned int threshold,
> + int accel_hz, int accel_uhz)
Similar comments as per above.
...
> +static int inv_icm42600_accel_enable_wom(struct iio_dev *indio_dev)
> +{
> + struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> + struct inv_icm42600_sensor_state *accel_st = iio_priv(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;
> +
> + ret = pm_runtime_resume_and_get(pdev);
> + if (ret)
> + return ret;
> +
> + scoped_guard(mutex, &st->lock) {
> + /* turn on accel sensor */
> + conf.mode = accel_st->power_mode;
> + conf.filter = accel_st->filter;
> + ret = inv_icm42600_set_accel_conf(st, &conf, &sleep_ms);
> + if (ret)
> + goto error_suspend;
> + }
> + if (sleep_ms)
Do you need this check?
> + msleep(sleep_ms);
> +
> + scoped_guard(mutex, &st->lock) {
> + ret = inv_icm42600_enable_wom(st);
> + if (ret)
> + goto error_suspend;
> + st->apex.on++;
> + st->apex.wom.enable = true;
> + }
> +
> + return 0;
> +
> +error_suspend:
> + pm_runtime_mark_last_busy(pdev);
> + pm_runtime_put_autosuspend(pdev);
> + return ret;
> +}
> +
> +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--;
> + ret = inv_icm42600_disable_wom(st);
> + if (ret)
> + break;
> + /* turn off accel sensor if not used */
> + if (!st->apex.on && !iio_buffer_enabled(indio_dev)) {
> + conf.mode = INV_ICM42600_SENSOR_MODE_OFF;
> + ret = inv_icm42600_set_accel_conf(st, &conf, &sleep_ms);
> + if (ret)
> + break;
> + }
> + }
> + if (sleep_ms)
Ditto.
> + msleep(sleep_ms);
> + pm_runtime_mark_last_busy(pdev);
> + pm_runtime_put_autosuspend(pdev);
> +
> + return ret;
> +}
...
> +void inv_icm42600_accel_handle_events(struct iio_dev *indio_dev,
> + unsigned int status2, unsigned int status3,
> + int64_t timestamp)
Okay, I believe here int64_t is inherited from IIO definitions.
> +{
> + struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> + uint64_t ev_code;
u64.
> + /* handle WoM event */
> + if (st->apex.wom.enable && (status2 & INV_ICM42600_INT_STATUS2_WOM_INT)) {
> + ev_code = IIO_MOD_EVENT_CODE(IIO_ACCEL, 0, IIO_MOD_X_OR_Y_OR_Z,
> + IIO_EV_TYPE_ROC, IIO_EV_DIR_RISING);
> + iio_push_event(indio_dev, ev_code, timestamp);
> + }
> +}
...
> +static int inv_icm42600_accel_write_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + bool state)
> +{
> + struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> +
> + /* handle WoM (roc rising) event */
> + if (type == IIO_EV_TYPE_ROC && dir == IIO_EV_DIR_RISING) {
Invert the conditional to reduce indentation of the below. It will
also follow the traditional pattern, i.e. checking for the errors
first.
> + scoped_guard(mutex, &st->lock) {
> + if (st->apex.wom.enable == state)
> + return 0;
> + }
> + if (state)
> + return inv_icm42600_accel_enable_wom(indio_dev);
> + else
Redundant 'else', but in this case for the sake of symmetry it can be left.
> + return inv_icm42600_accel_disable_wom(indio_dev);
> + }
> +
> + return -EINVAL;
> +}
...
> + if (type == IIO_EV_TYPE_ROC && dir == IIO_EV_DIR_RISING) {
Invert the conditional.
> + /* return value in micro */
> + *val = div_u64_rem(st->apex.wom.value, 1000000U, &rem);
> + *val2 = rem;
> + return IIO_VAL_INT_PLUS_MICRO;
> + }
> +
> + return -EINVAL;
...
> +static int inv_icm42600_accel_write_event_value(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info,
> + int val, int val2)
> +{
> + struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> + struct device *dev = regmap_get_device(st->map);
> + uint64_t value;
u64
> + unsigned int accel_hz, accel_uhz;
> + int ret;
> +
> + /* handle WoM (roc rising) event value */
> + if (type == IIO_EV_TYPE_ROC && dir == IIO_EV_DIR_RISING) {
Invert the conditional.
> + if (val < 0 || val2 < 0)
> + return -EINVAL;
This can be checked even before anything else, but see above. With
that it will automatically get on the same indentation level as
previous one.
> + value = (uint64_t)val * 1000000ULL + (uint64_t)val2;
MICRO.
ULL is not needed, the first one already ULL.
> + pm_runtime_get_sync(dev);
> + scoped_guard(mutex, &st->lock) {
> + ret = inv_icm42600_accel_read_odr(st, &accel_hz, &accel_uhz);
> + if (ret >= 0)
> + ret = inv_icm42600_accel_set_wom_threshold(st, value,
> + accel_hz, accel_uhz);
> + }
> + pm_runtime_mark_last_busy(dev);
> + pm_runtime_put_autosuspend(dev);
> + return ret;
> + }
> +
> + return -EINVAL;
> +}
--
With Best Regards,
Andy Shevchenko