Re: [PATCH] iio: light: ltr390: Add runtime PM support

[Andy Shevchenko]

On Fri, Aug 22, 2025 at 9:03 PM Akshay Jindal <akshayaj.lkd@xxxxxxxxx> wrote:
>
> Implement runtime power management for the LTR390 sensor.
> The device would now autosuspend after 1s of idle time.
> This would save the overall power consumption by the sensor.
>
> Ensure that interrupts continue to be delivered during
> runtime suspend by disabling the sensor only when no
> interrupts are enabled. This prevents loss of events
> while still allowing power savings when IRQs are unused.

Have you tried to enable it as a wake source and disable it?

...

> --- a/drivers/iio/light/ltr390.c
> +++ b/drivers/iio/light/ltr390.c
> @@ -30,6 +30,7 @@
>
>  #include <linux/iio/iio.h>
>  #include <linux/iio/events.h>

> +#include <linux/pm_runtime.h>

Please, preserve ordering.

>  #include <linux/unaligned.h>

(This is here due to historical reasons when mass move from
asm/unaligned to linux/unaligned happened)

...

> +static int ltr390_set_power_state(struct ltr390_data *data, bool on)
> +{
> +       struct device *dev = &data->client->dev;
> +       int ret = 0;

Replace this assignment...

> +       if (on) {
> +               ret = pm_runtime_resume_and_get(dev);
> +               if (ret) {
> +                       dev_err(dev, "failed to resume runtime PM: %d\n", ret);
> +                       return ret;
> +               }
> +       } else {
> +               pm_runtime_mark_last_busy(dev);
> +               pm_runtime_put_autosuspend(dev);

mark_last_busy is redundant.

> +       }

> +       return ret;

...calling return 0; here.

> +}


...

> +       ltr390_set_power_state(data, true);

The boolean parameter is a sign for refactoring to have just two
functions for false and for true cases respectively.

...

>                 default:
> -                       return -EINVAL;
> +                       ret = -EINVAL;
>                 }
> +               break;
>
>         case IIO_CHAN_INFO_INT_TIME:
>                 *val = data->int_time_us;
> -               return IIO_VAL_INT;
> +               ret = IIO_VAL_INT;
> +               break;
>
>         case IIO_CHAN_INFO_SAMP_FREQ:
>                 *val = ltr390_get_samp_freq_or_period(data, LTR390_GET_FREQ);
> -               return IIO_VAL_INT;
> +               ret = IIO_VAL_INT;
> +               break;
>
>         default:
> -               return -EINVAL;
> +               ret = -EINVAL;
>         }
> +
> +handle_pm:
> +       ltr390_set_power_state(data, false);
> +       return ret;


Instead, refactor the code the way that it just will have a wrapper
with power state calls. The change will be much smaller and easier to
understand, review, etc.

...

>  static int ltr390_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
>                                 int val, int val2, long mask)
>  {
> +       int ret;
>         struct ltr390_data *data = iio_priv(indio_dev);
>
> +       ltr390_set_power_state(data, true);
> +
>         switch (mask) {
>         case IIO_CHAN_INFO_SCALE:
> -               if (val2 != 0)
> -                       return -EINVAL;
> -
> -               return ltr390_set_gain(data, val);
> +               if (val2 != 0) {
> +                       ret = -EINVAL;
> +                       goto handle_pm;
> +               }

Ditto.

And so on. I stop here, because this seems needlessly invasive change.
Just refactor first.

...

> +       ret = devm_pm_runtime_enable(dev);
> +       if (ret)
> +               return dev_err_probe(dev, ret,
> +                                       "failed to enable powermanagement\n");

Missed space.

...

> +static _DEFINE_DEV_PM_OPS(ltr390_pm_ops,

Why _DEFINE_... macro? This one is internal to the header.

> +               ltr390_suspend, ltr390_resume,
> +               ltr390_runtime_suspend, ltr390_runtime_resume, NULL);

-- 
With Best Regards,
Andy Shevchenko