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

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

[Akshay Jindal]

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.

Signed-off-by: Akshay Jindal <akshayaj.lkd@gmail.com>
---

Testing summary:
================
-> Tested on Raspberrypi 4B. Following tests were performed.
1. Verified that /sys/bus/i2c/devices/i2c-1/1-0053/power/control contains ‘auto’ value.
2. Verified the /sys/bus/i2c/devices/i2c-1/1-0053/power/autosuspend_delay_ms contains 1000 which is assigned by the driver.
3. Changed the autosuspend_delay_ms value from 1000 to 2000ms and verified it.
	3.1 Verified through the timestamp that whatever autosuspend_delay_ms is set, it is being honoured.
4. Verified that runtime_suspend and runtime_resume callbacks are called whenever any IO is done on a channel attribute.
	4.1 Verified that power/runtime_status first becomes active and then becomes suspended.
	4.2 Verified that power/runtime_active_time keeps on increasing with a delta of autosuspend_delay_ms.

Interrupt Handling Verification:
--------------------------------
1. Verified that when interrupts are enabled on the device, then the device does not get put in standby mode and keeps sampling.
	a. As a result interrupts are delivered to the driver and are handled.
2. Verified that when interrupts are disabled, the device is put in standby mode and stops sampling.
	a.Since there is no sampling, so no IRQs will be generated. They are only generated when the device is resumed due to I/O on some sysfs attribute from userspace.

 drivers/iio/light/ltr390.c | 246 +++++++++++++++++++++++++++++--------
 1 file changed, 193 insertions(+), 53 deletions(-)

diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
index 2e1cf62e8201..9e2f33a401f2 100644
--- 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>
 
 #include <linux/unaligned.h>
 
@@ -105,6 +106,7 @@ struct ltr390_data {
 	enum ltr390_mode mode;
 	int gain;
 	int int_time_us;
+	bool irq_enabled;
 };
 
 static const struct regmap_range ltr390_readable_reg_ranges[] = {
@@ -154,6 +156,25 @@ static const int ltr390_samp_freq_table[][2] = {
 		[7] = { 500, 2000 },
 };
 
+static int ltr390_set_power_state(struct ltr390_data *data, bool on)
+{
+	struct device *dev = &data->client->dev;
+	int ret = 0;
+
+	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);
+	}
+
+	return ret;
+}
+
 static int ltr390_register_read(struct ltr390_data *data, u8 register_address)
 {
 	struct device *dev = &data->client->dev;
@@ -223,61 +244,76 @@ static int ltr390_read_raw(struct iio_dev *iio_device,
 	struct ltr390_data *data = iio_priv(iio_device);
 
 	guard(mutex)(&data->lock);
+
+	ltr390_set_power_state(data, true);
+
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
 		switch (chan->type) {
 		case IIO_UVINDEX:
 			ret = ltr390_set_mode(data, LTR390_SET_UVS_MODE);
 			if (ret < 0)
-				return ret;
+				goto handle_pm;
 
 			ret = ltr390_register_read(data, LTR390_UVS_DATA);
 			if (ret < 0)
-				return ret;
+				goto handle_pm;
 			break;
 
 		case IIO_LIGHT:
 			ret = ltr390_set_mode(data, LTR390_SET_ALS_MODE);
 			if (ret < 0)
-				return ret;
+				goto handle_pm;
 
 			ret = ltr390_register_read(data, LTR390_ALS_DATA);
 			if (ret < 0)
-				return ret;
+				goto handle_pm;
 			break;
 
 		default:
-			return -EINVAL;
+			ret = -EINVAL;
+			goto handle_pm;
 		}
 		*val = ret;
-		return IIO_VAL_INT;
+		ret = IIO_VAL_INT;
+		break;
+
 	case IIO_CHAN_INFO_SCALE:
 		switch (chan->type) {
 		case IIO_UVINDEX:
 			*val = LTR390_WINDOW_FACTOR * LTR390_FRACTIONAL_PRECISION;
 			*val2 = ltr390_counts_per_uvi(data);
-			return IIO_VAL_FRACTIONAL;
+			ret = IIO_VAL_FRACTIONAL;
+			break;
 
 		case IIO_LIGHT:
 			*val = LTR390_WINDOW_FACTOR * 6 * 100;
 			*val2 = data->gain * data->int_time_us;
-			return IIO_VAL_FRACTIONAL;
+			ret = IIO_VAL_FRACTIONAL;
+			break;
 
 		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;
 }
 
 /* integration time in us */
@@ -418,30 +454,43 @@ static int ltr390_read_avail(struct iio_dev *indio_dev, struct iio_chan_spec con
 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;
+		}
 
+		ret = ltr390_set_gain(data, val);
+		break;
 	case IIO_CHAN_INFO_INT_TIME:
-		if (val2 != 0)
-			return -EINVAL;
-
-		return ltr390_set_int_time(data, val);
+		if (val2 != 0) {
+			ret = -EINVAL;
+			goto handle_pm;
+		}
 
+		ret = ltr390_set_int_time(data, val);
+		break;
 	case IIO_CHAN_INFO_SAMP_FREQ:
-		if (val2 != 0)
-			return -EINVAL;
-
-		return ltr390_set_samp_freq(data, val);
+		if (val2 != 0) {
+			ret = -EINVAL;
+			goto handle_pm;
+		}
 
+		ret = ltr390_set_samp_freq(data, val);
+		break;
 	default:
-		return -EINVAL;
+		ret = -EINVAL;
 	}
+
+handle_pm:
+	ltr390_set_power_state(data, false);
+	return ret;
 }
 
 static int ltr390_read_intr_prst(struct ltr390_data *data, int *val)
