Re: [PATCH v2 02/17] iio: accel: kxcjk-1013: Discard pm_runtime_put_autosuspend() return values

[Andy Shevchenko]

On Wed, Oct 21, 2026 at 04:57:03PM -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 discard the
> pm_runtime_put_autosuspend() return value, and inline these pm_runtime
> calls for simplicity.
> 
> While doing this, it becomes obvious that kxcjk1013_write_event_config()
> and kxcjk1013_data_rdy_trigger_set_state() are intentionally mismanaging
> runtime PM reference counts and instead are relying on (racy)
> autosuspend delay for correct ordering. Undo this choice, and instead
> properly drop the reference count only after we're done touching the
> hardware.
> 
> Use guard() for managing the mutex in kxcjk1013_write_event_config(),
> because it helps simplify the above refactoring, making it less
> error-prone.
> 
> 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 kxcjk1013_read_raw(struct iio_dev *indio_dev,

>  		else {
> -			ret = kxcjk1013_set_power_state(data, true);
> +			ret = pm_runtime_resume_and_get(&data->client->dev);
>  			if (ret < 0) {
> +				dev_err(&data->client->dev,
> +					"failed to resume: %d\n", ret);
>  				mutex_unlock(&data->mutex);
>  				return ret;
>  			}
>  			ret = kxcjk1013_get_acc_reg(data, chan->scan_index);
>  			if (ret < 0) {
> -				kxcjk1013_set_power_state(data, false);
> +				pm_runtime_put_autosuspend(&data->client->dev);
>  				mutex_unlock(&data->mutex);
>  				return ret;
>  			}
>  			*val = sign_extend32(ret >> chan->scan_type.shift,
>  					     chan->scan_type.realbits - 1);
> -			ret = kxcjk1013_set_power_state(data, false);
> +			pm_runtime_put_autosuspend(&data->client->dev);

So, now you can use on of PM_RUNTIME_ACQUIRE*() and drop unneeded calls?
And why not using guard()() or scoped_guard() here for mutex lock?

>  		}

...

> +	if (state) {
> +		ret = pm_runtime_resume_and_get(&data->client->dev);
> +		if (ret < 0) {
> +			dev_err(&data->client->dev, "failed to resume: %d\n",
> +				ret);
> +			return ret;
> +		}
> +
> +		ret = kxcjk1013_setup_interrupt(data, true,
> +						data->motion_trig != trig);
> +		if (ret < 0) {
> +			pm_runtime_put_autosuspend(&data->client->dev);
> +			return ret;
> +		}
> +	} else {
> +		ret = kxcjk1013_setup_interrupt(data, false,
> +						data->motion_trig != trig);
> +		if (ret < 0)
> +			return ret;
> +
> +		pm_runtime_put_autosuspend(&data->client->dev);
>  	}

Sounds like a dup code, can't we have PM runtime calls along with
kxcjk1013_setup_interrupt() be provided as two helpers and be called
as necessary?

-- 
With Best Regards,
Andy Shevchenko