Re: [PATCH v2 09/17] iio: hid-sensors: Discard pm_runtime_put_autosuspend() return values

[srinivas pandruvada <srinivas.pandruvada@linux.intel.com>]

On Mon, 2026-11-02 at 22:05 +0000, Jonathan Cameron wrote:
> On Mon, 02 Nov 2026 08:55:44 -0800
> srinivas pandruvada <srinivas.pandruvada@linux.intel.com> wrote:
> 
> > On Mon, 2026-11-02 at 01:52 +0000, Jonathan Cameron wrote:
> > > On Wed, 21 Oct 2026 16:57:10 -0700
> > > Brian Norris <briannorris@chromium.org> wrote:
> > >   
> > > > The hid-sensors 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.
> > > > 
> > > > 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").
> > > > 
> > > > Signed-off-by: Brian Norris <briannorris@chromium.org>
> > > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>  
> > > +CC the various hid-sensors maintainers.
> > >   
> > 
> >     Acked-by: Srinivas Pandruvada
> > <srinivas.pandruvada@linux.intel.com>
> > 
> > 
> > > Anyone remember why this does
> > > pm_runtime_use_autosuspend(&st->pdev->dev);
> > > in this code?
> > > 
> > > That is unusual...  My immediate instinct is to
> > > get rid of it, but maybe there is something subtle
> > > going on.  
> > 
> > When reading sensor via raw interface, you need to power-up, read
> > and
> > power-down (This is done via runtime PM). To avoid repeating this
> > sequence of power up and power down for consecutive fast reads,
> > auto
> > suspend is used. If there are reads with in 3 seconds, the sensor
> > will
> > keep power up.
> > 
> Ah. I was too vague in that query.  Why is it enabled only the power
> state interface rather than at the start when runtime pm is enabled?
> 
> That seems to be done via a slightly unusually on first turning on
> of the trigger rather than more generally.  Normally we'd expect
> it to be enabled in driver probe and all paths that need the power
> on to do a pm_runtime_resume_and_get().
> 

This function is called from probe() of each sensor instead of calling
in each driver. I agree this can be different function than this
particular one. This happen to be a common function.

We can separate the function in a follow up patch.

Thanks,
Srinivas


> I'm not seeing why it is in this particularly location which doesn't
> seem to be particularly sensible to me.
> 
> Jonathan
> 
> 
>   
> 
> 
> 
> > Thanks,
> > Srinivas
> > 
> > >   
> > > > ---
> > > > 
> > > > Changes in v2:
> > > >  * Split iio patch into per-driver patches
> > > > 
> > > >  .../iio/common/hid-sensors/hid-sensor-trigger.c    | 14 +++++-
> > > > ----
> > > > ----
> > > >  1 file changed, 5 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/drivers/iio/common/hid-sensors/hid-sensor-
> > > > trigger.c
> > > > b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> > > > index 60808e2430ca..d76a6d9f04f6 100644
> > > > --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> > > > +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> > > > @@ -154,21 +154,17 @@ int hid_sensor_power_state(struct
> > > > hid_sensor_common *st, bool state)
> > > >  {
> > > >  
> > > >  #ifdef CONFIG_PM
> > > > -	int ret;
> > > > -
> > > >  	if (atomic_add_unless(&st->runtime_pm_enable, 1, 1))
> > > >  		pm_runtime_enable(&st->pdev->dev);
> > > >  
> > > >  	if (state) {
> > > >  		atomic_inc(&st->user_requested_state);
> > > > -		ret = pm_runtime_resume_and_get(&st->pdev-
> > > > >dev);
> > > > -	} else {
> > > > -		atomic_dec(&st->user_requested_state);
> > > > -		pm_runtime_use_autosuspend(&st->pdev->dev);
> > > > -		ret = pm_runtime_put_autosuspend(&st->pdev-
> > > > >dev);
> > > > +		return pm_runtime_resume_and_get(&st->pdev-
> > > > >dev);
> > > >  	}
> > > > -	if (ret < 0)
> > > > -		return ret;
> > > > +
> > > > +	atomic_dec(&st->user_requested_state);
> > > > +	pm_runtime_use_autosuspend(&st->pdev->dev);  
> > > This is odd enough to make me nervous even though the patch is
> > > obviously a no-op change.
> > > 
> > >   
> > > > +	pm_runtime_put_autosuspend(&st->pdev->dev);
> > > >  
> > > >  	return 0;
> > > >  #else  
> > >   
>