@@ -534,16 +583,24 @@ static int ltr390_read_event_value(struct iio_dev *indio_dev,
 				enum iio_event_info info,
 				int *val, int *val2)
 {
+	int ret;
+	struct ltr390_data *data = iio_priv(indio_dev);
+
+	ltr390_set_power_state(data, true);
+
 	switch (info) {
 	case IIO_EV_INFO_VALUE:
-		return ltr390_read_threshold(indio_dev, dir, val, val2);
-
+		ret = ltr390_read_threshold(indio_dev, dir, val, val2);
+		break;
 	case IIO_EV_INFO_PERIOD:
-		return ltr390_read_intr_prst(iio_priv(indio_dev), val);
-
+		ret = ltr390_read_intr_prst(iio_priv(indio_dev), val);
+		break;
 	default:
-		return -EINVAL;
+		ret = -EINVAL;
 	}
+
+	ltr390_set_power_state(data, false);
+	return ret;
 }
 
 static int ltr390_write_event_value(struct iio_dev *indio_dev,
@@ -553,22 +610,35 @@ static int ltr390_write_event_value(struct iio_dev *indio_dev,
 				enum iio_event_info info,
 				int val, int val2)
 {
+	int ret;
+	struct ltr390_data *data = iio_priv(indio_dev);
+
+	ltr390_set_power_state(data, true);
+
 	switch (info) {
 	case IIO_EV_INFO_VALUE:
-		if (val2 != 0)
-			return -EINVAL;
-
-		return ltr390_write_threshold(indio_dev, dir, val, val2);
+		if (val2 != 0) {
+			ret = -EINVAL;
+			goto handle_pm;
+		}
 
+		ret = ltr390_write_threshold(indio_dev, dir, val, val2);
+		break;
 	case IIO_EV_INFO_PERIOD:
-		if (val2 != 0)
-			return -EINVAL;
-
-		return ltr390_write_intr_prst(iio_priv(indio_dev), val);
+		if (val2 != 0) {
+			ret = -EINVAL;
+			goto handle_pm;
+		}
 
+		ret = ltr390_write_intr_prst(iio_priv(indio_dev), val);
+		break;
 	default:
-		return -EINVAL;
+		ret = -EINVAL;
 	}
+
+handle_pm:
+	ltr390_set_power_state(data, false);
+	return ret;
 }
 
 static int ltr390_read_event_config(struct iio_dev *indio_dev,
@@ -579,7 +649,12 @@ static int ltr390_read_event_config(struct iio_dev *indio_dev,
 	struct ltr390_data *data = iio_priv(indio_dev);
 	int ret, status;
 
+	ltr390_set_power_state(data, true);
+
 	ret = regmap_read(data->regmap, LTR390_INT_CFG, &status);
+
+	ltr390_set_power_state(data, false);
+
 	if (ret < 0)
 		return ret;
 
@@ -595,32 +670,43 @@ static int ltr390_write_event_config(struct iio_dev *indio_dev,
 	struct ltr390_data *data = iio_priv(indio_dev);
 	int ret;
 
-	if (!state)
-		return regmap_clear_bits(data->regmap, LTR390_INT_CFG, LTR390_LS_INT_EN);
+	ltr390_set_power_state(data, true);
 
 	guard(mutex)(&data->lock);
+
+	if (!state) {
+		ret = regmap_clear_bits(data->regmap, LTR390_INT_CFG, LTR390_LS_INT_EN);
+		data->irq_enabled = false;
+		goto handle_pm;
+	}
+
 	ret = regmap_set_bits(data->regmap, LTR390_INT_CFG, LTR390_LS_INT_EN);
 	if (ret < 0)
-		return ret;
+		goto handle_pm;
+	data->irq_enabled = true;
 
 	switch (chan->type) {
 	case IIO_LIGHT:
 		ret = ltr390_set_mode(data, LTR390_SET_ALS_MODE);
 		if (ret < 0)
-			return ret;
-
-		return regmap_clear_bits(data->regmap, LTR390_INT_CFG, LTR390_LS_INT_SEL_UVS);
+			goto handle_pm;
 
+		ret = regmap_clear_bits(data->regmap, LTR390_INT_CFG, LTR390_LS_INT_SEL_UVS);
+		break;
 	case IIO_UVINDEX:
 		ret = ltr390_set_mode(data, LTR390_SET_UVS_MODE);
 		if (ret < 0)
-			return ret;
-
-		return regmap_set_bits(data->regmap, LTR390_INT_CFG, LTR390_LS_INT_SEL_UVS);
+			goto handle_pm;
 
+		ret = regmap_set_bits(data->regmap, LTR390_INT_CFG, LTR390_LS_INT_SEL_UVS);
+		break;
 	default:
-		return -EINVAL;
+		ret = -EINVAL;
 	}
+
+handle_pm:
+	ltr390_set_power_state(data, false);
+	return ret;
 }
 
 static int ltr390_debugfs_reg_access(struct iio_dev *indio_dev,
@@ -628,13 +714,19 @@ static int ltr390_debugfs_reg_access(struct iio_dev *indio_dev,
 						unsigned int *readval)
 {
 	struct ltr390_data *data = iio_priv(indio_dev);
+	int ret;
 
 	guard(mutex)(&data->lock);
 
+	ltr390_set_power_state(data, true);
+
 	if (readval)
-		return regmap_read(data->regmap, reg, readval);
+		ret = regmap_read(data->regmap, reg, readval);
+	else
+		ret = regmap_write(data->regmap, reg, writeval);
 
-	return regmap_write(data->regmap, reg, writeval);
+	ltr390_set_power_state(data, false);
+	return ret;
 }
 
 static const struct iio_info ltr390_info = {
@@ -690,12 +782,32 @@ static void ltr390_powerdown(void *priv)
 	if (regmap_clear_bits(data->regmap, LTR390_INT_CFG,
 				LTR390_LS_INT_EN) < 0)
 		dev_err(&data->client->dev, "failed to disable interrupts\n");
+	data->irq_enabled = false;
 
 	if (regmap_clear_bits(data->regmap, LTR390_MAIN_CTRL,
 			LTR390_SENSOR_ENABLE) < 0)
 		dev_err(&data->client->dev, "failed to disable sensor\n");
 }
 
+static int ltr390_pm_init(struct ltr390_data *data)
+{
+	int ret;
+	struct device *dev = &data->client->dev;
+
+	ret = pm_runtime_set_active(dev);
+	if (ret)
+		return ret;
+
+	ret = devm_pm_runtime_enable(dev);
+	if (ret)
+		return dev_err_probe(dev, ret,
+					"failed to enable powermanagement\n");
+
+	pm_runtime_set_autosuspend_delay(dev, 1000);
+	pm_runtime_use_autosuspend(dev);
+	return 0;
+}
+
 static int ltr390_probe(struct i2c_client *client)
 {
 	struct ltr390_data *data;
@@ -708,6 +820,8 @@ static int ltr390_probe(struct i2c_client *client)
 	if (!indio_dev)
 		return -ENOMEM;
 
+	i2c_set_clientdata(client, indio_dev);
+
 	data = iio_priv(indio_dev);
 	data->regmap = devm_regmap_init_i2c(client, &ltr390_regmap_config);
 	if (IS_ERR(data->regmap))
@@ -721,6 +835,8 @@ static int ltr390_probe(struct i2c_client *client)
 	data->gain = 3;
 	/* default mode for ltr390 is ALS mode */
 	data->mode = LTR390_SET_ALS_MODE;
+	/* default irq_enabled is false */
+	data->irq_enabled = false;
 
 	mutex_init(&data->lock);
 
@@ -763,6 +879,7 @@ static int ltr390_probe(struct i2c_client *client)
 					     "request irq (%d) failed\n", client->irq);
 	}
 
+	ltr390_pm_init(data);
 	return devm_iio_device_register(dev, indio_dev);
 }
 
@@ -784,7 +901,30 @@ static int ltr390_resume(struct device *dev)
 				LTR390_SENSOR_ENABLE);
 }
 
