Re: [PATCH v5 14/18] iio: magnetometer: ak8975: switch to using managed resources

[Joshua Crofts]

On Wed, 6 May 2026 at 19:09, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Tue, 05 May 2026 13:46:10 +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.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > Co-developed-by: Joshua Crofts <joshua.crofts1@xxxxxxxxx>
> > Signed-off-by: Joshua Crofts <joshua.crofts1@xxxxxxxxx>
> Hi Joshua
>
> Looks fine, but one stale comment needs an update.
>
> Jonathan
>
> > ---
> >  drivers/iio/magnetometer/ak8975.c | 77 ++++++++++++++++++++++-----------------
> >  1 file changed, 44 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
> > index 1c05e380a4d4f968f505a7d5c0acbec86ad949e1..84ccbca758b0121a9c4930a368cb113471d389da 100644
> > --- a/drivers/iio/magnetometer/ak8975.c
> > +++ b/drivers/iio/magnetometer/ak8975.c
> > @@ -900,9 +900,27 @@ 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;
> > +     /*
> > +      * The device may not be runtime suspended when the driver is
>
> If we get here it's definitely not runtime suspended.  Hence
> this comment needs a rewrite.

Missed that one, thanks.

> > +      * removed, so we soft-stop the chip before hard-stopping the
> > +      * regulators.
> > +      */
> > +     ak8975_set_mode(data, POWER_DOWN);
> I went looking into how we get into different modes...
> This deals with a latent bug in ak8975_setup() in that it can be in fuse mode
> in some exit paths.  Arguably if we had an i2c bus fail we probably can't
> change the mode anyway, but we should be trying from point of view
> of no side effects. I wonder if we should fix that in a precursor to this
> patch (given the amount of churn from earlier patches an obscure corner
> case of the bug - I don't plan to mark it for backports either way!)
>
> Maybe it's just not worth the bother given this is now here anyway.
>
> > +     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;
> > @@ -970,10 +988,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(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);
> >
> > @@ -981,10 +1010,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;
> > @@ -992,52 +1024,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);
>
> huh. Not sure what this was doing in original code.
> I guess forced an immediate power down rather that after an autosuspend
> period.  I think that's fine to change - however perhaps do a bit
> of analysis on whether my assumption is correct on what this did
> and then mention it in the commit message.

This is a fun one. pm_runtime_get_noresume() increments the counter,
then later on pm_runtime_put() was called, essentially tricking pm_runtime
into thinking that some task has ended, forcing it to autosuspend. Looking
at the code now, the device won't autosuspend after probe() is finished, it
will just be active until the driver is accessed from userspace (and
subsequently completes the task).

Sashiko is pretty impressive, it took me a long time to see what's going on.

> > -     pm_runtime_set_active(&client->dev);
>
> > -     pm_runtime_enable(&client->dev);
> > +     ret = devm_pm_runtime_enable(dev);
> > +     if (ret)
> > +             return ret;
> > +
> >       /*
> >        * 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);

I'd just add pm_runtime_mark_last_busy() and pm_request_autosuspend() here
to make the device suspend.

Perhaps unrelated, however I found an issue with the pm_runtime documentation,
as it incorrectly says that pm_runtime_mark_last_busy() is always called with an
autosuspend function (which is not the case, looking at the definition).

"Drivers may also continue to use the non-autosuspend helper
functions; they will
behave normally, which means sometimes taking the autosuspend delay into
account (see pm_runtime_idle). The autosuspend variants of the
functions also call
pm_runtime_mark_last_busy()."

-- 
Kind regards

CJD