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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75VcgEJaWRPCGV1NOUKd1Kxw90=cyMT6d4mcRvQSWJ99RUg@mail.gmail.com>
Date: Tue, 15 Apr 2025 21:39:06 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: jean-baptiste.maneyrol@....com
Cc: Jonathan Cameron <jic23@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>, 
	David Lechner <dlechner@...libre.com>, Nuno Sá <nuno.sa@...log.com>, 
	Andy Shevchenko <andy@...nel.org>, linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] iio: imu: inv_icm42600: add wakeup functionality
 for Wake-on-Motion

On Tue, Apr 15, 2025 at 5:47 PM Jean-Baptiste Maneyrol via B4 Relay
<devnull+jean-baptiste.maneyrol.tdk.com@...nel.org> wrote:
>
> From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@....com>
>
> 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