-static DEFINE_SIMPLE_DEV_PM_OPS(ltr390_pm_ops, ltr390_suspend, ltr390_resume);
+static int ltr390_runtime_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct ltr390_data *data = iio_priv(indio_dev);
+
+	guard(mutex)(&data->lock);
+	if (data->irq_enabled)
+		return 0;
+	return regmap_clear_bits(data->regmap, LTR390_MAIN_CTRL,
+				LTR390_SENSOR_ENABLE);
+}
+
+static int ltr390_runtime_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct ltr390_data *data = iio_priv(indio_dev);
+
+	return regmap_set_bits(data->regmap, LTR390_MAIN_CTRL,
+				LTR390_SENSOR_ENABLE);
+}
+
+static _DEFINE_DEV_PM_OPS(ltr390_pm_ops,
+		ltr390_suspend, ltr390_resume,
+		ltr390_runtime_suspend, ltr390_runtime_resume, NULL);
 
 static const struct i2c_device_id ltr390_id[] = {
 	{ "ltr390" },
@@ -802,7 +942,7 @@ static struct i2c_driver ltr390_driver = {
 	.driver = {
 		.name = "ltr390",
 		.of_match_table = ltr390_of_table,
-		.pm = pm_sleep_ptr(&ltr390_pm_ops),
+		.pm = pm_ptr(&ltr390_pm_ops),
 	},
 	.probe = ltr390_probe,
 	.id_table = ltr390_id,
-- 
2.43.0

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

[Andy Shevchenko]

On Fri, Aug 22, 2025 at 9:03 PM Akshay Jindal <akshayaj.lkd@gmail.com> 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

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

[David Lechner]

On 8/22/25 1:03 PM, Akshay Jindal 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.

How much power does it actually save?

> 
> 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.
> 
> Signed-off-by: Akshay Jindal <akshayaj.lkd@gmail.com>
> ---
> 
> Testing summary:
> ================
> -> Tested on Raspberrypi 4B. Following tests were performed.
> 1. Verified that /sys/bus/i2c/devices/i2c-1/1-0053/power/control contains ‘auto’ value.
> 2. Verified the /sys/bus/i2c/devices/i2c-1/1-0053/power/autosuspend_delay_ms contains 1000 which is assigned by the driver.
> 3. Changed the autosuspend_delay_ms value from 1000 to 2000ms and verified it.
> 	3.1 Verified through the timestamp that whatever autosuspend_delay_ms is set, it is being honoured.
> 4. Verified that runtime_suspend and runtime_resume callbacks are called whenever any IO is done on a channel attribute.
> 	4.1 Verified that power/runtime_status first becomes active and then becomes suspended.
> 	4.2 Verified that power/runtime_active_time keeps on increasing with a delta of autosuspend_delay_ms.
> 
> Interrupt Handling Verification:
> --------------------------------
> 1. Verified that when interrupts are enabled on the device, then the device does not get put in standby mode and keeps sampling.
> 	a. As a result interrupts are delivered to the driver and are handled.
> 2. Verified that when interrupts are disabled, the device is put in standby mode and stops sampling.
> 	a.Since there is no sampling, so no IRQs will be generated. They are only generated when the device is resumed due to I/O on some sysfs attribute from userspace.
> 
>  drivers/iio/light/ltr390.c | 246 +++++++++++++++++++++++++++++--------
>  1 file changed, 193 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
> index 2e1cf62e8201..9e2f33a401f2 100644
> --- 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>
>  
>  #include <linux/unaligned.h>
>  
> @@ -105,6 +106,7 @@ struct ltr390_data {
>  	enum ltr390_mode mode;
>  	int gain;
>  	int int_time_us;
> +	bool irq_enabled;
>  };
>  
>  static const struct regmap_range ltr390_readable_reg_ranges[] = {
> @@ -154,6 +156,25 @@ static const int ltr390_samp_freq_table[][2] = {
>  		[7] = { 500, 2000 },
>  };
>  
> +static int ltr390_set_power_state(struct ltr390_data *data, bool on)

This function does completely different things depending on if the
last argument is true or false. It should just be two separate
functions and avoid the parameter and the if statement.

> +{
> +	struct device *dev = &data->client->dev;
> +	int ret = 0;
> +
> +	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);
> +	}
> +
> +	return ret;
> +}
> +

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

[Andy Shevchenko]

On Sat, Aug 23, 2025 at 12:10 PM Akshay Jindal <akshayaj.lkd@gmail.com> wrote:
> On Sat, Aug 23, 2025 at 1:41 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Fri, Aug 22, 2025 at 9:03 PM Akshay Jindal <akshayaj.lkd@gmail.com> wrote:

...

> > >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?
>
> Yes, before coming onto this approach, I had given it a thought and hence did some R&D for this.
> Official documentation here & here talks about power.wakeup being an attribute for system wakeup
> NOT runtime. This checks out because in the complete IIO subsystem, there is not a single driver
> which implements both runtime PM and wakeup functionality.
> As of now there are 4 drivers in the whole of IIO, which enable this wakeup capability
> and that too are only using it in system_suspend callbacks. Hence I had to come up with this approach.

Please, make sure in the next version the summary of the above is
present in the commit message or comment block.

...

> > > +               pm_runtime_mark_last_busy(dev);
> > > +               pm_runtime_put_autosuspend(dev);
> >
> > mark_last_busy is redundant.
> >
> Pardon me here, but I am not able to see the redundancy.
> I think this should be very much there before we call _put_autosuspend which further calls
> autosuspend_expiration() which reads this last_busy field.
> Did you mean dev->power.last_busy is being updated elsewhere too? or something else?
> Can you please clarify?

https://elixir.bootlin.com/linux/v6.17-rc2/source/include/linux/pm_runtime.h#L603


-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Cameron]

