Re: [PATCH v2 01/17] iio: accel: fxls8962af: Discard pm_runtime_put_autosuspend() return values

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 22 Oct 2026 10:51:43 -0700
Brian Norris <briannorris@chromium.org> wrote:

> On Thu, Oct 22, 2026 at 09:53:12AM +0300, Andy Shevchenko wrote:
> > On Wed, Oct 21, 2026 at 04:57:02PM -0700, Brian Norris wrote:  
> > > This driver propagates the return value of pm_runtime_put_autosuspend(),
> > > which is not particularly useful.
> > > 
> > > Returning an error code from pm_runtime_put_autosuspend() merely means
> > > that it has not queued up a timer or work item to check whether or not
> > > the device can be suspended and there are many perfectly valid
> > > situations in which that can happen, like after writing "on" to the
> > > device's runtime PM "control" attribute in sysfs for one example.
> > > 
> > > For this reason, modify this driver to simply discard the
> > > pm_runtime_put_autosuspend() return value, and inline the
> > > fxls8962af_power_off/on() helpers to their callsites, as they're even
> > > more trivial now.
> > > 
> > > This will facilitate a planned change of the
> > > pm_runtime_put_autosuspend() return type to void in the future, similar
> > > to commit 3afd8df02433 ("PM: runtime: Change pm_runtime_put() return
> > > type to void").  
> > 
> > ...
> >   
> > > static int fxls8962af_get_out(struct fxls8962af_data *data,  
> >   
> > >  
> > >  	is_active = fxls8962af_is_active(data);  
> >   
> > >  	if (!is_active) {  
> > 
> > Why do we have this check at all?  
> 
> Frankly, I don't know. It seems logically wrong; you shouldn't even be
> reading device registers without first grabbing a runtime PM reference.

In this case the runtime suspend is all about accessing registers
as it's putting the device into a low power state. Not removing the power.
So the same access is used if runtime resume occurs.  Hence this is safe
but probably pointless.

My guess for why it looks like this evolution.  There was probably something
more complex in power_on that has since gone away.

Whilst it should be fine to drop that check I'm in agreement with Brian
that it is a more complex change, so getting a bit risky without
an means to test.



> 
> But I'm also loathe to go tweak a bunch of driver-specific details I'm
> completely unfamiliar with, when my main purpose is extremely simple --
> to stop checking one return code.
> 
> All in all, I think this is beside the point, and I'd rather not try to
> address it in this series. For one, even if I try to address it, there's
> more chance of regressions that will distract from my purpose.
> 
> > > -		ret = fxls8962af_power_on(data);
> > > -		if (ret)
> > > +		ret = pm_runtime_resume_and_get(dev);
> > > +		if (ret) {
> > > +			dev_err(dev, "failed to power on: %d\n", ret);
> > >  			return ret;
> > > +		}
> > >  	}
> > >  
> > >  	ret = regmap_bulk_read(data->regmap, chan->address,
> > >  			       &raw_val, sizeof(data->lower_thres));
> > >  
> > >  	if (!is_active)
> > > -		fxls8962af_power_off(data);
> > > +		pm_runtime_put_autosuspend(dev);  
> > 
> > If it's active my understanding that runtime PM keeps device already resumed,
> > so these two will be just a reference counting calls, no?  
> 
> Right. I don't really see the point.
> 
> Brian