Re: [PATCH v2 13/17] iio: light: apds9960: Discard pm_runtime_put_autosuspend() return values

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 21 Oct 2026 16:57:14 -0700
Brian Norris <briannorris@chromium.org> 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. Inline the "off" helper, as
> it's trivial now, and rename to apds9960_power_on().
> 
> 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>

I took a look at this one and the current code is a complete mess.

I'm going to apply what you have here as a step in the right direction
simply because I can't see a quick path to fixing it properly.

If anyone wants a fun project then unwinding the mess in here would
be a good place to start.

Anyhow, applied this one.

thanks,

Jonathan

> ---
> I'm not sure if apds9960_write_event_config()'s existing logic is
> correct, but I don't want to mess with it.
> 
> Changes in v2:
>  * Split iio patch into per-driver patches
>  * Inline the put() helper
> 
>  drivers/iio/light/apds9960.c | 48 ++++++++++++++----------------------
>  1 file changed, 18 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/iio/light/apds9960.c b/drivers/iio/light/apds9960.c
> index afe520de60e2..0e084ffba1a6 100644
> --- a/drivers/iio/light/apds9960.c
> +++ b/drivers/iio/light/apds9960.c
> @@ -476,38 +476,23 @@ static int apds9960_set_als_gain(struct apds9960_data *data, int val)
>  	return ret;
>  }
>  
> -#ifdef CONFIG_PM
> -static int apds9960_set_power_state(struct apds9960_data *data, bool on)
> +static void apds9960_power_on(struct apds9960_data *data)
>  {
>  	struct device *dev = &data->client->dev;
> -	int ret = 0;
> +	int suspended;
>  
>  	mutex_lock(&data->lock);
>  
> -	if (on) {
> -		int suspended;
> -
> -		suspended = pm_runtime_suspended(dev);
> -		ret = pm_runtime_get_sync(dev);
> +	suspended = pm_runtime_suspended(dev);
> +	pm_runtime_get_sync(dev);
>  
> -		/* Allow one integration cycle before allowing a reading */
> -		if (suspended)
> -			usleep_range(data->als_adc_int_us,
> -				     APDS9960_MAX_INT_TIME_IN_US);
> -	} else {
> -		ret = pm_runtime_put_autosuspend(dev);
> -	}
> +	/* Allow one integration cycle before allowing a reading */
> +	if (suspended)
> +		usleep_range(data->als_adc_int_us,
> +			     APDS9960_MAX_INT_TIME_IN_US);
>  
>  	mutex_unlock(&data->lock);
> -
> -	return ret;
>  }
> -#else
> -static int apds9960_set_power_state(struct apds9960_data *data, bool on)
> -{
> -	return 0;
> -}
> -#endif
>  
>  static int apds9960_read_raw(struct iio_dev *indio_dev,
>  			     struct iio_chan_spec const *chan,
> @@ -522,7 +507,7 @@ static int apds9960_read_raw(struct iio_dev *indio_dev,
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> -		apds9960_set_power_state(data, true);
> +		apds9960_power_on(data);
>  		switch (chan->type) {
>  		case IIO_PROXIMITY:
>  			ret = regmap_read(data->regmap, chan->address, val);
> @@ -540,7 +525,7 @@ static int apds9960_read_raw(struct iio_dev *indio_dev,
>  		default:
>  			ret = -EINVAL;
>  		}
> -		apds9960_set_power_state(data, false);
> +		pm_runtime_put_autosuspend(&data->client->dev);
>  		break;
>  	case IIO_CHAN_INFO_INT_TIME:
>  		/* RGB + ALS sensors only have integration time */
> @@ -767,7 +752,6 @@ static int apds9960_write_event_config(struct iio_dev *indio_dev,
>  		if (ret)
>  			return ret;
>  		data->pxs_int = state;
> -		apds9960_set_power_state(data, state);
>  		break;
>  	case IIO_INTENSITY:
>  		if (data->als_int == state)
> @@ -777,12 +761,16 @@ static int apds9960_write_event_config(struct iio_dev *indio_dev,
>  		if (ret)
>  			return ret;
>  		data->als_int = state;
> -		apds9960_set_power_state(data, state);
>  		break;
>  	default:
>  		return -EINVAL;
>  	}
>  
> +	if (state)
> +		apds9960_power_on(data);
> +	else
> +		pm_runtime_put_autosuspend(&data->client->dev);
> +
>  	return 0;
>  }
>  
> @@ -1078,7 +1066,7 @@ static int apds9960_probe(struct i2c_client *client)
>  	pm_runtime_set_autosuspend_delay(&client->dev, 5000);
>  	pm_runtime_use_autosuspend(&client->dev);
>  
> -	apds9960_set_power_state(data, true);
> +	apds9960_power_on(data);
>  
>  	ret = apds9960_regfield_init(data);
>  	if (ret)
> @@ -1105,12 +1093,12 @@ static int apds9960_probe(struct i2c_client *client)
>  	if (ret)
>  		goto error_power_down;
>  
> -	apds9960_set_power_state(data, false);
> +	pm_runtime_put_autosuspend(&client->dev);
>  
>  	return 0;
>  
>  error_power_down:
> -	apds9960_set_power_state(data, false);
> +	pm_runtime_put_autosuspend(&client->dev);
>  
>  	return ret;
>  }