Re: [PATCH 3/8] iio: magnetometer: ak8975: switch to using managed resources

["Nuno Sá" <noname.nuno@gmail.com>]

On Thu, 2026-05-07 at 16:35 +0200, Joshua Crofts via B4 Relay wrote:
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> 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@linux.intel.com>
> Co-developed-by: Joshua Crofts <joshua.crofts1@gmail.com>
> Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
> ---
>  drivers/iio/magnetometer/ak8975.c | 74 ++++++++++++++++++++++-----------------
>  1 file changed, 41 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
> index
> 63b6e8465f5f3873841550a1cd03ce86b95d1d67..1d8f448d5179fe9b33af989cc2f456ac91bc2f17
> 100644
> --- a/drivers/iio/magnetometer/ak8975.c
> +++ b/drivers/iio/magnetometer/ak8975.c
> @@ -898,9 +898,24 @@ static irqreturn_t ak8975_handle_trigger(int irq, void *p)
>  	return IRQ_HANDLED;
>  }
>  
> +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_status_suspended(dev))
> +		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;
> @@ -968,10 +983,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;

This looks a bit hackish to me. Why don't we just register this powerdown action
after enabling PM?
> +
>  	ret = ak8975_who_i_am(client, 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);
>  
> @@ -979,10 +1005,13 @@ static int ak8975_probe(struct i2c_client *client)
>  	ret = ak8975_setup(client);
>  	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;
> @@ -990,52 +1019,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,
> -					 NULL);
> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> +					      ak8975_handle_trigger, NULL);
>  	if (ret) {
>  		dev_err(&client->dev, "triggered buffer setup failed\n");
> -		goto power_off;
> +		return ret;
>  	}
>  
> -	ret = iio_device_register(indio_dev);
> +	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);
> -	pm_runtime_set_active(&client->dev);
> -	pm_runtime_enable(&client->dev);
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return ret;

Maybe it would make sense to move this before devm_iio_device_register(). At the
point we register the device, userspace can start to interact with the device where
we have pm calls? Not that it is a problem (I think) but makes sense to me to enable
PM before exposing the device.

- Nuno Sá

> +
>  	/*
>  	 * 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);
> -	pm_runtime_put_noidle(&client->dev);
> -	pm_runtime_disable(&client->dev);
> -	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)
> @@ -1129,7 +1138,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);