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 v6:
=================
1. Jonathan's feedback:
-> Check return value of ltr390_pm_init() and act accordingly.

Changes since v5:
=================
1. Andy's feedback:
-> Fix indentation of parameters.
-> Make dev_err single lined.
-> Ensure proper spacing in the comments.

Changes since v4:
=================
1. Andy's feedback:
-> Fix indentation at various places.
-> Enhance readability of the code by:
--->putting regmap API from multiple lines to a single line.
--->using dev instead of &data->client->dev in dev_info.

Changes since v3:
=================
1. Jonathan's feedback:
-> Keep runtime PM calls only in read_raw, write_event_config and powerdown.
-> Updated Testing details for the changes.

2. Andy's feedback:
-> Move include of pm_runtime.h above include of regmap.h

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:
================

Extra Testing for v4:
---------------------
1. Verified no change in refcount while:
-> write_raw(): did write on in_illuminance_scale sysfs attribute.
-> read_event_value(): did read the interrupt threshold & interrupt period sysfs attributes.
-> read_event_config(): did read on interrupt_en sysfs attributes.
-> write_event_value(): did write on interrupt threshold & period sysfs attributes.

2. Debugfs testing:
-> did write from debugfs into INT_PST (0x1a) register. Verified the value change by reading processed value from interrupt period.
No change in refcount observed.

3. Refcount change only observed when following are triggered:
-> read_raw(): reading in_illuminance_raw, in_illuminance_scale
-> write_event_config(): enabling or disabling interrupts.
-> powerdown(): drops from 1 to 0, if events were enabled before rmmod, else remains 0.

Testing done till v3 (repeated for v4):
---------------------------------------
-> 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 (repeated for v4 ):
--------------------------------------------------
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 (repeated for v4):
--------------------------------------------------
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 | 136 ++++++++++++++++++++++++++++++++-----
 1 file changed, 119 insertions(+), 17 deletions(-)

diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
index 2e1cf62e8201..2d8449aeab18 100644
--- a/drivers/iio/light/ltr390.c
+++ b/drivers/iio/light/ltr390.c
@@ -26,6 +26,7 @@
 #include <linux/math.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 
 #include <linux/iio/iio.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_do_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_do_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 };
@@ -586,11 +610,11 @@ static int ltr390_read_event_config(struct iio_dev *indio_dev,
 	return FIELD_GET(LTR390_LS_INT_EN, status);
 }
 
-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)
+static int ltr390_do_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)
 {
 	struct ltr390_data *data = iio_priv(indio_dev);
 	int ret;
@@ -598,7 +622,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,6 +646,37 @@ 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_do_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)
@@ -683,17 +737,38 @@ static irqreturn_t ltr390_interrupt_handler(int irq, void *private)
 static void ltr390_powerdown(void *priv)
 {
 	struct ltr390_data *data = priv;
+	struct device *dev = &data->client->dev;
+	int ret;
 
 	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) {
+		ret = regmap_clear_bits(data->regmap, LTR390_INT_CFG, LTR390_LS_INT_EN);
+		if (ret < 0)
+			dev_err(dev, "failed to disable interrupts\n");
+
+		data->irq_enabled = false;
+		pm_runtime_put_autosuspend(&data->client->dev);
+	}
+
+	ret = regmap_clear_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_SENSOR_ENABLE);
+	if (ret < 0)
+		dev_err(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");
 
-	if (regmap_clear_bits(data->regmap, LTR390_MAIN_CTRL,
-			LTR390_SENSOR_ENABLE) < 0)
-		dev_err(&data->client->dev, "failed to disable sensor\n");
+	pm_runtime_set_autosuspend_delay(dev, 1000);
+	pm_runtime_use_autosuspend(dev);
+	return 0;
 }
 
 static int ltr390_probe(struct i2c_client *client)
@@ -708,6 +783,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 +798,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 +842,10 @@ static int ltr390_probe(struct i2c_client *client)
 					     "request irq (%d) failed\n", client->irq);
 	}
 
+	ret = ltr390_pm_init(data);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to initialize runtime PM\n");
+
 	return devm_iio_device_register(dev, indio_dev);
 }
 
@@ -784,7 +867,26 @@ 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 +904,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

On Tue, Sep 9, 2025 at 10:47 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.

...

> +static int ltr390_read_raw(struct iio_dev *iio_device,
> +                          struct iio_chan_spec const *chan, int *val,
> +                          int *val2, long mask)

