Message-ID: <20250910201212.5d9f57bc@jic23-huawei>
Date: Wed, 10 Sep 2025 20:12:12 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Akshay Jindal <akshayaj.lkd@...il.com>
Cc: Andy Shevchenko <andy.shevchenko@...il.com>, 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 v7] iio: light: ltr390: Implement runtime PM support

On Wed, 10 Sep 2025 18:06:32 +0530
Akshay Jindal <akshayaj.lkd@...il.com> wrote:

> Hi Andy,
> Thank you very much for your valuable feedback.
> I do have a small request regarding the review process. Over the past 3–4
> versions,most of the comments have been about fixing indentations and
> improving code readability. I would kindly request if it would be possible
> to consolidate such cosmetic comments into a single review round.
> 
> I completely understand that incremental feedback makes sense when the code
> is actively changing, but if the changes are minimal, spreading out minor
> suggestions over multiple review cycles tends to unnecessarily increase the
> turnaround time.
> 
> Your support in this would help me address the comments more efficiently.
> 
> On Wed, Sep 10, 2025 at 12:47 PM Andy Shevchenko
> <andy.shevchenko@...il.com> wrote:
> >
> > On Tue, Sep 9, 2025 at 10:47 PM Akshay Jindal <akshayaj.lkd@...il.com> wrote:  
> > > +static int ltr390_read_raw(struct iio_dev *iio_device,
> > > +                          struct iio_chan_spec const *chan, int *val,
> > > +                          int *val2, long mask)  
> >
> >
> > Also logical split might be better, i.e. putting val and val2 on the
> > same line. Then mask will be on the next one  
> Ok, will fix.
> 
> > >  static void ltr390_powerdown(void *priv)
> > >  {
> > >         struct ltr390_data *data = priv;
> > > +       struct device *dev = &data->client->dev;
> > > +       int ret;
> > >
> > >         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) {
> > > +               ret = regmap_clear_bits(data->regmap, LTR390_INT_CFG, LTR390_LS_INT_EN);
> > > +               if (ret < 0)
> > > +                       dev_err(dev, "failed to disable interrupts\n");  
> >
> > In event_config we assure that IRQ is enabled.  
> What do you mean here?
> >  
> > > +               data->irq_enabled = false;  
> >
> > Here we may lie about the facts. What will the driver do, if the IRQ
> > is triggered just before this line?  
> I don't see why the device will trigger an IRQ, when we are disabling
> the INT via
> regmap_clear_bits before this.

There may be a theoretical race with an incoming interrupt only being observed
after the regmap_clear_bits() has returned.  The question becomes whether that
matters.  data->irq_enabled is not relevant to the actual interrupt handling
and the power down isn't stopping registers being read, just new data being
captured.  So should be fine.

> 
> >  
> > > +               pm_runtime_put_autosuspend(&data->client->dev);  
> >
> > You have dev, use it.  
> Ok, will fix.
> 
> >
> > But where is the symmetrical pm_runtime_get*()?  

> 
> This is the fundamental approach of managing IRQ handling + runtime PM.
> suggested by Jonathan in preliminary rounds and employed by many drivers.
> "When enabling IRQ, increase the refcount, and decrease when disabling"
> This is done because ltr390 does not have a wakeup functionality.

It's not really about enabling the irq. It's more about turning on autonomous
data capture that in turn is related to enabling events (which indeed cause
irqs).  So similar to what we tend to do with runtime pm in the buffer enable
callbacks as we are intentionally holding the power on.

So this function is effectively disabling the events - be it in a fast
fashion given it happens on driver unbind.


> 
> put_autosuspend is tied to disable which can happen in 2 places:
> 1. event_config.
> 2. powerdown (if irq enabled).
> 
> pm_runtime_get* is tied to enable which can happen only at 1 place:
> 1. event_config.
> 
> If IRQ was enabled before power down, that means in event_config
> we had already called pm_runtime_get* and increased the refcount to 1.
> This will come down to 0 as a result of either of disabling event_config
> or powerdown.

Exactly this bit.  Maybe this would all have been cleaner if we had
just called this events_enabled rather than irq_enabled?

Andy, if you are fine with the explanation I'll tidy up the minor stuff
whilst applying.

Jonathan

> 
> Thanks,
> Akshay.

Powered by blists - more mailing lists