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

[Joshua Crofts]

On Sat, 9 May 2026 at 11:02, Nuno Sá <noname.nuno@xxxxxxxxx> wrote:
>
> On Thu, 2026-05-07 at 16:35 +0200, Joshua Crofts via B4 Relay 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 | 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?

This is to prevent a resource leak. If we register the power_off
function at the end,
any potential probe() failures will leave the regulators on.

> > +
> >       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.

Sure, I agree with this.

-- 
Kind regards

CJD