Isn't the mask unsigned long? Jonathan, do we get this fixed already?

Also logical split might be better, i.e. putting val and val2 on the
same line. Then mask will be on the next one

...

>  static void ltr390_powerdown(void *priv)
>  {
>         struct ltr390_data *data = priv;
> +       struct device *dev = &data->client->dev;
> +       int ret;
>
>         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) {
> +               ret = regmap_clear_bits(data->regmap, LTR390_INT_CFG, LTR390_LS_INT_EN);
> +               if (ret < 0)
> +                       dev_err(dev, "failed to disable interrupts\n");

In event_config we assure that IRQ is enabled.

> +               data->irq_enabled = false;

Here we may lie about the facts. What will the driver do, if the IRQ
is triggered just before this line?

> +               pm_runtime_put_autosuspend(&data->client->dev);

You have dev, use it.

But where is the symmetrical pm_runtime_get*()?

> +       }
> +
> +       ret = regmap_clear_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_SENSOR_ENABLE);
> +       if (ret < 0)
> +               dev_err(dev, "failed to disable sensor\n");
> +}


-- 
With Best Regards,
Andy Shevchenko

Hi Andy,
Thank you very much for your valuable feedback.
I do have a small request regarding the review process. Over the past 3–4
versions,most of the comments have been about fixing indentations and
improving code readability. I would kindly request if it would be possible
to consolidate such cosmetic comments into a single review round.

I completely understand that incremental feedback makes sense when the code
is actively changing, but if the changes are minimal, spreading out minor
suggestions over multiple review cycles tends to unnecessarily increase the
turnaround time.

Your support in this would help me address the comments more efficiently.

On Wed, Sep 10, 2025 at 12:47 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Tue, Sep 9, 2025 at 10:47 PM Akshay Jindal <akshayaj.lkd@gmail.com> wrote:
> > +static int ltr390_read_raw(struct iio_dev *iio_device,
> > +                          struct iio_chan_spec const *chan, int *val,
> > +                          int *val2, long mask)
>
>
> Also logical split might be better, i.e. putting val and val2 on the
> same line. Then mask will be on the next one
Ok, will fix.

