Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210119124622.9490-1-mike.looijmans@topic.nl> <20210119124622.9490-2-mike.looijmans@topic.nl>
In-Reply-To: <20210119124622.9490-2-mike.looijmans@topic.nl>
From:   Linus Walleij <linus.walleij@linaro.org>
Date:   Fri, 22 Jan 2021 23:38:48 +0100
Message-ID: <CACRpkdbFgskpPUoVK7bU1EyChEvD4e9WHhvcUJh4e1UUO2WFdA@mail.gmail.com>
Subject: Re: [PATCH v6 2/2] iio: accel: Add support for the Bosch-Sensortec BMI088
To:     Mike Looijmans <mike.looijmans@topic.nl>
Cc:     linux-iio <linux-iio@vger.kernel.org>,
        Dan Robertson <dan@dlrobertson.com>,
        =?UTF-8?B?R2HDq3RhbiBBbmRyw6k=?= <rvlander@gaetanandre.eu>,
        Jonathan Bakker <xc-racer2@live.ca>,
        Jonathan Cameron <jic23@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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 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