[PATCH v3] iio: light: ltr390: Implement runtime PM support

[PATCH v3] iio: light: ltr390: Implement runtime PM support

[Akshay Jindal]

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.

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

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.
-> 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.

2. David's feedback:
-> Update Changelog with stats of power savings mentioned in datasheet.
-> Dissolve ltr390_set_power_state() function.

3. Jonathan's feedback:
-> Adopt the approach of increment refcount when event enable and
vice-versa.
-> Use devm_pm_runtime_set_active_enabled() function.
-> Better error handling.

4. Testing changes:
-> Add a test section for module load/unload while event is enabled or disabled.
-> Add an idempotency check in the Interrupt Handling Verification subsection.

5. Change the heading word from Add-->Implement.

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.
3. Did idempotency check for event enable or disable. This means that occurences like event enable or disable should not
	erroneously increase or decrease the refcount of the device. 

Module load/unload Verification:
--------------------------------
1. Tested that the refcount should reach 0 when events are enabled.
2. Did a test of load after unload.

 drivers/iio/light/ltr390.c | 223 ++++++++++++++++++++++++++++++++++---
 1 file changed, 206 insertions(+), 17 deletions(-)

diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
index 2e1cf62e8201..155aa4cfa7e5 100644
--- a/drivers/iio/light/ltr390.c
+++ b/drivers/iio/light/ltr390.c
@@ -27,6 +27,7 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/regmap.h>
+#include <linux/pm_runtime.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/events.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[] = {
@@ -215,9 +217,10 @@ static int ltr390_get_samp_freq_or_period(struct ltr390_data *data,
 	return ltr390_samp_freq_table[value][option];
 }
 
-static int ltr390_read_raw(struct iio_dev *iio_device,
-			   struct iio_chan_spec const *chan, int *val,
-			   int *val2, long mask)
+
+static int __ltr390_read_raw(struct iio_dev *iio_device,
+			struct iio_chan_spec const *chan, int *val,
+			int *val2, long mask)
 {
 	int ret;
 	struct ltr390_data *data = iio_priv(iio_device);
@@ -280,6 +283,27 @@ static int ltr390_read_raw(struct iio_dev *iio_device,
 	}
 }
 
+static int ltr390_read_raw(struct iio_dev *iio_device,
+			   struct iio_chan_spec const *chan, int *val,
+			   int *val2, long mask)
+{
+	int ret;
+	struct ltr390_data *data = iio_priv(iio_device);
+	struct device *dev = &data->client->dev;
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret < 0) {
+		dev_err(dev, "runtime PM failed to resume: %d\n", ret);
+		return ret;
+	}
+
+	ret = __ltr390_read_raw(iio_device, chan, val, val2, mask);
+
+	pm_runtime_put_autosuspend(dev);
+
+	return ret;
+}
+
 /* integration time in us */
 static const int ltr390_int_time_map_us[] = { 400000, 200000, 100000, 50000, 25000, 12500 };
 static const int ltr390_gain_map[] = { 1, 3, 6, 9, 18 };
@@ -415,7 +439,7 @@ 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,
+static int __ltr390_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
 				int val, int val2, long mask)
 {
 	struct ltr390_data *data = iio_priv(indio_dev);
@@ -444,6 +468,26 @@ static int ltr390_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec cons
 	}
 }
 
+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);
+	struct device *dev = &data->client->dev;
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret < 0) {
+		dev_err(dev, "runtime PM failed to resume: %d\n", ret);
+		return ret;
+	}
+
+	ret = __ltr390_write_raw(indio_dev, chan, val, val2, mask);
+
+	pm_runtime_put_autosuspend(dev);
+
+	return ret;
+}
+
 static int ltr390_read_intr_prst(struct ltr390_data *data, int *val)
 {
 	int ret, prst, samp_period;
@@ -527,7 +571,7 @@ static int ltr390_write_threshold(struct iio_dev *indio_dev,
 	}
 }
 
