linux-kernel - Re: [PATCH v3] iio: light: ltr390: Implement runtime PM support

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: <CAHp75Vc6J+Qm4hsV=PJn9Oyfn5xr9SZLGMagHm9NdFrkk9Y_5A@mail.gmail.com>
Date: Sat, 30 Aug 2025 15:34:06 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Akshay Jindal <akshayaj.lkd@...il.com>
Cc: anshulusr@...il.com, jic23@...nel.org, dlechner@...libre.com, 
	nuno.sa@...log.com, andy@...nel.org, shuah@...nel.org, 
	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] iio: light: ltr390: Implement runtime PM support

On Sat, Aug 30, 2025 at 2:35 PM Akshay Jindal <akshayaj.lkd@...il.com> wrote:
>
> Implement runtime power management for the LTR390 sensor. The device
> autosuspends after 1s of idle time, reducing current consumption from
> 100 µA in active mode to 1 µA in standby mode as per the datasheet.
>
> Ensure that interrupts continue to be delivered with runtime PM.
> Since the LTR390 cannot be used as a wakeup source during runtime
> suspend, therefore increment the runtime PM refcount when enabling
> events and decrement it when disabling events or powering down.
> This prevents event loss while still allowing power savings when IRQs
> are unused.

...

> Changes since v2:
> =================
> 1. Andy's feedback:
> -> Check return value of pm_runtime_resume_and_get().
> -> Do not check return value of pm_runtime_put_autosuspend().
>
> 2. Set data->irq_enabled = true after checking return value of pm_runtime_resume_and_get() only.
>
> Changes since v1:
> ================
> 1. Andy's feedback:
> -> Refactor iio_info callbacks.

> -> Preserve the order of header file includes.

You missed my comment. This has not been addressed yet.

> -> Avoid redundant usage of pm_runtime_mark_last_busy.
> -> Dissolve the ltr390_set_power_state(data, [true|false]) function.
> -> Avoid macro usage which is internal to header file.
> -> Update changelog with reason of not using wakeup as a source
> capability.

...

> @@ -27,6 +27,7 @@
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/regmap.h>
> +#include <linux/pm_runtime.h>

You missed my comment.

...

>  static int ltr390_read_event_config(struct iio_dev *indio_dev,
>                                 const struct iio_chan_spec *chan,
>                                 enum iio_event_type type,
>                                 enum iio_event_direction dir)
>  {
>         struct ltr390_data *data = iio_priv(indio_dev);
> +       struct device *dev = &data->client->dev;
>         int ret, status;
>
> +       ret = pm_runtime_resume_and_get(dev);
> +       if (ret < 0) {
> +               dev_err(dev, "runtime PM failed to resume: %d\n", ret);
> +               return ret;
> +       }
> +
>         ret = regmap_read(data->regmap, LTR390_INT_CFG, &status);
> +
> +       pm_runtime_put_autosuspend(dev);

>         if (ret < 0)
>                 return ret;
> -
>         return FIELD_GET(LTR390_LS_INT_EN, status);

Stray '-' line.

>  }

...

> +static int ltr390_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)
> +{
> +       int ret;
> +       struct ltr390_data *data = iio_priv(indio_dev);
> +       struct device *dev = &data->client->dev;
> +
> +       guard(mutex)(&data->lock);
> +
> +       if (state && !data->irq_enabled) {
> +               ret = pm_runtime_resume_and_get(dev);
> +               if (ret < 0) {
> +                       dev_err(dev, "runtime PM failed to resume: %d\n", ret);
> +                       return ret;
> +               }
> +               data->irq_enabled = true;
> +       }
> +
> +       ret = __ltr390_write_event_config(indio_dev, chan, type, dir, state);
> +
> +       if (!state && data->irq_enabled) {
> +               data->irq_enabled = false;
> +               pm_runtime_put_autosuspend(dev);
> +       }
> +
> +       return ret;
> +}

This looks like overcomplicated and duplicate checks. Just make two
functions with and without IRQ enabling handling.

-- 
With Best Regards,
Andy Shevchenko