Message-ID: <20250831161149.27547d16@jic23-huawei>
Date: Sun, 31 Aug 2025 16:11:49 +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 Sun, 31 Aug 2025 11:09:26 +0530
Akshay Jindal <akshayaj.lkd@...il.com> wrote:

> On Sat, Aug 30, 2025 at 11:55 PM Jonathan Cameron <jic23@...nel.org> wrote:
> >
> > 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.  
> bit (1) of the MAIN Control register simply puts the sensor in standby mode.
> It does not restrict register access. The logic behind doing pm_resume
> is to increase the refcount before doing any IO on the sensor.
> 

Why do we care about that refcount?  We care if the device is in a state
to perform IO. In this particular case runtime pm callbacks don't affect
that unless I'm missing something more complex!


> >
> > Also, we should just read the config register to find out if the
> > event is enabled, not bother having a separate cache of that one bit.  
> 
> I have seen the code. At Least 30+ drivers are maintaining this flag.
> If we go down the road of not using a flag, it would be an unnecessary
> load on the bus everytime an interrupt is configured.
> Consider a scenario, where instead of toggling, you are doing enable after
> enable or disable after disable. This is expected to be kind of a no-op.
> Here the callback will not do anything except for reading the interrupt config
> register. I say why to read this register also when a simple flag can
> give us that
> information.

As per the other thread I'll agree to the flag, though to my mind it
is  an inferior solution and in many of those other drivers is there
because of the history of how they evolved, not because it is the best
solution.

> >
> > 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.
> >  
> > > +     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.  
> Are you suggesting to remove pm_runtime_* calls from all the functions or
> just the debugfs function?

All the ones where it's not needed.  So as far as I can see it is only
needed when actually grabbing date.

Jonathan

> 
> Thanks,
> Akshay

Powered by blists - more mailing lists