-static int ltr390_read_event_value(struct iio_dev *indio_dev,
+static int __ltr390_read_event_value(struct iio_dev *indio_dev,
 				const struct iio_chan_spec *chan,
 				enum iio_event_type type,
 				enum iio_event_direction dir,
@@ -546,7 +590,31 @@ static int ltr390_read_event_value(struct iio_dev *indio_dev,
 	}
 }
 
-static int ltr390_write_event_value(struct iio_dev *indio_dev,
+static int ltr390_read_event_value(struct iio_dev *indio_dev,
+				const struct iio_chan_spec *chan,
+				enum iio_event_type type,
+				enum iio_event_direction dir,
+				enum iio_event_info info,
+				int *val, int *val2)
+{
+	int ret;
+	struct ltr390_data *data = iio_priv(indio_dev);
+	struct device *dev = &data->client->dev;
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret < 0) {
+		dev_err(dev, "runtime PM failed to resume: %d\n", ret);
+		return ret;
+	}
+
+	ret = __ltr390_read_event_value(indio_dev, chan, type, dir,
+					info, val, val2);
+	pm_runtime_put_autosuspend(dev);
+
+	return ret;
+}
+
+static int __ltr390_write_event_value(struct iio_dev *indio_dev,
 				const struct iio_chan_spec *chan,
 				enum iio_event_type type,
 				enum iio_event_direction dir,
@@ -571,22 +639,55 @@ static int ltr390_write_event_value(struct iio_dev *indio_dev,
 	}
 }
 
+static int ltr390_write_event_value(struct iio_dev *indio_dev,
+				const struct iio_chan_spec *chan,
+				enum iio_event_type type,
+				enum iio_event_direction dir,
+				enum iio_event_info info,
+				int val, int val2)
+{
+	int ret;
+	struct ltr390_data *data = iio_priv(indio_dev);
+	struct device *dev = &data->client->dev;
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret < 0) {
+		dev_err(dev, "runtime PM failed to resume: %d\n", ret);
+		return ret;
+	}
+
+	ret = __ltr390_write_event_value(indio_dev, chan, type, dir,
+					info, val, val2);
+	pm_runtime_put_autosuspend(dev);
+
+	return ret;
+}
+
 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);
 }
 
-static int ltr390_write_event_config(struct iio_dev *indio_dev,
+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,
@@ -598,7 +699,6 @@ static int ltr390_write_event_config(struct iio_dev *indio_dev,
 	if (!state)
 		return regmap_clear_bits(data->regmap, LTR390_INT_CFG, LTR390_LS_INT_EN);
 
-	guard(mutex)(&data->lock);
 	ret = regmap_set_bits(data->regmap, LTR390_INT_CFG, LTR390_LS_INT_EN);
 	if (ret < 0)
 		return ret;
@@ -623,18 +723,60 @@ static int ltr390_write_event_config(struct iio_dev *indio_dev,
 	}
 }
 
+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;
+}
+
 static int ltr390_debugfs_reg_access(struct iio_dev *indio_dev,
 						unsigned int reg, unsigned int writeval,
 						unsigned int *readval)
 {
 	struct ltr390_data *data = iio_priv(indio_dev);
+	struct device *dev = &data->client->dev;
+	int ret;
 
-	guard(mutex)(&data->lock);
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret < 0) {
+		dev_err(dev, "runtime PM failed to resume: %d\n", ret);
+		return ret;
+	}
 
+	guard(mutex)(&data->lock);
 	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);
