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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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