On Fri, 22 Aug 2025 23:33:26 +0530
Akshay Jindal <akshayaj.lkd@gmail.com> 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.

Wrap closer to 75 chars.

Main comment that follows is that runtime pm is reference
counted.  That is you can take multiple references in different
paths at the same time and only when they are all released does
the put actually cause it to be suspended.  So for events
just grab an extra reference.  Lots of drivers do this in the
buffer enables for example - probably some in event handling
as well I just can't remember which one right now and am too lazy
to go find out.


> 
> Signed-off-by: Akshay Jindal <akshayaj.lkd@gmail.com>
> ---
> 
> Testing summary:
> ================
> -> Tested on Raspberrypi 4B. Following tests were performed.  
> 1. Verified that /sys/bus/i2c/devices/i2c-1/1-0053/power/control contains ‘auto’ value.
> 2. Verified the /sys/bus/i2c/devices/i2c-1/1-0053/power/autosuspend_delay_ms contains 1000 which is assigned by the driver.
> 3. Changed the autosuspend_delay_ms value from 1000 to 2000ms and verified it.
> 	3.1 Verified through the timestamp that whatever autosuspend_delay_ms is set, it is being honoured.
> 4. Verified that runtime_suspend and runtime_resume callbacks are called whenever any IO is done on a channel attribute.
> 	4.1 Verified that power/runtime_status first becomes active and then becomes suspended.
> 	4.2 Verified that power/runtime_active_time keeps on increasing with a delta of autosuspend_delay_ms.
> 
> Interrupt Handling Verification:
> --------------------------------
> 1. Verified that when interrupts are enabled on the device, then the device does not get put in standby mode and keeps sampling.
> 	a. As a result interrupts are delivered to the driver and are handled.
> 2. Verified that when interrupts are disabled, the device is put in standby mode and stops sampling.
> 	a.Since there is no sampling, so no IRQs will be generated. They are only generated when the device is resumed due to I/O on some sysfs attribute from userspace.
> 
>  drivers/iio/light/ltr390.c | 246 +++++++++++++++++++++++++++++--------
>  1 file changed, 193 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
> index 2e1cf62e8201..9e2f33a401f2 100644
> --- 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>
>  
>  #include <linux/unaligned.h>
>  
> @@ -105,6 +106,7 @@ struct ltr390_data {
>  	enum ltr390_mode mode;
>  	int gain;
>  	int int_time_us;
> +	bool irq_enabled;
>  };
>  
>  static const struct regmap_range ltr390_readable_reg_ranges[] = {
> @@ -154,6 +156,25 @@ static const int ltr390_samp_freq_table[][2] = {
>  		[7] = { 500, 2000 },
>  };
>  
> +static int ltr390_set_power_state(struct ltr390_data *data, bool on)
> +{
> +	struct device *dev = &data->client->dev;
> +	int ret = 0;
> +
> +	if (on) {
> +		ret = pm_runtime_resume_and_get(dev);

David touched on this.  Put the calls inline - there is no benefit to this
function as it calls one of two unrelated paths at each call site.

> +		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);
> +	}
> +
> +	return ret;
> +}
> +
>  static int ltr390_register_read(struct ltr390_data *data, u8 register_address)
>  {
>  	struct device *dev = &data->client->dev;
> @@ -223,61 +244,76 @@ static int ltr390_read_raw(struct iio_dev *iio_device,
>  	struct ltr390_data *data = iio_priv(iio_device);
>  
>  	guard(mutex)(&data->lock);
> +
> +	ltr390_set_power_state(data, true);
Can fail so you should check.

Wrap ltr390_register_read() in an outer function that does the powerstate
management.  Then no need to have all the gotos in here.

I am intending to see what appetite there is for a ACQUIRE() conditional
guard set of macros around autosuspend, but for now a separate wrapper function
is the cleanest path. Be careful with the locking though.


> +
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
>  		switch (chan->type) {
>  		case IIO_UVINDEX:
>  			ret = ltr390_set_mode(data, LTR390_SET_UVS_MODE);
>  			if (ret < 0)
> -				return ret;
> +				goto handle_pm;
>  
>  			ret = ltr390_register_read(data, LTR390_UVS_DATA);
>  			if (ret < 0)
> -				return ret;
> +				goto handle_pm;
>  			break;
>  
>  		case IIO_LIGHT:
>  			ret = ltr390_set_mode(data, LTR390_SET_ALS_MODE);
>  			if (ret < 0)
> -				return ret;
> +				goto handle_pm;
>  
>  			ret = ltr390_register_read(data, LTR390_ALS_DATA);
>  			if (ret < 0)
> -				return ret;
> +				goto handle_pm;
>  			break;
>  
>  		default:
> -			return -EINVAL;
> +			ret = -EINVAL;
> +			goto handle_pm;
>  		}
>  		*val = ret;
> -		return IIO_VAL_INT;
> +		ret = IIO_VAL_INT;
> +		break;
> +
>  	case IIO_CHAN_INFO_SCALE:
>  		switch (chan->type) {
>  		case IIO_UVINDEX:
>  			*val = LTR390_WINDOW_FACTOR * LTR390_FRACTIONAL_PRECISION;
>  			*val2 = ltr390_counts_per_uvi(data);
> -			return IIO_VAL_FRACTIONAL;
> +			ret = IIO_VAL_FRACTIONAL;
> +			break;
>  
>  		case IIO_LIGHT:
>  			*val = LTR390_WINDOW_FACTOR * 6 * 100;
>  			*val2 = data->gain * data->int_time_us;
> -			return IIO_VAL_FRACTIONAL;
> +			ret = IIO_VAL_FRACTIONAL;
> +			break;
>  
>  		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;
>  }

> @@ -595,32 +670,43 @@ static int ltr390_write_event_config(struct iio_dev *indio_dev,
>  	struct ltr390_data *data = iio_priv(indio_dev);
>  	int ret;
>  
> -	if (!state)
> -		return regmap_clear_bits(data->regmap, LTR390_INT_CFG, LTR390_LS_INT_EN);
> +	ltr390_set_power_state(data, true);
>  
>  	guard(mutex)(&data->lock);
> +
> +	if (!state) {
> +		ret = regmap_clear_bits(data->regmap, LTR390_INT_CFG, LTR390_LS_INT_EN);
> +		data->irq_enabled = false;

Just take an extra reference to runtime pm on enable of event and put it disable.
Then no need for special handling with a local flag etc.



> +static int ltr390_pm_init(struct ltr390_data *data)
> +{
> +	int ret;
> +	struct device *dev = &data->client->dev;
> +
> +	ret = pm_runtime_set_active(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_pm_runtime_enable(dev);

devm_pm_runtime_set_active_enabled() I think would work here.

That shortens this setup enough I'd not bother with this function.

> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +					"failed to enable powermanagement\n");
> +
> +	pm_runtime_set_autosuspend_delay(dev, 1000);
> +	pm_runtime_use_autosuspend(dev);
> +	return 0;
> +}
> +
>  static int ltr390_probe(struct i2c_client *client)
>  {
>  	struct ltr390_data *data;
> @@ -708,6 +820,8 @@ static int ltr390_probe(struct i2c_client *client)
>  	if (!indio_dev)
>  		return -ENOMEM;
>  
> +	i2c_set_clientdata(client, indio_dev);
> +
>  	data = iio_priv(indio_dev);
>  	data->regmap = devm_regmap_init_i2c(client, &ltr390_regmap_config);
>  	if (IS_ERR(data->regmap))
> @@ -721,6 +835,8 @@ static int ltr390_probe(struct i2c_client *client)
>  	data->gain = 3;
>  	/* default mode for ltr390 is ALS mode */
>  	data->mode = LTR390_SET_ALS_MODE;
> +	/* default irq_enabled is false */
> +	data->irq_enabled = false;
>  
>  	mutex_init(&data->lock);
>  
> @@ -763,6 +879,7 @@ static int ltr390_probe(struct i2c_client *client)
>  					     "request irq (%d) failed\n", client->irq);
>  	}
>  
> +	ltr390_pm_init(data);
>  	return devm_iio_device_register(dev, indio_dev);
>  }
>  
> @@ -784,7 +901,30 @@ static int ltr390_resume(struct device *dev)
>  				LTR390_SENSOR_ENABLE);
>  }
>  
> -static DEFINE_SIMPLE_DEV_PM_OPS(ltr390_pm_ops, ltr390_suspend, ltr390_resume);
> +static int ltr390_runtime_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct ltr390_data *data = iio_priv(indio_dev);
> +
> +	guard(mutex)(&data->lock);
> +	if (data->irq_enabled)