+	pm_runtime_put_autosuspend(dev);
+
+	return ret;
 }
 
 static const struct iio_info ltr390_info = {
@@ -687,15 +829,36 @@ static void ltr390_powerdown(void *priv)
 	guard(mutex)(&data->lock);
 
 	/* Ensure that power off and interrupts are disabled */
-	if (regmap_clear_bits(data->regmap, LTR390_INT_CFG,
-				LTR390_LS_INT_EN) < 0)
-		dev_err(&data->client->dev, "failed to disable interrupts\n");
+	if (data->irq_enabled) {
+		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;
+
+		pm_runtime_put_autosuspend(&data->client->dev);
+	}
 
 	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 = devm_pm_runtime_set_active_enabled(dev);
+	if (ret)
+		return dev_err_probe(dev, ret,
+					"failed to enable runtime PM\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 +871,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 +886,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 value of irq_enabled is false*/
+	data->irq_enabled = false;
 
 	mutex_init(&data->lock);
 
@@ -763,6 +930,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 +952,28 @@ 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);
+
+	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 const struct dev_pm_ops ltr390_pm_ops = {
+	SYSTEM_SLEEP_PM_OPS(ltr390_suspend, ltr390_resume)
+	RUNTIME_PM_OPS(ltr390_runtime_suspend, ltr390_runtime_resume, NULL)
+};
 
 static const struct i2c_device_id ltr390_id[] = {
 	{ "ltr390" },
@@ -802,7 +991,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 v3] iio: light: ltr390: Implement runtime PM support

[Andy Shevchenko]

On Sat, Aug 30, 2025 at 2:35 PM Akshay Jindal <akshayaj.lkd@gmail.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

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

[Akshay Jindal]

Thanks for the speedy review Andy. Follow-up inline.

On Sat, Aug 30, 2025 at 6:04 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sat, Aug 30, 2025 at 2:35 PM Akshay Jindal <akshayaj.lkd@gmail.com> wrote:
> > @@ -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.
Yeah, this got missed. Will address this.

>
> > +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.
>
LTR390 only supports 1 event/interrupt which is toggled in this callback based
on value provided to sysfs entry. There cannot be a version of this without IRQ
handling. It is supposed to do IRQ handling only.

Pseudo code of the said function will be something as follows:
ltr390_write_event_config() {
if (interrupt needs to be enabled && previously it was disabled)
     pm_runtime_resume_and_get()
do_actual_reg_writes()
if (interrupt needs to be disabled && previously it was enabled)
    pm_runtime_put_autosuspend().
}

With the current function , we achieve the following objectives:
1. idempotency in refcount change. Meaning if IRQ is already enabled and
if someone enables it again, it will not increase the refcount, same goes for
double disable case. This has been tested as well.
2. Only if the new and previous config is different, then only the refcount will
change.
3. Adheres to previous comments received regarding checking return value
of _get and ignoring that of _put.

I genuinely don't see any duplicate checks here. In addition, I feel the above
function is fine from a simplification point of view and cannot be bifurcated
further.

Although, if you could clarify what you mean by further bifurcation, I might be
able to connect with your thoughts.

Thanks,
Akshay

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

[Andy Shevchenko]

On Sat, Aug 30, 2025 at 4:24 PM Akshay Jindal <akshayaj.lkd@gmail.com> wrote:
> On Sat, Aug 30, 2025 at 6:04 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Sat, Aug 30, 2025 at 2:35 PM Akshay Jindal <akshayaj.lkd@gmail.com> wrote:

...

> > > +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.
> >
> LTR390 only supports 1 event/interrupt which is toggled in this callback based
> on value provided to sysfs entry. There cannot be a version of this without IRQ
> handling. It is supposed to do IRQ handling only.
>
> Pseudo code of the said function will be something as follows:
> ltr390_write_event_config() {
> if (interrupt needs to be enabled && previously it was disabled)
>      pm_runtime_resume_and_get()
> do_actual_reg_writes()
> if (interrupt needs to be disabled && previously it was enabled)
>     pm_runtime_put_autosuspend().
> }

I see now, yeah, this is unfortunate.
Just rename the leading __ to the _unlocked() suffix. It's easier to read.

> With the current function , we achieve the following objectives:
> 1. idempotency in refcount change. Meaning if IRQ is already enabled and
> if someone enables it again, it will not increase the refcount, same goes for
> double disable case. This has been tested as well.
> 2. Only if the new and previous config is different, then only the refcount will
> change.
> 3. Adheres to previous comments received regarding checking return value
> of _get and ignoring that of _put.
>
> I genuinely don't see any duplicate checks here. In addition, I feel the above
> function is fine from a simplification point of view and cannot be bifurcated
> further.

You are right I missed the asymmetric conditionals.

> Although, if you could clarify what you mean by further bifurcation, I might be
> able to connect with your thoughts.

-- 
With Best Regards,
Andy Shevchenko

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

[Akshay Jindal]

Thanks Andy for acknowledging. Follow-up inline.

On Sat, Aug 30, 2025 at 8:16 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sat, Aug 30, 2025 at 4:24 PM Akshay Jindal <akshayaj.lkd@gmail.com> wrote:
> > On Sat, Aug 30, 2025 at 6:04 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Sat, Aug 30, 2025 at 2:35 PM Akshay Jindal <akshayaj.lkd@gmail.com> wrote:
>
>
> I see now, yeah, this is unfortunate.
> Just rename the leading __ to  the _unlocked() suffix. It's easier to read.
I did not get this part. Which function do you think needs to be renamed?

Thanks,
Akshay.

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

[Jonathan Cameron]

On Sat, 30 Aug 2025 17:05:00 +0530
Akshay Jindal <akshayaj.lkd@gmail.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.
> 
> Signed-off-by: Akshay Jindal <akshayaj.lkd@gmail.com>

Sorry it took me a while to reply to the last email on the v1 thread.
Busy week.

I may have this all wrong though if the runtime pm disable you have
here (bit (1) of MAIN Control) restricts which other registers we can
access. Perhaps I'm missing where that is stated in the datasheet,
or maybe it's an omission and you have seen it to be the case
from experimentation?

If that isn't required a lot of the runtime pm calls in here go away.

Also, we should just read the config register to find out if the
even is enabled not bother having a separate cache of that one bit.

Jonathan


> ---
> 
> 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.
> -> 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.
> 
> 2. David's feedback:
> -> Update Changelog with stats of power savings mentioned in datasheet.
> -> Dissolve ltr390_set_power_state() function.  
> 
> 3. Jonathan's feedback:
> -> Adopt the approach of increment refcount when event enable and  
> vice-versa.

> +static int __ltr390_write_event_value(struct iio_dev *indio_dev,
>  				const struct iio_chan_spec *chan,
>  				enum iio_event_type type,
>  				enum iio_event_direction dir,
> @@ -571,22 +639,55 @@ static int ltr390_write_event_value(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +static int ltr390_write_event_value(struct iio_dev *indio_dev,
> +				const struct iio_chan_spec *chan,
> +				enum iio_event_type type,
> +				enum iio_event_direction dir,
> +				enum iio_event_info info,
> +				int val, int val2)
> +{
> +	int ret;
> +	struct ltr390_data *data = iio_priv(indio_dev);
> +	struct device *dev = &data->client->dev;
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret < 0) {
> +		dev_err(dev, "runtime PM failed to resume: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = __ltr390_write_event_value(indio_dev, chan, type, dir,
> +					info, val, val2);
> +	pm_runtime_put_autosuspend(dev);
> +
> +	return ret;
> +}
> +
>  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);
I may be misreading the datasheet but I'd kind of expect to be
able to read this register in the (slightly) powered down state.

> +	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);
>  }
>  
> -static int ltr390_write_event_config(struct iio_dev *indio_dev,
> +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,
> @@ -598,7 +699,6 @@ static int ltr390_write_event_config(struct iio_dev *indio_dev,
>  	if (!state)
>  		return regmap_clear_bits(data->regmap, LTR390_INT_CFG, LTR390_LS_INT_EN);
>  
> -	guard(mutex)(&data->lock);
>  	ret = regmap_set_bits(data->regmap, LTR390_INT_CFG, LTR390_LS_INT_EN);
>  	if (ret < 0)
>  		return ret;
> @@ -623,18 +723,60 @@ static int ltr390_write_event_config(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +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);
> +
As per v1 (late) reply. I'd expect to find query the register to find
out if what we are potentially setting here is already on or not and
exit early if we aren't changing that state.

Then we don't need the data->irq_enabled, we can just use runtime pm reference
counting to ensure the right things happen.

> +	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;
> +}
> +
>  static int ltr390_debugfs_reg_access(struct iio_dev *indio_dev,
>  						unsigned int reg, unsigned int writeval,
>  						unsigned int *readval)
>  {
>  	struct ltr390_data *data = iio_priv(indio_dev);
> +	struct device *dev = &data->client->dev;
> +	int ret;
>  
> -	guard(mutex)(&data->lock);
> +	ret = pm_runtime_resume_and_get(dev);

So this makes me wonder.  I'd been assuming our disable was only turning the
sensor off, not register access?  Seeing as it's controlled by a register
we can clearly access at least some.

If that's the case why do we need to runtime resume for debug register
read/write?  We shouldn't care if the sensors are reading or not. In fact
if we do turn the power on we changed the state we are debugging which is
probably not a good plan.

> +	if (ret < 0) {
> +		dev_err(dev, "runtime PM failed to resume: %d\n", ret);
> +		return ret;
> +	}
>  
> +	guard(mutex)(&data->lock);
>  	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);
> +	pm_runtime_put_autosuspend(dev);
> +
> +	return ret;
>  }

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

