Linux-IIO Archive on lore.kernel.org
 help / color / Atom feed
From: Jonathan Cameron <jic23@kernel.org>
To: Linus Walleij <linus.walleij@linaro.org>
Cc: "Mike Looijmans" <mike.looijmans@topic.nl>,
	linux-iio <linux-iio@vger.kernel.org>,
	"Dan Robertson" <dan@dlrobertson.com>,
	"Gaëtan André" <rvlander@gaetanandre.eu>,
	"Jonathan Bakker" <xc-racer2@live.ca>,
	"Lars-Peter Clausen" <lars@metafoo.de>,
	"Peter Meerwald-Stadler" <pmeerw@pmeerw.net>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v6 2/2] iio: accel: Add support for the Bosch-Sensortec BMI088
Date: Sat, 23 Jan 2021 15:35:11 +0000
Message-ID: <20210123153511.1802a15a@archlinux> (raw)
In-Reply-To: <CACRpkdbFgskpPUoVK7bU1EyChEvD4e9WHhvcUJh4e1UUO2WFdA@mail.gmail.com>

On Fri, 22 Jan 2021 23:38:48 +0100
Linus Walleij <linus.walleij@linaro.org> 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@topic.nl> 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@topic.nl>  
> (...)
> 
> > +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


  reply index

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-19 12:46 [PATCH v6 1/2] dt-bindings: iio: accel: Add bmi088 accelerometer bindings Mike Looijmans
2021-01-19 12:46 ` [PATCH v6 2/2] iio: accel: Add support for the Bosch-Sensortec BMI088 Mike Looijmans
2021-01-20 10:09   ` kernel test robot
2021-01-20 20:22   ` Jonathan Cameron
     [not found]     ` <1b153bce-a66a-45ee-a5c6-963ea6fb1c82.949ef384-8293-46b8-903f-40a477c056ae.c4ec3c42-7acb-4ce8-997f-adf405d31335@emailsignatures365.codetwo.com>
     [not found]       ` <1b153bce-a66a-45ee-a5c6-963ea6fb1c82.0d2bd5fa-15cc-4b27-b94e-83614f9e5b38.ca49b241-105e-4dde-a295-e8168cb6d390@emailsignatures365.codetwo.com>
2021-01-21  9:02         ` Mike Looijmans
2021-01-22 22:38   ` Linus Walleij
2021-01-23 15:35     ` Jonathan Cameron [this message]
2021-01-23 23:21       ` Linus Walleij
2021-01-24 13:23         ` Jonathan Cameron
     [not found]           ` <1b153bce-a66a-45ee-a5c6-963ea6fb1c82.949ef384-8293-46b8-903f-40a477c056ae.e23f1b65-3084-4bd7-abd5-c186f8c4c35c@emailsignatures365.codetwo.com>
     [not found]             ` <1b153bce-a66a-45ee-a5c6-963ea6fb1c82.0d2bd5fa-15cc-4b27-b94e-83614f9e5b38.4d211e5c-af1f-4f0a-9714-ec208ef9be8d@emailsignatures365.codetwo.com>
2021-01-25  7:59               ` Mike Looijmans
2021-01-19 23:28 ` [PATCH v6 1/2] dt-bindings: iio: accel: Add bmi088 accelerometer bindings Rob Herring
2021-01-20  1:31 ` Rob Herring
     [not found]   ` <1b153bce-a66a-45ee-a5c6-963ea6fb1c82.949ef384-8293-46b8-903f-40a477c056ae.06120990-9bfb-4196-a6ce-19c5b16aae9a@emailsignatures365.codetwo.com>
     [not found]     ` <1b153bce-a66a-45ee-a5c6-963ea6fb1c82.0d2bd5fa-15cc-4b27-b94e-83614f9e5b38.70d7c9ca-f015-4fc4-8136-1c0364cd5511@emailsignatures365.codetwo.com>
2021-01-20  7:21       ` Mike Looijmans
2021-01-20 18:50 ` Jonathan Cameron
     [not found]   ` <1b153bce-a66a-45ee-a5c6-963ea6fb1c82.949ef384-8293-46b8-903f-40a477c056ae.c9179140-7138-467c-85e9-419e68c95bd4@emailsignatures365.codetwo.com>
     [not found]     ` <1b153bce-a66a-45ee-a5c6-963ea6fb1c82.0d2bd5fa-15cc-4b27-b94e-83614f9e5b38.02f2b6e0-f94c-4e13-b820-c0a2b10c9a96@emailsignatures365.codetwo.com>
2021-01-21  8:46       ` Mike Looijmans

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210123153511.1802a15a@archlinux \
    --to=jic23@kernel.org \
    --cc=dan@dlrobertson.com \
    --cc=lars@metafoo.de \
    --cc=linus.walleij@linaro.org \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mike.looijmans@topic.nl \
    --cc=pmeerw@pmeerw.net \
    --cc=rvlander@gaetanandre.eu \
    --cc=xc-racer2@live.ca \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-IIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iio/0 linux-iio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iio linux-iio/ https://lore.kernel.org/linux-iio \
		linux-iio@vger.kernel.org
	public-inbox-index linux-iio

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-iio


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git