Message-ID: <20250830192538.3b508c5c@jic23-huawei>
Date: Sat, 30 Aug 2025 19:25:38 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Akshay Jindal <akshayaj.lkd@...il.com>
Cc: anshulusr@...il.com, dlechner@...libre.com, nuno.sa@...log.com,
 andy@...nel.org, shuah@...nel.org, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] iio: light: ltr390: Implement runtime PM support

On Sat, 30 Aug 2025 17:05:00 +0530
Akshay Jindal <akshayaj.lkd@...il.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@...il.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;
>  }

Powered by blists - more mailing lists