As above. When you have events enabled, grab an extra reference and don't
put it until you disable the event. That way we never enter
runtime_suspend whilst they are enabled.

> +		return 0;
> +	return regmap_clear_bits(data->regmap, LTR390_MAIN_CTRL,
> +				LTR390_SENSOR_ENABLE);
> +}
> +
> +static int ltr390_runtime_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct ltr390_data *data = iio_priv(indio_dev);
> +
> +	return regmap_set_bits(data->regmap, LTR390_MAIN_CTRL,
> +				LTR390_SENSOR_ENABLE);
> +}
> +
> +static _DEFINE_DEV_PM_OPS(ltr390_pm_ops,
> +		ltr390_suspend, ltr390_resume,
> +		ltr390_runtime_suspend, ltr390_runtime_resume, NULL);
>  
>  static const struct i2c_device_id ltr390_id[] = {
>  	{ "ltr390" },
> @@ -802,7 +942,7 @@ static struct i2c_driver ltr390_driver = {
>  	.driver = {
>  		.name = "ltr390",
>  		.of_match_table = ltr390_of_table,
> -		.pm = pm_sleep_ptr(&ltr390_pm_ops),
> +		.pm = pm_ptr(&ltr390_pm_ops),
>  	},
>  	.probe = ltr390_probe,
>  	.id_table = ltr390_id,

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

[Akshay Jindal]

Hi Jonathan,
Thanks for your review. Please see my followup inline.

On Mon, Aug 25, 2025 at 7:56 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri, 22 Aug 2025 23:33:26 +0530
> Akshay Jindal <akshayaj.lkd@gmail.com> wrote:
> > +
> > +     if (!state) {
> > +             ret = regmap_clear_bits(data->regmap, LTR390_INT_CFG, LTR390_LS_INT_EN);
> > +             data->irq_enabled = false;
>
> Just take an extra reference to runtime pm on enable of event and put it disable.
> Then no need for special handling with a local flag etc.

Consider a scenario, where the user only disables the event instead of
enabling it,
(i.e. user wrote 0 on the sysfs attribute before it was 1). In this case,
If enable means inc ref count and disable means dec ref count, then
this would lead to refcount underflow and the suspend callback will
not be called.

To handle this case, we would need to check whether irq/event was
enabled or not.
For that either we can use the local flag as I did, or I would need to
do a read and test
for the interrupt bit being set. I feel using the local flag would be
cleaner and would
require less code.
If you are fine with local flag usage, then shall I not stick to only
local flag usage?

Thanks,
Akshay

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

[Jonathan Cameron]

On Tue, 26 Aug 2025 02:29:12 +0530
Akshay Jindal <akshayaj.lkd@gmail.com> wrote:

> Hi Jonathan,
> Thanks for your review. Please see my followup inline.
> 
> On Mon, Aug 25, 2025 at 7:56 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Fri, 22 Aug 2025 23:33:26 +0530
> > Akshay Jindal <akshayaj.lkd@gmail.com> wrote:  
> > > +
> > > +     if (!state) {
> > > +             ret = regmap_clear_bits(data->regmap, LTR390_INT_CFG, LTR390_LS_INT_EN);
> > > +             data->irq_enabled = false;  
> >
> > Just take an extra reference to runtime pm on enable of event and put it disable.
> > Then no need for special handling with a local flag etc.  
> 
> Consider a scenario, where the user only disables the event instead of
> enabling it,
> (i.e. user wrote 0 on the sysfs attribute before it was 1). In this case,
> If enable means inc ref count and disable means dec ref count, then
> this would lead to refcount underflow and the suspend callback will
> not be called.
> 
> To handle this case, we would need to check whether irq/event was
> enabled or not.
> For that either we can use the local flag as I did, or I would need to
> do a read and test
> for the interrupt bit being set. I feel using the local flag would be
> cleaner and would
> require less code.
> If you are fine with local flag usage, then shall I not stick to only
> local flag usage?
> 

Normally we'd check the register to find out what whether the event
is enabled or not.  If we are asking for the state it is already in
then just return having done nothing.  If bus reads are an overhead
worth avoiding regcache will ensure we only do it once.

Then if we are doing something, do the runtime pm get / put as appropriate.

Jonathan


> Thanks,
> Akshay

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

[Akshay Jindal]

On Sat, Aug 30, 2025 at 11:38 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 26 Aug 2025 02:29:12 +0530
> Akshay Jindal <akshayaj.lkd@gmail.com> wrote:
> Normally we'd check the register to find out what whether the event
> is enabled or not.  If we are asking for the state it is already in
> then just return having done nothing.  If bus reads are an overhead
> worth avoiding regcache will ensure we only do it once.
>
> Then if we are doing something, do the runtime pm get / put as appropriate.

Hi Jonathan,
I looked into the code of multiple drivers. Almost every other driver
uses a flag
inside driver specific data structure to denote whether events are enabled or
disabled. Some use masks to mark which events are enabled in case multiple
types of events are supported.
For eg: bmc150, fxls8962af, kxcjk1013, ad7173, ad7291, ad799x, hi8435,
max1363, nct7201, pac1921, palmas_adc, bd79124, ads1015, ad5421, bmg160,
bmi323, inv_mpu6050, and the list goes on. I see this in at least 30+ drivers,
maybe more.

On the other hand, regcache is used by only 4-5 drivers.

On the basis of the above arguments, I am strongly advocating the usage of
irq_enabled flag in ltr390_data. Moreover, using register read to
determine existing
event config every time we do event configuration, seems to be an unnecessary
burden on the bus and increases lines of code which I am finding it tough to
convince myself for.

Having said that, I have prepared a v3. Kindly give your feedback on it.

Thanks,
Akshay.

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

[Jonathan Cameron]

On Sun, 31 Aug 2025 01:19:39 +0530
Akshay Jindal <akshayaj.lkd@gmail.com> wrote:

> On Sat, Aug 30, 2025 at 11:38 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Tue, 26 Aug 2025 02:29:12 +0530
> > Akshay Jindal <akshayaj.lkd@gmail.com> wrote:
> > Normally we'd check the register to find out what whether the event
> > is enabled or not.  If we are asking for the state it is already in
> > then just return having done nothing.  If bus reads are an overhead
> > worth avoiding regcache will ensure we only do it once.
> >
> > Then if we are doing something, do the runtime pm get / put as appropriate.  
> 
> Hi Jonathan,
> I looked into the code of multiple drivers. Almost every other driver
> uses a flag
> inside driver specific data structure to denote whether events are enabled or
> disabled. Some use masks to mark which events are enabled in case multiple
> types of events are supported.
> For eg: bmc150, fxls8962af, kxcjk1013, ad7173, ad7291, ad799x, hi8435,
> max1363, nct7201, pac1921, palmas_adc, bd79124, ads1015, ad5421, bmg160,
> bmi323, inv_mpu6050, and the list goes on. I see this in at least 30+ drivers,
> maybe more.
> 
> On the other hand, regcache is used by only 4-5 drivers.
> 
> On the basis of the above arguments, I am strongly advocating the usage of
> irq_enabled flag in ltr390_data. Moreover, using register read to
> determine existing
> event config every time we do event configuration, seems to be an unnecessary
> burden on the bus and increases lines of code which I am finding it tough to
> convince myself for.

Alright I'll give in on this though I consider the regcache approach
superior in this case. It might also be applicable to a number of the above
but they are old drivers on the whole and I'm not going to pursue changing
this in those.  Some of those are IIRC conversions from pre regmap code
where they just didn't go as far as replacing the existing driver specific
state caching.

Anyhow, not worth arguing about.

Jonathan


> 
> Having said that, I have prepared a v3. Kindly give your feedback on it.
> 
> Thanks,
> Akshay.