Re: [PATCH v2 2/2] iio: imu: inv_icm42600: add wakeup functionality for Wake-on-Motion

[Andy Shevchenko]

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>
>
> When Wake-on-Motion is on, enable system wakeup and keep chip on for

keep the chip

> waking up system with interrupt.

up the system
with an interrupt.




> +       /* keep chip on and wake-up capable if APEX and wakeup on */
> +       accel_dev = &st->indio_accel->dev;
> +       wakeup = (st->apex.on && device_may_wakeup(accel_dev)) ? true : false;

Redundant ternary.

> +

This blank line should be rather after the accel_dev = ... line.

> +       if (!wakeup) {

Can we use positive conditionals? Generally they are easier to read.

> +               /* disable APEX features and accel if wakeup disabled */
> +               if (st->apex.wom.enable) {
> +                       ret = inv_icm42600_disable_wom(st);
> +                       if (ret)
> +                               goto out_unlock;
> +               }
> +               accel_conf = INV_ICM42600_SENSOR_MODE_OFF;
> +       } else {
> +               /* keep accel on and setup irq for wakeup */
> +               accel_conf = st->conf.accel.mode;
> +               enable_irq_wake(st->irq);
> +               disable_irq(st->irq);
>         }

...

> +       /* check wakeup capability */
> +       accel_dev = &st->indio_accel->dev;
> +       wakeup = (st->apex.on && device_may_wakeup(accel_dev)) ? true : false;
> +
> +       /* restore vddio if cut off or irq state */
> +       if (!wakeup) {

Same comments as per above.

> +               ret = inv_icm42600_enable_regulator_vddio(st);
> +               if (ret)
> +                       goto out_unlock;
> +       } else {
> +               enable_irq(st->irq);
> +               disable_irq_wake(st->irq);
> +       }

...

> +       /* restore APEX features if disabled */
> +       if (!wakeup) {
> +               if (st->apex.wom.enable) {

This is effectively if (foo && bar).

> +                       ret = inv_icm42600_enable_wom(st);
> +                       if (ret)
> +                               goto out_unlock;
> +               }
>         }

...

> @@ -924,6 +955,8 @@ static int inv_icm42600_resume(struct device *dev)
>                 inv_sensors_timestamp_reset(&accel_st->ts);
>                 ret = regmap_write(st->map, INV_ICM42600_REG_FIFO_CONFIG,
>                                    INV_ICM42600_FIFO_CONFIG_STREAM);
> +               if (ret)
> +                       goto out_unlock;
>         }

>  out_unlock:

Stray / unneeded change.

-- 
With Best Regards,
Andy Shevchenko