Re: [PATCH v8 1/5] iio: magnetometer: ak8975: switch to using managed resources

[Jonathan Cameron]

On Mon, 18 May 2026 09:50:25 +0200
Joshua Crofts via B4 Relay <devnull+joshua.crofts1.gmail.com@xxxxxxxxxx> wrote:

> From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> 
> Switch the driver to use managed resources (devm_*) which simplifier
> error handling and allows removing ak8975_remove() method from
> the driver.
> 
> Note, on error path we now also set mode to POWER_DOWN state which is
> fine. Even if the device is in that mode, there is no problem to set
> that mode again, it should be no-op.
> 
> Additionally, remove any pm_runtime_get/put*() function calls that
> dummy cycled the counter to autosuspend the device.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> Co-developed-by: Joshua Crofts <joshua.crofts1@xxxxxxxxx>
> Signed-off-by: Joshua Crofts <joshua.crofts1@xxxxxxxxx>
> ---
>  drivers/iio/magnetometer/ak8975.c | 77 ++++++++++++++++++++++-----------------
>  1 file changed, 43 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
> index bb74abb648f138352576a68c49d3c7dce7cc068e..de4402201783430fc69369f1accaab35145e9f0c 100644
> --- a/drivers/iio/magnetometer/ak8975.c
> +++ b/drivers/iio/magnetometer/ak8975.c
> @@ -935,9 +935,25 @@ static const struct iio_buffer_setup_ops ak8975_buffer_setup_ops = {
>  	.preenable = ak8975_buffer_preenable,
>  	.postdisable = ak8975_buffer_postdisable,
>  };
> +
> +static void devm_ak8975_power_off(void *data)
> +{
> +	struct ak8975_data *ak = data;
> +	struct device *dev = &ak->client->dev;
> +
> +	/* Only power down if currently active */
> +	if (pm_runtime_suspended(dev))
I don't think we established that this actually works without underflowing 
the regular reference counts if in runtime pm suspsended state when
we tear get to devm cleanup.

Sashiko correctly moans about this and points out you refer to a different
check anyway on the comment below.
https://sashiko.dev/#/patchset/20260518-magnetometer-fixes-post-pickup-v8-0-088d610108a0%40gmail.com

I'm not convinced pm_runtime_status_suspended() is sufficient either.

The only path I'm 100% sure of is when we add tracking flag in the
iio_priv() to indicate if we are already runtime suspended.  Ugly but
at least we know that will not run into these odd race conditions.

This is 'probably' a new bug rather than a latent one because of the
unusual handling below.  I think there is a race in original code but
it is narrow so unlikely anyone ever hit it.

Whilst I do discuss what else you might do - I'd throw a flag in iio_priv()
because it is a simpler solution.

Now, if you happen to have some sort of real hardware or emulation to test
the different flows, then I'd be interested to see if there are advantages
to doing something closer to what this driver originally did.

> +		return;
> +
> +	/* Soft-stop the chip before hard-stopping the regulators */
> +	ak8975_set_mode(data, POWER_DOWN);
> +	ak8975_power_off(data);
> +}
> +
>  static int ak8975_probe(struct i2c_client *client)
>  {
>  	const struct i2c_device_id *id = i2c_client_get_device_id(client);
> +	struct device *dev = &client->dev;
>  	struct ak8975_data *data;
>  	struct iio_dev *indio_dev;
>  	struct gpio_desc *eoc_gpiod;
> @@ -1005,10 +1021,21 @@ static int ak8975_probe(struct i2c_client *client)
>  	if (ret)
>  		return ret;
>  
> +	/*
> +	 * Set device as active early so pm_runtime_status_suspended() works
> +	 * correctly if probe fails before pm_runtime is enabled. Do not attempt
> +	 * to move this line lower.
> +	 */
> +	pm_runtime_set_active(dev);
> +
> +	ret = devm_add_action_or_reset(dev, devm_ak8975_power_off, data);
> +	if (ret)
> +		return ret;
> +
>  	ret = ak8975_who_i_am(data, data->def->type);
>  	if (ret) {
>  		dev_err(&client->dev, "Unexpected device\n");
> -		goto power_off;
> +		return ret;
>  	}
>  	dev_dbg(&client->dev, "Asahi compass chip %s\n", name);
>  
> @@ -1016,10 +1043,13 @@ static int ak8975_probe(struct i2c_client *client)
>  	ret = ak8975_setup(data);
>  	if (ret) {
>  		dev_err(&client->dev, "%s initialization fails\n", name);
> -		goto power_off;
> +		return ret;
>  	}
>  
> -	mutex_init(&data->lock);
> +	ret = devm_mutex_init(dev, &data->lock);
> +	if (ret)
> +		return ret;
> +
>  	indio_dev->channels = ak8975_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(ak8975_channels);
>  	indio_dev->info = &ak8975_info;
> @@ -1027,52 +1057,32 @@ static int ak8975_probe(struct i2c_client *client)
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->name = name;
>  
> -	ret = iio_triggered_buffer_setup(indio_dev, NULL, ak8975_handle_trigger,
> -					 &ak8975_buffer_setup_ops);
> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> +					      ak8975_handle_trigger,
> +					      &ak8975_buffer_setup_ops);
>  	if (ret) {
>  		dev_err(&client->dev, "triggered buffer setup failed\n");
> -		goto power_off;
> +		return ret;
>  	}
>  
> -	ret = iio_device_register(indio_dev);
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_iio_device_register(dev, indio_dev);
>  	if (ret) {
>  		dev_err(&client->dev, "device register failed\n");
> -		goto cleanup_buffer;
> +		return ret;
>  	}
>  
> -	/* Enable runtime PM */
> -	pm_runtime_get_noresume(&client->dev);

