On Mon, Dec 02, 2024 at 09:43:36PM +0200, Andy Shevchenko wrote: > On Mon, Dec 02, 2024 at 08:23:41PM +0100, Vasileios Amoiridis wrote: > > Add runtime power management to the device. > > ... > > > + ret = pm_runtime_resume_and_get(dev); > > + if (ret) > > + return ret; > > + > > + ret = __bme680_read_raw(indio_dev, chan, val, val2, mask); > > + pm_runtime_mark_last_busy(dev); > > + pm_runtime_put_autosuspend(dev); > > Side note: as long as idle method is not defined (NULL) the above dance is > already taken into account in the regular put. > > ... > Hi Andy, Thanks again for the review! Indeed by looking at the code a bit, it looks like the suspend callback is being called if the idle one is not found. But I have seen this dance that you mention much more often in the IIO that's why I used it. We can see what Jonathan has to say as well, I think what you propose, simplifies things. > > +static int bme680_buffer_preenable(struct iio_dev *indio_dev) > > +{ > > + struct bme680_data *data = iio_priv(indio_dev); > > + struct device *dev = regmap_get_device(data->regmap); > > + int ret; > > > + ret = pm_runtime_resume_and_get(dev); > > + if (ret) > > + return ret; > > + > > + return 0; > > Either this is broken (if the above can return positive codes), or can be > replaced with direct return: > > return pm_... > > (but I believe it's the former and you wanted something like if (ret < 0) > there). > > > +} > Well, pm_runtime_resume_and_get() looks like it returns 0 on success and negative value on error so I think the if (ret) is correct, no? But I agree with you that it can be simplified as you proposed. Cheers, Vasilis > -- > With Best Regards, > Andy Shevchenko > >