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

[PATCH v4] 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 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 | 121 ++++++++++++++++++++++++++++++++++---
 1 file changed, 111 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
index 2e1cf62e8201..2cb72cc463ef 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_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 };
@@ -586,7 +610,7 @@ 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,
+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 +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_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)
@@ -687,15 +741,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 +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,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 +864,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 +903,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 v4] iio: light: ltr390: Implement runtime PM support

[Andy Shevchenko]

On Tue, Sep 02, 2025 at 12:12:36AM +0530, Akshay Jindal 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)
> +
> +static int __ltr390_read_raw(struct iio_dev *iio_device,
> +			struct iio_chan_spec const *chan, int *val,
> +			int *val2, long mask)

Can we avoid using leading __ (double underscore)? Better name is
ltr390_read_raw_no_pm(). But you may find even better one.

...

> -static int ltr390_write_event_config(struct iio_dev *indio_dev,
> +static int __ltr390_write_event_config(struct iio_dev *indio_dev,

Ditto.

...

> +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;

^^^ (1) for the reference below.

> +	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;
> +}

...

>  	/* 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)

Wrong indentation, hard to read line, either one line, or do better. Actually
why not assign it to ret? The above not only simple style issue, but also makes
readability much harder as the semantics of '0' is completely hidden. This style
is discouraged.

> +			dev_err(&data->client->dev,
> +					"failed to disable interrupts\n");

Why not doing (1) here as well and with that

			dev_err(dev, "failed to disable interrupts\n");

besides the fact of wrong indentation.

> +		data->irq_enabled = false;
> +
> +		pm_runtime_put_autosuspend(&data->client->dev);
> +	}

...

> +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");

Something wrong with your editor or so, please check and make proper
indentation _everywhere_ (in your changes).

> +	pm_runtime_set_autosuspend_delay(dev, 1000);
> +	pm_runtime_use_autosuspend(dev);
> +	return 0;
> +}

...

> +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);

I would make it one line despite being 87 character long.

> +}
> +
> +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);

Ditto. (Here it's even shorter)

> +}

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Sep 02, 2025 at 03:57:24PM +0300, Andy Shevchenko wrote:
> On Tue, Sep 02, 2025 at 12:12:36AM +0530, Akshay Jindal wrote:

...

> > +		if (regmap_clear_bits(data->regmap, LTR390_INT_CFG,
> > +					LTR390_LS_INT_EN) < 0)
> 
> Wrong indentation, hard to read line, either one line, or do better. Actually
> why not assign it to ret? The above not only simple style issue, but also makes
> readability much harder as the semantics of '0' is completely hidden. This style
> is discouraged.

Noticing that this is existing style I recommend to have a precursor patch to
fix this and any other place like this first.

-- 
With Best Regards,
Andy Shevchenko

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

[Akshay Jindal]

Thanks Andy for the review. Follow-ups inline.

On Tue, Sep 2, 2025 at 6:27 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Tue, Sep 02, 2025 at 12:12:36AM +0530, Akshay Jindal wrote:
> > Implement runtime power management for the LTR390 sensor. The device
> > autosuspends after 1s of idle time, reducing current consumption from
> > 100 渙 in active mode to 1 渙 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)
> > +
> > +static int __ltr390_read_raw(struct iio_dev *iio_device,
> > +                     struct iio_chan_spec const *chan, int *val,
> > +                     int *val2, long mask)
>
> Can we avoid using leading __ (double underscore)? Better name is
> ltr390_read_raw_no_pm(). But you may find even better one.

renamed as follows:
__ltr390_read_raw()-->ltr390_do_read_raw()

>
> ...
>
> > -static int ltr390_write_event_config(struct iio_dev *indio_dev,
> > +static int __ltr390_write_event_config(struct iio_dev *indio_dev,
>
> Ditto.
__ltr390_write_event_config--->ltr390_do_event_config()

>
> ...
>
> > +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;
>
> ^^^ (1) for the reference below.
>
> >       /* 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)
>
> Wrong indentation, hard to read line, either one line, or do better. Actually
> why not assign it to ret? The above not only simple style issue, but also makes
> readability much harder as the semantics of '0' is completely hidden. This style
> is discouraged.
Earlier did not use ret here, because powerdown function is of type void.
But if readability is the issue, I have used ret.

Regarding clubbing into 1 line, I have my reservations there. I think
we should not
violate the 80 char line limit. Also since the line is already 1-level
indented (begins
at 9th column, due to if(data->irq_enabled) check), the spillover will
be too much.
The readability does not seem to be taking a substantial hit here.
Let me know if this is non-negotiable for you. Will happily make the changes.
>
> > +                     dev_err(&data->client->dev,
> > +                                     "failed to disable interrupts\n");
>
> Why not doing (1) here as well and with that
done
>
>                         dev_err(dev, "failed to disable interrupts\n");
>
> besides the fact of wrong indentation.
fixed
>
> > +             data->irq_enabled = false;
> > +
> > +             pm_runtime_put_autosuspend(&data->client->dev);
> > +     }
>
> ...
>
> > +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");
>
> Something wrong with your editor or so, please check and make proper
> indentation _everywhere_ (in your changes).
Fixed.
Editor is fine, just did not pay attention here. Matched with existing dev_err
statements and understood what you meant.
>
> > +     pm_runtime_set_autosuspend_delay(dev, 1000);
> > +     pm_runtime_use_autosuspend(dev);
> > +     return 0;
> > +}
>
> ...
>
> > +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);
>
> I would make it one line despite being 87 character long.
Same as above
>
> > +}
> > +
> > +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);
>
> Ditto. (Here it's even shorter)
Same as above

Thanks,
Akshay

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

[Andy Shevchenko]

On Wed, Sep 03, 2025 at 09:15:53AM +0530, Akshay Jindal wrote:
> On Tue, Sep 2, 2025 at 6:27 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Tue, Sep 02, 2025 at 12:12:36AM +0530, Akshay Jindal wrote:

...

> > >       /* 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)
> >
> > Wrong indentation, hard to read line, either one line, or do better.
> > Actually why not assign it to ret? The above not only simple style issue,
> > but also makes readability much harder as the semantics of '0' is
> > completely hidden. This style is discouraged.
> Earlier did not use ret here, because powerdown function is of type void.
> But if readability is the issue, I have used ret.
> 
> Regarding clubbing into 1 line, I have my reservations there. I think we
> should not violate the 80 char line limit.

Shouldn't != mustn't, esp. when it's about readability.

> Also since the line is already 1-level indented (begins at 9th column, due to
> if(data->irq_enabled) check), the spillover will be too much. The readability
> does not seem to be taking a substantial hit here. Let me know if this is
> non-negotiable for you. Will happily make the changes.

		ret = regmap_clear_bits(data->regmap, LTR390_INT_CFG, LTR390_LS_INT_EN);

only 88 characters. One can make it shorter, yes

	struct regmap *map = data->regmap;

		ret = regmap_clear_bits(map, LTR390_INT_CFG, LTR390_LS_INT_EN);


79 characters.

-- 
With Best Regards,
Andy Shevchenko

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

[Akshay Jindal]

On Wed, Sep 3, 2025 at 4:07 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Wed, Sep 03, 2025 at 09:15:53AM +0530, Akshay Jindal wrote:
> > On Tue, Sep 2, 2025 at 6:27 PM Andy Shevchenko
> > <andriy.shevchenko@intel.com> wrote:
> > > On Tue, Sep 02, 2025 at 12:12:36AM +0530, Akshay Jindal wrote:
>
> ...
>
> > > >       /* 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)
> > >
> > > Wrong indentation, hard to read line, either one line, or do better.
> > > Actually why not assign it to ret? The above not only simple style issue,
> > > but also makes readability much harder as the semantics of '0' is
> > > completely hidden. This style is discouraged.
> > Earlier did not use ret here, because powerdown function is of type void.
> > But if readability is the issue, I have used ret.
> >
> > Regarding clubbing into 1 line, I have my reservations there. I think we
> > should not violate the 80 char line limit.
>
> Shouldn't != mustn't, esp. when it's about readability.
Ok, made the changes.
>
> > Also since the line is already 1-level indented (begins at 9th column, due to
> > if(data->irq_enabled) check), the spillover will be too much. The readability
> > does not seem to be taking a substantial hit here. Let me know if this is
> > non-negotiable for you. Will happily make the changes.
>
>                 ret = regmap_clear_bits(data->regmap, LTR390_INT_CFG, LTR390_LS_INT_EN);
>
> only 88 characters. One can make it shorter, yes
>
>         struct regmap *map = data->regmap;
>
>                 ret = regmap_clear_bits(map, LTR390_INT_CFG, LTR390_LS_INT_EN);
>
>
> 79 characters.
>
Sent a v5. Kindly review.

Thanks,
Akshay