[Akshay Jindal]

On Sat, Aug 30, 2025 at 11:55 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sat, 30 Aug 2025 17:05:00 +0530
> Akshay Jindal <akshayaj.lkd@gmail.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.
> >
> > Signed-off-by: Akshay Jindal <akshayaj.lkd@gmail.com>
>
> Sorry it took me a while to reply to the last email on the v1 thread.
> Busy week.
>
> I may have this all wrong though if the runtime pm disable you have
> here (bit (1) of MAIN Control) restricts which other registers we can
> access. Perhaps I'm missing where that is stated in the datasheet,
> or maybe it's an omission and you have seen it to be the case
> from experimentation?
>
> If that isn't required a lot of the runtime pm calls in here go away.
bit (1) of the MAIN Control register simply puts the sensor in standby mode.
It does not restrict register access. The logic behind doing pm_resume
is to increase the refcount before doing any IO on the sensor.

>
> Also, we should just read the config register to find out if the
> event is enabled, not bother having a separate cache of that one bit.

I have seen the code. At Least 30+ drivers are maintaining this flag.
If we go down the road of not using a flag, it would be an unnecessary
load on the bus everytime an interrupt is configured.
Consider a scenario, where instead of toggling, you are doing enable after
enable or disable after disable. This is expected to be kind of a no-op.
Here the callback will not do anything except for reading the interrupt config
register. I say why to read this register also when a simple flag can
give us that
information.
>
> Jonathan
>
>
> > ---
> >
> > 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.
> > -> 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.
> >
> > 2. David's feedback:
> > -> Update Changelog with stats of power savings mentioned in datasheet.
> > -> Dissolve ltr390_set_power_state() function.
> >
> > 3. Jonathan's feedback:
> > -> Adopt the approach of increment refcount when event enable and
> > vice-versa.
>
> > +static int __ltr390_write_event_value(struct iio_dev *indio_dev,
> >                               const struct iio_chan_spec *chan,
> >                               enum iio_event_type type,
> >                               enum iio_event_direction dir,
> > @@ -571,22 +639,55 @@ static int ltr390_write_event_value(struct iio_dev *indio_dev,
> >       }
> >  }
> >
> > +static int ltr390_write_event_value(struct iio_dev *indio_dev,
> > +                             const struct iio_chan_spec *chan,
> > +                             enum iio_event_type type,
> > +                             enum iio_event_direction dir,
> > +                             enum iio_event_info info,
> > +                             int val, int val2)
> > +{
> > +     int ret;
> > +     struct ltr390_data *data = iio_priv(indio_dev);
> > +     struct device *dev = &data->client->dev;
> > +
> > +     ret = pm_runtime_resume_and_get(dev);
> > +     if (ret < 0) {
> > +             dev_err(dev, "runtime PM failed to resume: %d\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     ret = __ltr390_write_event_value(indio_dev, chan, type, dir,
> > +                                     info, val, val2);
> > +     pm_runtime_put_autosuspend(dev);
> > +
> > +     return ret;
> > +}
> > +
> >  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);
> I may be misreading the datasheet but I'd kind of expect to be
> able to read this register in the (slightly) powered down state.
>
> > +     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);
> >  }
> >
> > -static int ltr390_write_event_config(struct iio_dev *indio_dev,
> > +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,
> > @@ -598,7 +699,6 @@ static int ltr390_write_event_config(struct iio_dev *indio_dev,
> >       if (!state)
> >               return regmap_clear_bits(data->regmap, LTR390_INT_CFG, LTR390_LS_INT_EN);
> >
> > -     guard(mutex)(&data->lock);
> >       ret = regmap_set_bits(data->regmap, LTR390_INT_CFG, LTR390_LS_INT_EN);
> >       if (ret < 0)
> >               return ret;
> > @@ -623,18 +723,60 @@ static int ltr390_write_event_config(struct iio_dev *indio_dev,
> >       }
> >  }
> >
> > +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);
> > +
> As per v1 (late) reply. I'd expect to find query the register to find
> out if what we are potentially setting here is already on or not and
> exit early if we aren't changing that state.
>
> Then we don't need the data->irq_enabled, we can just use runtime pm reference
> counting to ensure the right things happen.
>
> > +     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;
> > +}
> > +
> >  static int ltr390_debugfs_reg_access(struct iio_dev *indio_dev,
> >                                               unsigned int reg, unsigned int writeval,
> >                                               unsigned int *readval)
> >  {
> >       struct ltr390_data *data = iio_priv(indio_dev);
> > +     struct device *dev = &data->client->dev;
> > +     int ret;
> >
> > -     guard(mutex)(&data->lock);
> > +     ret = pm_runtime_resume_and_get(dev);
>
> So this makes me wonder.  I'd been assuming our disable was only turning the
> sensor off, not register access?  Seeing as it's controlled by a register
> we can clearly access at least some.
>
> If that's the case why do we need to runtime resume for debug register
> read/write?  We shouldn't care if the sensors are reading or not. In fact
> if we do turn the power on we changed the state we are debugging which is
> probably not a good plan.
Are you suggesting to remove pm_runtime_* calls from all the functions or
just the debugfs function?

Thanks,
Akshay

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

[Jonathan Cameron]

On Sun, 31 Aug 2025 11:09:26 +0530
Akshay Jindal <akshayaj.lkd@gmail.com> wrote:

> On Sat, Aug 30, 2025 at 11:55 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Sat, 30 Aug 2025 17:05:00 +0530
> > Akshay Jindal <akshayaj.lkd@gmail.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.
> > >
> > > Signed-off-by: Akshay Jindal <akshayaj.lkd@gmail.com>  
> >
> > Sorry it took me a while to reply to the last email on the v1 thread.
> > Busy week.
> >
> > I may have this all wrong though if the runtime pm disable you have
> > here (bit (1) of MAIN Control) restricts which other registers we can
> > access. Perhaps I'm missing where that is stated in the datasheet,
> > or maybe it's an omission and you have seen it to be the case
> > from experimentation?
> >
> > If that isn't required a lot of the runtime pm calls in here go away.  
> bit (1) of the MAIN Control register simply puts the sensor in standby mode.
> It does not restrict register access. The logic behind doing pm_resume
> is to increase the refcount before doing any IO on the sensor.
> 

Why do we care about that refcount?  We care if the device is in a state
to perform IO. In this particular case runtime pm callbacks don't affect
that unless I'm missing something more complex!


> >
> > Also, we should just read the config register to find out if the
> > event is enabled, not bother having a separate cache of that one bit.  
> 
> I have seen the code. At Least 30+ drivers are maintaining this flag.
> If we go down the road of not using a flag, it would be an unnecessary
> load on the bus everytime an interrupt is configured.
> Consider a scenario, where instead of toggling, you are doing enable after
> enable or disable after disable. This is expected to be kind of a no-op.
> Here the callback will not do anything except for reading the interrupt config
> register. I say why to read this register also when a simple flag can
> give us that
> information.

As per the other thread I'll agree to the flag, though to my mind it
is  an inferior solution and in many of those other drivers is there
because of the history of how they evolved, not because it is the best
solution.

> >
> > Jonathan
> >
> >  
> > > ---
> > >
> > > 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.
> > > -> 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.
> > >
> > > 2. David's feedback:  
> > > -> Update Changelog with stats of power savings mentioned in datasheet.
> > > -> Dissolve ltr390_set_power_state() function.  
> > >
> > > 3. Jonathan's feedback:  
> > > -> Adopt the approach of increment refcount when event enable and  
> > > vice-versa.  
> >  
> > > +static int __ltr390_write_event_value(struct iio_dev *indio_dev,
> > >                               const struct iio_chan_spec *chan,
> > >                               enum iio_event_type type,
> > >                               enum iio_event_direction dir,
> > > @@ -571,22 +639,55 @@ static int ltr390_write_event_value(struct iio_dev *indio_dev,
> > >       }
> > >  }
> > >
> > > +static int ltr390_write_event_value(struct iio_dev *indio_dev,
> > > +                             const struct iio_chan_spec *chan,
> > > +                             enum iio_event_type type,
> > > +                             enum iio_event_direction dir,
> > > +                             enum iio_event_info info,
> > > +                             int val, int val2)
> > > +{
> > > +     int ret;
> > > +     struct ltr390_data *data = iio_priv(indio_dev);
> > > +     struct device *dev = &data->client->dev;
> > > +
> > > +     ret = pm_runtime_resume_and_get(dev);
> > > +     if (ret < 0) {
> > > +             dev_err(dev, "runtime PM failed to resume: %d\n", ret);
> > > +             return ret;
> > > +     }
> > > +
> > > +     ret = __ltr390_write_event_value(indio_dev, chan, type, dir,
> > > +                                     info, val, val2);
> > > +     pm_runtime_put_autosuspend(dev);
> > > +
> > > +     return ret;
> > > +}
> > > +
> > >  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);  
> > I may be misreading the datasheet but I'd kind of expect to be
> > able to read this register in the (slightly) powered down state.
> >  
> > > +     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);
> > >  }
> > >
> > > -static int ltr390_write_event_config(struct iio_dev *indio_dev,
> > > +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,
> > > @@ -598,7 +699,6 @@ static int ltr390_write_event_config(struct iio_dev *indio_dev,
> > >       if (!state)
> > >               return regmap_clear_bits(data->regmap, LTR390_INT_CFG, LTR390_LS_INT_EN);
> > >
> > > -     guard(mutex)(&data->lock);
> > >       ret = regmap_set_bits(data->regmap, LTR390_INT_CFG, LTR390_LS_INT_EN);
> > >       if (ret < 0)
> > >               return ret;
> > > @@ -623,18 +723,60 @@ static int ltr390_write_event_config(struct iio_dev *indio_dev,
> > >       }
> > >  }
> > >
> > > +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);
> > > +  
> > As per v1 (late) reply. I'd expect to find query the register to find
> > out if what we are potentially setting here is already on or not and
> > exit early if we aren't changing that state.
> >
> > Then we don't need the data->irq_enabled, we can just use runtime pm reference
> > counting to ensure the right things happen.
> >  
> > > +     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;
> > > +}
> > > +
> > >  static int ltr390_debugfs_reg_access(struct iio_dev *indio_dev,
> > >                                               unsigned int reg, unsigned int writeval,
> > >                                               unsigned int *readval)
> > >  {
> > >       struct ltr390_data *data = iio_priv(indio_dev);
> > > +     struct device *dev = &data->client->dev;
> > > +     int ret;
> > >
> > > -     guard(mutex)(&data->lock);
> > > +     ret = pm_runtime_resume_and_get(dev);  
> >
> > So this makes me wonder.  I'd been assuming our disable was only turning the
> > sensor off, not register access?  Seeing as it's controlled by a register
> > we can clearly access at least some.
> >
> > If that's the case why do we need to runtime resume for debug register
> > read/write?  We shouldn't care if the sensors are reading or not. In fact
> > if we do turn the power on we changed the state we are debugging which is
> > probably not a good plan.  
> Are you suggesting to remove pm_runtime_* calls from all the functions or
> just the debugfs function?

All the ones where it's not needed.  So as far as I can see it is only
needed when actually grabbing date.

Jonathan

> 
> Thanks,
> Akshay

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

[Akshay Jindal]

On Sun, Aug 31, 2025 at 8:41 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sun, 31 Aug 2025 11:09:26 +0530
> Akshay Jindal <akshayaj.lkd@gmail.com> wrote:
>
> > On Sat, Aug 30, 2025 at 11:55 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > On Sat, 30 Aug 2025 17:05:00 +0530
> > > Akshay Jindal <akshayaj.lkd@gmail.com> wrote:
> > > >  static int ltr390_debugfs_reg_access(struct iio_dev *indio_dev,
> > > >                                               unsigned int reg, unsigned int writeval,
> > > >                                               unsigned int *readval)
> > > >  {
> > > >       struct ltr390_data *data = iio_priv(indio_dev);
> > > > +     struct device *dev = &data->client->dev;
> > > > +     int ret;
> > > >
> > > > -     guard(mutex)(&data->lock);
> > > > +     ret = pm_runtime_resume_and_get(dev);
> > >
> > > So this makes me wonder.  I'd been assuming our disable was only turning the
> > > sensor off, not register access?  Seeing as it's controlled by a register
> > > we can clearly access at least some.
> > >
> > > If that's the case why do we need to runtime resume for debug register
> > > read/write?  We shouldn't care if the sensors are reading or not. In fact
> > > if we do turn the power on we changed the state we are debugging which is
> > > probably not a good plan.
> > Are you suggesting to remove pm_runtime_* calls from all the functions or
> > just the debugfs function?
>
> All the ones where it's not needed.  So as far as I can see it is only
> needed when actually grabbing date.

These are the functions where I have used it currently.
static const struct iio_info ltr390_info = {
        .read_raw = ltr390_read_raw,----> reads sensor data, scale,
int_time, samp_frequency from register
        .write_raw = ltr390_write_raw,-------> writes sampling
frequency, scale, int_time into register.
        .read_avail = ltr390_read_avail, ----------------> no reg read
        .read_event_value = ltr390_read_event_value,-----> reads
interrupt threshold value from register
        .read_event_config = ltr390_read_event_config,----> reads
INT_CFG register
        .write_event_value = ltr390_write_event_value,------> writes
threshold value into register
        .write_event_config = ltr390_write_event_config,-----> writes
into INT_CFG register
        .debugfs_reg_access = ltr390_debugfs_reg_access,-->
reads/writes into all registers
};

Going by your comment, I think I need it only in read_raw and
write_event_config.
There is a similar sensor, ltrf216a (same company, liteon). There only
read_raw has
these pm_runtime_* calls. Let me know your thoughts. I will make the change and
include updated testing details.

Thanks,
Akshay

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

[Akshay Jindal]

On Mon, Sep 1, 2025 at 1:41 AM Akshay Jindal <akshayaj.lkd@gmail.com> wrote:
>
> On Sun, Aug 31, 2025 at 8:41 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Sun, 31 Aug 2025 11:09:26 +0530
> > Akshay Jindal <akshayaj.lkd@gmail.com> wrote:
> >
> > > On Sat, Aug 30, 2025 at 11:55 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > > >
> > > > On Sat, 30 Aug 2025 17:05:00 +0530
> > > > Akshay Jindal <akshayaj.lkd@gmail.com> wrote:
> These are the functions where I have used it currently.
> static const struct iio_info ltr390_info = {
>         .read_raw = ltr390_read_raw,----> reads sensor data, scale,
> int_time, samp_frequency from register
>         .write_raw = ltr390_write_raw,-------> writes sampling
> frequency, scale, int_time into register.
>         .read_avail = ltr390_read_avail, ----------------> no reg read
Update: I have not used pm_runtime_* calls here as there is no
interaction with any register here. In the rest of them I have used it.

Thanks,
Akshay

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

[Jonathan Cameron]

On Mon, 1 Sep 2025 01:41:33 +0530
Akshay Jindal <akshayaj.lkd@gmail.com> wrote:

> On Sun, Aug 31, 2025 at 8:41 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Sun, 31 Aug 2025 11:09:26 +0530
> > Akshay Jindal <akshayaj.lkd@gmail.com> wrote:
> >  
> > > On Sat, Aug 30, 2025 at 11:55 PM Jonathan Cameron <jic23@kernel.org> wrote:  
> > > >
> > > > On Sat, 30 Aug 2025 17:05:00 +0530
> > > > Akshay Jindal <akshayaj.lkd@gmail.com> wrote:  
> > > > >  static int ltr390_debugfs_reg_access(struct iio_dev *indio_dev,
> > > > >                                               unsigned int reg, unsigned int writeval,
> > > > >                                               unsigned int *readval)
> > > > >  {
> > > > >       struct ltr390_data *data = iio_priv(indio_dev);
> > > > > +     struct device *dev = &data->client->dev;
> > > > > +     int ret;
> > > > >
> > > > > -     guard(mutex)(&data->lock);
> > > > > +     ret = pm_runtime_resume_and_get(dev);  
> > > >
> > > > So this makes me wonder.  I'd been assuming our disable was only turning the
> > > > sensor off, not register access?  Seeing as it's controlled by a register
> > > > we can clearly access at least some.
> > > >
> > > > If that's the case why do we need to runtime resume for debug register
> > > > read/write?  We shouldn't care if the sensors are reading or not. In fact
> > > > if we do turn the power on we changed the state we are debugging which is
> > > > probably not a good plan.  
> > > Are you suggesting to remove pm_runtime_* calls from all the functions or
> > > just the debugfs function?  
> >
> > All the ones where it's not needed.  So as far as I can see it is only
> > needed when actually grabbing date.  
> 
> These are the functions where I have used it currently.
> static const struct iio_info ltr390_info = {
>         .read_raw = ltr390_read_raw,----> reads sensor data, scale,
> int_time, samp_frequency from register
>         .write_raw = ltr390_write_raw,-------> writes sampling
> frequency, scale, int_time into register.
>         .read_avail = ltr390_read_avail, ----------------> no reg read
>         .read_event_value = ltr390_read_event_value,-----> reads
> interrupt threshold value from register
>         .read_event_config = ltr390_read_event_config,----> reads
> INT_CFG register
>         .write_event_value = ltr390_write_event_value,------> writes
> threshold value into register
>         .write_event_config = ltr390_write_event_config,-----> writes
> into INT_CFG register
>         .debugfs_reg_access = ltr390_debugfs_reg_access,-->
> reads/writes into all registers
> };
> 
> Going by your comment, I think I need it only in read_raw and
> write_event_config.

Yes, I think so but you should test that nothing fails  on the other calls
without it.

> There is a similar sensor, ltrf216a (same company, liteon). There only
> read_raw has
> these pm_runtime_* calls. Let me know your thoughts. I will make the change and
> include updated testing details.
> 
> Thanks,
> Akshay
>