Re: [PATCH v5 2/3] iio: imu: inv_icm42600: add WoM support

[David Lechner <dlechner@baylibre.com>]

On Mon, Jun 23, 2025 at 6:56 AM 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.
>

...

> +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)
> +               msleep(sleep_ms);
> +
> +       scoped_guard(mutex, &st->lock) {
> +               ret = inv_icm42600_enable_wom(st);
> +               if (ret)
> +                       break;

I reviewed this from the bottom up, so see comments on similar code below.

> +               st->apex.on++;
> +               st->apex.wom.enable = true;
> +       }
> +       if (ret)
> +               goto error_suspend;
> +
> +       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) {
> +               /*
> +                * Consider that turning off WoM is always working to avoid
> +                * blocking the chip in on mode and prevent going back to sleep.
> +                * If there is an error, the chip will anyway go back to sleep
> +                * and the feature will not work anymore.
> +                */
> +               st->apex.wom.enable = false;
> +               st->apex.on--;
> +               ret = inv_icm42600_disable_wom(st);
> +               if (ret)
> +                       break;

The fact that scoped_guard() uses a for loop is an implementation
detail so using break here makes this look like improper C code. I
think this would be better to split out the protected section to a
separate function and just use the regular guard() macro.

> +               /* 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)
> +               msleep(sleep_ms);

Probably don't need the if here. msleep() won't sleep if we pass 0 to it.

> +       pm_runtime_mark_last_busy(pdev);
> +       pm_runtime_put_autosuspend(pdev);
> +
> +       return ret;
> +}
> +

...

> +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 only WoM (roc rising) event */
> +       if (type != IIO_EV_TYPE_ROC || dir != IIO_EV_DIR_RISING)
> +               return -EINVAL;
> +
> +       scoped_guard(mutex, &st->lock) {
> +               if (st->apex.wom.enable == state)
> +                       return 0;
> +       }
> +
> +       if (state)
> +               return inv_icm42600_accel_enable_wom(indio_dev);
> +       else

This is redundant else and can be removed.

> +               return inv_icm42600_accel_disable_wom(indio_dev);
> +}
> +

...

> +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);
> +       u64 value;
> +       unsigned int accel_hz, accel_uhz;
> +       int ret;
> +
> +       /* handle only WoM (roc rising) event value */
> +       if (type != IIO_EV_TYPE_ROC || dir != IIO_EV_DIR_RISING)
> +               return -EINVAL;
> +
> +       if (val < 0 || val2 < 0)
> +               return -EINVAL;
> +
> +       value = (u64)val * MICRO + (u64)val2;
> +       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);

At least we aren't using break here, but still this could be better
split out to a separate function so that we can use the regular return
on error pattern.

> +       }
> +       pm_runtime_mark_last_busy(dev);
> +       pm_runtime_put_autosuspend(dev);
> +
> +       return ret;
> +}
> +