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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Wed, Sep 03, 2025 at 09:15:53AM +0530, Akshay Jindal wrote:
> On Tue, Sep 2, 2025 at 6:27 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Tue, Sep 02, 2025 at 12:12:36AM +0530, Akshay Jindal wrote:

...

> > >       /* 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.

Shouldn't != mustn't, esp. when it's about readability.

> 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.

		ret = regmap_clear_bits(data->regmap, LTR390_INT_CFG, LTR390_LS_INT_EN);

only 88 characters. One can make it shorter, yes

	struct regmap *map = data->regmap;

		ret = regmap_clear_bits(map, LTR390_INT_CFG, LTR390_LS_INT_EN);


79 characters.

-- 
With Best Regards,
Andy Shevchenko