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

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Wed, 10 Sep 2025 10:17:00 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Sep 9, 2025 at 10:47 PM Akshay Jindal <akshayaj.lkd@gmail.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.  
> 
> ...
> 
> > +static int ltr390_read_raw(struct iio_dev *iio_device,
> > +                          struct iio_chan_spec const *chan, int *val,
> > +                          int *val2, long mask)  
> 
> Isn't the mask unsigned long? Jonathan, do we get this fixed already?

Whilst it could (and probably should) be unsigned, it's not actually a mask.
That naming is a historical mess up / evolution thing - long ago it was a bitmap.
It is now the index of a bit in the mask.  So this is unrelated(ish) to the
recent fixes around the actual bitmaps/bitmasks.

Changing this one is a lot more painful than the recent fix to the infomask
as it means changing the signature in every driver.
I'm doubtful on whether this one is worth the churn.

> 
> Also logical split might be better, i.e. putting val and val2 on the
> same line. Then mask will be on the next one
> 
> ...
> 
> >  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.
> 
> > +               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?
> 
> > +               pm_runtime_put_autosuspend(&data->client->dev);  
> 
> You have dev, use it.
> 
> But where is the symmetrical pm_runtime_get*()?
> 
> > +       }
> > +
> > +       ret = regmap_clear_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_SENSOR_ENABLE);
> > +       if (ret < 0)
> > +               dev_err(dev, "failed to disable sensor\n");
> > +}  
> 
>