> >  static void ltr390_powerdown(void *priv)
> >  {
> >         struct ltr390_data *data = priv;
> > +       struct device *dev = &data->client->dev;
> > +       int ret;
> >
> >         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) {
> > +               ret = regmap_clear_bits(data->regmap, LTR390_INT_CFG, LTR390_LS_INT_EN);
> > +               if (ret < 0)
> > +                       dev_err(dev, "failed to disable interrupts\n");
>
> In event_config we assure that IRQ is enabled.
What do you mean here?
>
> > +               data->irq_enabled = false;
>
> Here we may lie about the facts. What will the driver do, if the IRQ
> is triggered just before this line?
I don't see why the device will trigger an IRQ, when we are disabling
the INT via
regmap_clear_bits before this.

>
> > +               pm_runtime_put_autosuspend(&data->client->dev);
>
> You have dev, use it.
Ok, will fix.

>
> But where is the symmetrical pm_runtime_get*()?

This is the fundamental approach of managing IRQ handling + runtime PM.
suggested by Jonathan in preliminary rounds and employed by many drivers.
"When enabling IRQ, increase the refcount, and decrease when disabling"
This is done because ltr390 does not have a wakeup functionality.

put_autosuspend is tied to disable which can happen in 2 places:
1. event_config.
2. powerdown (if irq enabled).

pm_runtime_get* is tied to enable which can happen only at 1 place:
1. event_config.

If IRQ was enabled before power down, that means in event_config
we had already called pm_runtime_get* and increased the refcount to 1.
This will come down to 0 as a result of either of disabling event_config
or powerdown.

Thanks,
Akshay.

On Wed, 10 Sep 2025 18:06:32 +0530
Akshay Jindal <akshayaj.lkd@gmail.com> wrote:

> Hi Andy,
> Thank you very much for your valuable feedback.
> I do have a small request regarding the review process. Over the past 3–4
> versions,most of the comments have been about fixing indentations and
> improving code readability. I would kindly request if it would be possible
> to consolidate such cosmetic comments into a single review round.
> 
> I completely understand that incremental feedback makes sense when the code
> is actively changing, but if the changes are minimal, spreading out minor
> suggestions over multiple review cycles tends to unnecessarily increase the
> turnaround time.
> 
> Your support in this would help me address the comments more efficiently.
> 
> On Wed, Sep 10, 2025 at 12:47 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Tue, Sep 9, 2025 at 10:47 PM Akshay Jindal <akshayaj.lkd@gmail.com> wrote:  
> > > +static int ltr390_read_raw(struct iio_dev *iio_device,
> > > +                          struct iio_chan_spec const *chan, int *val,
> > > +                          int *val2, long mask)  
> >
> >
> > Also logical split might be better, i.e. putting val and val2 on the
> > same line. Then mask will be on the next one  
> Ok, will fix.
> 
> > >  static void ltr390_powerdown(void *priv)
> > >  {
> > >         struct ltr390_data *data = priv;
> > > +       struct device *dev = &data->client->dev;
> > > +       int ret;
> > >
> > >         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) {
> > > +               ret = regmap_clear_bits(data->regmap, LTR390_INT_CFG, LTR390_LS_INT_EN);
> > > +               if (ret < 0)
> > > +                       dev_err(dev, "failed to disable interrupts\n");  
> >
> > In event_config we assure that IRQ is enabled.  
> What do you mean here?
> >  
> > > +               data->irq_enabled = false;  
> >
> > Here we may lie about the facts. What will the driver do, if the IRQ
> > is triggered just before this line?  
> I don't see why the device will trigger an IRQ, when we are disabling
> the INT via
> regmap_clear_bits before this.

There may be a theoretical race with an incoming interrupt only being observed
after the regmap_clear_bits() has returned.  The question becomes whether that
matters.  data->irq_enabled is not relevant to the actual interrupt handling
and the power down isn't stopping registers being read, just new data being
captured.  So should be fine.

> 
> >  
> > > +               pm_runtime_put_autosuspend(&data->client->dev);  
> >
> > You have dev, use it.  
> Ok, will fix.
> 
> >
> > But where is the symmetrical pm_runtime_get*()?  

> 
> This is the fundamental approach of managing IRQ handling + runtime PM.
> suggested by Jonathan in preliminary rounds and employed by many drivers.
> "When enabling IRQ, increase the refcount, and decrease when disabling"
> This is done because ltr390 does not have a wakeup functionality.

It's not really about enabling the irq. It's more about turning on autonomous
data capture that in turn is related to enabling events (which indeed cause
irqs).  So similar to what we tend to do with runtime pm in the buffer enable
callbacks as we are intentionally holding the power on.

So this function is effectively disabling the events - be it in a fast
fashion given it happens on driver unbind.


> 
> put_autosuspend is tied to disable which can happen in 2 places:
> 1. event_config.
> 2. powerdown (if irq enabled).
> 
> pm_runtime_get* is tied to enable which can happen only at 1 place:
> 1. event_config.
> 
> If IRQ was enabled before power down, that means in event_config
> we had already called pm_runtime_get* and increased the refcount to 1.
> This will come down to 0 as a result of either of disabling event_config
> or powerdown.

Exactly this bit.  Maybe this would all have been cleaner if we had
just called this events_enabled rather than irq_enabled?

Andy, if you are fine with the explanation I'll tidy up the minor stuff
whilst applying.

Jonathan

> 
> Thanks,
> Akshay.

On Wed, Sep 10, 2025 at 10:12 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Wed, 10 Sep 2025 18:06:32 +0530
> Akshay Jindal <akshayaj.lkd@gmail.com> wrote:

> > Thank you very much for your valuable feedback.
> > I do have a small request regarding the review process. Over the past 3–4
> > versions,most of the comments have been about fixing indentations and
> > improving code readability. I would kindly request if it would be possible
> > to consolidate such cosmetic comments into a single review round.
> >
> > I completely understand that incremental feedback makes sense when the code
> > is actively changing, but if the changes are minimal, spreading out minor
> > suggestions over multiple review cycles tends to unnecessarily increase the
> > turnaround time.
> >
> > Your support in this would help me address the comments more efficiently.

I can't always see _all_ problems at once, I am not a robot. I will
try my best, though.

...

> Andy, if you are fine with the explanation I'll tidy up the minor stuff
> whilst applying.

Yes, I am fine, go with it, thanks!

-- 
With Best Regards,
Andy Shevchenko

On Wed, 10 Sep 2025 23:24:22 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Wed, Sep 10, 2025 at 10:12 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > On Wed, 10 Sep 2025 18:06:32 +0530
> > Akshay Jindal <akshayaj.lkd@gmail.com> wrote:  
> 
> > > Thank you very much for your valuable feedback.
> > > I do have a small request regarding the review process. Over the past 3–4
> > > versions,most of the comments have been about fixing indentations and
> > > improving code readability. I would kindly request if it would be possible
> > > to consolidate such cosmetic comments into a single review round.
> > >
> > > I completely understand that incremental feedback makes sense when the code
> > > is actively changing, but if the changes are minimal, spreading out minor
> > > suggestions over multiple review cycles tends to unnecessarily increase the
> > > turnaround time.
> > >
> > > Your support in this would help me address the comments more efficiently.  
> 
> I can't always see _all_ problems at once, I am not a robot. I will
> try my best, though.
> 
> ...
> 
> > Andy, if you are fine with the explanation I'll tidy up the minor stuff
> > whilst applying.  
> 
> Yes, I am fine, go with it, thanks!
> 

Applied with this diff;
diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
index 2d8449aeab18..a2b804e9089a 100644
--- a/drivers/iio/light/ltr390.c
+++ b/drivers/iio/light/ltr390.c
@@ -284,8 +284,8 @@ static int ltr390_do_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)
+                          struct iio_chan_spec const *chan,
+                          int *val, int *val2, long mask)
 {
        int ret;
        struct ltr390_data *data = iio_priv(iio_device);
@@ -749,7 +749,7 @@ static void ltr390_powerdown(void *priv)
                        dev_err(dev, "failed to disable interrupts\n");
 
                data->irq_enabled = false;
-               pm_runtime_put_autosuspend(&data->client->dev);
+               pm_runtime_put_autosuspend(dev);
        }
 
        ret = regmap_clear_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_SENSOR_ENABLE);