See below for comment on this.

> -	pm_runtime_set_active(&client->dev);
> -	pm_runtime_enable(&client->dev);
>  	/*
>  	 * The device comes online in 500us, so add two orders of magnitude
>  	 * of delay before autosuspending: 50 ms.
>  	 */
>  	pm_runtime_set_autosuspend_delay(&client->dev, 50);
>  	pm_runtime_use_autosuspend(&client->dev);
> -	pm_runtime_put(&client->dev);
>  
>  	return 0;
> -
> -cleanup_buffer:
> -	iio_triggered_buffer_cleanup(indio_dev);
> -power_off:
> -	ak8975_power_off(data);
> -	return ret;
> -}
> -
> -static void ak8975_remove(struct i2c_client *client)
> -{
> -	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> -	struct ak8975_data *data = iio_priv(indio_dev);
> -
> -	pm_runtime_get_sync(&client->dev);
This forces device to wake up.

> -	pm_runtime_put_noidle(&client->dev);
This forces the counter down to 0 but doesn't trigger a runtime
pm suspend.   I'm nervous though as userspace is still in place
(before iio_device_register() - so it's possible another
get / put cycle can occur before the next call).

> -	pm_runtime_disable(&client->dev);
Hence when we disable runtime pm the device is powered up and
the rest won't underflow.

Now interestingly there is a :
devm_pm_runtime_get_noresume() which calls
pm_runtime_get_noresume() and schedules the
pm_runtime_put_noidle() in the tear down path.. 

However it is vanishingly rarely used and the sequence to me
is non obvious - the two users are inconsistent :(


> -	iio_device_unregister(indio_dev);
> -	iio_triggered_buffer_cleanup(indio_dev);
> -	ak8975_set_mode(data, POWER_DOWN);
> -	ak8975_power_off(data);
>  }
>  
>  static int ak8975_runtime_suspend(struct device *dev)
> @@ -1166,7 +1176,6 @@ static struct i2c_driver ak8975_driver = {
>  		.acpi_match_table = ak_acpi_match,
>  	},
>  	.probe		= ak8975_probe,
> -	.remove		= ak8975_remove,
>  	.id_table	= ak8975_id,
>  };
>  module_i2c_driver(ak8975_driver);
>