On Fri, 22 Jan 2021 23:38:48 +0100
Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
> Hi Mike,
>
> thanks for your patch!
>
> I have a comment about PM:
>
> On Tue, Jan 19, 2021 at 1:46 PM Mike Looijmans <mike.looijmans@xxxxxxxx> wrote:
>
> > The BMI088 is a combined module with both accelerometer and gyroscope.
> > This adds the accelerometer driver support for the SPI interface.
> > The gyroscope part is already supported by the BMG160 driver.
> >
> > Signed-off-by: Mike Looijmans <mike.looijmans@xxxxxxxx>
> (...)
>
> > +static int bmi088_accel_set_power_state_on(struct bmi088_accel_data *data)
> > +{
> > + struct device *dev = regmap_get_device(data->regmap);
> > + int ret;
> > +
> > + ret = pm_runtime_get_sync(dev);
> > + if (ret < 0) {
> > + pm_runtime_put_noidle(dev);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int bmi088_accel_set_power_state_off(struct bmi088_accel_data *data)
> > +{
> > + struct device *dev = regmap_get_device(data->regmap);
> > + int ret;
> > +
> > + pm_runtime_mark_last_busy(dev);
> > + ret = pm_runtime_put_autosuspend(dev);
> > +
> > + return ret < 0 ? ret : 0;
> > +}
>
> I'm not sure you should wrap the pm_runtime calls like this.
> I think it is better to inline them.
>
> Next, I think it is better to let suspend/resume, i.e. system PM
> reuse runtime PM since you're implementing that. This is why
> we invented PM runtime force resume and force suspend.
Here the driver is turning more off for full suspend than in the
runtime path. If that results in significant extra delay then
it's not appropriate to have that in the runtime suspend path.
Maybe the simplification of not doing the deeper power saving
mode is worth the extra power cost or extra delay, but
I'm not yet convinced.
I'll hold off on applying v7 though whilst we discuss this.
J
>
> Here are some drivers that I implemented using that model:
> drivers/iio/gyro/mpu3050-core.c
> drivers/iio/accel/kxsd9.c
> drivers/iio/magnetometer/ak8974.c
>
> The short summary is:
> - Only implement runtime suspend/resume.
> - SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> pm_runtime_force_resume)
> - In probe() enable runtime PM with autosuspend:
> pm_runtime_get_noresume(dev);
> pm_runtime_set_active(dev);
> pm_runtime_enable(dev);
> pm_runtime_set_autosuspend_delay(dev, NNNN);
> pm_runtime_use_autosuspend(dev);
> pm_runtime_put(dev);
> - In remove() disable it like this:
> pm_runtime_get_sync(dev);
> pm_runtime_put_noidle(dev);
> pm_runtime_disable(dev);
> - Any time the driver needs the hardware, call:
> pm_runtime_get_sync(dev);
> - As soon as you're done using the hardware call:
> pm_runtime_mark_last_busy(dev);
> pm_runtime_put_autosuspend(dev);
>
> The system PM will just hook into the same callbacks and suspend
> the hardware using the existing runtime PM hooks.
>
> This works fine in my drivers and saves some complexity and avoids
> bugs.
>
> Yours,
> Linus Walleij