Thanks,

Jonathan

Thanks Jonathan and Andy for the support.

Regards,
Akshay.

On Sat, Sep 13, 2025 at 5:47 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Wed, 10 Sep 2025 23:24:22 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> > On Wed, Sep 10, 2025 at 10:12 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > > On Wed, 10 Sep 2025 18:06:32 +0530
> > > Akshay Jindal <akshayaj.lkd@gmail.com> wrote:
> >
> > > > Thank you very much for your valuable feedback.
> > > > I do have a small request regarding the review process. Over the past 3–4
> > > > versions,most of the comments have been about fixing indentations and
> > > > improving code readability. I would kindly request if it would be possible
> > > > to consolidate such cosmetic comments into a single review round.
> > > >
> > > > I completely understand that incremental feedback makes sense when the code
> > > > is actively changing, but if the changes are minimal, spreading out minor
> > > > suggestions over multiple review cycles tends to unnecessarily increase the
> > > > turnaround time.
> > > >
> > > > Your support in this would help me address the comments more efficiently.
> >
> > I can't always see _all_ problems at once, I am not a robot. I will
> > try my best, though.
> >
> > ...
> >
> > > Andy, if you are fine with the explanation I'll tidy up the minor stuff
> > > whilst applying.
> >
> > Yes, I am fine, go with it, thanks!
> >
>
> Applied with this diff;
> diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
> index 2d8449aeab18..a2b804e9089a 100644
> --- a/drivers/iio/light/ltr390.c
> +++ b/drivers/iio/light/ltr390.c
> @@ -284,8 +284,8 @@ static int ltr390_do_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)
> +                          struct iio_chan_spec const *chan,
> +                          int *val, int *val2, long mask)
>  {
>         int ret;
>         struct ltr390_data *data = iio_priv(iio_device);
> @@ -749,7 +749,7 @@ static void ltr390_powerdown(void *priv)
>                         dev_err(dev, "failed to disable interrupts\n");
>
>                 data->irq_enabled = false;
> -               pm_runtime_put_autosuspend(&data->client->dev);
> +               pm_runtime_put_autosuspend(dev);
>         }
>
>         ret = regmap_clear_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_SENSOR_ENABLE);
>
> Thanks,
>
> Jonathan

On Wed, 10 Sep 2025 10:17:00 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Sep 9, 2025 at 10:47 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.  
> 
> ...
> 
> > +static int ltr390_read_raw(struct iio_dev *iio_device,
> > +                          struct iio_chan_spec const *chan, int *val,
> > +                          int *val2, long mask)  
> 
> Isn't the mask unsigned long? Jonathan, do we get this fixed already?

