Message-ID: <CAE3SzaTnNckFDRMDqGPDAg471bRskJ=_n5C_qSLKQeq3F-Lu_g@mail.gmail.com>
Date: Wed, 3 Sep 2025 09:15:53 +0530
From: Akshay Jindal <akshayaj.lkd@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...el.com>
Cc: anshulusr@...il.com, jic23@...nel.org, 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 v4] iio: light: ltr390: Implement runtime PM support

Thanks Andy for the review. Follow-ups inline.

On Tue, Sep 2, 2025 at 6:27 PM Andy Shevchenko
<andriy.shevchenko@...el.com> wrote:
>
> On Tue, Sep 02, 2025 at 12:12:36AM +0530, Akshay Jindal wrote:
> > Implement runtime power management for the LTR390 sensor. The device
> > autosuspends after 1s of idle time, reducing current consumption from
> > 100 渙 in active mode to 1 渙 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)
> > +
> > +static int __ltr390_read_raw(struct iio_dev *iio_device,
> > +                     struct iio_chan_spec const *chan, int *val,
> > +                     int *val2, long mask)
>
> Can we avoid using leading __ (double underscore)? Better name is
> ltr390_read_raw_no_pm(). But you may find even better one.

renamed as follows:
__ltr390_read_raw()-->ltr390_do_read_raw()

>
> ...
>
> > -static int ltr390_write_event_config(struct iio_dev *indio_dev,
> > +static int __ltr390_write_event_config(struct iio_dev *indio_dev,
>
> Ditto.
__ltr390_write_event_config--->ltr390_do_event_config()

>
> ...
>
> > +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;
>
> ^^^ (1) for the reference below.
>
> >       /* 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) {
> > +             if (regmap_clear_bits(data->regmap, LTR390_INT_CFG,
> > +                                     LTR390_LS_INT_EN) < 0)
>
> Wrong indentation, hard to read line, either one line, or do better. Actually
> why not assign it to ret? The above not only simple style issue, but also makes
> readability much harder as the semantics of '0' is completely hidden. This style
> is discouraged.
Earlier did not use ret here, because powerdown function is of type void.
But if readability is the issue, I have used ret.

Regarding clubbing into 1 line, I have my reservations there. I think
we should not
violate the 80 char line limit. Also since the line is already 1-level
indented (begins
at 9th column, due to if(data->irq_enabled) check), the spillover will
be too much.
The readability does not seem to be taking a substantial hit here.
Let me know if this is non-negotiable for you. Will happily make the changes.
>
> > +                     dev_err(&data->client->dev,
> > +                                     "failed to disable interrupts\n");
>
> Why not doing (1) here as well and with that
done
>
>                         dev_err(dev, "failed to disable interrupts\n");
>
> besides the fact of wrong indentation.
fixed
>
> > +             data->irq_enabled = false;
> > +
> > +             pm_runtime_put_autosuspend(&data->client->dev);
> > +     }
>
> ...
>
> > +static int ltr390_pm_init(struct ltr390_data *data)
> > +{
> > +     int ret;
> > +     struct device *dev = &data->client->dev;
> > +
> > +     ret = devm_pm_runtime_set_active_enabled(dev);
> > +     if (ret)
> > +             return dev_err_probe(dev, ret,
> > +                                     "failed to enable runtime PM\n");
>
> Something wrong with your editor or so, please check and make proper
> indentation _everywhere_ (in your changes).
Fixed.
Editor is fine, just did not pay attention here. Matched with existing dev_err
statements and understood what you meant.
>
> > +     pm_runtime_set_autosuspend_delay(dev, 1000);
> > +     pm_runtime_use_autosuspend(dev);
> > +     return 0;
> > +}
>
> ...
>
> > +static int ltr390_runtime_suspend(struct device *dev)
> > +{
> > +     struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > +     struct ltr390_data *data = iio_priv(indio_dev);
> > +
> > +     return regmap_clear_bits(data->regmap, LTR390_MAIN_CTRL,
> > +                             LTR390_SENSOR_ENABLE);
>
> I would make it one line despite being 87 character long.
Same as above
>
> > +}
> > +
> > +static int ltr390_runtime_resume(struct device *dev)
> > +{
> > +     struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > +     struct ltr390_data *data = iio_priv(indio_dev);
> > +
> > +     return regmap_set_bits(data->regmap, LTR390_MAIN_CTRL,
> > +                             LTR390_SENSOR_ENABLE);
>
> Ditto. (Here it's even shorter)
Same as above

Thanks,
Akshay

Powered by blists - more mailing lists