Rework the power management in inv_icm42600_core_probe() to use
devm_pm_runtime_set_active_enabled(), which simplifies the runtime PM
setup by handling activation and enabling in one step.
Remove the separate inv_icm42600_disable_pm callback, as it's no longer
needed with the devm-managed approach.
Using devm_pm_runtime_enable() also fixes the missing disable of
autosuspend.
Update inv_icm42600_disable_vddio_reg() to only disable the regulator if
the device is not suspended i.e. powered-down, preventing unbalanced
disables.
Also remove redundant error msg on regulator_disable(), the regulator
framework already emits an error message when regulator_disable() fails.
This simplifies the PM setup and avoids manipulating the usage counter
unnecessarily.
Fixes: 31c24c1e93c3 ("iio: imu: inv_icm42600: add core of new inv_icm42600 driver")
Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
drivers/iio/imu/inv_icm42600/inv_icm42600_core.c | 24 ++++++------------------
1 file changed, 6 insertions(+), 18 deletions(-)
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
index a4d42e7e21807f7954def431e9cf03dffaa5bd5e..c19615750c717101312f358a9160dd2c455cfb14 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
@@ -711,20 +711,10 @@ static void inv_icm42600_disable_vdd_reg(void *_data)
static void inv_icm42600_disable_vddio_reg(void *_data)
{
struct inv_icm42600_state *st = _data;
- const struct device *dev = regmap_get_device(st->map);
- int ret;
-
- ret = regulator_disable(st->vddio_supply);
- if (ret)
- dev_err(dev, "failed to disable vddio error %d\n", ret);
-}
-
-static void inv_icm42600_disable_pm(void *_data)
-{
- struct device *dev = _data;
+ struct device *dev = regmap_get_device(st->map);
- pm_runtime_put_sync(dev);
- pm_runtime_disable(dev);
+ if (!pm_runtime_status_suspended(dev))
+ regulator_disable(st->vddio_supply);
}
int inv_icm42600_core_probe(struct regmap *regmap, int chip,
@@ -824,16 +814,14 @@ int inv_icm42600_core_probe(struct regmap *regmap, int chip,
return ret;
/* setup runtime power management */
- ret = pm_runtime_set_active(dev);
+ ret = devm_pm_runtime_set_active_enabled(dev);
if (ret)
return ret;
- pm_runtime_get_noresume(dev);
- pm_runtime_enable(dev);
+
pm_runtime_set_autosuspend_delay(dev, INV_ICM42600_SUSPEND_DELAY_MS);
pm_runtime_use_autosuspend(dev);
- pm_runtime_put(dev);
- return devm_add_action_or_reset(dev, inv_icm42600_disable_pm, dev);
+ return ret;
}
EXPORT_SYMBOL_NS_GPL(inv_icm42600_core_probe, "IIO_ICM42600");
--
2.50.1On Fri, Aug 8, 2025 at 5:58 PM Sean Nyekjaer <sean@geanix.com> wrote: > > Rework the power management in inv_icm42600_core_probe() to use > devm_pm_runtime_set_active_enabled(), which simplifies the runtime PM > setup by handling activation and enabling in one step. > Remove the separate inv_icm42600_disable_pm callback, as it's no longer > needed with the devm-managed approach. > Using devm_pm_runtime_enable() also fixes the missing disable of > autosuspend. > Update inv_icm42600_disable_vddio_reg() to only disable the regulator if > the device is not suspended i.e. powered-down, preventing unbalanced > disables. > Also remove redundant error msg on regulator_disable(), the regulator > framework already emits an error message when regulator_disable() fails. > > This simplifies the PM setup and avoids manipulating the usage counter > unnecessarily. ... > + struct device *dev = regmap_get_device(st->map); > > + if (!pm_runtime_status_suspended(dev)) > + regulator_disable(st->vddio_supply); I would rather use positive conditional as it seems to me more scalable > } -- With Best Regards, Andy Shevchenko
On Fri, 8 Aug 2025 23:37:51 +0200 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Aug 8, 2025 at 5:58 PM Sean Nyekjaer <sean@geanix.com> wrote: > > > > Rework the power management in inv_icm42600_core_probe() to use > > devm_pm_runtime_set_active_enabled(), which simplifies the runtime PM > > setup by handling activation and enabling in one step. > > Remove the separate inv_icm42600_disable_pm callback, as it's no longer > > needed with the devm-managed approach. > > Using devm_pm_runtime_enable() also fixes the missing disable of > > autosuspend. > > Update inv_icm42600_disable_vddio_reg() to only disable the regulator if > > the device is not suspended i.e. powered-down, preventing unbalanced > > disables. > > Also remove redundant error msg on regulator_disable(), the regulator > > framework already emits an error message when regulator_disable() fails. > > > > This simplifies the PM setup and avoids manipulating the usage counter > > unnecessarily. > > ... > > > + struct device *dev = regmap_get_device(st->map); > > > > + if (!pm_runtime_status_suspended(dev)) > > + regulator_disable(st->vddio_supply); > > I would rather use positive conditional as it seems to me more scalable Hi Andy, > To potentially save time when Sean looks at this. I don't follow. Do you mean something like if (pm_runtime_status_suspended(dev)) return; regulator_disable(st->vddio_supply); ? If so I'm not seeing why we'd want this to scale as it's a single use devm_set_action_or_reset() callback doing just one thing. Jonathan > > } > >
On Sat, Aug 9, 2025 at 8:06 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Fri, 8 Aug 2025 23:37:51 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Fri, Aug 8, 2025 at 5:58 PM Sean Nyekjaer <sean@geanix.com> wrote:
...
> > > + struct device *dev = regmap_get_device(st->map);
> > >
> > > + if (!pm_runtime_status_suspended(dev))
> > > + regulator_disable(st->vddio_supply);
> >
> > I would rather use positive conditional as it seems to me more scalable
>
> To potentially save time when Sean looks at this. I don't follow. Do you mean
> something like
> if (pm_runtime_status_suspended(dev))
> return;
>
> regulator_disable(st->vddio_supply);
>
> ?
Yes.
> If so I'm not seeing why we'd want this to scale as it's a single use
> devm_set_action_or_reset() callback doing just one thing.
While I agree in _this_ case, in general the check and return
immediately is more scalable for reading purposes, e.g., indentation
will be one level less. Also it won't require additional churn in
adding {, i.e. changing conditional line just for that.
--
With Best Regards,
Andy Shevchenko
On Sat, Aug 09, 2025 at 10:27:52PM +0100, Andy Shevchenko wrote:
> On Sat, Aug 9, 2025 at 8:06 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > On Fri, 8 Aug 2025 23:37:51 +0200
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > On Fri, Aug 8, 2025 at 5:58 PM Sean Nyekjaer <sean@geanix.com> wrote:
>
> ...
>
> > > > + struct device *dev = regmap_get_device(st->map);
> > > >
> > > > + if (!pm_runtime_status_suspended(dev))
> > > > + regulator_disable(st->vddio_supply);
> > >
> > > I would rather use positive conditional as it seems to me more scalable
> >
> > To potentially save time when Sean looks at this. I don't follow. Do you mean
> > something like
> > if (pm_runtime_status_suspended(dev))
> > return;
> >
> > regulator_disable(st->vddio_supply);
> >
> > ?
>
> Yes.
>
> > If so I'm not seeing why we'd want this to scale as it's a single use
> > devm_set_action_or_reset() callback doing just one thing.
>
> While I agree in _this_ case, in general the check and return
> immediately is more scalable for reading purposes, e.g., indentation
> will be one level less. Also it won't require additional churn in
> adding {, i.e. changing conditional line just for that.
>
I like the return early if pm_runtime_status_suspended() is true.
Andy, when doing reviews please keep the function name, as it's much
easier to add the changes.
Jonathan, do we think checking pm_runtime_status_suspended() is a viable
option? Should we ask on the linux-pm list?
If it's ok; I will patch:
drivers/iio/adc/ti-ads1100.c
drivers/iio/pressure/bmp280-core.c
drivers/iio/pressure/icp10100.c
/Sean
On Mon, Aug 11, 2025 at 02:21:26PM +0100, Sean Nyekjaer wrote:
> On Sat, Aug 09, 2025 at 10:27:52PM +0100, Andy Shevchenko wrote:
> > On Sat, Aug 9, 2025 at 8:06 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > > On Fri, 8 Aug 2025 23:37:51 +0200
> > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > On Fri, Aug 8, 2025 at 5:58 PM Sean Nyekjaer <sean@geanix.com> wrote:
> >
> > ...
> >
> > > > > + struct device *dev = regmap_get_device(st->map);
> > > > >
> > > > > + if (!pm_runtime_status_suspended(dev))
> > > > > + regulator_disable(st->vddio_supply);
> > > >
> > > > I would rather use positive conditional as it seems to me more scalable
> > >
> > > To potentially save time when Sean looks at this. I don't follow. Do you mean
> > > something like
> > > if (pm_runtime_status_suspended(dev))
> > > return;
> > >
> > > regulator_disable(st->vddio_supply);
> > >
> > > ?
> >
> > Yes.
> >
> > > If so I'm not seeing why we'd want this to scale as it's a single use
> > > devm_set_action_or_reset() callback doing just one thing.
> >
> > While I agree in _this_ case, in general the check and return
> > immediately is more scalable for reading purposes, e.g., indentation
> > will be one level less. Also it won't require additional churn in
> > adding {, i.e. changing conditional line just for that.
> >
>
> I like the return early if pm_runtime_status_suspended() is true.
>
> Andy, when doing reviews please keep the function name, as it's much
> easier to add the changes.
>
> Jonathan, do we think checking pm_runtime_status_suspended() is a viable
> option? Should we ask on the linux-pm list?
>
> If it's ok; I will patch:
> drivers/iio/adc/ti-ads1100.c
> drivers/iio/pressure/bmp280-core.c
> drivers/iio/pressure/icp10100.c
Hi Jonathan,
Did you see my question here?
Just ignore this if you are vacationing...
/Sean
On Mon, 25 Aug 2025 15:14:15 +0000
Sean Nyekjaer <sean@geanix.com> wrote:
> On Mon, Aug 11, 2025 at 02:21:26PM +0100, Sean Nyekjaer wrote:
> > On Sat, Aug 09, 2025 at 10:27:52PM +0100, Andy Shevchenko wrote:
> > > On Sat, Aug 9, 2025 at 8:06 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > > > On Fri, 8 Aug 2025 23:37:51 +0200
> > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > On Fri, Aug 8, 2025 at 5:58 PM Sean Nyekjaer <sean@geanix.com> wrote:
> > >
> > > ...
> > >
> > > > > > + struct device *dev = regmap_get_device(st->map);
> > > > > >
> > > > > > + if (!pm_runtime_status_suspended(dev))
> > > > > > + regulator_disable(st->vddio_supply);
> > > > >
> > > > > I would rather use positive conditional as it seems to me more scalable
> > > >
> > > > To potentially save time when Sean looks at this. I don't follow. Do you mean
> > > > something like
> > > > if (pm_runtime_status_suspended(dev))
> > > > return;
> > > >
> > > > regulator_disable(st->vddio_supply);
> > > >
> > > > ?
> > >
> > > Yes.
> > >
> > > > If so I'm not seeing why we'd want this to scale as it's a single use
> > > > devm_set_action_or_reset() callback doing just one thing.
> > >
> > > While I agree in _this_ case, in general the check and return
> > > immediately is more scalable for reading purposes, e.g., indentation
> > > will be one level less. Also it won't require additional churn in
> > > adding {, i.e. changing conditional line just for that.
> > >
> >
> > I like the return early if pm_runtime_status_suspended() is true.
> >
> > Andy, when doing reviews please keep the function name, as it's much
> > easier to add the changes.
> >
> > Jonathan, do we think checking pm_runtime_status_suspended() is a viable
> > option? Should we ask on the linux-pm list?
Perhaps +CC linux-pm and Rafael on one of the patches and see if any replies.
> >
> > If it's ok; I will patch:
> > drivers/iio/adc/ti-ads1100.c
> > drivers/iio/pressure/bmp280-core.c
> > drivers/iio/pressure/icp10100.c
>
> Hi Jonathan,
>
> Did you see my question here?
Indeed not. Wise to poke!
> Just ignore this if you are vacationing...
>
> /Sean
>
>
On Mon, Aug 11, 2025 at 4:21 PM Sean Nyekjaer <sean@geanix.com> wrote: > On Sat, Aug 09, 2025 at 10:27:52PM +0100, Andy Shevchenko wrote: > > On Sat, Aug 9, 2025 at 8:06 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > On Fri, 8 Aug 2025 23:37:51 +0200 > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > On Fri, Aug 8, 2025 at 5:58 PM Sean Nyekjaer <sean@geanix.com> wrote: ... > > > > > + struct device *dev = regmap_get_device(st->map); > > > > > > > > > > + if (!pm_runtime_status_suspended(dev)) > > > > > + regulator_disable(st->vddio_supply); > Andy, when doing reviews please keep the function name, as it's much > easier to add the changes. Sure, sorry I missed it, I thought it was obvious, but you are right, it's better to keep more context. -- With Best Regards, Andy Shevchenko
© 2016 - 2025 Red Hat, Inc.