Whilst it could (and probably should) be unsigned, it's not actually a mask.
That naming is a historical mess up / evolution thing - long ago it was a bitmap.
It is now the index of a bit in the mask.  So this is unrelated(ish) to the
recent fixes around the actual bitmaps/bitmasks.

Changing this one is a lot more painful than the recent fix to the infomask
as it means changing the signature in every driver.
I'm doubtful on whether this one is worth the churn.

> 
> Also logical split might be better, i.e. putting val and val2 on the
> same line. Then mask will be on the next one
> 
> ...
> 
> >  static void ltr390_powerdown(void *priv)
> >  {
> >         struct ltr390_data *data = priv;
> > +       struct device *dev = &data->client->dev;
> > +       int ret;
> >
> >         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) {
> > +               ret = regmap_clear_bits(data->regmap, LTR390_INT_CFG, LTR390_LS_INT_EN);
> > +               if (ret < 0)
> > +                       dev_err(dev, "failed to disable interrupts\n");  
> 
> In event_config we assure that IRQ is enabled.
> 
> > +               data->irq_enabled = false;  
> 
> Here we may lie about the facts. What will the driver do, if the IRQ
> is triggered just before this line?
> 
> > +               pm_runtime_put_autosuspend(&data->client->dev);  
> 
> You have dev, use it.
> 
> But where is the symmetrical pm_runtime_get*()?
> 
> > +       }
> > +
> > +       ret = regmap_clear_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_SENSOR_ENABLE);
> > +       if (ret < 0)
> > +               dev_err(dev, "failed to disable sensor\n");
> > +}  
> 
> 

Hi Jonathan,
Seeking your approval. If there is a requirement for v8, I can send that too.

Thanks,
Akshay

On Wed, Sep 10, 2025 at 4:32 PM Jonathan Cameron
<jonathan.cameron@huawei.com> wrote:
>
> On Wed, 10 Sep 2025 10:17:00 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> > On Tue, Sep 9, 2025 at 10:47 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.
> >
> > ...
> >
> > > +static int ltr390_read_raw(struct iio_dev *iio_device,
> > > +                          struct iio_chan_spec const *chan, int *val,
> > > +                          int *val2, long mask)
> >
> > Isn't the mask unsigned long? Jonathan, do we get this fixed already?
>
> Whilst it could (and probably should) be unsigned, it's not actually a mask.
> That naming is a historical mess up / evolution thing - long ago it was a bitmap.
> It is now the index of a bit in the mask.  So this is unrelated(ish) to the
> recent fixes around the actual bitmaps/bitmasks.
>
> Changing this one is a lot more painful than the recent fix to the infomask
> as it means changing the signature in every driver.
> I'm doubtful on whether this one is worth the churn.

On Wed, 10 Sep 2025 23:30:36 +0530
Akshay Jindal <akshayaj.lkd@gmail.com> wrote:

> Hi Jonathan,
> Seeking your approval. If there is a requirement for v8, I can send that too.

Wait a day or so just to see if Andy is fine with your reply to him.

Thanks,

Jonathan

> 
> Thanks,
> Akshay
> 
> On Wed, Sep 10, 2025 at 4:32 PM Jonathan Cameron
> <jonathan.cameron@huawei.com> wrote:
> >
> > On Wed, 10 Sep 2025 10:17:00 +0300
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> >  
> > > On Tue, Sep 9, 2025 at 10:47 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.  
> > >
> > > ...
> > >  
> > > > +static int ltr390_read_raw(struct iio_dev *iio_device,
> > > > +                          struct iio_chan_spec const *chan, int *val,
> > > > +                          int *val2, long mask)  
> > >
> > > Isn't the mask unsigned long? Jonathan, do we get this fixed already?  
> >
> > Whilst it could (and probably should) be unsigned, it's not actually a mask.
> > That naming is a historical mess up / evolution thing - long ago it was a bitmap.
> > It is now the index of a bit in the mask.  So this is unrelated(ish) to the
> > recent fixes around the actual bitmaps/bitmasks.
> >
> > Changing this one is a lot more painful than the recent fix to the infomask
> > as it means changing the signature in every driver.
> > I'm doubtful on whether this one is worth the churn.  
>