Re: [PATCH] iio: light: ltr390: Add runtime PM support

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 26 Aug 2025 02:29:12 +0530
Akshay Jindal <akshayaj.lkd@gmail.com> wrote:

> Hi Jonathan,
> Thanks for your review. Please see my followup inline.
> 
> On Mon, Aug 25, 2025 at 7:56 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Fri, 22 Aug 2025 23:33:26 +0530
> > Akshay Jindal <akshayaj.lkd@gmail.com> wrote:  
> > > +
> > > +     if (!state) {
> > > +             ret = regmap_clear_bits(data->regmap, LTR390_INT_CFG, LTR390_LS_INT_EN);
> > > +             data->irq_enabled = false;  
> >
> > Just take an extra reference to runtime pm on enable of event and put it disable.
> > Then no need for special handling with a local flag etc.  
> 
> Consider a scenario, where the user only disables the event instead of
> enabling it,
> (i.e. user wrote 0 on the sysfs attribute before it was 1). In this case,
> If enable means inc ref count and disable means dec ref count, then
> this would lead to refcount underflow and the suspend callback will
> not be called.
> 
> To handle this case, we would need to check whether irq/event was
> enabled or not.
> For that either we can use the local flag as I did, or I would need to
> do a read and test
> for the interrupt bit being set. I feel using the local flag would be
> cleaner and would
> require less code.
> If you are fine with local flag usage, then shall I not stick to only
> local flag usage?
> 

Normally we'd check the register to find out what whether the event
is enabled or not.  If we are asking for the state it is already in
then just return having done nothing.  If bus reads are an overhead
worth avoiding regcache will ensure we only do it once.

Then if we are doing something, do the runtime pm get / put as appropriate.

Jonathan


> Thanks,
> Akshay