| 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
| ||
|
Message-ID: <20240817134911.6043c798@jic23-huawei>
Date: Sat, 17 Aug 2024 13:49:11 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Denis Benato <benato.denis96@...il.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Jagath Jog J
<jagathjog1996@...il.com>, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org, "Luke D . Jones" <luke@...nes.dev>, Jonathan
LoBue <jlobue10@...il.com>
Subject: Re: [PATCH 1/1] iio: bmi323: peripheral in lowest power state on
suspend
On Sun, 11 Aug 2024 18:12:02 +0200
Denis Benato <benato.denis96@...il.com> wrote:
> The bmi323 is mounted on some devices that are powered
> by an internal battery: help in reducing system overall power drain
> while the imu is not in use by resetting it in its lowest power
> draining state.
>
> Signed-off-by: Denis Benato <benato.denis96@...il.com>
Hi Denis.
This is rather an expensive path for runtime PM.
Maybe mention this still only applies in s2idle.
The driver doesn't have a more sophisticated runtime pm to
bring the power down when it's simply not being used:
autosuspend etc which would need a lot more infrastructure
as we need to add when the device must be resumed + when we
are done with talking to it.
Jonathan
> ---
> drivers/iio/imu/bmi323/bmi323.h | 1 +
> drivers/iio/imu/bmi323/bmi323_core.c | 183 ++++++++++++++++++++++++++-
> drivers/iio/imu/bmi323/bmi323_i2c.c | 8 ++
> drivers/iio/imu/bmi323/bmi323_spi.c | 8 ++
> 4 files changed, 194 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iio/imu/bmi323/bmi323.h b/drivers/iio/imu/bmi323/bmi323.h
> index 209bccb1f335..9fd3c5db7520 100644
> --- a/drivers/iio/imu/bmi323/bmi323.h
> +++ b/drivers/iio/imu/bmi323/bmi323.h
> @@ -204,6 +204,7 @@
>
> struct device;
> int bmi323_core_probe(struct device *dev);
> +void bmi323_core_remove(struct device *dev);
> extern const struct regmap_config bmi323_regmap_config;
> extern const struct dev_pm_ops bmi323_core_pm_ops;
>
> diff --git a/drivers/iio/imu/bmi323/bmi323_core.c b/drivers/iio/imu/bmi323/bmi323_core.c
> index 4b2b211a3e88..edb9ce4e66a0 100644
> --- a/drivers/iio/imu/bmi323/bmi323_core.c
> +++ b/drivers/iio/imu/bmi323/bmi323_core.c
> @@ -118,6 +118,24 @@ static const struct bmi323_hw bmi323_hw[2] = {
> },
> };
>
> +struct bmi323_ext_regs_settings {
> + unsigned int reg;
> + unsigned int val;
> +};
> +
> +struct bmi323_regs_settings {
> + unsigned int reg;
> + unsigned int val;
> +};
Why are two types useful?
> +
> +#define EXT_SETTING_REGISTERS 12
> +#define SETTING_REGISTERS 9
> +
> +struct bmi323_regs_runtime_pm {
> + struct bmi323_regs_settings reg_settings[SETTING_REGISTERS];
> + struct bmi323_ext_regs_settings ext_settings[EXT_SETTING_REGISTERS];
As below. Break these part so that the register addresses are just const data
and the values are all that is stored in here.
> +};
> +
> struct bmi323_data {
> struct device *dev;
> struct regmap *regmap;
> @@ -130,6 +148,7 @@ struct bmi323_data {
> u32 odrns[BMI323_SENSORS_CNT];
> u32 odrhz[BMI323_SENSORS_CNT];
> unsigned int feature_events;
> + struct bmi323_regs_runtime_pm runtime_pm_status;
>
> /*
> * Lock to protect the members of device's private data from concurrent
> @@ -1982,7 +2001,7 @@ static int bmi323_set_bw(struct bmi323_data *data,
> FIELD_PREP(BMI323_ACC_GYRO_CONF_BW_MSK, bw));
> }
>
> -static int bmi323_init(struct bmi323_data *data)
> +static int bmi323_init(struct bmi323_data *data, bool first_init)
> {
> int ret, val;
>
> @@ -2030,6 +2049,9 @@ static int bmi323_init(struct bmi323_data *data)
> return dev_err_probe(data->dev, -EINVAL,
> "Sensor power error = 0x%x\n", val);
>
Maybe better to split the function into two parts and call both in probe()
but not in resume()
> + if (!first_init)
> + return 0;
> +
> /*
> * Set the Bandwidth coefficient which defines the 3 dB cutoff
> * frequency in relation to the ODR.
> @@ -2078,9 +2100,32 @@ int bmi323_core_probe(struct device *dev)
> data = iio_priv(indio_dev);
> data->dev = dev;
> data->regmap = regmap;
> + data->irq_pin = BMI323_IRQ_DISABLED;
> + data->state = BMI323_IDLE;
> + data->runtime_pm_status.reg_settings[0].reg = BMI323_INT_MAP1_REG;
Use a local pointer to reg_settings / ext_settings to shorten all these a lot.
Or better still separate the register addresses into a const arrays with only
the values (in a 1 D array) being changed by code.
> + data->runtime_pm_status.reg_settings[1].reg = BMI323_INT_MAP2_REG;
> + data->runtime_pm_status.reg_settings[2].reg = BMI323_IO_INT_CTR_REG;
> + data->runtime_pm_status.reg_settings[3].reg = BMI323_IO_INT_CONF_REG;
> + data->runtime_pm_status.reg_settings[4].reg = BMI323_ACC_CONF_REG;
> + data->runtime_pm_status.reg_settings[5].reg = BMI323_GYRO_CONF_REG;
> + data->runtime_pm_status.reg_settings[6].reg = BMI323_FEAT_IO0_REG;
> + data->runtime_pm_status.reg_settings[7].reg = BMI323_FIFO_WTRMRK_REG;
> + data->runtime_pm_status.reg_settings[8].reg = BMI323_FIFO_CONF_REG;
> + data->runtime_pm_status.ext_settings[0].reg = BMI323_GEN_SET1_REG;
> + data->runtime_pm_status.ext_settings[1].reg = BMI323_TAP1_REG;
> + data->runtime_pm_status.ext_settings[2].reg = BMI323_TAP2_REG;
> + data->runtime_pm_status.ext_settings[3].reg = BMI323_TAP3_REG;
> + data->runtime_pm_status.ext_settings[4].reg = BMI323_FEAT_IO0_S_TAP_MSK;
> + data->runtime_pm_status.ext_settings[5].reg = BMI323_STEP_SC1_REG;
> + data->runtime_pm_status.ext_settings[6].reg = BMI323_ANYMO1_REG;
> + data->runtime_pm_status.ext_settings[7].reg = BMI323_NOMO1_REG;
> + data->runtime_pm_status.ext_settings[8].reg = BMI323_ANYMO1_REG + BMI323_MO2_OFFSET;
> + data->runtime_pm_status.ext_settings[9].reg = BMI323_NOMO1_REG + BMI323_MO2_OFFSET;
> + data->runtime_pm_status.ext_settings[10].reg = BMI323_ANYMO1_REG + BMI323_MO3_OFFSET;
> + data->runtime_pm_status.ext_settings[11].reg = BMI323_NOMO1_REG + BMI323_MO3_OFFSET;
> mutex_init(&data->mutex);
>
> - ret = bmi323_init(data);
> + ret = bmi323_init(data, true);
> if (ret)
> return -EINVAL;
>
> @@ -2117,21 +2162,147 @@ int bmi323_core_probe(struct device *dev)
> return dev_err_probe(data->dev, ret,
> "Unable to register iio device\n");
>
> - return 0;
> + return bmi323_fifo_disable(data);
> }
> EXPORT_SYMBOL_NS_GPL(bmi323_core_probe, IIO_BMI323);
>
> +void bmi323_core_remove(struct device *dev)
> +{
> + struct regmap *const regmap = dev_get_regmap(dev, NULL);
> +
> + /*
> + * Place the peripheral in its lowest power consuming state.
> + */
> + if (regmap)
> + regmap_write(regmap, BMI323_CMD_REG, BMI323_RST_VAL);
This is happening before the userspace interfaces are removed.
Unlikely to be a good idea. Use devm_add_action_or_reset()
to register this during probe so that it is torn down
at the correct point.
> +}
> +EXPORT_SYMBOL_NS_GPL(bmi323_core_remove, IIO_BMI323);
> +
> #if defined(CONFIG_PM)
> static int bmi323_core_runtime_suspend(struct device *dev)
>
> static int bmi323_core_runtime_resume(struct device *dev)
> {
> - struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct iio_dev *const indio_dev = dev_get_drvdata(dev);
Whilst true that it's const, do we care? I'd drop this
modification.
> + struct bmi323_data *const data = iio_priv(indio_dev);
Likewise, unlikely the const really helps us.
> +
> + int ret = 0;
Always set, so don't initialize here.
> +
> + guard(mutex)(&data->mutex);
> +
> + /*
> + * Perform the device power-on and initial setup once again
> + * after being reset in the lower power state by runtime-pm.
> + */
> + ret = bmi323_init(data, false);
> + if (!ret)
> + return ret;
> +
> + /* Register must be cleared before changing an active config */
> + ret = regmap_write(data->regmap, BMI323_FEAT_IO0_REG, 0);
> + if (ret) {
> + dev_err(data->dev, "Error stopping feature engine\n");
> + return ret;
> + }
> +
> + /*
> + * Restore external registers saved by suspend pm callback.
> + */
> + for (unsigned int i = 0; i < EXT_SETTING_REGISTERS; ++i) {
i++ preferred. It's more common and doesn't matter here.
I'd use a local variable for a pointer to data->runtime_pm_status.ext_settings[i]
to make these more readable. Same for other similar cases.
> + ret = bmi323_write_ext_reg(data,
> + data->runtime_pm_status.ext_settings[i].reg,
> + data->runtime_pm_status.ext_settings[i].val);
> + if (ret) {
> + dev_err(data->dev, "Error writing bmi323 external reg 0x%x: %d\n",
> + data->runtime_pm_status.ext_settings[i].reg, ret);
> + return ret;
> + }
> + }
> +
> + /*
> + * Restore registers saved by suspend pm callback.
> + */
> + for (unsigned int i = 0; i < SETTING_REGISTERS; ++i) {
> + ret = regmap_write(data->regmap,
> + data->runtime_pm_status.reg_settings[i].reg,
> + data->runtime_pm_status.reg_settings[i].val);
> + if (ret) {
> + dev_err(data->dev, "Error writing bmi323 reg 0x%x: %d\n",
> + data->runtime_pm_status.reg_settings[i].reg, ret);
> + return ret;
> + }
> + }
> +
> + if (data->state == BMI323_BUFFER_FIFO) {
> + ret = regmap_write(data->regmap, BMI323_FIFO_CTRL_REG,
> + BMI323_FIFO_FLUSH_MSK);
> + if (ret) {
> + dev_err(data->dev, "Error flushing FIFO buffer: %d\n", ret);
> + return ret;
> + }
> + }
> +
> + unsigned int val;
> +
> + ret = regmap_read(data->regmap, BMI323_ERR_REG, &val);
> + if (ret) {
> + dev_err(data->dev, "Error reading bmi323 error register: %d\n", ret);
> + return ret;
> + }
> +
> + if (val) {
> + dev_err(data->dev, "Sensor power error in PM = 0x%x\n", val);
> + return -EINVAL;
> + }
>
> return iio_device_resume_triggering(indio_dev);
> }
> diff --git a/drivers/iio/imu/bmi323/bmi323_spi.c b/drivers/iio/imu/bmi323/bmi323_spi.c
> index 9de3ade78d71..f42c49c471c1 100644
> --- a/drivers/iio/imu/bmi323/bmi323_spi.c
> +++ b/drivers/iio/imu/bmi323/bmi323_spi.c
> @@ -64,6 +64,13 @@ static int bmi323_spi_probe(struct spi_device *spi)
> return bmi323_core_probe(dev);
> }
>
> +static void bmi323_spi_remove(struct spi_device *spi)
> +{
> + struct device *const dev = &spi->dev;
> +
> + bmi323_core_remove(dev);
Register anything that needs doing with devm_add_action_or_reset()
after whatever is undoing is first done in probe().
There shouldn't be a need for a remove function and it is very
likely to have ordering issues given driver previously didn't
have one.
> +}
> +
> static const struct spi_device_id bmi323_spi_ids[] = {
> { "bmi323" },
> { }
> @@ -83,6 +90,7 @@ static struct spi_driver bmi323_spi_driver = {
> .of_match_table = bmi323_of_spi_match,
> },
> .probe = bmi323_spi_probe,
> + .remove = bmi323_spi_remove,
> .id_table = bmi323_spi_ids,
> };
> module_spi_driver(bmi323_spi_driver);
Powered by blists - more mailing lists