linux-kernel - Re: [PATCH v6 2/2] iio: accel: Add support for the Bosch-Sensortec BMI088

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 22 Jan 2021 23:38:48 +0100
From:   Linus Walleij <linus.walleij@...aro.org>
To:     Mike Looijmans <mike.looijmans@...ic.nl>
Cc:     linux-iio <linux-iio@...r.kernel.org>,
        Dan Robertson <dan@...obertson.com>,
        Gaëtan André <rvlander@...tanandre.eu>,
        Jonathan Bakker <xc-racer2@...e.ca>,
        Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v6 2/2] iio: accel: Add support for the Bosch-Sensortec BMI088

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@...ic.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@...ic.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