Re: [PATCH v0 10/14] iio: magnetometer: ak8975: switch to using managed resources

[Joshua Crofts]

On Tue, 28 Apr 2026 at 18:33, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Mon, 27 Apr 2026 22:09:55 +0200
> Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> > 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>
>
> One day I'll have time to write up some notes on patterns that work
> for runtime pm + devm. I don't think think this one does - note original
> code didn't work either as would underflow on regulators in some paths
> and hence print warnings.
> > ---
> >  drivers/iio/magnetometer/ak8975.c | 59 ++++++++++++++-----------------
> >  1 file changed, 26 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
> > index 53158ffd173b..d666d9d171bd 100644
> > --- a/drivers/iio/magnetometer/ak8975.c
> > +++ b/drivers/iio/magnetometer/ak8975.c
> > @@ -888,9 +888,16 @@ static irqreturn_t ak8975_handle_trigger(int irq, void *p)
> >       return IRQ_HANDLED;
> >  }
> >
> > +static void devm_ak8975_power_off(void *data)
> > +{
> > +     ak8975_set_mode(data, POWER_DOWN);
>
> Add a comment somewhere appropriate on what we are undoing with this.
>
> It's a 'might not be runtime suspended' at time of remove I think
> rather than the mode being set at that point in probe.
>
> If it is suspended, we shouldn't call the regulator disables
> in power_off.  So maybe gate this whole thing on
> whether runtime suspended.  If you do that be careful with what
> state we are in until runtime PM is enabled.  Maybe you already have that
> covered elsewhere.

Okay, I'll add a pm_runtime_status_suspended() check here.

> > +     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;
> > @@ -958,10 +965,14 @@ static int ak8975_probe(struct i2c_client *client)
> >       if (ret)
> >               return ret;
>
> If following comments above and below you'd need to set the state
> to active here.  If you go that way, make sure to add a comment to
> say why it is up here (and hopefully prevent someone thinking they can
> move it down).

I'll make sure to write a comment warning others about moving the function call.

> >
> > +     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);
> >
> > @@ -969,10 +980,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;
> > @@ -980,52 +994,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_set_active_enabled(dev);
>
> This hits the race with whether we can query if the device is disabled mentioned
> above. Dance where we need to be able to check that before we turn runtime pm on
> so that we can correctly do regulator disables if we error out before here.

So just do a devm_pm_runtime_enable() instead?

-- 
Kind regards

CJD