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

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 21 Oct 2026 16:57:05 -0700
Brian Norris <briannorris@chromium.org> wrote:

> These drivers propagate 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 these drivers to simply discard the
> pm_runtime_put_autosuspend() return value, and inline the pm_runtime
> helpers now that they behave the same with and without CONFIG_PM.
> 
> While at it, I see that mma9553_write_event_config() mismanages the
> runtime PM reference count. It should only put() a reference after it's
> done touching the hardware, not before. Resolve this ordering issue.
> 
> 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>

Mostly for my own reference, this looks fine to me, but if there
is time it would be nice to use some ACQUIRE magic to simplify some error
handling etc.

I'd also ideally like some eyes on the actual functional changes
were reordering occurs just in case there is something subtle
going on.


> ---
> 
> Changes in v2:
>  * Split iio patch into per-driver patches
>  * Inlining
>  * Refcount ordering
> 
>  drivers/iio/accel/mma9551.c      | 21 +++++++++--------
>  drivers/iio/accel/mma9551_core.c | 39 ++++----------------------------
>  drivers/iio/accel/mma9551_core.h |  1 -
>  drivers/iio/accel/mma9553.c      | 36 ++++++++++++++++++++---------
>  4 files changed, 42 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/iio/accel/mma9551.c b/drivers/iio/accel/mma9551.c
> index 182f488edab1..4c549fb106bc 100644
> --- a/drivers/iio/accel/mma9551.c
> +++ b/drivers/iio/accel/mma9551.c
> @@ -71,9 +71,11 @@ static int mma9551_read_incli_chan(struct i2c_client *client,
>  		return -EINVAL;
>  	}
>  
> -	ret = mma9551_set_power_state(client, true);
> -	if (ret < 0)
> +	ret = pm_runtime_resume_and_get(&client->dev);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to resume: %d\n", ret);
>  		return ret;
> +	}
>  
>  	ret = mma9551_read_status_byte(client, MMA9551_APPID_TILT,
>  				       reg_addr, &angle);
> @@ -96,7 +98,7 @@ static int mma9551_read_incli_chan(struct i2c_client *client,
>  	ret = IIO_VAL_INT;
>  
>  out_poweroff:
> -	mma9551_set_power_state(client, false);
> +	pm_runtime_put_autosuspend(&client->dev);
>  	return ret;
>  }
>  
> @@ -178,9 +180,7 @@ static int mma9551_config_incli_event(struct iio_dev *indio_dev,
>  		if (ret < 0)
>  			return ret;
>  
> -		ret = mma9551_set_power_state(data->client, false);
> -		if (ret < 0)
> -			return ret;
> +		pm_runtime_put_autosuspend(&data->client->dev);
>  	} else {
>  		int bitnum;
>  
> @@ -200,15 +200,18 @@ static int mma9551_config_incli_event(struct iio_dev *indio_dev,
>  		}
>  
>  
> -		ret = mma9551_set_power_state(data->client, true);
> -		if (ret < 0)
> +		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 = mma9551_gpio_config(data->client,
>  					  (enum mma9551_gpio_pin)mma_axis,
>  					  MMA9551_APPID_TILT, bitnum, 0);
>  		if (ret < 0) {
> -			mma9551_set_power_state(data->client, false);
> +			pm_runtime_put_autosuspend(&data->client->dev);
>  			return ret;
>  		}
>  	}
> diff --git a/drivers/iio/accel/mma9551_core.c b/drivers/iio/accel/mma9551_core.c
> index 2ccb1fb19b96..68df195d5b24 100644
> --- a/drivers/iio/accel/mma9551_core.c
> +++ b/drivers/iio/accel/mma9551_core.c
> @@ -654,37 +654,6 @@ int mma9551_set_device_state(struct i2c_client *client, bool enable)
>  }
>  EXPORT_SYMBOL_NS(mma9551_set_device_state, "IIO_MMA9551");
>  
> -/**
> - * mma9551_set_power_state() - sets runtime PM state
> - * @client:	I2C client
> - * @on:		Use true to power on device, false to power off
> - *
> - * Resume or suspend the device using Runtime PM.
> - * The device will suspend after the autosuspend delay.
> - *
> - * Returns: 0 on success, negative value on failure.
> - */
> -int mma9551_set_power_state(struct i2c_client *client, bool on)
> -{
> -#ifdef CONFIG_PM
> -	int ret;
> -
> -	if (on)
> -		ret = pm_runtime_resume_and_get(&client->dev);
> -	else
> -		ret = pm_runtime_put_autosuspend(&client->dev);
> -	if (ret < 0) {
> -		dev_err(&client->dev,
> -			"failed to change power state to %d\n", on);
> -
> -		return ret;
> -	}
> -#endif
> -
> -	return 0;
> -}
> -EXPORT_SYMBOL_NS(mma9551_set_power_state, "IIO_MMA9551");
> -
>  /**
>   * mma9551_sleep() - sleep
>   * @freq:	Application frequency
> @@ -741,9 +710,11 @@ int mma9551_read_accel_chan(struct i2c_client *client,
>  		return -EINVAL;
>  	}
>  
> -	ret = mma9551_set_power_state(client, true);
> -	if (ret < 0)
> +	ret = pm_runtime_resume_and_get(&client->dev);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to resume: %d\n", ret);
>  		return ret;
> +	}
>  
>  	ret = mma9551_read_status_word(client, MMA9551_APPID_AFE,
>  				       reg_addr, &raw_accel);
> @@ -755,7 +726,7 @@ int mma9551_read_accel_chan(struct i2c_client *client,
>  	ret = IIO_VAL_INT;
>  
>  out_poweroff:
> -	mma9551_set_power_state(client, false);
> +	pm_runtime_put_autosuspend(&client->dev);
>  	return ret;
>  }
>  EXPORT_SYMBOL_NS(mma9551_read_accel_chan, "IIO_MMA9551");
> diff --git a/drivers/iio/accel/mma9551_core.h b/drivers/iio/accel/mma9551_core.h
> index 543454a31dc0..1d55fdfd0a92 100644
> --- a/drivers/iio/accel/mma9551_core.h
> +++ b/drivers/iio/accel/mma9551_core.h
> @@ -62,7 +62,6 @@ int mma9551_gpio_config(struct i2c_client *client, enum mma9551_gpio_pin pin,
>  			u8 app_id, u8 bitnum, int polarity);
>  int mma9551_read_version(struct i2c_client *client);
>  int mma9551_set_device_state(struct i2c_client *client, bool enable);
> -int mma9551_set_power_state(struct i2c_client *client, bool on);
>  void mma9551_sleep(int freq);
>  int mma9551_read_accel_chan(struct i2c_client *client,
>  			    const struct iio_chan_spec *chan,
> diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
> index 5a36d415a837..2ccb68dfbcd4 100644
> --- a/drivers/iio/accel/mma9553.c
> +++ b/drivers/iio/accel/mma9553.c
> @@ -610,10 +610,16 @@ static int mma9553_write_raw(struct iio_dev *indio_dev,
>  		if (data->stepcnt_enabled == !!val)
>  			return 0;
>  		mutex_lock(&data->mutex);
> -		ret = mma9551_set_power_state(data->client, val);
> -		if (ret < 0) {
> -			mutex_unlock(&data->mutex);
> -			return ret;
> +		if (val) {
> +			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;
> +			}
> +		} else {
> +			pm_runtime_put_autosuspend(&data->client->dev);
>  		}
>  		data->stepcnt_enabled = val;
>  		mutex_unlock(&data->mutex);
> @@ -737,24 +743,32 @@ static int mma9553_write_event_config(struct iio_dev *indio_dev,
>  
>  	mutex_lock(&data->mutex);
>  
> -	ret = mma9551_set_power_state(data->client, state);
> -	if (ret < 0)
> -		goto err_out;
> +	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);
> +			goto err_out;
> +		}
> +	}
> +
>  	event->enabled = state;
>  
>  	ret = mma9553_conf_gpio(data);
>  	if (ret < 0)
>  		goto err_conf_gpio;
>  
> +	if (!state)
> +		pm_runtime_put_autosuspend(&data->client->dev);
> +
>  	mutex_unlock(&data->mutex);
>  
>  	return 0;
>  
>  err_conf_gpio:
> -	if (state) {
> -		event->enabled = false;
> -		mma9551_set_power_state(data->client, false);
> -	}
> +	if (state)
> +		pm_runtime_put_autosuspend(&data->client->dev);
> +	event->enabled = !state;
>  err_out:
>  	mutex_unlock(&data->mutex);
>  	return ret;