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

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

On Wed, Sep 03, 2025 at 04:56:43PM +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_do_read_raw(struct iio_dev *iio_device,
> +			struct iio_chan_spec const *chan, int *val,
> +			int *val2, long mask)

The new indentation is broken.

static int ltr390_do_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)

For example here it's okay.

...

> -static int ltr390_write_event_config(struct iio_dev *indio_dev,
> +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,

You forgot fixing an indentation of the parameters.

...

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

Broken indentation.

...

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

I think we still can refactor this as following if _do_event_config() does not
have side-effects on irq_enabled field. (Otherwise that side-effect should be
documented.)

// we also assume indio_dev can be retrieved from data
_on_and_do_event_config()
{
	struct device *dev = &data->client->dev;
	int ret;

	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;

	return ltr390_do_event_config(indio_dev, chan, type, dir, state);
}

_do_event_config_and_off()
{
	struct device *dev = &data->client->dev;
	int ret;

	ret = ltr390_do_event_config(indio_dev, chan, type, dir, state);

	data->irq_enabled = false;
	pm_runtime_put_autosuspend(dev);

	return ret;
}

write_event_config()
{
	struct ltr390_data *data = iio_priv(indio_dev);

	guard(mutex)(&data->lock);

	if (state == data->irq_enabled)
		return ltr390_do_event_config(indio_dev, chan, type, dir, state);

	if (state)
		return _on_and_event_config(data, ...)
	else // yes, it's redundant, but for better coupling of these two branches
		return _event_config_and_off(data, ...)

}

But this is quite verbose, hence I leave it to you and others to decide if the
originally proposed code is better.

...

> +	ret = devm_pm_runtime_set_active_enabled(dev);
> +	if (ret)

> +		return dev_err_probe(dev, ret,
> +				     "failed to enable runtime PM\n");

It's exactly one line of 80 characters!

...

> +	/* default value of irq_enabled is false*/

Missing space

> +	data->irq_enabled = false;

-- 
With Best Regards,
Andy Shevchenko