Linux-IIO Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 00/13] Rework sensors engines and power management
@ 2020-02-12 17:40 Jean-Baptiste Maneyrol
  2020-02-12 17:40 ` [PATCH 01/13] iio: imu: inv_mpu6050: enable i2c aux mux bypass only once Jean-Baptiste Maneyrol
                   ` (12 more replies)
  0 siblings, 13 replies; 21+ messages in thread
From: Jean-Baptiste Maneyrol @ 2020-02-12 17:40 UTC (permalink / raw)
  To: jic23, linux-iio; +Cc: Jean-Baptiste Maneyrol

The goal of this series is to rework sensors engines and power
management using runtime power with autosuspend. By reducing power
on/off it enables correct usage of the polling interface at low
frequencies. It also simplifies a lot of things inside the driver,
making it much more simpler and easier to read.

The series also include several reworks required to have the
implementation working.

Jean-Baptiste Maneyrol (13):
  iio: imu: inv_mpu6050: enable i2c aux mux bypass only once
  iio: imu: inv_mpu6050: delete useless check
  iio: imu: inv_mpu6050: set power on/off only once during all init
  iio: imu: inv_mpu6050: simplify polling magnetometer
  iio: imu: inv_mpu6050: early init of chip_config for use at setup
  iio: imu: inv_mpu6050: add all signal path resets at init
  iio: imu: inv_mpu6050: fix sleep time when turning regulators on
  iio: imu: inv_mpu6050: rewrite power and engine management
  iio: imu: inv_mpu6050: fix data polling interface
  iio: imu: inv_mpu6050: factorize fifo enable/disable
  iio: imu: inv_mpu6050: dynamic sampling rate change
  iio: imu: inv_mpu6050: use runtime pm with autosuspend
  iio: imu: inv_mpu6050: temperature only work with accel/gyro

 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c    | 560 +++++++++++++-----
 drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c     |  62 +-
 drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h     |  38 +-
 drivers/iio/imu/inv_mpu6050/inv_mpu_magn.c    |  49 +-
 drivers/iio/imu/inv_mpu6050/inv_mpu_magn.h    |   5 +-
 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c    |  57 +-
 drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c     |  10 +-
 drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c | 156 ++---
 8 files changed, 568 insertions(+), 369 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 01/13] iio: imu: inv_mpu6050: enable i2c aux mux bypass only once
  2020-02-12 17:40 [PATCH 00/13] Rework sensors engines and power management Jean-Baptiste Maneyrol
@ 2020-02-12 17:40 ` Jean-Baptiste Maneyrol
  2020-02-15 17:30   ` Jonathan Cameron
  2020-02-12 17:40 ` [PATCH 02/13] iio: imu: inv_mpu6050: delete useless check Jean-Baptiste Maneyrol
                   ` (11 subsequent siblings)
  12 siblings, 1 reply; 21+ messages in thread
From: Jean-Baptiste Maneyrol @ 2020-02-12 17:40 UTC (permalink / raw)
  To: jic23, linux-iio; +Cc: Jean-Baptiste Maneyrol

i2c auxiliary mux is done by analog switches. You do not need to
set them for every i2c transfer.
Just set i2c bypass bit at init and do noting in i2c de/select.

Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c | 71 +++++++++--------------
 1 file changed, 28 insertions(+), 43 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
index 1363d3776523..24df880248f2 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
@@ -20,38 +20,6 @@ static const struct regmap_config inv_mpu_regmap_config = {
 
 static int inv_mpu6050_select_bypass(struct i2c_mux_core *muxc, u32 chan_id)
 {
-	struct iio_dev *indio_dev = i2c_mux_priv(muxc);
-	struct inv_mpu6050_state *st = iio_priv(indio_dev);
-	int ret;
-
-	mutex_lock(&st->lock);
-
-	ret = inv_mpu6050_set_power_itg(st, true);
-	if (ret)
-		goto error_unlock;
-
-	ret = regmap_write(st->map, st->reg->int_pin_cfg,
-			   st->irq_mask | INV_MPU6050_BIT_BYPASS_EN);
-
-error_unlock:
-	mutex_unlock(&st->lock);
-
-	return ret;
-}
-
-static int inv_mpu6050_deselect_bypass(struct i2c_mux_core *muxc, u32 chan_id)
-{
-	struct iio_dev *indio_dev = i2c_mux_priv(muxc);
-	struct inv_mpu6050_state *st = iio_priv(indio_dev);
-
-	mutex_lock(&st->lock);
-
-	/* It doesn't really matter if any of the calls fail */
-	regmap_write(st->map, st->reg->int_pin_cfg, st->irq_mask);
-	inv_mpu6050_set_power_itg(st, false);
-
-	mutex_unlock(&st->lock);
-
 	return 0;
 }
 
@@ -79,19 +47,20 @@ static bool inv_mpu_i2c_aux_bus(struct device *dev)
 	}
 }
 
-/*
- * MPU9xxx magnetometer support requires to disable i2c auxiliary bus support.
- * To ensure backward compatibility with existing setups, do not disable
- * i2c auxiliary bus if it used.
- * Check for i2c-gate node in devicetree and set magnetometer disabled.
- * Only MPU6500 is supported by ACPI, no need to check.
- */
-static int inv_mpu_magn_disable(struct iio_dev *indio_dev)
+static int inv_mpu_i2c_aux_setup(struct iio_dev *indio_dev)
 {
 	struct inv_mpu6050_state *st = iio_priv(indio_dev);
 	struct device *dev = indio_dev->dev.parent;
 	struct device_node *mux_node;
+	int ret;
 
+	/*
+	 * MPU9xxx magnetometer support requires to disable i2c auxiliary bus.
+	 * To ensure backward compatibility with existing setups, do not disable
+	 * i2c auxiliary bus if it used.
+	 * Check for i2c-gate node in devicetree and set magnetometer disabled.
+	 * Only MPU6500 is supported by ACPI, no need to check.
+	 */
 	switch (st->chip_type) {
 	case INV_MPU9150:
 	case INV_MPU9250:
@@ -107,7 +76,24 @@ static int inv_mpu_magn_disable(struct iio_dev *indio_dev)
 		break;
 	}
 
+	/* enable i2c bypass when using i2c auxiliary bus */
+	if (inv_mpu_i2c_aux_bus(dev)) {
+		ret = inv_mpu6050_set_power_itg(st, true);
+		if (ret)
+			return ret;
+		ret = regmap_write(st->map, st->reg->int_pin_cfg,
+				   st->irq_mask | INV_MPU6050_BIT_BYPASS_EN);
+		if (ret)
+			goto error;
+		ret = inv_mpu6050_set_power_itg(st, false);
+		if (ret)
+			goto error;
+	}
+
 	return 0;
+error:
+	inv_mpu6050_set_power_itg(st, false);
+	return ret;
 }
 
 /**
@@ -151,7 +137,7 @@ static int inv_mpu_probe(struct i2c_client *client,
 	}
 
 	result = inv_mpu_core_probe(regmap, client->irq, name,
-				    inv_mpu_magn_disable, chip_type);
+				    inv_mpu_i2c_aux_setup, chip_type);
 	if (result < 0)
 		return result;
 
@@ -160,8 +146,7 @@ static int inv_mpu_probe(struct i2c_client *client,
 		/* declare i2c auxiliary bus */
 		st->muxc = i2c_mux_alloc(client->adapter, &client->dev,
 					 1, 0, I2C_MUX_LOCKED | I2C_MUX_GATE,
-					 inv_mpu6050_select_bypass,
-					 inv_mpu6050_deselect_bypass);
+					 inv_mpu6050_select_bypass, NULL);
 		if (!st->muxc)
 			return -ENOMEM;
 		st->muxc->priv = dev_get_drvdata(&client->dev);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 02/13] iio: imu: inv_mpu6050: delete useless check
  2020-02-12 17:40 [PATCH 00/13] Rework sensors engines and power management Jean-Baptiste Maneyrol
  2020-02-12 17:40 ` [PATCH 01/13] iio: imu: inv_mpu6050: enable i2c aux mux bypass only once Jean-Baptiste Maneyrol
@ 2020-02-12 17:40 ` Jean-Baptiste Maneyrol
  2020-02-12 17:40 ` [PATCH 03/13] iio: imu: inv_mpu6050: set power on/off only once during all init Jean-Baptiste Maneyrol
                   ` (10 subsequent siblings)
  12 siblings, 0 replies; 21+ messages in thread
From: Jean-Baptiste Maneyrol @ 2020-02-12 17:40 UTC (permalink / raw)
  To: jic23, linux-iio; +Cc: Jean-Baptiste Maneyrol

If we are here it means we have fifo enabled for 1 sensor
at least. And interrupt is always required for using trigger.

Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
index f9fdf4302a91..d7397705974e 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
@@ -123,14 +123,11 @@ int inv_reset_fifo(struct iio_dev *indio_dev)
 		goto reset_fifo_fail;
 
 	/* enable interrupt */
-	if (st->chip_config.accl_fifo_enable ||
-	    st->chip_config.gyro_fifo_enable ||
-	    st->chip_config.magn_fifo_enable) {
-		result = regmap_write(st->map, st->reg->int_enable,
-				      INV_MPU6050_BIT_DATA_RDY_EN);
-		if (result)
-			return result;
-	}
+	result = regmap_write(st->map, st->reg->int_enable,
+			      INV_MPU6050_BIT_DATA_RDY_EN);
+	if (result)
+		return result;
+
 	/* enable FIFO reading */
 	d = st->chip_config.user_ctrl | INV_MPU6050_BIT_FIFO_EN;
 	result = regmap_write(st->map, st->reg->user_ctrl, d);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 03/13] iio: imu: inv_mpu6050: set power on/off only once during all init
  2020-02-12 17:40 [PATCH 00/13] Rework sensors engines and power management Jean-Baptiste Maneyrol
  2020-02-12 17:40 ` [PATCH 01/13] iio: imu: inv_mpu6050: enable i2c aux mux bypass only once Jean-Baptiste Maneyrol
  2020-02-12 17:40 ` [PATCH 02/13] iio: imu: inv_mpu6050: delete useless check Jean-Baptiste Maneyrol
@ 2020-02-12 17:40 ` Jean-Baptiste Maneyrol
  2020-02-12 17:40 ` [PATCH 04/13] iio: imu: inv_mpu6050: simplify polling magnetometer Jean-Baptiste Maneyrol
                   ` (9 subsequent siblings)
  12 siblings, 0 replies; 21+ messages in thread
From: Jean-Baptiste Maneyrol @ 2020-02-12 17:40 UTC (permalink / raw)
  To: jic23, linux-iio; +Cc: Jean-Baptiste Maneyrol

This way there is no need anymore to export the power function to
i2c and spi modules.
Bus setup is done inside init when power is on and the result is
now checked.

Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 41 ++++++++++++----------
 drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c  | 11 +-----
 drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c  | 10 +-----
 3 files changed, 24 insertions(+), 38 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index c4db9086775c..0b06d6aa6469 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -301,7 +301,6 @@ int inv_mpu6050_set_power_itg(struct inv_mpu6050_state *st, bool power_on)
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(inv_mpu6050_set_power_itg);
 
 static int inv_mpu6050_set_gyro_fsr(struct inv_mpu6050_state *st,
 				    enum inv_mpu6050_fsr_e val)
@@ -371,27 +370,23 @@ static int inv_mpu6050_init_config(struct iio_dev *indio_dev)
 	u8 d;
 	struct inv_mpu6050_state *st = iio_priv(indio_dev);
 
-	result = inv_mpu6050_set_power_itg(st, true);
-	if (result)
-		return result;
-
 	result = inv_mpu6050_set_gyro_fsr(st, INV_MPU6050_FSR_2000DPS);
 	if (result)
-		goto error_power_off;
+		return result;
 
 	result = inv_mpu6050_set_lpf_regs(st, INV_MPU6050_FILTER_20HZ);
 	if (result)
-		goto error_power_off;
+		return result;
 
 	d = INV_MPU6050_FIFO_RATE_TO_DIVIDER(INV_MPU6050_INIT_FIFO_RATE);
 	result = regmap_write(st->map, st->reg->sample_rate_div, d);
 	if (result)
-		goto error_power_off;
+		return result;
 
 	d = (INV_MPU6050_FS_02G << INV_MPU6050_ACCL_CONFIG_FSR_SHIFT);
 	result = regmap_write(st->map, st->reg->accl_config, d);
 	if (result)
-		goto error_power_off;
+		return result;
 
 	result = regmap_write(st->map, st->reg->int_pin_cfg, st->irq_mask);
 	if (result)
@@ -410,13 +405,9 @@ static int inv_mpu6050_init_config(struct iio_dev *indio_dev)
 	/* magn chip init, noop if not present in the chip */
 	result = inv_mpu_magn_probe(st);
 	if (result)
-		goto error_power_off;
-
-	return inv_mpu6050_set_power_itg(st, false);
+		return result;
 
-error_power_off:
-	inv_mpu6050_set_power_itg(st, false);
-	return result;
+	return 0;
 }
 
 static int inv_mpu6050_sensor_set(struct inv_mpu6050_state  *st, int reg,
@@ -1176,7 +1167,7 @@ static int inv_check_and_setup_chip(struct inv_mpu6050_state *st)
 	if (result)
 		goto error_power_off;
 
-	return inv_mpu6050_set_power_itg(st, false);
+	return 0;
 
 error_power_off:
 	inv_mpu6050_set_power_itg(st, false);
@@ -1341,7 +1332,7 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
 	result = inv_mpu6050_init_config(indio_dev);
 	if (result) {
 		dev_err(dev, "Could not initialize device.\n");
-		return result;
+		goto error_power_off;
 	}
 
 	dev_set_drvdata(dev, indio_dev);
@@ -1353,8 +1344,16 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
 		indio_dev->name = dev_name(dev);
 
 	/* requires parent device set in indio_dev */
-	if (inv_mpu_bus_setup)
-		inv_mpu_bus_setup(indio_dev);
+	if (inv_mpu_bus_setup) {
+		result = inv_mpu_bus_setup(indio_dev);
+		if (result)
+			goto error_power_off;
+	}
+
+	/* chip init is done, turning off */
+	result = inv_mpu6050_set_power_itg(st, false);
+	if (result)
+		return result;
 
 	switch (chip_type) {
 	case INV_MPU9150:
@@ -1413,6 +1412,10 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
 	}
 
 	return 0;
+
+error_power_off:
+	inv_mpu6050_set_power_itg(st, false);
+	return result;
 }
 EXPORT_SYMBOL_GPL(inv_mpu_core_probe);
 
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
index 24df880248f2..6993d3b87bb0 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
@@ -78,22 +78,13 @@ static int inv_mpu_i2c_aux_setup(struct iio_dev *indio_dev)
 
 	/* enable i2c bypass when using i2c auxiliary bus */
 	if (inv_mpu_i2c_aux_bus(dev)) {
-		ret = inv_mpu6050_set_power_itg(st, true);
-		if (ret)
-			return ret;
 		ret = regmap_write(st->map, st->reg->int_pin_cfg,
 				   st->irq_mask | INV_MPU6050_BIT_BYPASS_EN);
 		if (ret)
-			goto error;
-		ret = inv_mpu6050_set_power_itg(st, false);
-		if (ret)
-			goto error;
+			return ret;
 	}
 
 	return 0;
-error:
-	inv_mpu6050_set_power_itg(st, false);
-	return ret;
 }
 
 /**
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c
index bc351dd58c53..673b198e6368 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c
@@ -21,10 +21,6 @@ static int inv_mpu_i2c_disable(struct iio_dev *indio_dev)
 	struct inv_mpu6050_state *st = iio_priv(indio_dev);
 	int ret = 0;
 
-	ret = inv_mpu6050_set_power_itg(st, true);
-	if (ret)
-		return ret;
-
 	if (st->reg->i2c_if) {
 		ret = regmap_write(st->map, st->reg->i2c_if,
 				   INV_ICM20602_BIT_I2C_IF_DIS);
@@ -33,12 +29,8 @@ static int inv_mpu_i2c_disable(struct iio_dev *indio_dev)
 		ret = regmap_write(st->map, st->reg->user_ctrl,
 				   st->chip_config.user_ctrl);
 	}
-	if (ret) {
-		inv_mpu6050_set_power_itg(st, false);
-		return ret;
-	}
 
-	return inv_mpu6050_set_power_itg(st, false);
+	return ret;
 }
 
 static int inv_mpu_probe(struct spi_device *spi)
-- 
2.17.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 04/13] iio: imu: inv_mpu6050: simplify polling magnetometer
  2020-02-12 17:40 [PATCH 00/13] Rework sensors engines and power management Jean-Baptiste Maneyrol
                   ` (2 preceding siblings ...)
  2020-02-12 17:40 ` [PATCH 03/13] iio: imu: inv_mpu6050: set power on/off only once during all init Jean-Baptiste Maneyrol
@ 2020-02-12 17:40 ` Jean-Baptiste Maneyrol
  2020-02-12 17:40 ` [PATCH 05/13] iio: imu: inv_mpu6050: early init of chip_config for use at setup Jean-Baptiste Maneyrol
                   ` (8 subsequent siblings)
  12 siblings, 0 replies; 21+ messages in thread
From: Jean-Baptiste Maneyrol @ 2020-02-12 17:40 UTC (permalink / raw)
  To: jic23, linux-iio; +Cc: Jean-Baptiste Maneyrol

Do not change the sampling rate value. Let userspace decide what
is the sampling rate to use.
Read only the requested axis.

Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_magn.c | 34 +++++++++-------------
 1 file changed, 13 insertions(+), 21 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_magn.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_magn.c
index 4f192352521e..607104a2631e 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_magn.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_magn.c
@@ -319,36 +319,36 @@ int inv_mpu_magn_set_orient(struct inv_mpu6050_state *st)
 int inv_mpu_magn_read(const struct inv_mpu6050_state *st, int axis, int *val)
 {
 	unsigned int user_ctrl, status;
-	__be16 data[3];
+	__be16 data;
 	uint8_t addr;
-	uint8_t d;
-	unsigned int period_ms;
+	unsigned int freq_hz, period_ms;
 	int ret;
 
 	/* quit if chip is not supported */
 	if (!inv_magn_supported(st))
 		return -ENODEV;
 
-	/* Mag data: X - Y - Z */
+	/* Mag data: XH,XL,YH,YL,ZH,ZL */
 	switch (axis) {
 	case IIO_MOD_X:
 		addr = 0;
 		break;
 	case IIO_MOD_Y:
-		addr = 1;
+		addr = 2;
 		break;
 	case IIO_MOD_Z:
-		addr = 2;
+		addr = 4;
 		break;
 	default:
 		return -EINVAL;
 	}
+	addr += INV_MPU6050_REG_EXT_SENS_DATA;
 
-	/* set sample rate to max mag freq */
-	d = INV_MPU6050_FIFO_RATE_TO_DIVIDER(INV_MPU_MAGN_FREQ_HZ_MAX);
-	ret = regmap_write(st->map, st->reg->sample_rate_div, d);
-	if (ret)
-		return ret;
+	/* compute period depending on current sampling rate */
+	freq_hz = INV_MPU6050_DIVIDER_TO_FIFO_RATE(st->chip_config.divider);
+	if (freq_hz > INV_MPU_MAGN_FREQ_HZ_MAX)
+		freq_hz = INV_MPU_MAGN_FREQ_HZ_MAX;
+	period_ms = 1000 / freq_hz;
 
 	/* start i2c master, wait for xfer, stop */
 	user_ctrl = st->chip_config.user_ctrl | INV_MPU6050_BIT_I2C_MST_EN;
@@ -357,19 +357,12 @@ int inv_mpu_magn_read(const struct inv_mpu6050_state *st, int axis, int *val)
 		return ret;
 
 	/* need to wait 2 periods + half-period margin */
-	period_ms = 1000 / INV_MPU_MAGN_FREQ_HZ_MAX;
 	msleep(period_ms * 2 + period_ms / 2);
 	user_ctrl = st->chip_config.user_ctrl;
 	ret = regmap_write(st->map, st->reg->user_ctrl, user_ctrl);
 	if (ret)
 		return ret;
 
-	/* restore sample rate */
-	d = st->chip_config.divider;
-	ret = regmap_write(st->map, st->reg->sample_rate_div, d);
-	if (ret)
-		return ret;
-
 	/* check i2c status and read raw data */
 	ret = regmap_read(st->map, INV_MPU6050_REG_I2C_MST_STATUS, &status);
 	if (ret)
@@ -379,12 +372,11 @@ int inv_mpu_magn_read(const struct inv_mpu6050_state *st, int axis, int *val)
 			status & INV_MPU6050_BIT_I2C_SLV1_NACK)
 		return -EIO;
 
-	ret = regmap_bulk_read(st->map, INV_MPU6050_REG_EXT_SENS_DATA,
-			       data, sizeof(data));
+	ret = regmap_bulk_read(st->map, addr, &data, sizeof(data));
 	if (ret)
 		return ret;
 
-	*val = (int16_t)be16_to_cpu(data[addr]);
+	*val = (int16_t)be16_to_cpu(data);
 
 	return IIO_VAL_INT;
 }
-- 
2.17.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 05/13] iio: imu: inv_mpu6050: early init of chip_config for use at setup
  2020-02-12 17:40 [PATCH 00/13] Rework sensors engines and power management Jean-Baptiste Maneyrol
                   ` (3 preceding siblings ...)
  2020-02-12 17:40 ` [PATCH 04/13] iio: imu: inv_mpu6050: simplify polling magnetometer Jean-Baptiste Maneyrol
@ 2020-02-12 17:40 ` Jean-Baptiste Maneyrol
  2020-02-12 17:40 ` [PATCH 06/13] iio: imu: inv_mpu6050: add all signal path resets at init Jean-Baptiste Maneyrol
                   ` (7 subsequent siblings)
  12 siblings, 0 replies; 21+ messages in thread
From: Jean-Baptiste Maneyrol @ 2020-02-12 17:40 UTC (permalink / raw)
  To: jic23, linux-iio; +Cc: Jean-Baptiste Maneyrol

Init chip_config early and use its values for initial setup.
More coherent, prevent possible mistakes.

Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 15 +++++++--------
 drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h  |  1 -
 2 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index 0b06d6aa6469..85872e55154f 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -101,7 +101,7 @@ static const struct inv_mpu6050_reg_map reg_set_6050 = {
 static const struct inv_mpu6050_chip_config chip_config_6050 = {
 	.fsr = INV_MPU6050_FSR_2000DPS,
 	.lpf = INV_MPU6050_FILTER_20HZ,
-	.divider = INV_MPU6050_FIFO_RATE_TO_DIVIDER(INV_MPU6050_INIT_FIFO_RATE),
+	.divider = INV_MPU6050_FIFO_RATE_TO_DIVIDER(50),
 	.gyro_fifo_enable = false,
 	.accl_fifo_enable = false,
 	.temp_fifo_enable = false,
@@ -370,20 +370,20 @@ static int inv_mpu6050_init_config(struct iio_dev *indio_dev)
 	u8 d;
 	struct inv_mpu6050_state *st = iio_priv(indio_dev);
 
-	result = inv_mpu6050_set_gyro_fsr(st, INV_MPU6050_FSR_2000DPS);
+	result = inv_mpu6050_set_gyro_fsr(st, st->chip_config.fsr);
 	if (result)
 		return result;
 
-	result = inv_mpu6050_set_lpf_regs(st, INV_MPU6050_FILTER_20HZ);
+	result = inv_mpu6050_set_lpf_regs(st, st->chip_config.lpf);
 	if (result)
 		return result;
 
-	d = INV_MPU6050_FIFO_RATE_TO_DIVIDER(INV_MPU6050_INIT_FIFO_RATE);
+	d = st->chip_config.divider;
 	result = regmap_write(st->map, st->reg->sample_rate_div, d);
 	if (result)
 		return result;
 
-	d = (INV_MPU6050_FS_02G << INV_MPU6050_ACCL_CONFIG_FSR_SHIFT);
+	d = (st->chip_config.accl_fs << INV_MPU6050_ACCL_CONFIG_FSR_SHIFT);
 	result = regmap_write(st->map, st->reg->accl_config, d);
 	if (result)
 		return result;
@@ -392,9 +392,6 @@ static int inv_mpu6050_init_config(struct iio_dev *indio_dev)
 	if (result)
 		return result;
 
-	memcpy(&st->chip_config, hw_info[st->chip_type].config,
-	       sizeof(struct inv_mpu6050_chip_config));
-
 	/*
 	 * Internal chip period is 1ms (1kHz).
 	 * Let's use at the beginning the theorical value before measuring
@@ -1116,6 +1113,8 @@ static int inv_check_and_setup_chip(struct inv_mpu6050_state *st)
 
 	st->hw  = &hw_info[st->chip_type];
 	st->reg = hw_info[st->chip_type].reg;
+	memcpy(&st->chip_config, hw_info[st->chip_type].config,
+	       sizeof(st->chip_config));
 
 	/* check chip self-identification */
 	result = regmap_read(st->map, INV_MPU6050_REG_WHOAMI, &regval);
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
index 9a81098a8b4d..d5edf903c076 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
@@ -321,7 +321,6 @@ struct inv_mpu6050_state {
 #define INV_MPU6050_TS_PERIOD_JITTER	4
 
 /* init parameters */
-#define INV_MPU6050_INIT_FIFO_RATE           50
 #define INV_MPU6050_MAX_FIFO_RATE            1000
 #define INV_MPU6050_MIN_FIFO_RATE            4
 
-- 
2.17.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 06/13] iio: imu: inv_mpu6050: add all signal path resets at init
  2020-02-12 17:40 [PATCH 00/13] Rework sensors engines and power management Jean-Baptiste Maneyrol
                   ` (4 preceding siblings ...)
  2020-02-12 17:40 ` [PATCH 05/13] iio: imu: inv_mpu6050: early init of chip_config for use at setup Jean-Baptiste Maneyrol
@ 2020-02-12 17:40 ` Jean-Baptiste Maneyrol
  2020-02-12 17:40 ` [PATCH 07/13] iio: imu: inv_mpu6050: fix sleep time when turning regulators on Jean-Baptiste Maneyrol
                   ` (6 subsequent siblings)
  12 siblings, 0 replies; 21+ messages in thread
From: Jean-Baptiste Maneyrol @ 2020-02-12 17:40 UTC (permalink / raw)
  To: jic23, linux-iio; +Cc: Jean-Baptiste Maneyrol

Old chips using spi require for a full reset to manually reset
all signal path. This does not harm when using i2c so do it
inconditionally. Exclude i2c only chips.

Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 18 ++++++++++++++++++
 drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h  |  6 ++++++
 2 files changed, 24 insertions(+)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index 85872e55154f..3502b996671c 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -1146,6 +1146,24 @@ static int inv_check_and_setup_chip(struct inv_mpu6050_state *st)
 	if (result)
 		return result;
 	msleep(INV_MPU6050_POWER_UP_TIME);
+	switch (st->chip_type) {
+	case INV_MPU6000:
+	case INV_MPU6500:
+	case INV_MPU6515:
+	case INV_MPU9250:
+	case INV_MPU9255:
+		/* reset signal path (required for spi connection) */
+		regval = INV_MPU6050_BIT_TEMP_RST | INV_MPU6050_BIT_ACCEL_RST |
+			 INV_MPU6050_BIT_GYRO_RST;
+		result = regmap_write(st->map, INV_MPU6050_REG_SIGNAL_PATH_RESET,
+				      regval);
+		if (result)
+			return result;
+		msleep(INV_MPU6050_POWER_UP_TIME);
+		break;
+	default:
+		break;
+	}
 
 	/*
 	 * Turn power on. After reset, the sleep bit could be on
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
index d5edf903c076..17f1f6a15f95 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
@@ -245,7 +245,13 @@ struct inv_mpu6050_state {
 #define INV_MPU6050_BIT_I2C_SLV3_DLY_EN     0x08
 #define INV_MPU6050_BIT_DELAY_ES_SHADOW     0x80
 
+#define INV_MPU6050_REG_SIGNAL_PATH_RESET   0x68
+#define INV_MPU6050_BIT_TEMP_RST            BIT(0)
+#define INV_MPU6050_BIT_ACCEL_RST           BIT(1)
+#define INV_MPU6050_BIT_GYRO_RST            BIT(2)
+
 #define INV_MPU6050_REG_USER_CTRL           0x6A
+#define INV_MPU6050_BIT_SIG_COND_RST        0x01
 #define INV_MPU6050_BIT_FIFO_RST            0x04
 #define INV_MPU6050_BIT_DMP_RST             0x08
 #define INV_MPU6050_BIT_I2C_MST_EN          0x20
-- 
2.17.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 07/13] iio: imu: inv_mpu6050: fix sleep time when turning regulators on
  2020-02-12 17:40 [PATCH 00/13] Rework sensors engines and power management Jean-Baptiste Maneyrol
                   ` (5 preceding siblings ...)
  2020-02-12 17:40 ` [PATCH 06/13] iio: imu: inv_mpu6050: add all signal path resets at init Jean-Baptiste Maneyrol
@ 2020-02-12 17:40 ` Jean-Baptiste Maneyrol
  2020-02-12 17:40 ` [PATCH 08/13] iio: imu: inv_mpu6050: rewrite power and engine management Jean-Baptiste Maneyrol
                   ` (5 subsequent siblings)
  12 siblings, 0 replies; 21+ messages in thread
From: Jean-Baptiste Maneyrol @ 2020-02-12 17:40 UTC (permalink / raw)
  To: jic23, linux-iio; +Cc: Jean-Baptiste Maneyrol

Turning vdd regulator on requires a consequent sleep for the
chip to power on correctly.
Turning vddio regulator is much faster.

Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index 3502b996671c..63cdde20df7e 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -1201,7 +1201,7 @@ static int inv_mpu_core_enable_regulator_vddio(struct inv_mpu6050_state *st)
 			"Failed to enable vddio regulator: %d\n", result);
 	} else {
 		/* Give the device a little bit of time to start up. */
-		usleep_range(35000, 70000);
+		usleep_range(3000, 5000);
 	}
 
 	return result;
@@ -1321,6 +1321,7 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
 		dev_err(dev, "Failed to enable vdd regulator: %d\n", result);
 		return result;
 	}
+	msleep(INV_MPU6050_POWER_UP_TIME);
 
 	result = inv_mpu_core_enable_regulator_vddio(st);
 	if (result) {
-- 
2.17.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 08/13] iio: imu: inv_mpu6050: rewrite power and engine management
  2020-02-12 17:40 [PATCH 00/13] Rework sensors engines and power management Jean-Baptiste Maneyrol
                   ` (6 preceding siblings ...)
  2020-02-12 17:40 ` [PATCH 07/13] iio: imu: inv_mpu6050: fix sleep time when turning regulators on Jean-Baptiste Maneyrol
@ 2020-02-12 17:40 ` Jean-Baptiste Maneyrol
  2020-02-15 18:09   ` Jonathan Cameron
  2020-02-12 17:40 ` [PATCH 09/13] iio: imu: inv_mpu6050: fix data polling interface Jean-Baptiste Maneyrol
                   ` (4 subsequent siblings)
  12 siblings, 1 reply; 21+ messages in thread
From: Jean-Baptiste Maneyrol @ 2020-02-12 17:40 UTC (permalink / raw)
  To: jic23, linux-iio; +Cc: Jean-Baptiste Maneyrol

Rewrite clock management to use automatic clock switching
present since MPU6500.
Sensors engine management can now turn on or off a batch of
sensors which simplifies usage a lot.
Temperature sensor is now turned on/off depending on usage.

Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c    | 264 +++++++++++++-----
 drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h     |  24 +-
 drivers/iio/imu/inv_mpu6050/inv_mpu_magn.c    |  12 +-
 drivers/iio/imu/inv_mpu6050/inv_mpu_magn.h    |   2 +-
 drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c |  91 +++---
 5 files changed, 262 insertions(+), 131 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index 63cdde20df7e..a51efc4c941b 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -99,9 +99,31 @@ static const struct inv_mpu6050_reg_map reg_set_6050 = {
 };
 
 static const struct inv_mpu6050_chip_config chip_config_6050 = {
+	.clk = INV_CLK_INTERNAL,
 	.fsr = INV_MPU6050_FSR_2000DPS,
 	.lpf = INV_MPU6050_FILTER_20HZ,
 	.divider = INV_MPU6050_FIFO_RATE_TO_DIVIDER(50),
+	.gyro_en = true,
+	.accl_en = true,
+	.temp_en = true,
+	.magn_en = false,
+	.gyro_fifo_enable = false,
+	.accl_fifo_enable = false,
+	.temp_fifo_enable = false,
+	.magn_fifo_enable = false,
+	.accl_fs = INV_MPU6050_FS_02G,
+	.user_ctrl = 0,
+};
+
+static const struct inv_mpu6050_chip_config chip_config_6500 = {
+	.clk = INV_CLK_PLL,
+	.fsr = INV_MPU6050_FSR_2000DPS,
+	.lpf = INV_MPU6050_FILTER_20HZ,
+	.divider = INV_MPU6050_FIFO_RATE_TO_DIVIDER(50),
+	.gyro_en = true,
+	.accl_en = true,
+	.temp_en = true,
+	.magn_en = false,
 	.gyro_fifo_enable = false,
 	.accl_fifo_enable = false,
 	.temp_fifo_enable = false,
@@ -124,7 +146,7 @@ static const struct inv_mpu6050_hw hw_info[] = {
 		.whoami = INV_MPU6500_WHOAMI_VALUE,
 		.name = "MPU6500",
 		.reg = &reg_set_6500,
-		.config = &chip_config_6050,
+		.config = &chip_config_6500,
 		.fifo_size = 512,
 		.temp = {INV_MPU6500_TEMP_OFFSET, INV_MPU6500_TEMP_SCALE},
 	},
@@ -132,7 +154,7 @@ static const struct inv_mpu6050_hw hw_info[] = {
 		.whoami = INV_MPU6515_WHOAMI_VALUE,
 		.name = "MPU6515",
 		.reg = &reg_set_6500,
-		.config = &chip_config_6050,
+		.config = &chip_config_6500,
 		.fifo_size = 512,
 		.temp = {INV_MPU6500_TEMP_OFFSET, INV_MPU6500_TEMP_SCALE},
 	},
@@ -156,7 +178,7 @@ static const struct inv_mpu6050_hw hw_info[] = {
 		.whoami = INV_MPU9250_WHOAMI_VALUE,
 		.name = "MPU9250",
 		.reg = &reg_set_6500,
-		.config = &chip_config_6050,
+		.config = &chip_config_6500,
 		.fifo_size = 512,
 		.temp = {INV_MPU6500_TEMP_OFFSET, INV_MPU6500_TEMP_SCALE},
 	},
@@ -164,7 +186,7 @@ static const struct inv_mpu6050_hw hw_info[] = {
 		.whoami = INV_MPU9255_WHOAMI_VALUE,
 		.name = "MPU9255",
 		.reg = &reg_set_6500,
-		.config = &chip_config_6050,
+		.config = &chip_config_6500,
 		.fifo_size = 512,
 		.temp = {INV_MPU6500_TEMP_OFFSET, INV_MPU6500_TEMP_SCALE},
 	},
@@ -172,7 +194,7 @@ static const struct inv_mpu6050_hw hw_info[] = {
 		.whoami = INV_ICM20608_WHOAMI_VALUE,
 		.name = "ICM20608",
 		.reg = &reg_set_6500,
-		.config = &chip_config_6050,
+		.config = &chip_config_6500,
 		.fifo_size = 512,
 		.temp = {INV_ICM20608_TEMP_OFFSET, INV_ICM20608_TEMP_SCALE},
 	},
@@ -180,7 +202,7 @@ static const struct inv_mpu6050_hw hw_info[] = {
 		.whoami = INV_ICM20609_WHOAMI_VALUE,
 		.name = "ICM20609",
 		.reg = &reg_set_6500,
-		.config = &chip_config_6050,
+		.config = &chip_config_6500,
 		.fifo_size = 4 * 1024,
 		.temp = {INV_ICM20608_TEMP_OFFSET, INV_ICM20608_TEMP_SCALE},
 	},
@@ -188,7 +210,7 @@ static const struct inv_mpu6050_hw hw_info[] = {
 		.whoami = INV_ICM20689_WHOAMI_VALUE,
 		.name = "ICM20689",
 		.reg = &reg_set_6500,
-		.config = &chip_config_6050,
+		.config = &chip_config_6500,
 		.fifo_size = 4 * 1024,
 		.temp = {INV_ICM20608_TEMP_OFFSET, INV_ICM20608_TEMP_SCALE},
 	},
@@ -196,15 +218,15 @@ static const struct inv_mpu6050_hw hw_info[] = {
 		.whoami = INV_ICM20602_WHOAMI_VALUE,
 		.name = "ICM20602",
 		.reg = &reg_set_icm20602,
-		.config = &chip_config_6050,
+		.config = &chip_config_6500,
 		.fifo_size = 1008,
 		.temp = {INV_ICM20608_TEMP_OFFSET, INV_ICM20608_TEMP_SCALE},
 	},
 	{
 		.whoami = INV_ICM20690_WHOAMI_VALUE,
 		.name = "ICM20690",
-		.reg = &reg_set_icm20602,
-		.config = &chip_config_6050,
+		.reg = &reg_set_6500,
+		.config = &chip_config_6500,
 		.fifo_size = 1024,
 		.temp = {INV_ICM20608_TEMP_OFFSET, INV_ICM20608_TEMP_SCALE},
 	},
@@ -212,61 +234,162 @@ static const struct inv_mpu6050_hw hw_info[] = {
 		.whoami = INV_IAM20680_WHOAMI_VALUE,
 		.name = "IAM20680",
 		.reg = &reg_set_6500,
-		.config = &chip_config_6050,
+		.config = &chip_config_6500,
 		.fifo_size = 512,
 		.temp = {INV_ICM20608_TEMP_OFFSET, INV_ICM20608_TEMP_SCALE},
 	},
 };
 
-int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en, u32 mask)
+static int inv_mpu6050_pwr_mgmt_1_write(struct inv_mpu6050_state *st, bool sleep,
+					int clock, int temp_dis)
 {
-	unsigned int d, mgmt_1;
-	int result;
-	/*
-	 * switch clock needs to be careful. Only when gyro is on, can
-	 * clock source be switched to gyro. Otherwise, it must be set to
-	 * internal clock
-	 */
-	if (mask == INV_MPU6050_BIT_PWR_GYRO_STBY) {
-		result = regmap_read(st->map, st->reg->pwr_mgmt_1, &mgmt_1);
-		if (result)
-			return result;
+	u8 val;
+
+	if (clock < 0)
+		clock = st->chip_config.clk;
+	if (temp_dis < 0)
+		temp_dis = !st->chip_config.temp_en;
+
+	val = clock & INV_MPU6050_BIT_CLK_MASK;
+	if (temp_dis)
+		val |= INV_MPU6050_BIT_TEMP_DIS;
+	if (sleep)
+		val |= INV_MPU6050_BIT_SLEEP;
+
+	dev_dbg(regmap_get_device(st->map), "pwr_mgmt_1: 0x%x\n", val);
+	return regmap_write(st->map, st->reg->pwr_mgmt_1, val);
+}
+
+static int inv_mpu6050_clock_switch(struct inv_mpu6050_state *st,
+				    unsigned int clock)
+{
+	int ret;
+
+	switch (st->chip_type) {
+	case INV_MPU6050:
+	case INV_MPU6000:
+	case INV_MPU9150:
+		/* old chips: switch clock manually */
+		ret = inv_mpu6050_pwr_mgmt_1_write(st, false, clock, -1);
+		if (ret)
+			return ret;
+		st->chip_config.clk = clock;
+		break;
+	default:
+		/* automatic clock switching, nothing to do */
+		break;
+	}
+
+	return 0;
+}
 
-		mgmt_1 &= ~INV_MPU6050_BIT_CLK_MASK;
+int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en,
+			      unsigned int mask)
+{
+	unsigned int sleep;
+	u8 pwr_mgmt2, user_ctrl;
+	int ret;
+
+	/* delete useless requests */
+	if (mask & INV_MPU6050_SENSOR_ACCL && en == st->chip_config.accl_en)
+		mask &= ~INV_MPU6050_SENSOR_ACCL;
+	if (mask & INV_MPU6050_SENSOR_GYRO && en == st->chip_config.gyro_en)
+		mask &= ~INV_MPU6050_SENSOR_GYRO;
+	if (mask & INV_MPU6050_SENSOR_TEMP && en == st->chip_config.temp_en)
+		mask &= ~INV_MPU6050_SENSOR_TEMP;
+	if (mask & INV_MPU6050_SENSOR_MAGN && en == st->chip_config.magn_en)
+		mask &= ~INV_MPU6050_SENSOR_MAGN;
+	if (mask == 0)
+		return 0;
+
+	/* turn on/off temperature sensor */
+	if (mask & INV_MPU6050_SENSOR_TEMP) {
+		ret = inv_mpu6050_pwr_mgmt_1_write(st, false, -1, !en);
+		if (ret)
+			return ret;
+		st->chip_config.temp_en = en;
 	}
 
-	if ((mask == INV_MPU6050_BIT_PWR_GYRO_STBY) && (!en)) {
-		/*
-		 * turning off gyro requires switch to internal clock first.
-		 * Then turn off gyro engine
-		 */
-		mgmt_1 |= INV_CLK_INTERNAL;
-		result = regmap_write(st->map, st->reg->pwr_mgmt_1, mgmt_1);
-		if (result)
-			return result;
+	/* update user_crtl for driving magnetometer */
+	if (mask & INV_MPU6050_SENSOR_MAGN) {
+		user_ctrl = st->chip_config.user_ctrl;
+		if (en)
+			user_ctrl |= INV_MPU6050_BIT_I2C_MST_EN;
+		else
+			user_ctrl &= ~INV_MPU6050_BIT_I2C_MST_EN;
+		ret = regmap_write(st->map, st->reg->user_ctrl, user_ctrl);
+		if (ret)
+			return ret;
+		st->chip_config.user_ctrl = user_ctrl;
+		st->chip_config.magn_en = en;
 	}
 
-	result = regmap_read(st->map, st->reg->pwr_mgmt_2, &d);
-	if (result)
-		return result;
-	if (en)
-		d &= ~mask;
-	else
-		d |= mask;
-	result = regmap_write(st->map, st->reg->pwr_mgmt_2, d);
-	if (result)
-		return result;
+	/* manage accel & gyro engines */
+	if (mask & (INV_MPU6050_SENSOR_ACCL | INV_MPU6050_SENSOR_GYRO)) {
+		/* compute power management 2 current value */
+		pwr_mgmt2 = 0;
+		if (!st->chip_config.accl_en)
+			pwr_mgmt2 |= INV_MPU6050_BIT_PWR_ACCL_STBY;
+		if (!st->chip_config.gyro_en)
+			pwr_mgmt2 |= INV_MPU6050_BIT_PWR_GYRO_STBY;
+
+		/* update to new requested value */
+		if (mask & INV_MPU6050_SENSOR_ACCL) {
+			if (en)
+				pwr_mgmt2 &= ~INV_MPU6050_BIT_PWR_ACCL_STBY;
+			else
+				pwr_mgmt2 |= INV_MPU6050_BIT_PWR_ACCL_STBY;
+		}
+		if (mask & INV_MPU6050_SENSOR_GYRO) {
+			if (en)
+				pwr_mgmt2 &= ~INV_MPU6050_BIT_PWR_GYRO_STBY;
+			else
+				pwr_mgmt2 |= INV_MPU6050_BIT_PWR_GYRO_STBY;
+		}
 
-	if (en) {
-		/* Wait for output to stabilize */
-		msleep(INV_MPU6050_TEMP_UP_TIME);
-		if (mask == INV_MPU6050_BIT_PWR_GYRO_STBY) {
-			/* switch internal clock to PLL */
-			mgmt_1 |= INV_CLK_PLL;
-			result = regmap_write(st->map,
-					      st->reg->pwr_mgmt_1, mgmt_1);
-			if (result)
-				return result;
+		/* switch clock to internal when turning gyro off */
+		if (mask & INV_MPU6050_SENSOR_GYRO && !en) {
+			ret = inv_mpu6050_clock_switch(st, INV_CLK_INTERNAL);
+			if (ret)
+				return ret;
+		}
+
+		/* update sensors engine */
+		dev_dbg(regmap_get_device(st->map), "pwr_mgmt_2: 0x%x\n",
+			pwr_mgmt2);
+		ret = regmap_write(st->map, st->reg->pwr_mgmt_2, pwr_mgmt2);
+		if (ret)
+			return ret;
+		if (mask & INV_MPU6050_SENSOR_ACCL)
+			st->chip_config.accl_en = en;
+		if (mask & INV_MPU6050_SENSOR_GYRO)
+			st->chip_config.gyro_en = en;
+
+		/* compute required time to have sensors stabilized */
+		sleep = 0;
+		if (en) {
+			if (mask & INV_MPU6050_SENSOR_ACCL) {
+				if (sleep < INV_MPU6050_ACCEL_UP_TIME)
+					sleep = INV_MPU6050_ACCEL_UP_TIME;
+			}
+			if (mask & INV_MPU6050_SENSOR_GYRO) {
+				if (sleep < INV_MPU6050_GYRO_UP_TIME)
+					sleep = INV_MPU6050_GYRO_UP_TIME;
+			}
+		} else {
+			if (mask & INV_MPU6050_SENSOR_GYRO) {
+				if (sleep < INV_MPU6050_GYRO_DOWN_TIME)
+					sleep = INV_MPU6050_GYRO_DOWN_TIME;
+			}
+		}
+		if (sleep)
+			msleep(sleep);
+
+		/* switch clock to PLL when turning gyro on */
+		if (mask & INV_MPU6050_SENSOR_GYRO && en) {
+			ret = inv_mpu6050_clock_switch(st, INV_CLK_PLL);
+			if (ret)
+				return ret;
 		}
 	}
 
@@ -279,7 +402,7 @@ int inv_mpu6050_set_power_itg(struct inv_mpu6050_state *st, bool power_on)
 
 	if (power_on) {
 		if (!st->powerup_count) {
-			result = regmap_write(st->map, st->reg->pwr_mgmt_1, 0);
+			result = inv_mpu6050_pwr_mgmt_1_write(st, false, -1, -1);
 			if (result)
 				return result;
 			usleep_range(INV_MPU6050_REG_UP_TIME_MIN,
@@ -288,8 +411,7 @@ int inv_mpu6050_set_power_itg(struct inv_mpu6050_state *st, bool power_on)
 		st->powerup_count++;
 	} else {
 		if (st->powerup_count == 1) {
-			result = regmap_write(st->map, st->reg->pwr_mgmt_1,
-					      INV_MPU6050_BIT_SLEEP);
+			result = inv_mpu6050_pwr_mgmt_1_write(st, true, -1, -1);
 			if (result)
 				return result;
 		}
@@ -451,33 +573,41 @@ static int inv_mpu6050_read_channel_data(struct iio_dev *indio_dev,
 	switch (chan->type) {
 	case IIO_ANGL_VEL:
 		result = inv_mpu6050_switch_engine(st, true,
-				INV_MPU6050_BIT_PWR_GYRO_STBY);
+				INV_MPU6050_SENSOR_GYRO);
 		if (result)
 			goto error_power_off;
 		ret = inv_mpu6050_sensor_show(st, st->reg->raw_gyro,
 					      chan->channel2, val);
 		result = inv_mpu6050_switch_engine(st, false,
-				INV_MPU6050_BIT_PWR_GYRO_STBY);
+				INV_MPU6050_SENSOR_GYRO);
 		if (result)
 			goto error_power_off;
 		break;
 	case IIO_ACCEL:
 		result = inv_mpu6050_switch_engine(st, true,
-				INV_MPU6050_BIT_PWR_ACCL_STBY);
+				INV_MPU6050_SENSOR_ACCL);
 		if (result)
 			goto error_power_off;
 		ret = inv_mpu6050_sensor_show(st, st->reg->raw_accl,
 					      chan->channel2, val);
 		result = inv_mpu6050_switch_engine(st, false,
-				INV_MPU6050_BIT_PWR_ACCL_STBY);
+				INV_MPU6050_SENSOR_ACCL);
 		if (result)
 			goto error_power_off;
 		break;
 	case IIO_TEMP:
+		result = inv_mpu6050_switch_engine(st, true,
+				INV_MPU6050_SENSOR_TEMP);
+		if (result)
+			goto error_power_off;
 		/* wait for stablization */
-		msleep(INV_MPU6050_SENSOR_UP_TIME);
+		msleep(INV_MPU6050_TEMP_UP_TIME);
 		ret = inv_mpu6050_sensor_show(st, st->reg->temperature,
 					      IIO_MOD_X, val);
+		result = inv_mpu6050_switch_engine(st, false,
+				INV_MPU6050_SENSOR_TEMP);
+		if (result)
+			goto error_power_off;
 		break;
 	case IIO_MAGN:
 		ret = inv_mpu_magn_read(st, chan->channel2, val);
@@ -1108,7 +1238,7 @@ static const struct iio_info mpu_info = {
 static int inv_check_and_setup_chip(struct inv_mpu6050_state *st)
 {
 	int result;
-	unsigned int regval;
+	unsigned int regval, mask;
 	int i;
 
 	st->hw  = &hw_info[st->chip_type];
@@ -1174,13 +1304,9 @@ static int inv_check_and_setup_chip(struct inv_mpu6050_state *st)
 	result = inv_mpu6050_set_power_itg(st, true);
 	if (result)
 		return result;
-
-	result = inv_mpu6050_switch_engine(st, false,
-					   INV_MPU6050_BIT_PWR_ACCL_STBY);
-	if (result)
-		goto error_power_off;
-	result = inv_mpu6050_switch_engine(st, false,
-					   INV_MPU6050_BIT_PWR_GYRO_STBY);
+	mask = INV_MPU6050_SENSOR_ACCL | INV_MPU6050_SENSOR_GYRO |
+			INV_MPU6050_SENSOR_TEMP | INV_MPU6050_SENSOR_MAGN;
+	result = inv_mpu6050_switch_engine(st, false, mask);
 	if (result)
 		goto error_power_off;
 
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
index 17f1f6a15f95..a578789c9210 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
@@ -83,11 +83,22 @@ enum inv_devices {
 	INV_NUM_PARTS
 };
 
+/* chip sensors mask: accelerometer, gyroscope, temperature, magnetometer */
+#define INV_MPU6050_SENSOR_ACCL		BIT(0)
+#define INV_MPU6050_SENSOR_GYRO		BIT(1)
+#define INV_MPU6050_SENSOR_TEMP		BIT(2)
+#define INV_MPU6050_SENSOR_MAGN		BIT(3)
+
 /**
  *  struct inv_mpu6050_chip_config - Cached chip configuration data.
+ *  @clk:		selected chip clock
  *  @fsr:		Full scale range.
  *  @lpf:		Digital low pass filter frequency.
  *  @accl_fs:		accel full scale range.
+ *  @accl_en:		accel engine enabled
+ *  @gyro_en:		gyro engine enabled
+ *  @temp_en:		temperature sensor enabled
+ *  @magn_en:		magn engine (i2c master) enabled
  *  @accl_fifo_enable:	enable accel data output
  *  @gyro_fifo_enable:	enable gyro data output
  *  @temp_fifo_enable:	enable temp data output
@@ -95,9 +106,14 @@ enum inv_devices {
  *  @divider:		chip sample rate divider (sample rate divider - 1)
  */
 struct inv_mpu6050_chip_config {
+	unsigned int clk:3;
 	unsigned int fsr:2;
 	unsigned int lpf:3;
 	unsigned int accl_fs:2;
+	unsigned int accl_en:1;
+	unsigned int gyro_en:1;
+	unsigned int temp_en:1;
+	unsigned int magn_en:1;
 	unsigned int accl_fifo_enable:1;
 	unsigned int gyro_fifo_enable:1;
 	unsigned int temp_fifo_enable:1;
@@ -262,6 +278,7 @@ struct inv_mpu6050_state {
 #define INV_MPU6050_REG_PWR_MGMT_1          0x6B
 #define INV_MPU6050_BIT_H_RESET             0x80
 #define INV_MPU6050_BIT_SLEEP               0x40
+#define INV_MPU6050_BIT_TEMP_DIS            0x08
 #define INV_MPU6050_BIT_CLK_MASK            0x7
 
 #define INV_MPU6050_REG_PWR_MGMT_2          0x6C
@@ -292,7 +309,9 @@ struct inv_mpu6050_state {
 /* delay time in milliseconds */
 #define INV_MPU6050_POWER_UP_TIME            100
 #define INV_MPU6050_TEMP_UP_TIME             100
-#define INV_MPU6050_SENSOR_UP_TIME           30
+#define INV_MPU6050_ACCEL_UP_TIME            20
+#define INV_MPU6050_GYRO_UP_TIME             35
+#define INV_MPU6050_GYRO_DOWN_TIME           150
 
 /* delay time in microseconds */
 #define INV_MPU6050_REG_UP_TIME_MIN          5000
@@ -417,7 +436,8 @@ enum inv_mpu6050_clock_sel_e {
 irqreturn_t inv_mpu6050_read_fifo(int irq, void *p);
 int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev, int irq_type);
 int inv_reset_fifo(struct iio_dev *indio_dev);
-int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en, u32 mask);
+int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en,
+			      unsigned int mask);
 int inv_mpu6050_write_reg(struct inv_mpu6050_state *st, int reg, u8 val);
 int inv_mpu6050_set_power_itg(struct inv_mpu6050_state *st, bool power_on);
 int inv_mpu_acpi_create_mux_client(struct i2c_client *client);
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_magn.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_magn.c
index 607104a2631e..3f402fa56d95 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_magn.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_magn.c
@@ -316,9 +316,9 @@ int inv_mpu_magn_set_orient(struct inv_mpu6050_state *st)
  *
  * Returns 0 on success, a negative error code otherwise
  */
-int inv_mpu_magn_read(const struct inv_mpu6050_state *st, int axis, int *val)
+int inv_mpu_magn_read(struct inv_mpu6050_state *st, int axis, int *val)
 {
-	unsigned int user_ctrl, status;
+	unsigned int status;
 	__be16 data;
 	uint8_t addr;
 	unsigned int freq_hz, period_ms;
@@ -350,16 +350,14 @@ int inv_mpu_magn_read(const struct inv_mpu6050_state *st, int axis, int *val)
 		freq_hz = INV_MPU_MAGN_FREQ_HZ_MAX;
 	period_ms = 1000 / freq_hz;
 
-	/* start i2c master, wait for xfer, stop */
-	user_ctrl = st->chip_config.user_ctrl | INV_MPU6050_BIT_I2C_MST_EN;
-	ret = regmap_write(st->map, st->reg->user_ctrl, user_ctrl);
+	ret = inv_mpu6050_switch_engine(st, true, INV_MPU6050_SENSOR_MAGN);
 	if (ret)
 		return ret;
 
 	/* need to wait 2 periods + half-period margin */
 	msleep(period_ms * 2 + period_ms / 2);
-	user_ctrl = st->chip_config.user_ctrl;
-	ret = regmap_write(st->map, st->reg->user_ctrl, user_ctrl);
+
+	ret = inv_mpu6050_switch_engine(st, false, INV_MPU6050_SENSOR_MAGN);
 	if (ret)
 		return ret;
 
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_magn.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_magn.h
index b41bd0578478..f7ad50ca6c1b 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_magn.h
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_magn.h
@@ -31,6 +31,6 @@ int inv_mpu_magn_set_rate(const struct inv_mpu6050_state *st, int fifo_rate);
 
 int inv_mpu_magn_set_orient(struct inv_mpu6050_state *st);
 
-int inv_mpu_magn_read(const struct inv_mpu6050_state *st, int axis, int *val);
+int inv_mpu_magn_read(struct inv_mpu6050_state *st, int axis, int *val);
 
 #endif		/* INV_MPU_MAGN_H_ */
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
index 5199fe790c30..cfd7243159f6 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
@@ -5,9 +5,10 @@
 
 #include "inv_mpu_iio.h"
 
-static void inv_scan_query_mpu6050(struct iio_dev *indio_dev)
+static unsigned int inv_scan_query_mpu6050(struct iio_dev *indio_dev)
 {
 	struct inv_mpu6050_state  *st = iio_priv(indio_dev);
+	unsigned int mask;
 
 	st->chip_config.gyro_fifo_enable =
 		test_bit(INV_MPU6050_SCAN_GYRO_X,
@@ -27,17 +28,28 @@ static void inv_scan_query_mpu6050(struct iio_dev *indio_dev)
 
 	st->chip_config.temp_fifo_enable =
 		test_bit(INV_MPU6050_SCAN_TEMP, indio_dev->active_scan_mask);
+
+	mask = 0;
+	if (st->chip_config.gyro_fifo_enable)
+		mask |= INV_MPU6050_SENSOR_GYRO;
+	if (st->chip_config.accl_fifo_enable)
+		mask |= INV_MPU6050_SENSOR_ACCL;
+	if (st->chip_config.temp_fifo_enable)
+		mask |= INV_MPU6050_SENSOR_TEMP;
+
+	return mask;
 }
 
-static void inv_scan_query_mpu9x50(struct iio_dev *indio_dev)
+static unsigned int inv_scan_query_mpu9x50(struct iio_dev *indio_dev)
 {
 	struct inv_mpu6050_state *st = iio_priv(indio_dev);
+	unsigned int mask;
 
-	inv_scan_query_mpu6050(indio_dev);
+	mask = inv_scan_query_mpu6050(indio_dev);
 
 	/* no magnetometer if i2c auxiliary bus is used */
 	if (st->magn_disabled)
-		return;
+		return mask;
 
 	st->chip_config.magn_fifo_enable =
 		test_bit(INV_MPU9X50_SCAN_MAGN_X,
@@ -46,9 +58,13 @@ static void inv_scan_query_mpu9x50(struct iio_dev *indio_dev)
 			 indio_dev->active_scan_mask) ||
 		test_bit(INV_MPU9X50_SCAN_MAGN_Z,
 			 indio_dev->active_scan_mask);
+	if (st->chip_config.magn_fifo_enable)
+		mask |= INV_MPU6050_SENSOR_MAGN;
+
+	return mask;
 }
 
-static void inv_scan_query(struct iio_dev *indio_dev)
+static unsigned int inv_scan_query(struct iio_dev *indio_dev)
 {
 	struct inv_mpu6050_state *st = iio_priv(indio_dev);
 
@@ -92,62 +108,40 @@ static unsigned int inv_compute_skip_samples(const struct inv_mpu6050_state *st)
 static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
 {
 	struct inv_mpu6050_state *st = iio_priv(indio_dev);
-	uint8_t d;
+	unsigned int scan;
 	int result;
 
+	scan = inv_scan_query(indio_dev);
+
 	if (enable) {
 		result = inv_mpu6050_set_power_itg(st, true);
 		if (result)
 			return result;
-		inv_scan_query(indio_dev);
-		if (st->chip_config.gyro_fifo_enable) {
-			result = inv_mpu6050_switch_engine(st, true,
-					INV_MPU6050_BIT_PWR_GYRO_STBY);
-			if (result)
-				goto error_power_off;
-		}
-		if (st->chip_config.accl_fifo_enable) {
-			result = inv_mpu6050_switch_engine(st, true,
-					INV_MPU6050_BIT_PWR_ACCL_STBY);
-			if (result)
-				goto error_gyro_off;
-		}
-		if (st->chip_config.magn_fifo_enable) {
-			d = st->chip_config.user_ctrl |
-					INV_MPU6050_BIT_I2C_MST_EN;
-			result = regmap_write(st->map, st->reg->user_ctrl, d);
-			if (result)
-				goto error_accl_off;
-			st->chip_config.user_ctrl = d;
-		}
+		result = inv_mpu6050_switch_engine(st, true, scan);
+		if (result)
+			goto error_power_off;
 		st->skip_samples = inv_compute_skip_samples(st);
 		result = inv_reset_fifo(indio_dev);
 		if (result)
-			goto error_magn_off;
+			goto error_sensors_off;
 	} else {
 		result = regmap_write(st->map, st->reg->fifo_en, 0);
 		if (result)
-			goto error_magn_off;
+			goto error_fifo_off;
 
 		result = regmap_write(st->map, st->reg->int_enable, 0);
 		if (result)
-			goto error_magn_off;
-
-		d = st->chip_config.user_ctrl & ~INV_MPU6050_BIT_I2C_MST_EN;
-		result = regmap_write(st->map, st->reg->user_ctrl, d);
-		if (result)
-			goto error_magn_off;
-		st->chip_config.user_ctrl = d;
+			goto error_fifo_off;
 
-		result = inv_mpu6050_switch_engine(st, false,
-					INV_MPU6050_BIT_PWR_ACCL_STBY);
+		/* restore user_ctrl for disabling FIFO reading */
+		result = regmap_write(st->map, st->reg->user_ctrl,
+				      st->chip_config.user_ctrl);
 		if (result)
-			goto error_accl_off;
+			goto error_sensors_off;
 
-		result = inv_mpu6050_switch_engine(st, false,
-					INV_MPU6050_BIT_PWR_GYRO_STBY);
+		result = inv_mpu6050_switch_engine(st, false, scan);
 		if (result)
-			goto error_gyro_off;
+			goto error_power_off;
 
 		result = inv_mpu6050_set_power_itg(st, false);
 		if (result)
@@ -156,18 +150,11 @@ static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
 
 	return 0;
 
-error_magn_off:
+error_fifo_off:
 	/* always restore user_ctrl to disable fifo properly */
-	st->chip_config.user_ctrl &= ~INV_MPU6050_BIT_I2C_MST_EN;
 	regmap_write(st->map, st->reg->user_ctrl, st->chip_config.user_ctrl);
-error_accl_off:
-	if (st->chip_config.accl_fifo_enable)
-		inv_mpu6050_switch_engine(st, false,
-					  INV_MPU6050_BIT_PWR_ACCL_STBY);
-error_gyro_off:
-	if (st->chip_config.gyro_fifo_enable)
-		inv_mpu6050_switch_engine(st, false,
-					  INV_MPU6050_BIT_PWR_GYRO_STBY);
+error_sensors_off:
+	inv_mpu6050_switch_engine(st, false, scan);
 error_power_off:
 	inv_mpu6050_set_power_itg(st, false);
 	return result;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 09/13] iio: imu: inv_mpu6050: fix data polling interface
  2020-02-12 17:40 [PATCH 00/13] Rework sensors engines and power management Jean-Baptiste Maneyrol
                   ` (7 preceding siblings ...)
  2020-02-12 17:40 ` [PATCH 08/13] iio: imu: inv_mpu6050: rewrite power and engine management Jean-Baptiste Maneyrol
@ 2020-02-12 17:40 ` Jean-Baptiste Maneyrol
  2020-02-12 17:40 ` [PATCH 10/13] iio: imu: inv_mpu6050: factorize fifo enable/disable Jean-Baptiste Maneyrol
                   ` (3 subsequent siblings)
  12 siblings, 0 replies; 21+ messages in thread
From: Jean-Baptiste Maneyrol @ 2020-02-12 17:40 UTC (permalink / raw)
  To: jic23, linux-iio; +Cc: Jean-Baptiste Maneyrol

When reading data with the polling interface, we need to wait
at 1 sampling period to have a sample.
For gyroscope and magnetometer, we need to wait for 2 periods
before having a correct sample.

Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 36 ++++++++++++++++++++--
 drivers/iio/imu/inv_mpu6050/inv_mpu_magn.c | 21 -------------
 drivers/iio/imu/inv_mpu6050/inv_mpu_magn.h |  3 ++
 3 files changed, 37 insertions(+), 23 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index a51efc4c941b..28f8079d4599 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -563,9 +563,14 @@ static int inv_mpu6050_read_channel_data(struct iio_dev *indio_dev,
 					 int *val)
 {
 	struct inv_mpu6050_state *st = iio_priv(indio_dev);
+	unsigned int freq_hz, period_us, min_sleep_us, max_sleep_us;
 	int result;
 	int ret;
 
+	/* compute sample period */
+	freq_hz = INV_MPU6050_DIVIDER_TO_FIFO_RATE(st->chip_config.divider);
+	period_us = 1000000 / freq_hz;
+
 	result = inv_mpu6050_set_power_itg(st, true);
 	if (result)
 		return result;
@@ -576,6 +581,10 @@ static int inv_mpu6050_read_channel_data(struct iio_dev *indio_dev,
 				INV_MPU6050_SENSOR_GYRO);
 		if (result)
 			goto error_power_off;
+		/* need to wait 2 periods to have first valid sample */
+		min_sleep_us = 2 * period_us;
+		max_sleep_us = 2 * (period_us + period_us / 2);
+		usleep_range(min_sleep_us, max_sleep_us);
 		ret = inv_mpu6050_sensor_show(st, st->reg->raw_gyro,
 					      chan->channel2, val);
 		result = inv_mpu6050_switch_engine(st, false,
@@ -588,6 +597,10 @@ static int inv_mpu6050_read_channel_data(struct iio_dev *indio_dev,
 				INV_MPU6050_SENSOR_ACCL);
 		if (result)
 			goto error_power_off;
+		/* wait 1 period for first sample availability */
+		min_sleep_us = period_us;
+		max_sleep_us = period_us + period_us / 2;
+		usleep_range(min_sleep_us, max_sleep_us);
 		ret = inv_mpu6050_sensor_show(st, st->reg->raw_accl,
 					      chan->channel2, val);
 		result = inv_mpu6050_switch_engine(st, false,
@@ -600,8 +613,10 @@ static int inv_mpu6050_read_channel_data(struct iio_dev *indio_dev,
 				INV_MPU6050_SENSOR_TEMP);
 		if (result)
 			goto error_power_off;
-		/* wait for stablization */
-		msleep(INV_MPU6050_TEMP_UP_TIME);
+		/* wait 1 period for first sample availability */
+		min_sleep_us = period_us;
+		max_sleep_us = period_us + period_us / 2;
+		usleep_range(min_sleep_us, max_sleep_us);
 		ret = inv_mpu6050_sensor_show(st, st->reg->temperature,
 					      IIO_MOD_X, val);
 		result = inv_mpu6050_switch_engine(st, false,
@@ -610,7 +625,24 @@ static int inv_mpu6050_read_channel_data(struct iio_dev *indio_dev,
 			goto error_power_off;
 		break;
 	case IIO_MAGN:
+		result = inv_mpu6050_switch_engine(st, true,
+				INV_MPU6050_SENSOR_MAGN);
+		if (result)
+			goto error_power_off;
+		/* frequency is limited for magnetometer */
+		if (freq_hz > INV_MPU_MAGN_FREQ_HZ_MAX) {
+			freq_hz = INV_MPU_MAGN_FREQ_HZ_MAX;
+			period_us = 1000000 / freq_hz;
+		}
+		/* need to wait 2 periods to have first valid sample */
+		min_sleep_us = 2 * period_us;
+		max_sleep_us = 2 * (period_us + period_us / 2);
+		usleep_range(min_sleep_us, max_sleep_us);
 		ret = inv_mpu_magn_read(st, chan->channel2, val);
+		result = inv_mpu6050_switch_engine(st, false,
+				INV_MPU6050_SENSOR_MAGN);
+		if (result)
+			goto error_power_off;
 		break;
 	default:
 		ret = -EINVAL;
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_magn.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_magn.c
index 3f402fa56d95..f282e9cc34c5 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_magn.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_magn.c
@@ -44,9 +44,6 @@
 #define INV_MPU_MAGN_REG_ASAY		0x11
 #define INV_MPU_MAGN_REG_ASAZ		0x12
 
-/* Magnetometer maximum frequency */
-#define INV_MPU_MAGN_FREQ_HZ_MAX	50
-
 static bool inv_magn_supported(const struct inv_mpu6050_state *st)
 {
 	switch (st->chip_type) {
@@ -321,7 +318,6 @@ int inv_mpu_magn_read(struct inv_mpu6050_state *st, int axis, int *val)
 	unsigned int status;
 	__be16 data;
 	uint8_t addr;
-	unsigned int freq_hz, period_ms;
 	int ret;
 
 	/* quit if chip is not supported */
@@ -344,23 +340,6 @@ int inv_mpu_magn_read(struct inv_mpu6050_state *st, int axis, int *val)
 	}
 	addr += INV_MPU6050_REG_EXT_SENS_DATA;
 
-	/* compute period depending on current sampling rate */
-	freq_hz = INV_MPU6050_DIVIDER_TO_FIFO_RATE(st->chip_config.divider);
-	if (freq_hz > INV_MPU_MAGN_FREQ_HZ_MAX)
-		freq_hz = INV_MPU_MAGN_FREQ_HZ_MAX;
-	period_ms = 1000 / freq_hz;
-
-	ret = inv_mpu6050_switch_engine(st, true, INV_MPU6050_SENSOR_MAGN);
-	if (ret)
-		return ret;
-
-	/* need to wait 2 periods + half-period margin */
-	msleep(period_ms * 2 + period_ms / 2);
-
-	ret = inv_mpu6050_switch_engine(st, false, INV_MPU6050_SENSOR_MAGN);
-	if (ret)
-		return ret;
-
 	/* check i2c status and read raw data */
 	ret = regmap_read(st->map, INV_MPU6050_REG_I2C_MST_STATUS, &status);
 	if (ret)
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_magn.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_magn.h
index f7ad50ca6c1b..185c000c697c 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_magn.h
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_magn.h
@@ -10,6 +10,9 @@
 
 #include "inv_mpu_iio.h"
 
+/* Magnetometer maximum frequency */
+#define INV_MPU_MAGN_FREQ_HZ_MAX	50
+
 int inv_mpu_magn_probe(struct inv_mpu6050_state *st);
 
 /**
-- 
2.17.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 10/13] iio: imu: inv_mpu6050: factorize fifo enable/disable
  2020-02-12 17:40 [PATCH 00/13] Rework sensors engines and power management Jean-Baptiste Maneyrol
                   ` (8 preceding siblings ...)
  2020-02-12 17:40 ` [PATCH 09/13] iio: imu: inv_mpu6050: fix data polling interface Jean-Baptiste Maneyrol
@ 2020-02-12 17:40 ` Jean-Baptiste Maneyrol
  2020-02-12 17:40 ` [PATCH 11/13] iio: imu: inv_mpu6050: dynamic sampling rate change Jean-Baptiste Maneyrol
                   ` (2 subsequent siblings)
  12 siblings, 0 replies; 21+ messages in thread
From: Jean-Baptiste Maneyrol @ 2020-02-12 17:40 UTC (permalink / raw)
  To: jic23, linux-iio; +Cc: Jean-Baptiste Maneyrol

Rework fifo enable/disable in a separate function.

Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h     |  2 +-
 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c    | 54 ++-------------
 drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c | 66 ++++++++++++++-----
 3 files changed, 55 insertions(+), 67 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
index a578789c9210..e328c98e362c 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
@@ -435,7 +435,7 @@ enum inv_mpu6050_clock_sel_e {
 
 irqreturn_t inv_mpu6050_read_fifo(int irq, void *p);
 int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev, int irq_type);
-int inv_reset_fifo(struct iio_dev *indio_dev);
+int inv_mpu6050_prepare_fifo(struct inv_mpu6050_state *st, bool enable);
 int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en,
 			      unsigned int mask);
 int inv_mpu6050_write_reg(struct inv_mpu6050_state *st, int reg, u8 val);
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
index d7397705974e..9511e4715e2c 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
@@ -90,60 +90,14 @@ static s64 inv_mpu6050_get_timestamp(struct inv_mpu6050_state *st)
 	return ts;
 }
 
-int inv_reset_fifo(struct iio_dev *indio_dev)
+static int inv_reset_fifo(struct iio_dev *indio_dev)
 {
 	int result;
-	u8 d;
 	struct inv_mpu6050_state  *st = iio_priv(indio_dev);
 
-	/* reset it timestamp validation */
-	st->it_timestamp = 0;
-
-	/* disable interrupt */
-	result = regmap_write(st->map, st->reg->int_enable, 0);
-	if (result) {
-		dev_err(regmap_get_device(st->map), "int_enable failed %d\n",
-			result);
-		return result;
-	}
-	/* disable the sensor output to FIFO */
-	result = regmap_write(st->map, st->reg->fifo_en, 0);
-	if (result)
-		goto reset_fifo_fail;
-	/* disable fifo reading */
-	result = regmap_write(st->map, st->reg->user_ctrl,
-			      st->chip_config.user_ctrl);
-	if (result)
-		goto reset_fifo_fail;
-
-	/* reset FIFO*/
-	d = st->chip_config.user_ctrl | INV_MPU6050_BIT_FIFO_RST;
-	result = regmap_write(st->map, st->reg->user_ctrl, d);
-	if (result)
-		goto reset_fifo_fail;
-
-	/* enable interrupt */
-	result = regmap_write(st->map, st->reg->int_enable,
-			      INV_MPU6050_BIT_DATA_RDY_EN);
-	if (result)
-		return result;
-
-	/* enable FIFO reading */
-	d = st->chip_config.user_ctrl | INV_MPU6050_BIT_FIFO_EN;
-	result = regmap_write(st->map, st->reg->user_ctrl, d);
-	if (result)
-		goto reset_fifo_fail;
-	/* enable sensor output to FIFO */
-	d = 0;
-	if (st->chip_config.gyro_fifo_enable)
-		d |= INV_MPU6050_BITS_GYRO_OUT;
-	if (st->chip_config.accl_fifo_enable)
-		d |= INV_MPU6050_BIT_ACCEL_OUT;
-	if (st->chip_config.temp_fifo_enable)
-		d |= INV_MPU6050_BIT_TEMP_OUT;
-	if (st->chip_config.magn_fifo_enable)
-		d |= INV_MPU6050_BIT_SLAVE_0;
-	result = regmap_write(st->map, st->reg->fifo_en, d);
+	/* disable fifo and reenable it */
+	inv_mpu6050_prepare_fifo(st, false);
+	result = inv_mpu6050_prepare_fifo(st, true);
 	if (result)
 		goto reset_fifo_fail;
 
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
index cfd7243159f6..f53f50d08b9e 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
@@ -100,6 +100,54 @@ static unsigned int inv_compute_skip_samples(const struct inv_mpu6050_state *st)
 	return skip_samples;
 }
 
+int inv_mpu6050_prepare_fifo(struct inv_mpu6050_state *st, bool enable)
+{
+	uint8_t d;
+	int ret;
+
+	if (enable) {
+		st->it_timestamp = 0;
+		/* reset FIFO */
+		d = st->chip_config.user_ctrl | INV_MPU6050_BIT_FIFO_RST;
+		ret = regmap_write(st->map, st->reg->user_ctrl, d);
+		if (ret)
+			return ret;
+		/* enable sensor output to FIFO */
+		d = 0;
+		if (st->chip_config.gyro_fifo_enable)
+			d |= INV_MPU6050_BITS_GYRO_OUT;
+		if (st->chip_config.accl_fifo_enable)
+			d |= INV_MPU6050_BIT_ACCEL_OUT;
+		if (st->chip_config.temp_fifo_enable)
+			d |= INV_MPU6050_BIT_TEMP_OUT;
+		if (st->chip_config.magn_fifo_enable)
+			d |= INV_MPU6050_BIT_SLAVE_0;
+		ret = regmap_write(st->map, st->reg->fifo_en, d);
+		if (ret)
+			return ret;
+		/* enable FIFO reading */
+		d = st->chip_config.user_ctrl | INV_MPU6050_BIT_FIFO_EN;
+		ret = regmap_write(st->map, st->reg->user_ctrl, d);
+		if (ret)
+			return ret;
+		/* enable interrupt */
+		ret = regmap_write(st->map, st->reg->int_enable,
+				   INV_MPU6050_BIT_DATA_RDY_EN);
+	} else {
+		ret = regmap_write(st->map, st->reg->int_enable, 0);
+		if (ret)
+			return ret;
+		ret = regmap_write(st->map, st->reg->fifo_en, 0);
+		if (ret)
+			return ret;
+		/* restore user_ctrl for disabling FIFO reading */
+		ret = regmap_write(st->map, st->reg->user_ctrl,
+				   st->chip_config.user_ctrl);
+	}
+
+	return ret;
+}
+
 /**
  *  inv_mpu6050_set_enable() - enable chip functions.
  *  @indio_dev:	Device driver instance.
@@ -121,24 +169,13 @@ static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
 		if (result)
 			goto error_power_off;
 		st->skip_samples = inv_compute_skip_samples(st);
-		result = inv_reset_fifo(indio_dev);
+		result = inv_mpu6050_prepare_fifo(st, true);
 		if (result)
 			goto error_sensors_off;
 	} else {
-		result = regmap_write(st->map, st->reg->fifo_en, 0);
-		if (result)
-			goto error_fifo_off;
-
-		result = regmap_write(st->map, st->reg->int_enable, 0);
-		if (result)
-			goto error_fifo_off;
-
-		/* restore user_ctrl for disabling FIFO reading */
-		result = regmap_write(st->map, st->reg->user_ctrl,
-				      st->chip_config.user_ctrl);
+		result = inv_mpu6050_prepare_fifo(st, false);
 		if (result)
 			goto error_sensors_off;
-
 		result = inv_mpu6050_switch_engine(st, false, scan);
 		if (result)
 			goto error_power_off;
@@ -150,9 +187,6 @@ static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
 
 	return 0;
 
-error_fifo_off:
-	/* always restore user_ctrl to disable fifo properly */
-	regmap_write(st->map, st->reg->user_ctrl, st->chip_config.user_ctrl);
 error_sensors_off:
 	inv_mpu6050_switch_engine(st, false, scan);
 error_power_off:
-- 
2.17.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 11/13] iio: imu: inv_mpu6050: dynamic sampling rate change
  2020-02-12 17:40 [PATCH 00/13] Rework sensors engines and power management Jean-Baptiste Maneyrol
                   ` (9 preceding siblings ...)
  2020-02-12 17:40 ` [PATCH 10/13] iio: imu: inv_mpu6050: factorize fifo enable/disable Jean-Baptiste Maneyrol
@ 2020-02-12 17:40 ` Jean-Baptiste Maneyrol
  2020-02-12 17:40 ` [PATCH 12/13] iio: imu: inv_mpu6050: use runtime pm with autosuspend Jean-Baptiste Maneyrol
  2020-02-12 17:40 ` [PATCH 13/13] iio: imu: inv_mpu6050: temperature only work with accel/gyro Jean-Baptiste Maneyrol
  12 siblings, 0 replies; 21+ messages in thread
From: Jean-Baptiste Maneyrol @ 2020-02-12 17:40 UTC (permalink / raw)
  To: jic23, linux-iio; +Cc: Jean-Baptiste Maneyrol

Sampling rate can be changed while the chip is running. It can
be useful thus do not prevent it.

Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index 28f8079d4599..f33fd04671cc 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -910,10 +910,6 @@ inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr,
 	    fifo_rate > INV_MPU6050_MAX_FIFO_RATE)
 		return -EINVAL;
 
-	result = iio_device_claim_direct_mode(indio_dev);
-	if (result)
-		return result;
-
 	/* compute the chip sample rate divider */
 	d = INV_MPU6050_FIFO_RATE_TO_DIVIDER(fifo_rate);
 	/* compute back the fifo rate to handle truncation cases */
@@ -946,7 +942,6 @@ inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr,
 	result |= inv_mpu6050_set_power_itg(st, false);
 fifo_rate_fail_unlock:
 	mutex_unlock(&st->lock);
-	iio_device_release_direct_mode(indio_dev);
 	if (result)
 		return result;
 
-- 
2.17.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 12/13] iio: imu: inv_mpu6050: use runtime pm with autosuspend
  2020-02-12 17:40 [PATCH 00/13] Rework sensors engines and power management Jean-Baptiste Maneyrol
                   ` (10 preceding siblings ...)
  2020-02-12 17:40 ` [PATCH 11/13] iio: imu: inv_mpu6050: dynamic sampling rate change Jean-Baptiste Maneyrol
@ 2020-02-12 17:40 ` Jean-Baptiste Maneyrol
  2020-02-15 18:22   ` Jonathan Cameron
  2020-02-12 17:40 ` [PATCH 13/13] iio: imu: inv_mpu6050: temperature only work with accel/gyro Jean-Baptiste Maneyrol
  12 siblings, 1 reply; 21+ messages in thread
From: Jean-Baptiste Maneyrol @ 2020-02-12 17:40 UTC (permalink / raw)
  To: jic23, linux-iio; +Cc: Jean-Baptiste Maneyrol

Use runtime power management for handling chip power and
sensor engines on/off. Simplifies things a lot since pm
runtime already has reference counter.
Usage of autosuspend reduces the number of power on/off. This
makes polling interface now usable to get data at low
frequency.

Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c    | 265 ++++++++++++------
 drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h     |   5 +-
 drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c |  29 +-
 3 files changed, 191 insertions(+), 108 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index f33fd04671cc..f698d2aa61f4 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -16,6 +16,8 @@
 #include <linux/acpi.h>
 #include <linux/platform_device.h>
 #include <linux/regulator/consumer.h>
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
 #include "inv_mpu_iio.h"
 #include "inv_mpu_magn.h"
 
@@ -400,26 +402,13 @@ int inv_mpu6050_set_power_itg(struct inv_mpu6050_state *st, bool power_on)
 {
 	int result;
 
-	if (power_on) {
-		if (!st->powerup_count) {
-			result = inv_mpu6050_pwr_mgmt_1_write(st, false, -1, -1);
-			if (result)
-				return result;
-			usleep_range(INV_MPU6050_REG_UP_TIME_MIN,
-				     INV_MPU6050_REG_UP_TIME_MAX);
-		}
-		st->powerup_count++;
-	} else {
-		if (st->powerup_count == 1) {
-			result = inv_mpu6050_pwr_mgmt_1_write(st, true, -1, -1);
-			if (result)
-				return result;
-		}
-		st->powerup_count--;
-	}
+	result = inv_mpu6050_pwr_mgmt_1_write(st, !power_on, -1, -1);
+	if (result)
+		return result;
 
-	dev_dbg(regmap_get_device(st->map), "set power %d, count=%u\n",
-		power_on, st->powerup_count);
+	if (power_on)
+		usleep_range(INV_MPU6050_REG_UP_TIME_MIN,
+			     INV_MPU6050_REG_UP_TIME_MAX);
 
 	return 0;
 }
@@ -563,6 +552,7 @@ static int inv_mpu6050_read_channel_data(struct iio_dev *indio_dev,
 					 int *val)
 {
 	struct inv_mpu6050_state *st = iio_priv(indio_dev);
+	struct device *pdev = regmap_get_device(st->map);
 	unsigned int freq_hz, period_us, min_sleep_us, max_sleep_us;
 	int result;
 	int ret;
@@ -571,92 +561,85 @@ static int inv_mpu6050_read_channel_data(struct iio_dev *indio_dev,
 	freq_hz = INV_MPU6050_DIVIDER_TO_FIFO_RATE(st->chip_config.divider);
 	period_us = 1000000 / freq_hz;
 
-	result = inv_mpu6050_set_power_itg(st, true);
-	if (result)
+	result = pm_runtime_get_sync(pdev);
+	if (result < 0) {
+		pm_runtime_put_noidle(pdev);
 		return result;
+	}
 
 	switch (chan->type) {
 	case IIO_ANGL_VEL:
-		result = inv_mpu6050_switch_engine(st, true,
-				INV_MPU6050_SENSOR_GYRO);
-		if (result)
-			goto error_power_off;
-		/* need to wait 2 periods to have first valid sample */
-		min_sleep_us = 2 * period_us;
-		max_sleep_us = 2 * (period_us + period_us / 2);
-		usleep_range(min_sleep_us, max_sleep_us);
+		if (!st->chip_config.gyro_en) {
+			result = inv_mpu6050_switch_engine(st, true,
+					INV_MPU6050_SENSOR_GYRO);
+			if (result)
+				goto error_power_off;
+			/* need to wait 2 periods to have first valid sample */
+			min_sleep_us = 2 * period_us;
+			max_sleep_us = 2 * (period_us + period_us / 2);
+			usleep_range(min_sleep_us, max_sleep_us);
+		}
 		ret = inv_mpu6050_sensor_show(st, st->reg->raw_gyro,
 					      chan->channel2, val);
-		result = inv_mpu6050_switch_engine(st, false,
-				INV_MPU6050_SENSOR_GYRO);
-		if (result)
-			goto error_power_off;
 		break;
 	case IIO_ACCEL:
-		result = inv_mpu6050_switch_engine(st, true,
-				INV_MPU6050_SENSOR_ACCL);
-		if (result)
-			goto error_power_off;
-		/* wait 1 period for first sample availability */
-		min_sleep_us = period_us;
-		max_sleep_us = period_us + period_us / 2;
-		usleep_range(min_sleep_us, max_sleep_us);
+		if (!st->chip_config.accl_en) {
+			result = inv_mpu6050_switch_engine(st, true,
+					INV_MPU6050_SENSOR_ACCL);
+			if (result)
+				goto error_power_off;
+			/* wait 1 period for first sample availability */
+			min_sleep_us = period_us;
+			max_sleep_us = period_us + period_us / 2;
+			usleep_range(min_sleep_us, max_sleep_us);
+		}
 		ret = inv_mpu6050_sensor_show(st, st->reg->raw_accl,
 					      chan->channel2, val);
-		result = inv_mpu6050_switch_engine(st, false,
-				INV_MPU6050_SENSOR_ACCL);
-		if (result)
-			goto error_power_off;
 		break;
 	case IIO_TEMP:
-		result = inv_mpu6050_switch_engine(st, true,
-				INV_MPU6050_SENSOR_TEMP);
-		if (result)
-			goto error_power_off;
-		/* wait 1 period for first sample availability */
-		min_sleep_us = period_us;
-		max_sleep_us = period_us + period_us / 2;
-		usleep_range(min_sleep_us, max_sleep_us);
+		if (!st->chip_config.temp_en) {
+			result = inv_mpu6050_switch_engine(st, true,
+					INV_MPU6050_SENSOR_TEMP);
+			if (result)
+				goto error_power_off;
+			/* wait 1 period for first sample availability */
+			min_sleep_us = period_us;
+			max_sleep_us = period_us + period_us / 2;
+			usleep_range(min_sleep_us, max_sleep_us);
+		}
 		ret = inv_mpu6050_sensor_show(st, st->reg->temperature,
 					      IIO_MOD_X, val);
-		result = inv_mpu6050_switch_engine(st, false,
-				INV_MPU6050_SENSOR_TEMP);
-		if (result)
-			goto error_power_off;
 		break;
 	case IIO_MAGN:
-		result = inv_mpu6050_switch_engine(st, true,
-				INV_MPU6050_SENSOR_MAGN);
-		if (result)
-			goto error_power_off;
-		/* frequency is limited for magnetometer */
-		if (freq_hz > INV_MPU_MAGN_FREQ_HZ_MAX) {
-			freq_hz = INV_MPU_MAGN_FREQ_HZ_MAX;
-			period_us = 1000000 / freq_hz;
+		if (!st->chip_config.magn_en) {
+			result = inv_mpu6050_switch_engine(st, true,
+					INV_MPU6050_SENSOR_MAGN);
+			if (result)
+				goto error_power_off;
+			/* frequency is limited for magnetometer */
+			if (freq_hz > INV_MPU_MAGN_FREQ_HZ_MAX) {
+				freq_hz = INV_MPU_MAGN_FREQ_HZ_MAX;
+				period_us = 1000000 / freq_hz;
+			}
+			/* need to wait 2 periods to have first valid sample */
+			min_sleep_us = 2 * period_us;
+			max_sleep_us = 2 * (period_us + period_us / 2);
+			usleep_range(min_sleep_us, max_sleep_us);
 		}
-		/* need to wait 2 periods to have first valid sample */
-		min_sleep_us = 2 * period_us;
-		max_sleep_us = 2 * (period_us + period_us / 2);
-		usleep_range(min_sleep_us, max_sleep_us);
 		ret = inv_mpu_magn_read(st, chan->channel2, val);
-		result = inv_mpu6050_switch_engine(st, false,
-				INV_MPU6050_SENSOR_MAGN);
-		if (result)
-			goto error_power_off;
 		break;
 	default:
 		ret = -EINVAL;
 		break;
 	}
 
-	result = inv_mpu6050_set_power_itg(st, false);
-	if (result)
-		goto error_power_off;
+	pm_runtime_mark_last_busy(pdev);
+	pm_runtime_put_autosuspend(pdev);
 
 	return ret;
 
 error_power_off:
-	inv_mpu6050_set_power_itg(st, false);
+	pm_runtime_put_autosuspend(pdev);
 	return result;
 }
 
@@ -795,6 +778,7 @@ static int inv_mpu6050_write_raw(struct iio_dev *indio_dev,
 				 int val, int val2, long mask)
 {
 	struct inv_mpu6050_state  *st = iio_priv(indio_dev);
+	struct device *pdev = regmap_get_device(st->map);
 	int result;
 
 	/*
@@ -806,9 +790,11 @@ static int inv_mpu6050_write_raw(struct iio_dev *indio_dev,
 		return result;
 
 	mutex_lock(&st->lock);
-	result = inv_mpu6050_set_power_itg(st, true);
-	if (result)
+	result = pm_runtime_get_sync(pdev);
+	if (result < 0) {
+		pm_runtime_put_noidle(pdev);
 		goto error_write_raw_unlock;
+	}
 
 	switch (mask) {
 	case IIO_CHAN_INFO_SCALE:
@@ -846,7 +832,8 @@ static int inv_mpu6050_write_raw(struct iio_dev *indio_dev,
 		break;
 	}
 
-	result |= inv_mpu6050_set_power_itg(st, false);
+	pm_runtime_mark_last_busy(pdev);
+	pm_runtime_put_autosuspend(pdev);
 error_write_raw_unlock:
 	mutex_unlock(&st->lock);
 	iio_device_release_direct_mode(indio_dev);
@@ -903,6 +890,7 @@ inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr,
 	int result;
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct inv_mpu6050_state *st = iio_priv(indio_dev);
+	struct device *pdev = regmap_get_device(st->map);
 
 	if (kstrtoint(buf, 10, &fifo_rate))
 		return -EINVAL;
@@ -920,9 +908,11 @@ inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr,
 		result = 0;
 		goto fifo_rate_fail_unlock;
 	}
-	result = inv_mpu6050_set_power_itg(st, true);
-	if (result)
+	result = pm_runtime_get_sync(pdev);
+	if (result < 0) {
+		pm_runtime_put_noidle(pdev);
 		goto fifo_rate_fail_unlock;
+	}
 
 	result = regmap_write(st->map, st->reg->sample_rate_div, d);
 	if (result)
@@ -938,8 +928,9 @@ inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr,
 	if (result)
 		goto fifo_rate_fail_power_off;
 
+	pm_runtime_mark_last_busy(pdev);
 fifo_rate_fail_power_off:
-	result |= inv_mpu6050_set_power_itg(st, false);
+	pm_runtime_put_autosuspend(pdev);
 fifo_rate_fail_unlock:
 	mutex_unlock(&st->lock);
 	if (result)
@@ -1385,6 +1376,14 @@ static void inv_mpu_core_disable_regulator_action(void *_data)
 	inv_mpu_core_disable_regulator_vddio(st);
 }
 
+static void inv_mpu_pm_disable(void *data)
+{
+	struct device *dev = data;
+
+	pm_runtime_put_sync_suspend(dev);
+	pm_runtime_disable(dev);
+}
+
 int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
 		int (*inv_mpu_bus_setup)(struct iio_dev *), int chip_type)
 {
@@ -1409,7 +1408,6 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
 	st = iio_priv(indio_dev);
 	mutex_init(&st->lock);
 	st->chip_type = chip_type;
-	st->powerup_count = 0;
 	st->irq = irq;
 	st->map = regmap;
 
@@ -1521,8 +1519,16 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
 			goto error_power_off;
 	}
 
-	/* chip init is done, turning off */
-	result = inv_mpu6050_set_power_itg(st, false);
+	/* chip init is done, turning on runtime power management */
+	result = pm_runtime_set_active(dev);
+	if (result)
+		goto error_power_off;
+	pm_runtime_get_noresume(dev);
+	pm_runtime_enable(dev);
+	pm_runtime_set_autosuspend_delay(dev, INV_MPU6050_SUSPEND_DELAY_MS);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_put(dev);
+	result = devm_add_action_or_reset(dev, inv_mpu_pm_disable, dev);
 	if (result)
 		return result;
 
@@ -1594,36 +1600,113 @@ EXPORT_SYMBOL_GPL(inv_mpu_core_probe);
 
 static int inv_mpu_resume(struct device *dev)
 {
-	struct inv_mpu6050_state *st = iio_priv(dev_get_drvdata(dev));
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct inv_mpu6050_state *st = iio_priv(indio_dev);
 	int result;
 
 	mutex_lock(&st->lock);
+
 	result = inv_mpu_core_enable_regulator_vddio(st);
 	if (result)
 		goto out_unlock;
 
 	result = inv_mpu6050_set_power_itg(st, true);
+	if (result)
+		goto out_unlock;
+
+	result = inv_mpu6050_switch_engine(st, true, st->suspended_sensors);
+	if (result)
+		goto out_unlock;
+
+	if (iio_buffer_enabled(indio_dev))
+		result = inv_mpu6050_prepare_fifo(st, true);
+
 out_unlock:
 	mutex_unlock(&st->lock);
-
 	return result;
 }
 
 static int inv_mpu_suspend(struct device *dev)
 {
-	struct inv_mpu6050_state *st = iio_priv(dev_get_drvdata(dev));
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct inv_mpu6050_state *st = iio_priv(indio_dev);
 	int result;
 
 	mutex_lock(&st->lock);
+
+	if (iio_buffer_enabled(indio_dev)) {
+		result = inv_mpu6050_prepare_fifo(st, false);
+		goto out_unlock;
+	}
+
+	st->suspended_sensors = 0;
+	if (st->chip_config.accl_en)
+		st->suspended_sensors |= INV_MPU6050_SENSOR_ACCL;
+	if (st->chip_config.gyro_en)
+		st->suspended_sensors |= INV_MPU6050_SENSOR_GYRO;
+	if (st->chip_config.temp_en)
+		st->suspended_sensors |= INV_MPU6050_SENSOR_TEMP;
+	if (st->chip_config.magn_en)
+		st->suspended_sensors |= INV_MPU6050_SENSOR_MAGN;
+	result = inv_mpu6050_switch_engine(st, false, st->suspended_sensors);
+	if (result)
+		goto out_unlock;
+
 	result = inv_mpu6050_set_power_itg(st, false);
+	if (result)
+		goto out_unlock;
+
 	inv_mpu_core_disable_regulator_vddio(st);
-	mutex_unlock(&st->lock);
 
+out_unlock:
+	mutex_unlock(&st->lock);
 	return result;
 }
 #endif /* CONFIG_PM_SLEEP */
 
-SIMPLE_DEV_PM_OPS(inv_mpu_pmops, inv_mpu_suspend, inv_mpu_resume);
+#ifdef CONFIG_PM
+static int inv_mpu_runtime_suspend(struct device *dev)
+{
+	struct inv_mpu6050_state *st = iio_priv(dev_get_drvdata(dev));
+	unsigned int sensors;
+	int ret;
+
+	mutex_lock(&st->lock);
+
+	sensors = INV_MPU6050_SENSOR_ACCL | INV_MPU6050_SENSOR_GYRO |
+			INV_MPU6050_SENSOR_TEMP | INV_MPU6050_SENSOR_MAGN;
+	ret = inv_mpu6050_switch_engine(st, false, sensors);
+	if (ret)
+		goto out_unlock;
+
+	ret = inv_mpu6050_set_power_itg(st, false);
+	if (ret)
+		goto out_unlock;
+
+	inv_mpu_core_disable_regulator_vddio(st);
+
+out_unlock:
+	mutex_unlock(&st->lock);
+	return ret;
+}
+
+static int inv_mpu_runtime_resume(struct device *dev)
+{
+	struct inv_mpu6050_state *st = iio_priv(dev_get_drvdata(dev));
+	int ret;
+
+	ret = inv_mpu_core_enable_regulator_vddio(st);
+	if (ret)
+		return ret;
+
+	return inv_mpu6050_set_power_itg(st, true);
+}
+#endif
+
+const struct dev_pm_ops inv_mpu_pmops = {
+	SET_SYSTEM_SLEEP_PM_OPS(inv_mpu_suspend, inv_mpu_resume)
+	SET_RUNTIME_PM_OPS(inv_mpu_runtime_suspend, inv_mpu_runtime_resume, NULL)
+};
 EXPORT_SYMBOL_GPL(inv_mpu_pmops);
 
 MODULE_AUTHOR("Invensense Corporation");
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
index e328c98e362c..cd38b3fccc7b 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
@@ -164,6 +164,7 @@ struct inv_mpu6050_hw {
  *  @magn_disabled:     magnetometer disabled for backward compatibility reason.
  *  @magn_raw_to_gauss:	coefficient to convert mag raw value to Gauss.
  *  @magn_orient:       magnetometer sensor chip orientation if available.
+ *  @suspended_sensors:	sensors mask of sensors turned off for suspend
  */
 struct inv_mpu6050_state {
 	struct mutex lock;
@@ -174,7 +175,6 @@ struct inv_mpu6050_state {
 	enum   inv_devices chip_type;
 	struct i2c_mux_core *muxc;
 	struct i2c_client *mux_client;
-	unsigned int powerup_count;
 	struct inv_mpu6050_platform_data plat_data;
 	struct iio_mount_matrix orientation;
 	struct regmap *map;
@@ -189,6 +189,7 @@ struct inv_mpu6050_state {
 	bool magn_disabled;
 	s32 magn_raw_to_gauss[3];
 	struct iio_mount_matrix magn_orient;
+	unsigned int suspended_sensors;
 };
 
 /*register and associated bit definition*/
@@ -312,6 +313,7 @@ struct inv_mpu6050_state {
 #define INV_MPU6050_ACCEL_UP_TIME            20
 #define INV_MPU6050_GYRO_UP_TIME             35
 #define INV_MPU6050_GYRO_DOWN_TIME           150
+#define INV_MPU6050_SUSPEND_DELAY_MS         2000
 
 /* delay time in microseconds */
 #define INV_MPU6050_REG_UP_TIME_MIN          5000
@@ -439,7 +441,6 @@ int inv_mpu6050_prepare_fifo(struct inv_mpu6050_state *st, bool enable);
 int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en,
 			      unsigned int mask);
 int inv_mpu6050_write_reg(struct inv_mpu6050_state *st, int reg, u8 val);
-int inv_mpu6050_set_power_itg(struct inv_mpu6050_state *st, bool power_on);
 int inv_mpu_acpi_create_mux_client(struct i2c_client *client);
 void inv_mpu_acpi_delete_mux_client(struct i2c_client *client);
 int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
index f53f50d08b9e..9eab39042443 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
@@ -3,6 +3,7 @@
 * Copyright (C) 2012 Invensense, Inc.
 */
 
+#include <linux/pm_runtime.h>
 #include "inv_mpu_iio.h"
 
 static unsigned int inv_scan_query_mpu6050(struct iio_dev *indio_dev)
@@ -156,41 +157,39 @@ int inv_mpu6050_prepare_fifo(struct inv_mpu6050_state *st, bool enable)
 static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
 {
 	struct inv_mpu6050_state *st = iio_priv(indio_dev);
+	struct device *pdev = regmap_get_device(st->map);
 	unsigned int scan;
 	int result;
 
-	scan = inv_scan_query(indio_dev);
-
 	if (enable) {
-		result = inv_mpu6050_set_power_itg(st, true);
-		if (result)
+		scan = inv_scan_query(indio_dev);
+		result = pm_runtime_get_sync(pdev);
+		if (result < 0) {
+			pm_runtime_put_noidle(pdev);
 			return result;
+		}
+		result = inv_mpu6050_switch_engine(st, false, ~scan);
+		if (result)
+			goto error_power_off;
 		result = inv_mpu6050_switch_engine(st, true, scan);
 		if (result)
 			goto error_power_off;
 		st->skip_samples = inv_compute_skip_samples(st);
 		result = inv_mpu6050_prepare_fifo(st, true);
 		if (result)
-			goto error_sensors_off;
+			goto error_power_off;
 	} else {
 		result = inv_mpu6050_prepare_fifo(st, false);
-		if (result)
-			goto error_sensors_off;
-		result = inv_mpu6050_switch_engine(st, false, scan);
-		if (result)
-			goto error_power_off;
-
-		result = inv_mpu6050_set_power_itg(st, false);
 		if (result)
 			goto error_power_off;
+		pm_runtime_mark_last_busy(pdev);
+		pm_runtime_put_autosuspend(pdev);
 	}
 
 	return 0;
 
-error_sensors_off:
-	inv_mpu6050_switch_engine(st, false, scan);
 error_power_off:
-	inv_mpu6050_set_power_itg(st, false);
+	pm_runtime_put_autosuspend(pdev);
 	return result;
 }
 
-- 
2.17.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 13/13] iio: imu: inv_mpu6050: temperature only work with accel/gyro
  2020-02-12 17:40 [PATCH 00/13] Rework sensors engines and power management Jean-Baptiste Maneyrol
                   ` (11 preceding siblings ...)
  2020-02-12 17:40 ` [PATCH 12/13] iio: imu: inv_mpu6050: use runtime pm with autosuspend Jean-Baptiste Maneyrol
@ 2020-02-12 17:40 ` Jean-Baptiste Maneyrol
  2020-02-15 18:23   ` Jonathan Cameron
  12 siblings, 1 reply; 21+ messages in thread
From: Jean-Baptiste Maneyrol @ 2020-02-12 17:40 UTC (permalink / raw)
  To: jic23, linux-iio; +Cc: Jean-Baptiste Maneyrol

Temperature sensor works correctly only when accel and/or gyro
is turned on. Prevent polling value if they are not running.
Anyway it doesn't make sense to use it without sensor engines
on.

Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index f698d2aa61f4..3212030cc083 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -597,6 +597,11 @@ static int inv_mpu6050_read_channel_data(struct iio_dev *indio_dev,
 					      chan->channel2, val);
 		break;
 	case IIO_TEMP:
+		/* temperature sensor work only with accel and/or gyro */
+		if (!st->chip_config.accl_en && !st->chip_config.gyro_en) {
+			result = -ENOTSUPP;
+			goto error_power_off;
+		}
 		if (!st->chip_config.temp_en) {
 			result = inv_mpu6050_switch_engine(st, true,
 					INV_MPU6050_SENSOR_TEMP);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 01/13] iio: imu: inv_mpu6050: enable i2c aux mux bypass only once
  2020-02-12 17:40 ` [PATCH 01/13] iio: imu: inv_mpu6050: enable i2c aux mux bypass only once Jean-Baptiste Maneyrol
@ 2020-02-15 17:30   ` Jonathan Cameron
  2020-02-18 16:03     ` Jean-Baptiste Maneyrol
  0 siblings, 1 reply; 21+ messages in thread
From: Jonathan Cameron @ 2020-02-15 17:30 UTC (permalink / raw)
  To: Jean-Baptiste Maneyrol; +Cc: linux-iio, Peter Rosin

On Wed, 12 Feb 2020 18:40:36 +0100
Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote:

> i2c auxiliary mux is done by analog switches. You do not need to
> set them for every i2c transfer.
> Just set i2c bypass bit at init and do noting in i2c de/select.
> 
> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
Fair enough.  Given we are making the decision based on DT and that
can't change on reprobing etc so this change makes sense to me I think.

It does leave us making rather odd use of the mux code, so I'd
just like Peter to take a quick look at this before I apply it.

Jonathan

> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c | 71 +++++++++--------------
>  1 file changed, 28 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> index 1363d3776523..24df880248f2 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> @@ -20,38 +20,6 @@ static const struct regmap_config inv_mpu_regmap_config = {
>  
>  static int inv_mpu6050_select_bypass(struct i2c_mux_core *muxc, u32 chan_id)
>  {
> -	struct iio_dev *indio_dev = i2c_mux_priv(muxc);
> -	struct inv_mpu6050_state *st = iio_priv(indio_dev);
> -	int ret;
> -
> -	mutex_lock(&st->lock);
> -
> -	ret = inv_mpu6050_set_power_itg(st, true);
> -	if (ret)
> -		goto error_unlock;
> -
> -	ret = regmap_write(st->map, st->reg->int_pin_cfg,
> -			   st->irq_mask | INV_MPU6050_BIT_BYPASS_EN);
> -
> -error_unlock:
> -	mutex_unlock(&st->lock);
> -
> -	return ret;
> -}
> -
> -static int inv_mpu6050_deselect_bypass(struct i2c_mux_core *muxc, u32 chan_id)
> -{
> -	struct iio_dev *indio_dev = i2c_mux_priv(muxc);
> -	struct inv_mpu6050_state *st = iio_priv(indio_dev);
> -
> -	mutex_lock(&st->lock);
> -
> -	/* It doesn't really matter if any of the calls fail */
> -	regmap_write(st->map, st->reg->int_pin_cfg, st->irq_mask);
> -	inv_mpu6050_set_power_itg(st, false);
> -
> -	mutex_unlock(&st->lock);
> -
>  	return 0;
>  }
>  
> @@ -79,19 +47,20 @@ static bool inv_mpu_i2c_aux_bus(struct device *dev)
>  	}
>  }
>  
> -/*
> - * MPU9xxx magnetometer support requires to disable i2c auxiliary bus support.
> - * To ensure backward compatibility with existing setups, do not disable
> - * i2c auxiliary bus if it used.
> - * Check for i2c-gate node in devicetree and set magnetometer disabled.
> - * Only MPU6500 is supported by ACPI, no need to check.
> - */
> -static int inv_mpu_magn_disable(struct iio_dev *indio_dev)
> +static int inv_mpu_i2c_aux_setup(struct iio_dev *indio_dev)
>  {
>  	struct inv_mpu6050_state *st = iio_priv(indio_dev);
>  	struct device *dev = indio_dev->dev.parent;
>  	struct device_node *mux_node;
> +	int ret;
>  
> +	/*
> +	 * MPU9xxx magnetometer support requires to disable i2c auxiliary bus.
> +	 * To ensure backward compatibility with existing setups, do not disable
> +	 * i2c auxiliary bus if it used.
> +	 * Check for i2c-gate node in devicetree and set magnetometer disabled.
> +	 * Only MPU6500 is supported by ACPI, no need to check.
> +	 */
>  	switch (st->chip_type) {
>  	case INV_MPU9150:
>  	case INV_MPU9250:
> @@ -107,7 +76,24 @@ static int inv_mpu_magn_disable(struct iio_dev *indio_dev)
>  		break;
>  	}
>  
> +	/* enable i2c bypass when using i2c auxiliary bus */
> +	if (inv_mpu_i2c_aux_bus(dev)) {
> +		ret = inv_mpu6050_set_power_itg(st, true);
> +		if (ret)
> +			return ret;
> +		ret = regmap_write(st->map, st->reg->int_pin_cfg,
> +				   st->irq_mask | INV_MPU6050_BIT_BYPASS_EN);
> +		if (ret)
> +			goto error;
> +		ret = inv_mpu6050_set_power_itg(st, false);
> +		if (ret)
> +			goto error;
> +	}
> +
>  	return 0;
> +error:
> +	inv_mpu6050_set_power_itg(st, false);
> +	return ret;
>  }
>  
>  /**
> @@ -151,7 +137,7 @@ static int inv_mpu_probe(struct i2c_client *client,
>  	}
>  
>  	result = inv_mpu_core_probe(regmap, client->irq, name,
> -				    inv_mpu_magn_disable, chip_type);
> +				    inv_mpu_i2c_aux_setup, chip_type);
>  	if (result < 0)
>  		return result;
>  
> @@ -160,8 +146,7 @@ static int inv_mpu_probe(struct i2c_client *client,
>  		/* declare i2c auxiliary bus */
>  		st->muxc = i2c_mux_alloc(client->adapter, &client->dev,
>  					 1, 0, I2C_MUX_LOCKED | I2C_MUX_GATE,
> -					 inv_mpu6050_select_bypass,
> -					 inv_mpu6050_deselect_bypass);
> +					 inv_mpu6050_select_bypass, NULL);
>  		if (!st->muxc)
>  			return -ENOMEM;
>  		st->muxc->priv = dev_get_drvdata(&client->dev);


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 08/13] iio: imu: inv_mpu6050: rewrite power and engine management
  2020-02-12 17:40 ` [PATCH 08/13] iio: imu: inv_mpu6050: rewrite power and engine management Jean-Baptiste Maneyrol
@ 2020-02-15 18:09   ` Jonathan Cameron
  0 siblings, 0 replies; 21+ messages in thread
From: Jonathan Cameron @ 2020-02-15 18:09 UTC (permalink / raw)
  To: Jean-Baptiste Maneyrol; +Cc: linux-iio

On Wed, 12 Feb 2020 18:40:43 +0100
Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote:

> Rewrite clock management to use automatic clock switching
> present since MPU6500.
> Sensors engine management can now turn on or off a batch of
> sensors which simplifies usage a lot.
> Temperature sensor is now turned on/off depending on usage.
> 
> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
At the fairly superficial level at which I'm reading this it all
looks fine.  I suspect I'll not go into this in enough detail to
understand it properly.  Will apply once the rest are ready to
go.

Thanks,

Jonathan

> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c    | 264 +++++++++++++-----
>  drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h     |  24 +-
>  drivers/iio/imu/inv_mpu6050/inv_mpu_magn.c    |  12 +-
>  drivers/iio/imu/inv_mpu6050/inv_mpu_magn.h    |   2 +-
>  drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c |  91 +++---
>  5 files changed, 262 insertions(+), 131 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> index 63cdde20df7e..a51efc4c941b 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> @@ -99,9 +99,31 @@ static const struct inv_mpu6050_reg_map reg_set_6050 = {
>  };
>  
>  static const struct inv_mpu6050_chip_config chip_config_6050 = {
> +	.clk = INV_CLK_INTERNAL,
>  	.fsr = INV_MPU6050_FSR_2000DPS,
>  	.lpf = INV_MPU6050_FILTER_20HZ,
>  	.divider = INV_MPU6050_FIFO_RATE_TO_DIVIDER(50),
> +	.gyro_en = true,
> +	.accl_en = true,
> +	.temp_en = true,
> +	.magn_en = false,
> +	.gyro_fifo_enable = false,
> +	.accl_fifo_enable = false,
> +	.temp_fifo_enable = false,
> +	.magn_fifo_enable = false,
> +	.accl_fs = INV_MPU6050_FS_02G,
> +	.user_ctrl = 0,
> +};
> +
> +static const struct inv_mpu6050_chip_config chip_config_6500 = {
> +	.clk = INV_CLK_PLL,
> +	.fsr = INV_MPU6050_FSR_2000DPS,
> +	.lpf = INV_MPU6050_FILTER_20HZ,
> +	.divider = INV_MPU6050_FIFO_RATE_TO_DIVIDER(50),
> +	.gyro_en = true,
> +	.accl_en = true,
> +	.temp_en = true,
> +	.magn_en = false,
>  	.gyro_fifo_enable = false,
>  	.accl_fifo_enable = false,
>  	.temp_fifo_enable = false,
> @@ -124,7 +146,7 @@ static const struct inv_mpu6050_hw hw_info[] = {
>  		.whoami = INV_MPU6500_WHOAMI_VALUE,
>  		.name = "MPU6500",
>  		.reg = &reg_set_6500,
> -		.config = &chip_config_6050,
> +		.config = &chip_config_6500,
>  		.fifo_size = 512,
>  		.temp = {INV_MPU6500_TEMP_OFFSET, INV_MPU6500_TEMP_SCALE},
>  	},
> @@ -132,7 +154,7 @@ static const struct inv_mpu6050_hw hw_info[] = {
>  		.whoami = INV_MPU6515_WHOAMI_VALUE,
>  		.name = "MPU6515",
>  		.reg = &reg_set_6500,
> -		.config = &chip_config_6050,
> +		.config = &chip_config_6500,
>  		.fifo_size = 512,
>  		.temp = {INV_MPU6500_TEMP_OFFSET, INV_MPU6500_TEMP_SCALE},
>  	},
> @@ -156,7 +178,7 @@ static const struct inv_mpu6050_hw hw_info[] = {
>  		.whoami = INV_MPU9250_WHOAMI_VALUE,
>  		.name = "MPU9250",
>  		.reg = &reg_set_6500,
> -		.config = &chip_config_6050,
> +		.config = &chip_config_6500,
>  		.fifo_size = 512,
>  		.temp = {INV_MPU6500_TEMP_OFFSET, INV_MPU6500_TEMP_SCALE},
>  	},
> @@ -164,7 +186,7 @@ static const struct inv_mpu6050_hw hw_info[] = {
>  		.whoami = INV_MPU9255_WHOAMI_VALUE,
>  		.name = "MPU9255",
>  		.reg = &reg_set_6500,
> -		.config = &chip_config_6050,
> +		.config = &chip_config_6500,
>  		.fifo_size = 512,
>  		.temp = {INV_MPU6500_TEMP_OFFSET, INV_MPU6500_TEMP_SCALE},
>  	},
> @@ -172,7 +194,7 @@ static const struct inv_mpu6050_hw hw_info[] = {
>  		.whoami = INV_ICM20608_WHOAMI_VALUE,
>  		.name = "ICM20608",
>  		.reg = &reg_set_6500,
> -		.config = &chip_config_6050,
> +		.config = &chip_config_6500,
>  		.fifo_size = 512,
>  		.temp = {INV_ICM20608_TEMP_OFFSET, INV_ICM20608_TEMP_SCALE},
>  	},
> @@ -180,7 +202,7 @@ static const struct inv_mpu6050_hw hw_info[] = {
>  		.whoami = INV_ICM20609_WHOAMI_VALUE,
>  		.name = "ICM20609",
>  		.reg = &reg_set_6500,
> -		.config = &chip_config_6050,
> +		.config = &chip_config_6500,
>  		.fifo_size = 4 * 1024,
>  		.temp = {INV_ICM20608_TEMP_OFFSET, INV_ICM20608_TEMP_SCALE},
>  	},
> @@ -188,7 +210,7 @@ static const struct inv_mpu6050_hw hw_info[] = {
>  		.whoami = INV_ICM20689_WHOAMI_VALUE,
>  		.name = "ICM20689",
>  		.reg = &reg_set_6500,
> -		.config = &chip_config_6050,
> +		.config = &chip_config_6500,
>  		.fifo_size = 4 * 1024,
>  		.temp = {INV_ICM20608_TEMP_OFFSET, INV_ICM20608_TEMP_SCALE},
>  	},
> @@ -196,15 +218,15 @@ static const struct inv_mpu6050_hw hw_info[] = {
>  		.whoami = INV_ICM20602_WHOAMI_VALUE,
>  		.name = "ICM20602",
>  		.reg = &reg_set_icm20602,
> -		.config = &chip_config_6050,
> +		.config = &chip_config_6500,
>  		.fifo_size = 1008,
>  		.temp = {INV_ICM20608_TEMP_OFFSET, INV_ICM20608_TEMP_SCALE},
>  	},
>  	{
>  		.whoami = INV_ICM20690_WHOAMI_VALUE,
>  		.name = "ICM20690",
> -		.reg = &reg_set_icm20602,
> -		.config = &chip_config_6050,
> +		.reg = &reg_set_6500,
> +		.config = &chip_config_6500,
>  		.fifo_size = 1024,
>  		.temp = {INV_ICM20608_TEMP_OFFSET, INV_ICM20608_TEMP_SCALE},
>  	},
> @@ -212,61 +234,162 @@ static const struct inv_mpu6050_hw hw_info[] = {
>  		.whoami = INV_IAM20680_WHOAMI_VALUE,
>  		.name = "IAM20680",
>  		.reg = &reg_set_6500,
> -		.config = &chip_config_6050,
> +		.config = &chip_config_6500,
>  		.fifo_size = 512,
>  		.temp = {INV_ICM20608_TEMP_OFFSET, INV_ICM20608_TEMP_SCALE},
>  	},
>  };
>  
> -int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en, u32 mask)
> +static int inv_mpu6050_pwr_mgmt_1_write(struct inv_mpu6050_state *st, bool sleep,
> +					int clock, int temp_dis)
>  {
> -	unsigned int d, mgmt_1;
> -	int result;
> -	/*
> -	 * switch clock needs to be careful. Only when gyro is on, can
> -	 * clock source be switched to gyro. Otherwise, it must be set to
> -	 * internal clock
> -	 */
> -	if (mask == INV_MPU6050_BIT_PWR_GYRO_STBY) {
> -		result = regmap_read(st->map, st->reg->pwr_mgmt_1, &mgmt_1);
> -		if (result)
> -			return result;
> +	u8 val;
> +
> +	if (clock < 0)
> +		clock = st->chip_config.clk;
> +	if (temp_dis < 0)
> +		temp_dis = !st->chip_config.temp_en;
> +
> +	val = clock & INV_MPU6050_BIT_CLK_MASK;
> +	if (temp_dis)
> +		val |= INV_MPU6050_BIT_TEMP_DIS;
> +	if (sleep)
> +		val |= INV_MPU6050_BIT_SLEEP;
> +
> +	dev_dbg(regmap_get_device(st->map), "pwr_mgmt_1: 0x%x\n", val);
> +	return regmap_write(st->map, st->reg->pwr_mgmt_1, val);
> +}
> +
> +static int inv_mpu6050_clock_switch(struct inv_mpu6050_state *st,
> +				    unsigned int clock)
> +{
> +	int ret;
> +
> +	switch (st->chip_type) {
> +	case INV_MPU6050:
> +	case INV_MPU6000:
> +	case INV_MPU9150:
> +		/* old chips: switch clock manually */
> +		ret = inv_mpu6050_pwr_mgmt_1_write(st, false, clock, -1);
> +		if (ret)
> +			return ret;
> +		st->chip_config.clk = clock;
> +		break;
> +	default:
> +		/* automatic clock switching, nothing to do */
> +		break;
> +	}
> +
> +	return 0;
> +}
>  
> -		mgmt_1 &= ~INV_MPU6050_BIT_CLK_MASK;
> +int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en,
> +			      unsigned int mask)
> +{
> +	unsigned int sleep;
> +	u8 pwr_mgmt2, user_ctrl;
> +	int ret;
> +
> +	/* delete useless requests */
> +	if (mask & INV_MPU6050_SENSOR_ACCL && en == st->chip_config.accl_en)
> +		mask &= ~INV_MPU6050_SENSOR_ACCL;
> +	if (mask & INV_MPU6050_SENSOR_GYRO && en == st->chip_config.gyro_en)
> +		mask &= ~INV_MPU6050_SENSOR_GYRO;
> +	if (mask & INV_MPU6050_SENSOR_TEMP && en == st->chip_config.temp_en)
> +		mask &= ~INV_MPU6050_SENSOR_TEMP;
> +	if (mask & INV_MPU6050_SENSOR_MAGN && en == st->chip_config.magn_en)
> +		mask &= ~INV_MPU6050_SENSOR_MAGN;
> +	if (mask == 0)
> +		return 0;
> +
> +	/* turn on/off temperature sensor */
> +	if (mask & INV_MPU6050_SENSOR_TEMP) {
> +		ret = inv_mpu6050_pwr_mgmt_1_write(st, false, -1, !en);
> +		if (ret)
> +			return ret;
> +		st->chip_config.temp_en = en;
>  	}
>  
> -	if ((mask == INV_MPU6050_BIT_PWR_GYRO_STBY) && (!en)) {
> -		/*
> -		 * turning off gyro requires switch to internal clock first.
> -		 * Then turn off gyro engine
> -		 */
> -		mgmt_1 |= INV_CLK_INTERNAL;
> -		result = regmap_write(st->map, st->reg->pwr_mgmt_1, mgmt_1);
> -		if (result)
> -			return result;
> +	/* update user_crtl for driving magnetometer */
> +	if (mask & INV_MPU6050_SENSOR_MAGN) {
> +		user_ctrl = st->chip_config.user_ctrl;
> +		if (en)
> +			user_ctrl |= INV_MPU6050_BIT_I2C_MST_EN;
> +		else
> +			user_ctrl &= ~INV_MPU6050_BIT_I2C_MST_EN;
> +		ret = regmap_write(st->map, st->reg->user_ctrl, user_ctrl);
> +		if (ret)
> +			return ret;
> +		st->chip_config.user_ctrl = user_ctrl;
> +		st->chip_config.magn_en = en;
>  	}
>  
> -	result = regmap_read(st->map, st->reg->pwr_mgmt_2, &d);
> -	if (result)
> -		return result;
> -	if (en)
> -		d &= ~mask;
> -	else
> -		d |= mask;
> -	result = regmap_write(st->map, st->reg->pwr_mgmt_2, d);
> -	if (result)
> -		return result;
> +	/* manage accel & gyro engines */
> +	if (mask & (INV_MPU6050_SENSOR_ACCL | INV_MPU6050_SENSOR_GYRO)) {
> +		/* compute power management 2 current value */
> +		pwr_mgmt2 = 0;
> +		if (!st->chip_config.accl_en)
> +			pwr_mgmt2 |= INV_MPU6050_BIT_PWR_ACCL_STBY;
> +		if (!st->chip_config.gyro_en)
> +			pwr_mgmt2 |= INV_MPU6050_BIT_PWR_GYRO_STBY;
> +
> +		/* update to new requested value */
> +		if (mask & INV_MPU6050_SENSOR_ACCL) {
> +			if (en)
> +				pwr_mgmt2 &= ~INV_MPU6050_BIT_PWR_ACCL_STBY;
> +			else
> +				pwr_mgmt2 |= INV_MPU6050_BIT_PWR_ACCL_STBY;
> +		}
> +		if (mask & INV_MPU6050_SENSOR_GYRO) {
> +			if (en)
> +				pwr_mgmt2 &= ~INV_MPU6050_BIT_PWR_GYRO_STBY;
> +			else
> +				pwr_mgmt2 |= INV_MPU6050_BIT_PWR_GYRO_STBY;
> +		}
>  
> -	if (en) {
> -		/* Wait for output to stabilize */
> -		msleep(INV_MPU6050_TEMP_UP_TIME);
> -		if (mask == INV_MPU6050_BIT_PWR_GYRO_STBY) {
> -			/* switch internal clock to PLL */
> -			mgmt_1 |= INV_CLK_PLL;
> -			result = regmap_write(st->map,
> -					      st->reg->pwr_mgmt_1, mgmt_1);
> -			if (result)
> -				return result;
> +		/* switch clock to internal when turning gyro off */
> +		if (mask & INV_MPU6050_SENSOR_GYRO && !en) {
> +			ret = inv_mpu6050_clock_switch(st, INV_CLK_INTERNAL);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		/* update sensors engine */
> +		dev_dbg(regmap_get_device(st->map), "pwr_mgmt_2: 0x%x\n",
> +			pwr_mgmt2);
> +		ret = regmap_write(st->map, st->reg->pwr_mgmt_2, pwr_mgmt2);
> +		if (ret)
> +			return ret;
> +		if (mask & INV_MPU6050_SENSOR_ACCL)
> +			st->chip_config.accl_en = en;
> +		if (mask & INV_MPU6050_SENSOR_GYRO)
> +			st->chip_config.gyro_en = en;
> +
> +		/* compute required time to have sensors stabilized */
> +		sleep = 0;
> +		if (en) {
> +			if (mask & INV_MPU6050_SENSOR_ACCL) {
> +				if (sleep < INV_MPU6050_ACCEL_UP_TIME)
> +					sleep = INV_MPU6050_ACCEL_UP_TIME;
> +			}
> +			if (mask & INV_MPU6050_SENSOR_GYRO) {
> +				if (sleep < INV_MPU6050_GYRO_UP_TIME)
> +					sleep = INV_MPU6050_GYRO_UP_TIME;
> +			}
> +		} else {
> +			if (mask & INV_MPU6050_SENSOR_GYRO) {
> +				if (sleep < INV_MPU6050_GYRO_DOWN_TIME)
> +					sleep = INV_MPU6050_GYRO_DOWN_TIME;
> +			}
> +		}
> +		if (sleep)
> +			msleep(sleep);
> +
> +		/* switch clock to PLL when turning gyro on */
> +		if (mask & INV_MPU6050_SENSOR_GYRO && en) {
> +			ret = inv_mpu6050_clock_switch(st, INV_CLK_PLL);
> +			if (ret)
> +				return ret;
>  		}
>  	}
>  
> @@ -279,7 +402,7 @@ int inv_mpu6050_set_power_itg(struct inv_mpu6050_state *st, bool power_on)
>  
>  	if (power_on) {
>  		if (!st->powerup_count) {
> -			result = regmap_write(st->map, st->reg->pwr_mgmt_1, 0);
> +			result = inv_mpu6050_pwr_mgmt_1_write(st, false, -1, -1);
>  			if (result)
>  				return result;
>  			usleep_range(INV_MPU6050_REG_UP_TIME_MIN,
> @@ -288,8 +411,7 @@ int inv_mpu6050_set_power_itg(struct inv_mpu6050_state *st, bool power_on)
>  		st->powerup_count++;
>  	} else {
>  		if (st->powerup_count == 1) {
> -			result = regmap_write(st->map, st->reg->pwr_mgmt_1,
> -					      INV_MPU6050_BIT_SLEEP);
> +			result = inv_mpu6050_pwr_mgmt_1_write(st, true, -1, -1);
>  			if (result)
>  				return result;
>  		}
> @@ -451,33 +573,41 @@ static int inv_mpu6050_read_channel_data(struct iio_dev *indio_dev,
>  	switch (chan->type) {
>  	case IIO_ANGL_VEL:
>  		result = inv_mpu6050_switch_engine(st, true,
> -				INV_MPU6050_BIT_PWR_GYRO_STBY);
> +				INV_MPU6050_SENSOR_GYRO);
>  		if (result)
>  			goto error_power_off;
>  		ret = inv_mpu6050_sensor_show(st, st->reg->raw_gyro,
>  					      chan->channel2, val);
>  		result = inv_mpu6050_switch_engine(st, false,
> -				INV_MPU6050_BIT_PWR_GYRO_STBY);
> +				INV_MPU6050_SENSOR_GYRO);
>  		if (result)
>  			goto error_power_off;
>  		break;
>  	case IIO_ACCEL:
>  		result = inv_mpu6050_switch_engine(st, true,
> -				INV_MPU6050_BIT_PWR_ACCL_STBY);
> +				INV_MPU6050_SENSOR_ACCL);
>  		if (result)
>  			goto error_power_off;
>  		ret = inv_mpu6050_sensor_show(st, st->reg->raw_accl,
>  					      chan->channel2, val);
>  		result = inv_mpu6050_switch_engine(st, false,
> -				INV_MPU6050_BIT_PWR_ACCL_STBY);
> +				INV_MPU6050_SENSOR_ACCL);
>  		if (result)
>  			goto error_power_off;
>  		break;
>  	case IIO_TEMP:
> +		result = inv_mpu6050_switch_engine(st, true,
> +				INV_MPU6050_SENSOR_TEMP);
> +		if (result)
> +			goto error_power_off;
>  		/* wait for stablization */
> -		msleep(INV_MPU6050_SENSOR_UP_TIME);
> +		msleep(INV_MPU6050_TEMP_UP_TIME);
>  		ret = inv_mpu6050_sensor_show(st, st->reg->temperature,
>  					      IIO_MOD_X, val);
> +		result = inv_mpu6050_switch_engine(st, false,
> +				INV_MPU6050_SENSOR_TEMP);
> +		if (result)
> +			goto error_power_off;
>  		break;
>  	case IIO_MAGN:
>  		ret = inv_mpu_magn_read(st, chan->channel2, val);
> @@ -1108,7 +1238,7 @@ static const struct iio_info mpu_info = {
>  static int inv_check_and_setup_chip(struct inv_mpu6050_state *st)
>  {
>  	int result;
> -	unsigned int regval;
> +	unsigned int regval, mask;
>  	int i;
>  
>  	st->hw  = &hw_info[st->chip_type];
> @@ -1174,13 +1304,9 @@ static int inv_check_and_setup_chip(struct inv_mpu6050_state *st)
>  	result = inv_mpu6050_set_power_itg(st, true);
>  	if (result)
>  		return result;
> -
> -	result = inv_mpu6050_switch_engine(st, false,
> -					   INV_MPU6050_BIT_PWR_ACCL_STBY);
> -	if (result)
> -		goto error_power_off;
> -	result = inv_mpu6050_switch_engine(st, false,
> -					   INV_MPU6050_BIT_PWR_GYRO_STBY);
> +	mask = INV_MPU6050_SENSOR_ACCL | INV_MPU6050_SENSOR_GYRO |
> +			INV_MPU6050_SENSOR_TEMP | INV_MPU6050_SENSOR_MAGN;
> +	result = inv_mpu6050_switch_engine(st, false, mask);
>  	if (result)
>  		goto error_power_off;
>  
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> index 17f1f6a15f95..a578789c9210 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> @@ -83,11 +83,22 @@ enum inv_devices {
>  	INV_NUM_PARTS
>  };
>  
> +/* chip sensors mask: accelerometer, gyroscope, temperature, magnetometer */
> +#define INV_MPU6050_SENSOR_ACCL		BIT(0)
> +#define INV_MPU6050_SENSOR_GYRO		BIT(1)
> +#define INV_MPU6050_SENSOR_TEMP		BIT(2)
> +#define INV_MPU6050_SENSOR_MAGN		BIT(3)
> +
>  /**
>   *  struct inv_mpu6050_chip_config - Cached chip configuration data.
> + *  @clk:		selected chip clock
>   *  @fsr:		Full scale range.
>   *  @lpf:		Digital low pass filter frequency.
>   *  @accl_fs:		accel full scale range.
> + *  @accl_en:		accel engine enabled
> + *  @gyro_en:		gyro engine enabled
> + *  @temp_en:		temperature sensor enabled
> + *  @magn_en:		magn engine (i2c master) enabled
>   *  @accl_fifo_enable:	enable accel data output
>   *  @gyro_fifo_enable:	enable gyro data output
>   *  @temp_fifo_enable:	enable temp data output
> @@ -95,9 +106,14 @@ enum inv_devices {
>   *  @divider:		chip sample rate divider (sample rate divider - 1)
>   */
>  struct inv_mpu6050_chip_config {
> +	unsigned int clk:3;
>  	unsigned int fsr:2;
>  	unsigned int lpf:3;
>  	unsigned int accl_fs:2;
> +	unsigned int accl_en:1;
> +	unsigned int gyro_en:1;
> +	unsigned int temp_en:1;
> +	unsigned int magn_en:1;
>  	unsigned int accl_fifo_enable:1;
>  	unsigned int gyro_fifo_enable:1;
>  	unsigned int temp_fifo_enable:1;
> @@ -262,6 +278,7 @@ struct inv_mpu6050_state {
>  #define INV_MPU6050_REG_PWR_MGMT_1          0x6B
>  #define INV_MPU6050_BIT_H_RESET             0x80
>  #define INV_MPU6050_BIT_SLEEP               0x40
> +#define INV_MPU6050_BIT_TEMP_DIS            0x08
>  #define INV_MPU6050_BIT_CLK_MASK            0x7
>  
>  #define INV_MPU6050_REG_PWR_MGMT_2          0x6C
> @@ -292,7 +309,9 @@ struct inv_mpu6050_state {
>  /* delay time in milliseconds */
>  #define INV_MPU6050_POWER_UP_TIME            100
>  #define INV_MPU6050_TEMP_UP_TIME             100
> -#define INV_MPU6050_SENSOR_UP_TIME           30
> +#define INV_MPU6050_ACCEL_UP_TIME            20
> +#define INV_MPU6050_GYRO_UP_TIME             35
> +#define INV_MPU6050_GYRO_DOWN_TIME           150
>  
>  /* delay time in microseconds */
>  #define INV_MPU6050_REG_UP_TIME_MIN          5000
> @@ -417,7 +436,8 @@ enum inv_mpu6050_clock_sel_e {
>  irqreturn_t inv_mpu6050_read_fifo(int irq, void *p);
>  int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev, int irq_type);
>  int inv_reset_fifo(struct iio_dev *indio_dev);
> -int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en, u32 mask);
> +int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en,
> +			      unsigned int mask);
>  int inv_mpu6050_write_reg(struct inv_mpu6050_state *st, int reg, u8 val);
>  int inv_mpu6050_set_power_itg(struct inv_mpu6050_state *st, bool power_on);
>  int inv_mpu_acpi_create_mux_client(struct i2c_client *client);
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_magn.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_magn.c
> index 607104a2631e..3f402fa56d95 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_magn.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_magn.c
> @@ -316,9 +316,9 @@ int inv_mpu_magn_set_orient(struct inv_mpu6050_state *st)
>   *
>   * Returns 0 on success, a negative error code otherwise
>   */
> -int inv_mpu_magn_read(const struct inv_mpu6050_state *st, int axis, int *val)
> +int inv_mpu_magn_read(struct inv_mpu6050_state *st, int axis, int *val)
>  {
> -	unsigned int user_ctrl, status;
> +	unsigned int status;
>  	__be16 data;
>  	uint8_t addr;
>  	unsigned int freq_hz, period_ms;
> @@ -350,16 +350,14 @@ int inv_mpu_magn_read(const struct inv_mpu6050_state *st, int axis, int *val)
>  		freq_hz = INV_MPU_MAGN_FREQ_HZ_MAX;
>  	period_ms = 1000 / freq_hz;
>  
> -	/* start i2c master, wait for xfer, stop */
> -	user_ctrl = st->chip_config.user_ctrl | INV_MPU6050_BIT_I2C_MST_EN;
> -	ret = regmap_write(st->map, st->reg->user_ctrl, user_ctrl);
> +	ret = inv_mpu6050_switch_engine(st, true, INV_MPU6050_SENSOR_MAGN);
>  	if (ret)
>  		return ret;
>  
>  	/* need to wait 2 periods + half-period margin */
>  	msleep(period_ms * 2 + period_ms / 2);
> -	user_ctrl = st->chip_config.user_ctrl;
> -	ret = regmap_write(st->map, st->reg->user_ctrl, user_ctrl);
> +
> +	ret = inv_mpu6050_switch_engine(st, false, INV_MPU6050_SENSOR_MAGN);
>  	if (ret)
>  		return ret;
>  
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_magn.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_magn.h
> index b41bd0578478..f7ad50ca6c1b 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_magn.h
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_magn.h
> @@ -31,6 +31,6 @@ int inv_mpu_magn_set_rate(const struct inv_mpu6050_state *st, int fifo_rate);
>  
>  int inv_mpu_magn_set_orient(struct inv_mpu6050_state *st);
>  
> -int inv_mpu_magn_read(const struct inv_mpu6050_state *st, int axis, int *val);
> +int inv_mpu_magn_read(struct inv_mpu6050_state *st, int axis, int *val);
>  
>  #endif		/* INV_MPU_MAGN_H_ */
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> index 5199fe790c30..cfd7243159f6 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> @@ -5,9 +5,10 @@
>  
>  #include "inv_mpu_iio.h"
>  
> -static void inv_scan_query_mpu6050(struct iio_dev *indio_dev)
> +static unsigned int inv_scan_query_mpu6050(struct iio_dev *indio_dev)
>  {
>  	struct inv_mpu6050_state  *st = iio_priv(indio_dev);
> +	unsigned int mask;
>  
>  	st->chip_config.gyro_fifo_enable =
>  		test_bit(INV_MPU6050_SCAN_GYRO_X,
> @@ -27,17 +28,28 @@ static void inv_scan_query_mpu6050(struct iio_dev *indio_dev)
>  
>  	st->chip_config.temp_fifo_enable =
>  		test_bit(INV_MPU6050_SCAN_TEMP, indio_dev->active_scan_mask);
> +
> +	mask = 0;
> +	if (st->chip_config.gyro_fifo_enable)
> +		mask |= INV_MPU6050_SENSOR_GYRO;
> +	if (st->chip_config.accl_fifo_enable)
> +		mask |= INV_MPU6050_SENSOR_ACCL;
> +	if (st->chip_config.temp_fifo_enable)
> +		mask |= INV_MPU6050_SENSOR_TEMP;
> +
> +	return mask;
>  }
>  
> -static void inv_scan_query_mpu9x50(struct iio_dev *indio_dev)
> +static unsigned int inv_scan_query_mpu9x50(struct iio_dev *indio_dev)
>  {
>  	struct inv_mpu6050_state *st = iio_priv(indio_dev);
> +	unsigned int mask;
>  
> -	inv_scan_query_mpu6050(indio_dev);
> +	mask = inv_scan_query_mpu6050(indio_dev);
>  
>  	/* no magnetometer if i2c auxiliary bus is used */
>  	if (st->magn_disabled)
> -		return;
> +		return mask;
>  
>  	st->chip_config.magn_fifo_enable =
>  		test_bit(INV_MPU9X50_SCAN_MAGN_X,
> @@ -46,9 +58,13 @@ static void inv_scan_query_mpu9x50(struct iio_dev *indio_dev)
>  			 indio_dev->active_scan_mask) ||
>  		test_bit(INV_MPU9X50_SCAN_MAGN_Z,
>  			 indio_dev->active_scan_mask);
> +	if (st->chip_config.magn_fifo_enable)
> +		mask |= INV_MPU6050_SENSOR_MAGN;
> +
> +	return mask;
>  }
>  
> -static void inv_scan_query(struct iio_dev *indio_dev)
> +static unsigned int inv_scan_query(struct iio_dev *indio_dev)
>  {
>  	struct inv_mpu6050_state *st = iio_priv(indio_dev);
>  
> @@ -92,62 +108,40 @@ static unsigned int inv_compute_skip_samples(const struct inv_mpu6050_state *st)
>  static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
>  {
>  	struct inv_mpu6050_state *st = iio_priv(indio_dev);
> -	uint8_t d;
> +	unsigned int scan;
>  	int result;
>  
> +	scan = inv_scan_query(indio_dev);
> +
>  	if (enable) {
>  		result = inv_mpu6050_set_power_itg(st, true);
>  		if (result)
>  			return result;
> -		inv_scan_query(indio_dev);
> -		if (st->chip_config.gyro_fifo_enable) {
> -			result = inv_mpu6050_switch_engine(st, true,
> -					INV_MPU6050_BIT_PWR_GYRO_STBY);
> -			if (result)
> -				goto error_power_off;
> -		}
> -		if (st->chip_config.accl_fifo_enable) {
> -			result = inv_mpu6050_switch_engine(st, true,
> -					INV_MPU6050_BIT_PWR_ACCL_STBY);
> -			if (result)
> -				goto error_gyro_off;
> -		}
> -		if (st->chip_config.magn_fifo_enable) {
> -			d = st->chip_config.user_ctrl |
> -					INV_MPU6050_BIT_I2C_MST_EN;
> -			result = regmap_write(st->map, st->reg->user_ctrl, d);
> -			if (result)
> -				goto error_accl_off;
> -			st->chip_config.user_ctrl = d;
> -		}
> +		result = inv_mpu6050_switch_engine(st, true, scan);
> +		if (result)
> +			goto error_power_off;
>  		st->skip_samples = inv_compute_skip_samples(st);
>  		result = inv_reset_fifo(indio_dev);
>  		if (result)
> -			goto error_magn_off;
> +			goto error_sensors_off;
>  	} else {
>  		result = regmap_write(st->map, st->reg->fifo_en, 0);
>  		if (result)
> -			goto error_magn_off;
> +			goto error_fifo_off;
>  
>  		result = regmap_write(st->map, st->reg->int_enable, 0);
>  		if (result)
> -			goto error_magn_off;
> -
> -		d = st->chip_config.user_ctrl & ~INV_MPU6050_BIT_I2C_MST_EN;
> -		result = regmap_write(st->map, st->reg->user_ctrl, d);
> -		if (result)
> -			goto error_magn_off;
> -		st->chip_config.user_ctrl = d;
> +			goto error_fifo_off;
>  
> -		result = inv_mpu6050_switch_engine(st, false,
> -					INV_MPU6050_BIT_PWR_ACCL_STBY);
> +		/* restore user_ctrl for disabling FIFO reading */
> +		result = regmap_write(st->map, st->reg->user_ctrl,
> +				      st->chip_config.user_ctrl);
>  		if (result)
> -			goto error_accl_off;
> +			goto error_sensors_off;
>  
> -		result = inv_mpu6050_switch_engine(st, false,
> -					INV_MPU6050_BIT_PWR_GYRO_STBY);
> +		result = inv_mpu6050_switch_engine(st, false, scan);
>  		if (result)
> -			goto error_gyro_off;
> +			goto error_power_off;
>  
>  		result = inv_mpu6050_set_power_itg(st, false);
>  		if (result)
> @@ -156,18 +150,11 @@ static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
>  
>  	return 0;
>  
> -error_magn_off:
> +error_fifo_off:
>  	/* always restore user_ctrl to disable fifo properly */
> -	st->chip_config.user_ctrl &= ~INV_MPU6050_BIT_I2C_MST_EN;
>  	regmap_write(st->map, st->reg->user_ctrl, st->chip_config.user_ctrl);
> -error_accl_off:
> -	if (st->chip_config.accl_fifo_enable)
> -		inv_mpu6050_switch_engine(st, false,
> -					  INV_MPU6050_BIT_PWR_ACCL_STBY);
> -error_gyro_off:
> -	if (st->chip_config.gyro_fifo_enable)
> -		inv_mpu6050_switch_engine(st, false,
> -					  INV_MPU6050_BIT_PWR_GYRO_STBY);
> +error_sensors_off:
> +	inv_mpu6050_switch_engine(st, false, scan);
>  error_power_off:
>  	inv_mpu6050_set_power_itg(st, false);
>  	return result;


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 12/13] iio: imu: inv_mpu6050: use runtime pm with autosuspend
  2020-02-12 17:40 ` [PATCH 12/13] iio: imu: inv_mpu6050: use runtime pm with autosuspend Jean-Baptiste Maneyrol
@ 2020-02-15 18:22   ` Jonathan Cameron
  2020-02-18 16:44     ` Jean-Baptiste Maneyrol
  0 siblings, 1 reply; 21+ messages in thread
From: Jonathan Cameron @ 2020-02-15 18:22 UTC (permalink / raw)
  To: Jean-Baptiste Maneyrol; +Cc: linux-iio

On Wed, 12 Feb 2020 18:40:47 +0100
Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote:

> Use runtime power management for handling chip power and
> sensor engines on/off. Simplifies things a lot since pm
> runtime already has reference counter.
> Usage of autosuspend reduces the number of power on/off. This
> makes polling interface now usable to get data at low
> frequency.
> 
> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>

Looks good.  A couple of questions / suggestions inline.

> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c    | 265 ++++++++++++------
>  drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h     |   5 +-
>  drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c |  29 +-
>  3 files changed, 191 insertions(+), 108 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> index f33fd04671cc..f698d2aa61f4 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> @@ -16,6 +16,8 @@
>  #include <linux/acpi.h>
>  #include <linux/platform_device.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/pm.h>
> +#include <linux/pm_runtime.h>
>  #include "inv_mpu_iio.h"
>  #include "inv_mpu_magn.h"
>  
> @@ -400,26 +402,13 @@ int inv_mpu6050_set_power_itg(struct inv_mpu6050_state *st, bool power_on)
>  {
>  	int result;
>  
> -	if (power_on) {
> -		if (!st->powerup_count) {
> -			result = inv_mpu6050_pwr_mgmt_1_write(st, false, -1, -1);
> -			if (result)
> -				return result;
> -			usleep_range(INV_MPU6050_REG_UP_TIME_MIN,
> -				     INV_MPU6050_REG_UP_TIME_MAX);
> -		}
> -		st->powerup_count++;
> -	} else {
> -		if (st->powerup_count == 1) {
> -			result = inv_mpu6050_pwr_mgmt_1_write(st, true, -1, -1);
> -			if (result)
> -				return result;
> -		}
> -		st->powerup_count--;
> -	}
> +	result = inv_mpu6050_pwr_mgmt_1_write(st, !power_on, -1, -1);
> +	if (result)
> +		return result;
>  
> -	dev_dbg(regmap_get_device(st->map), "set power %d, count=%u\n",
> -		power_on, st->powerup_count);
> +	if (power_on)
> +		usleep_range(INV_MPU6050_REG_UP_TIME_MIN,
> +			     INV_MPU6050_REG_UP_TIME_MAX);
>  
>  	return 0;
>  }
> @@ -563,6 +552,7 @@ static int inv_mpu6050_read_channel_data(struct iio_dev *indio_dev,
>  					 int *val)
>  {
>  	struct inv_mpu6050_state *st = iio_priv(indio_dev);
> +	struct device *pdev = regmap_get_device(st->map);
>  	unsigned int freq_hz, period_us, min_sleep_us, max_sleep_us;
>  	int result;
>  	int ret;
> @@ -571,92 +561,85 @@ static int inv_mpu6050_read_channel_data(struct iio_dev *indio_dev,
>  	freq_hz = INV_MPU6050_DIVIDER_TO_FIFO_RATE(st->chip_config.divider);
>  	period_us = 1000000 / freq_hz;
>  
> -	result = inv_mpu6050_set_power_itg(st, true);
> -	if (result)
> +	result = pm_runtime_get_sync(pdev);
> +	if (result < 0) {
> +		pm_runtime_put_noidle(pdev);
>  		return result;
> +	}
>  
>  	switch (chan->type) {
>  	case IIO_ANGL_VEL:
> -		result = inv_mpu6050_switch_engine(st, true,
> -				INV_MPU6050_SENSOR_GYRO);
> -		if (result)
> -			goto error_power_off;
> -		/* need to wait 2 periods to have first valid sample */
> -		min_sleep_us = 2 * period_us;
> -		max_sleep_us = 2 * (period_us + period_us / 2);
> -		usleep_range(min_sleep_us, max_sleep_us);
> +		if (!st->chip_config.gyro_en) {

Are these changes due to runtime pm?  Or do they make sense in general?

> +			result = inv_mpu6050_switch_engine(st, true,
> +					INV_MPU6050_SENSOR_GYRO);
> +			if (result)
> +				goto error_power_off;
> +			/* need to wait 2 periods to have first valid sample */
> +			min_sleep_us = 2 * period_us;
> +			max_sleep_us = 2 * (period_us + period_us / 2);
> +			usleep_range(min_sleep_us, max_sleep_us);
> +		}
>  		ret = inv_mpu6050_sensor_show(st, st->reg->raw_gyro,
>  					      chan->channel2, val);
> -		result = inv_mpu6050_switch_engine(st, false,
> -				INV_MPU6050_SENSOR_GYRO);
> -		if (result)
> -			goto error_power_off;
>  		break;
>  	case IIO_ACCEL:
> -		result = inv_mpu6050_switch_engine(st, true,
> -				INV_MPU6050_SENSOR_ACCL);
> -		if (result)
> -			goto error_power_off;
> -		/* wait 1 period for first sample availability */
> -		min_sleep_us = period_us;
> -		max_sleep_us = period_us + period_us / 2;
> -		usleep_range(min_sleep_us, max_sleep_us);
> +		if (!st->chip_config.accl_en) {
> +			result = inv_mpu6050_switch_engine(st, true,
> +					INV_MPU6050_SENSOR_ACCL);
> +			if (result)
> +				goto error_power_off;
> +			/* wait 1 period for first sample availability */
> +			min_sleep_us = period_us;
> +			max_sleep_us = period_us + period_us / 2;
> +			usleep_range(min_sleep_us, max_sleep_us);
> +		}
>  		ret = inv_mpu6050_sensor_show(st, st->reg->raw_accl,
>  					      chan->channel2, val);
> -		result = inv_mpu6050_switch_engine(st, false,
> -				INV_MPU6050_SENSOR_ACCL);
> -		if (result)
> -			goto error_power_off;
>  		break;
>  	case IIO_TEMP:
> -		result = inv_mpu6050_switch_engine(st, true,
> -				INV_MPU6050_SENSOR_TEMP);
> -		if (result)
> -			goto error_power_off;
> -		/* wait 1 period for first sample availability */
> -		min_sleep_us = period_us;
> -		max_sleep_us = period_us + period_us / 2;
> -		usleep_range(min_sleep_us, max_sleep_us);
> +		if (!st->chip_config.temp_en) {
> +			result = inv_mpu6050_switch_engine(st, true,
> +					INV_MPU6050_SENSOR_TEMP);
> +			if (result)
> +				goto error_power_off;
> +			/* wait 1 period for first sample availability */
> +			min_sleep_us = period_us;
> +			max_sleep_us = period_us + period_us / 2;
> +			usleep_range(min_sleep_us, max_sleep_us);
> +		}
>  		ret = inv_mpu6050_sensor_show(st, st->reg->temperature,
>  					      IIO_MOD_X, val);
> -		result = inv_mpu6050_switch_engine(st, false,
> -				INV_MPU6050_SENSOR_TEMP);
> -		if (result)
> -			goto error_power_off;
>  		break;
>  	case IIO_MAGN:
> -		result = inv_mpu6050_switch_engine(st, true,
> -				INV_MPU6050_SENSOR_MAGN);
> -		if (result)
> -			goto error_power_off;
> -		/* frequency is limited for magnetometer */
> -		if (freq_hz > INV_MPU_MAGN_FREQ_HZ_MAX) {
> -			freq_hz = INV_MPU_MAGN_FREQ_HZ_MAX;
> -			period_us = 1000000 / freq_hz;
> +		if (!st->chip_config.magn_en) {
> +			result = inv_mpu6050_switch_engine(st, true,
> +					INV_MPU6050_SENSOR_MAGN);
> +			if (result)
> +				goto error_power_off;
> +			/* frequency is limited for magnetometer */
> +			if (freq_hz > INV_MPU_MAGN_FREQ_HZ_MAX) {
> +				freq_hz = INV_MPU_MAGN_FREQ_HZ_MAX;
> +				period_us = 1000000 / freq_hz;
> +			}
> +			/* need to wait 2 periods to have first valid sample */
> +			min_sleep_us = 2 * period_us;
> +			max_sleep_us = 2 * (period_us + period_us / 2);
> +			usleep_range(min_sleep_us, max_sleep_us);
>  		}
> -		/* need to wait 2 periods to have first valid sample */
> -		min_sleep_us = 2 * period_us;
> -		max_sleep_us = 2 * (period_us + period_us / 2);
> -		usleep_range(min_sleep_us, max_sleep_us);
>  		ret = inv_mpu_magn_read(st, chan->channel2, val);
> -		result = inv_mpu6050_switch_engine(st, false,
> -				INV_MPU6050_SENSOR_MAGN);
> -		if (result)
> -			goto error_power_off;
>  		break;
>  	default:
>  		ret = -EINVAL;
>  		break;
>  	}
>  
> -	result = inv_mpu6050_set_power_itg(st, false);
> -	if (result)
> -		goto error_power_off;
> +	pm_runtime_mark_last_busy(pdev);
> +	pm_runtime_put_autosuspend(pdev);
>  
>  	return ret;
>  
>  error_power_off:
> -	inv_mpu6050_set_power_itg(st, false);
> +	pm_runtime_put_autosuspend(pdev);
>  	return result;
>  }
>  
> @@ -795,6 +778,7 @@ static int inv_mpu6050_write_raw(struct iio_dev *indio_dev,
>  				 int val, int val2, long mask)
>  {
>  	struct inv_mpu6050_state  *st = iio_priv(indio_dev);
> +	struct device *pdev = regmap_get_device(st->map);
>  	int result;
>  
>  	/*
> @@ -806,9 +790,11 @@ static int inv_mpu6050_write_raw(struct iio_dev *indio_dev,
>  		return result;
>  
>  	mutex_lock(&st->lock);
> -	result = inv_mpu6050_set_power_itg(st, true);
> -	if (result)
> +	result = pm_runtime_get_sync(pdev);
> +	if (result < 0) {
> +		pm_runtime_put_noidle(pdev);
>  		goto error_write_raw_unlock;
> +	}
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SCALE:
> @@ -846,7 +832,8 @@ static int inv_mpu6050_write_raw(struct iio_dev *indio_dev,
>  		break;
>  	}
>  
> -	result |= inv_mpu6050_set_power_itg(st, false);
> +	pm_runtime_mark_last_busy(pdev);
> +	pm_runtime_put_autosuspend(pdev);
>  error_write_raw_unlock:
>  	mutex_unlock(&st->lock);
>  	iio_device_release_direct_mode(indio_dev);
> @@ -903,6 +890,7 @@ inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr,
>  	int result;
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  	struct inv_mpu6050_state *st = iio_priv(indio_dev);
> +	struct device *pdev = regmap_get_device(st->map);
>  
>  	if (kstrtoint(buf, 10, &fifo_rate))
>  		return -EINVAL;
> @@ -920,9 +908,11 @@ inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr,
>  		result = 0;
>  		goto fifo_rate_fail_unlock;
>  	}
> -	result = inv_mpu6050_set_power_itg(st, true);
> -	if (result)
> +	result = pm_runtime_get_sync(pdev);
> +	if (result < 0) {
> +		pm_runtime_put_noidle(pdev);
>  		goto fifo_rate_fail_unlock;
> +	}
>  
>  	result = regmap_write(st->map, st->reg->sample_rate_div, d);
>  	if (result)
> @@ -938,8 +928,9 @@ inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr,
>  	if (result)
>  		goto fifo_rate_fail_power_off;
>  
> +	pm_runtime_mark_last_busy(pdev);
>  fifo_rate_fail_power_off:
> -	result |= inv_mpu6050_set_power_itg(st, false);
> +	pm_runtime_put_autosuspend(pdev);
>  fifo_rate_fail_unlock:
>  	mutex_unlock(&st->lock);
>  	if (result)
> @@ -1385,6 +1376,14 @@ static void inv_mpu_core_disable_regulator_action(void *_data)
>  	inv_mpu_core_disable_regulator_vddio(st);
>  }
>  
> +static void inv_mpu_pm_disable(void *data)
> +{
> +	struct device *dev = data;
> +
> +	pm_runtime_put_sync_suspend(dev);
> +	pm_runtime_disable(dev);
> +}
> +
>  int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
>  		int (*inv_mpu_bus_setup)(struct iio_dev *), int chip_type)
>  {
> @@ -1409,7 +1408,6 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
>  	st = iio_priv(indio_dev);
>  	mutex_init(&st->lock);
>  	st->chip_type = chip_type;
> -	st->powerup_count = 0;
>  	st->irq = irq;
>  	st->map = regmap;
>  
> @@ -1521,8 +1519,16 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
>  			goto error_power_off;
>  	}
>  
> -	/* chip init is done, turning off */
> -	result = inv_mpu6050_set_power_itg(st, false);
> +	/* chip init is done, turning on runtime power management */
> +	result = pm_runtime_set_active(dev);
> +	if (result)
> +		goto error_power_off;
> +	pm_runtime_get_noresume(dev);
> +	pm_runtime_enable(dev);
> +	pm_runtime_set_autosuspend_delay(dev, INV_MPU6050_SUSPEND_DELAY_MS);
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_put(dev);
> +	result = devm_add_action_or_reset(dev, inv_mpu_pm_disable, dev);
>  	if (result)
>  		return result;
>  
> @@ -1594,36 +1600,113 @@ EXPORT_SYMBOL_GPL(inv_mpu_core_probe);
>  
>  static int inv_mpu_resume(struct device *dev)
>  {
> -	struct inv_mpu6050_state *st = iio_priv(dev_get_drvdata(dev));
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct inv_mpu6050_state *st = iio_priv(indio_dev);
>  	int result;
>  
>  	mutex_lock(&st->lock);
> +
>  	result = inv_mpu_core_enable_regulator_vddio(st);
>  	if (result)
>  		goto out_unlock;
>  
>  	result = inv_mpu6050_set_power_itg(st, true);
> +	if (result)
> +		goto out_unlock;
> +
> +	result = inv_mpu6050_switch_engine(st, true, st->suspended_sensors);
> +	if (result)
> +		goto out_unlock;
> +
> +	if (iio_buffer_enabled(indio_dev))
> +		result = inv_mpu6050_prepare_fifo(st, true);
> +
>  out_unlock:
>  	mutex_unlock(&st->lock);
> -

Not keen on random white space tweaks in a complex patch like this.

>  	return result;
>  }
>  
>  static int inv_mpu_suspend(struct device *dev)
>  {
> -	struct inv_mpu6050_state *st = iio_priv(dev_get_drvdata(dev));
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct inv_mpu6050_state *st = iio_priv(indio_dev);
>  	int result;
>  
>  	mutex_lock(&st->lock);
> +
> +	if (iio_buffer_enabled(indio_dev)) {
> +		result = inv_mpu6050_prepare_fifo(st, false);
> +		goto out_unlock;
> +	}
> +
> +	st->suspended_sensors = 0;
> +	if (st->chip_config.accl_en)
> +		st->suspended_sensors |= INV_MPU6050_SENSOR_ACCL;
> +	if (st->chip_config.gyro_en)
> +		st->suspended_sensors |= INV_MPU6050_SENSOR_GYRO;
> +	if (st->chip_config.temp_en)
> +		st->suspended_sensors |= INV_MPU6050_SENSOR_TEMP;
> +	if (st->chip_config.magn_en)
> +		st->suspended_sensors |= INV_MPU6050_SENSOR_MAGN;
> +	result = inv_mpu6050_switch_engine(st, false, st->suspended_sensors);
> +	if (result)
> +		goto out_unlock;
> +
>  	result = inv_mpu6050_set_power_itg(st, false);
> +	if (result)
> +		goto out_unlock;
> +
>  	inv_mpu_core_disable_regulator_vddio(st);
> -	mutex_unlock(&st->lock);
>  
> +out_unlock:
> +	mutex_unlock(&st->lock);
>  	return result;
>  }
>  #endif /* CONFIG_PM_SLEEP */
>  
> -SIMPLE_DEV_PM_OPS(inv_mpu_pmops, inv_mpu_suspend, inv_mpu_resume);
> +#ifdef CONFIG_PM

Preferred to avoid the ifdef fun and mark the functions __maybe_unused
instead.  Avoids weird build issues that tend to happen around the
CONFIG_PM options.  Same for the CONFIG_PM_SLEEP cases above.
Note this preference has changed over the years since this driver
was introduced so entirely reasonable that we might want to update
this now.

> +static int inv_mpu_runtime_suspend(struct device *dev)
> +{
> +	struct inv_mpu6050_state *st = iio_priv(dev_get_drvdata(dev));
> +	unsigned int sensors;
> +	int ret;
> +
> +	mutex_lock(&st->lock);
> +
> +	sensors = INV_MPU6050_SENSOR_ACCL | INV_MPU6050_SENSOR_GYRO |
> +			INV_MPU6050_SENSOR_TEMP | INV_MPU6050_SENSOR_MAGN;
> +	ret = inv_mpu6050_switch_engine(st, false, sensors);
> +	if (ret)
> +		goto out_unlock;
> +
> +	ret = inv_mpu6050_set_power_itg(st, false);
> +	if (ret)
> +		goto out_unlock;
> +
> +	inv_mpu_core_disable_regulator_vddio(st);
> +
> +out_unlock:
> +	mutex_unlock(&st->lock);
> +	return ret;
> +}
> +
> +static int inv_mpu_runtime_resume(struct device *dev)
> +{
> +	struct inv_mpu6050_state *st = iio_priv(dev_get_drvdata(dev));
> +	int ret;
> +
> +	ret = inv_mpu_core_enable_regulator_vddio(st);
> +	if (ret)
> +		return ret;
> +
> +	return inv_mpu6050_set_power_itg(st, true);
> +}
> +#endif
> +
> +const struct dev_pm_ops inv_mpu_pmops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(inv_mpu_suspend, inv_mpu_resume)
> +	SET_RUNTIME_PM_OPS(inv_mpu_runtime_suspend, inv_mpu_runtime_resume, NULL)
> +};
>  EXPORT_SYMBOL_GPL(inv_mpu_pmops);
>  
>  MODULE_AUTHOR("Invensense Corporation");
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> index e328c98e362c..cd38b3fccc7b 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> @@ -164,6 +164,7 @@ struct inv_mpu6050_hw {
>   *  @magn_disabled:     magnetometer disabled for backward compatibility reason.
>   *  @magn_raw_to_gauss:	coefficient to convert mag raw value to Gauss.
>   *  @magn_orient:       magnetometer sensor chip orientation if available.
> + *  @suspended_sensors:	sensors mask of sensors turned off for suspend
>   */
>  struct inv_mpu6050_state {
>  	struct mutex lock;
> @@ -174,7 +175,6 @@ struct inv_mpu6050_state {
>  	enum   inv_devices chip_type;
>  	struct i2c_mux_core *muxc;
>  	struct i2c_client *mux_client;
> -	unsigned int powerup_count;
>  	struct inv_mpu6050_platform_data plat_data;
>  	struct iio_mount_matrix orientation;
>  	struct regmap *map;
> @@ -189,6 +189,7 @@ struct inv_mpu6050_state {
>  	bool magn_disabled;
>  	s32 magn_raw_to_gauss[3];
>  	struct iio_mount_matrix magn_orient;
> +	unsigned int suspended_sensors;
>  };
>  
>  /*register and associated bit definition*/
> @@ -312,6 +313,7 @@ struct inv_mpu6050_state {
>  #define INV_MPU6050_ACCEL_UP_TIME            20
>  #define INV_MPU6050_GYRO_UP_TIME             35
>  #define INV_MPU6050_GYRO_DOWN_TIME           150
> +#define INV_MPU6050_SUSPEND_DELAY_MS         2000
>  
>  /* delay time in microseconds */
>  #define INV_MPU6050_REG_UP_TIME_MIN          5000
> @@ -439,7 +441,6 @@ int inv_mpu6050_prepare_fifo(struct inv_mpu6050_state *st, bool enable);
>  int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en,
>  			      unsigned int mask);
>  int inv_mpu6050_write_reg(struct inv_mpu6050_state *st, int reg, u8 val);
> -int inv_mpu6050_set_power_itg(struct inv_mpu6050_state *st, bool power_on);
>  int inv_mpu_acpi_create_mux_client(struct i2c_client *client);
>  void inv_mpu_acpi_delete_mux_client(struct i2c_client *client);
>  int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> index f53f50d08b9e..9eab39042443 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> @@ -3,6 +3,7 @@
>  * Copyright (C) 2012 Invensense, Inc.
>  */
>  
> +#include <linux/pm_runtime.h>
>  #include "inv_mpu_iio.h"
>  
>  static unsigned int inv_scan_query_mpu6050(struct iio_dev *indio_dev)
> @@ -156,41 +157,39 @@ int inv_mpu6050_prepare_fifo(struct inv_mpu6050_state *st, bool enable)
>  static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
>  {
>  	struct inv_mpu6050_state *st = iio_priv(indio_dev);
> +	struct device *pdev = regmap_get_device(st->map);
>  	unsigned int scan;
>  	int result;
>  
> -	scan = inv_scan_query(indio_dev);
> -
>  	if (enable) {
> -		result = inv_mpu6050_set_power_itg(st, true);
> -		if (result)
> +		scan = inv_scan_query(indio_dev);
> +		result = pm_runtime_get_sync(pdev);
> +		if (result < 0) {
> +			pm_runtime_put_noidle(pdev);
>  			return result;
> +		}
> +		result = inv_mpu6050_switch_engine(st, false, ~scan);
> +		if (result)
> +			goto error_power_off;

I'd like a comment on this block.  I guess this off / on sequence is
now necessary as we were previously relying on the power having been
turned off?

>  		result = inv_mpu6050_switch_engine(st, true, scan);
>  		if (result)
>  			goto error_power_off;
>  		st->skip_samples = inv_compute_skip_samples(st);
>  		result = inv_mpu6050_prepare_fifo(st, true);
>  		if (result)
> -			goto error_sensors_off;
> +			goto error_power_off;
>  	} else {
>  		result = inv_mpu6050_prepare_fifo(st, false);
> -		if (result)
> -			goto error_sensors_off;
> -		result = inv_mpu6050_switch_engine(st, false, scan);
> -		if (result)
> -			goto error_power_off;
> -
> -		result = inv_mpu6050_set_power_itg(st, false);
>  		if (result)
>  			goto error_power_off;
> +		pm_runtime_mark_last_busy(pdev);
> +		pm_runtime_put_autosuspend(pdev);
>  	}
>  
>  	return 0;
>  
> -error_sensors_off:
> -	inv_mpu6050_switch_engine(st, false, scan);
>  error_power_off:
> -	inv_mpu6050_set_power_itg(st, false);
> +	pm_runtime_put_autosuspend(pdev);
>  	return result;
>  }
>  


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 13/13] iio: imu: inv_mpu6050: temperature only work with accel/gyro
  2020-02-12 17:40 ` [PATCH 13/13] iio: imu: inv_mpu6050: temperature only work with accel/gyro Jean-Baptiste Maneyrol
@ 2020-02-15 18:23   ` Jonathan Cameron
  2020-02-18 16:46     ` Jean-Baptiste Maneyrol
  0 siblings, 1 reply; 21+ messages in thread
From: Jonathan Cameron @ 2020-02-15 18:23 UTC (permalink / raw)
  To: Jean-Baptiste Maneyrol; +Cc: linux-iio

On Wed, 12 Feb 2020 18:40:48 +0100
Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote:

> Temperature sensor works correctly only when accel and/or gyro
> is turned on. Prevent polling value if they are not running.
> Anyway it doesn't make sense to use it without sensor engines
> on.
> 
> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> index f698d2aa61f4..3212030cc083 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> @@ -597,6 +597,11 @@ static int inv_mpu6050_read_channel_data(struct iio_dev *indio_dev,
>  					      chan->channel2, val);
>  		break;
>  	case IIO_TEMP:
> +		/* temperature sensor work only with accel and/or gyro */
> +		if (!st->chip_config.accl_en && !st->chip_config.gyro_en) {
> +			result = -ENOTSUPP;

I'd suggest -EBUSY to indicate that it might work sometime in the future.

> +			goto error_power_off;
> +		}
>  		if (!st->chip_config.temp_en) {
>  			result = inv_mpu6050_switch_engine(st, true,
>  					INV_MPU6050_SENSOR_TEMP);


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 01/13] iio: imu: inv_mpu6050: enable i2c aux mux bypass only once
  2020-02-15 17:30   ` Jonathan Cameron
@ 2020-02-18 16:03     ` Jean-Baptiste Maneyrol
  0 siblings, 0 replies; 21+ messages in thread
From: Jean-Baptiste Maneyrol @ 2020-02-18 16:03 UTC (permalink / raw)
  To: Jonathan Cameron; +Cc: linux-iio, Peter Rosin

Hello,

sorry first for the desastrous text formatting.

The main idea behind is to be backward compatible with existing setup. Otherwise using i2c-mux to do nothing is not very useful. But this way we ensure backward compatibility and gain the advantage of not having to do i2c transaction with the mpu chip before every transaction with the auxiliary chip.

JB


From: Jonathan Cameron <jic23@kernel.org>

Sent: Saturday, February 15, 2020 18:30

To: Jean-Baptiste Maneyrol <JManeyrol@invensense.com>

Cc: linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; Peter Rosin <peda@axentia.se>

Subject: Re: [PATCH 01/13] iio: imu: inv_mpu6050: enable i2c aux mux bypass only once

 


 CAUTION: This email originated from outside of the organization. Please make sure the sender is who they say they are and do not click links or open attachments unless you recognize the sender and know the content is safe.



On Wed, 12 Feb 2020 18:40:36 +0100

Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote:



> i2c auxiliary mux is done by analog switches. You do not need to

> set them for every i2c transfer.

> Just set i2c bypass bit at init and do noting in i2c de/select.

> 

> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>

Fair enough.  Given we are making the decision based on DT and that

can't change on reprobing etc so this change makes sense to me I think.



It does leave us making rather odd use of the mux code, so I'd

just like Peter to take a quick look at this before I apply it.



Jonathan



> ---

>  drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c | 71 +++++++++--------------

>  1 file changed, 28 insertions(+), 43 deletions(-)

> 

> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c

> index 1363d3776523..24df880248f2 100644

> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c

> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c

> @@ -20,38 +20,6 @@ static const struct regmap_config inv_mpu_regmap_config = {


>  static int inv_mpu6050_select_bypass(struct i2c_mux_core *muxc, u32 chan_id)

>  {

> -     struct iio_dev *indio_dev = i2c_mux_priv(muxc);

> -     struct inv_mpu6050_state *st = iio_priv(indio_dev);

> -     int ret;

> -

> -     mutex_lock(&st->lock);

> -

> -     ret = inv_mpu6050_set_power_itg(st, true);

> -     if (ret)

> -             goto error_unlock;

> -

> -     ret = regmap_write(st->map, st->reg->int_pin_cfg,

> -                        st->irq_mask | INV_MPU6050_BIT_BYPASS_EN);

> -

> -error_unlock:

> -     mutex_unlock(&st->lock);

> -

> -     return ret;

> -}

> -

> -static int inv_mpu6050_deselect_bypass(struct i2c_mux_core *muxc, u32 chan_id)

> -{

> -     struct iio_dev *indio_dev = i2c_mux_priv(muxc);

> -     struct inv_mpu6050_state *st = iio_priv(indio_dev);

> -

> -     mutex_lock(&st->lock);

> -

> -     /* It doesn't really matter if any of the calls fail */

> -     regmap_write(st->map, st->reg->int_pin_cfg, st->irq_mask);

> -     inv_mpu6050_set_power_itg(st, false);

> -

> -     mutex_unlock(&st->lock);

> -

>        return 0;

>  }


> @@ -79,19 +47,20 @@ static bool inv_mpu_i2c_aux_bus(struct device *dev)

>        }

>  }


> -/*

> - * MPU9xxx magnetometer support requires to disable i2c auxiliary bus support.

> - * To ensure backward compatibility with existing setups, do not disable

> - * i2c auxiliary bus if it used.

> - * Check for i2c-gate node in devicetree and set magnetometer disabled.

> - * Only MPU6500 is supported by ACPI, no need to check.

> - */

> -static int inv_mpu_magn_disable(struct iio_dev *indio_dev)

> +static int inv_mpu_i2c_aux_setup(struct iio_dev *indio_dev)

>  {

>        struct inv_mpu6050_state *st = iio_priv(indio_dev);

>        struct device *dev = indio_dev->dev.parent;

>        struct device_node *mux_node;

> +     int ret;


> +     /*

> +      * MPU9xxx magnetometer support requires to disable i2c auxiliary bus.

> +      * To ensure backward compatibility with existing setups, do not disable

> +      * i2c auxiliary bus if it used.

> +      * Check for i2c-gate node in devicetree and set magnetometer disabled.

> +      * Only MPU6500 is supported by ACPI, no need to check.

> +      */

>        switch (st->chip_type) {

>        case INV_MPU9150:

>        case INV_MPU9250:

> @@ -107,7 +76,24 @@ static int inv_mpu_magn_disable(struct iio_dev *indio_dev)

>                break;

>        }


> +     /* enable i2c bypass when using i2c auxiliary bus */

> +     if (inv_mpu_i2c_aux_bus(dev)) {

> +             ret = inv_mpu6050_set_power_itg(st, true);

> +             if (ret)

> +                     return ret;

> +             ret = regmap_write(st->map, st->reg->int_pin_cfg,

> +                                st->irq_mask | INV_MPU6050_BIT_BYPASS_EN);

> +             if (ret)

> +                     goto error;

> +             ret = inv_mpu6050_set_power_itg(st, false);

> +             if (ret)

> +                     goto error;

> +     }

> +

>        return 0;

> +error:

> +     inv_mpu6050_set_power_itg(st, false);

> +     return ret;

>  }


>  /**

> @@ -151,7 +137,7 @@ static int inv_mpu_probe(struct i2c_client *client,

>        }


>        result = inv_mpu_core_probe(regmap, client->irq, name,

> -                                 inv_mpu_magn_disable, chip_type);

> +                                 inv_mpu_i2c_aux_setup, chip_type);

>        if (result < 0)

>                return result;


> @@ -160,8 +146,7 @@ static int inv_mpu_probe(struct i2c_client *client,

>                /* declare i2c auxiliary bus */

>                st->muxc = i2c_mux_alloc(client->adapter, &client->dev,

>                                         1, 0, I2C_MUX_LOCKED | I2C_MUX_GATE,

> -                                      inv_mpu6050_select_bypass,

> -                                      inv_mpu6050_deselect_bypass);

> +                                      inv_mpu6050_select_bypass, NULL);

>                if (!st->muxc)

>                        return -ENOMEM;

>                st->muxc->priv = dev_get_drvdata(&client->dev);




^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 12/13] iio: imu: inv_mpu6050: use runtime pm with autosuspend
  2020-02-15 18:22   ` Jonathan Cameron
@ 2020-02-18 16:44     ` Jean-Baptiste Maneyrol
  0 siblings, 0 replies; 21+ messages in thread
From: Jean-Baptiste Maneyrol @ 2020-02-18 16:44 UTC (permalink / raw)
  To: Jonathan Cameron; +Cc: linux-iio

Hello,

responses are inline.

Hope text formatting is better.
JB

> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Saturday, February 15, 2020 19:22
> To: Jean-Baptiste Maneyrol <JManeyrol@invensense.com>
> Cc: linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>
> Subject: Re: [PATCH 12/13] iio: imu: inv_mpu6050: use runtime pm with autosuspend
>  
> 
> On Wed, 12 Feb 2020 18:40:47 +0100
> Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote:
> 
> > Use runtime power management for handling chip power and
> > sensor engines on/off. Simplifies things a lot since pm
> > runtime already has reference counter.
> > Usage of autosuspend reduces the number of power on/off. This
> > makes polling interface now usable to get data at low
> > frequency.
> > 
> > Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
> 
> Looks good.  A couple of questions / suggestions inline.
> 
> > ---
> >  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c    | 265 ++++++++++++------
> >  drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h     |   5 +-
> >  drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c |  29 +-
> >  3 files changed, 191 insertions(+), 108 deletions(-)
> > 
> > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> > index f33fd04671cc..f698d2aa61f4 100644
> > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> > @@ -16,6 +16,8 @@
> >  #include <linux/acpi.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/regulator/consumer.h>
> > +#include <linux/pm.h>
> > +#include <linux/pm_runtime.h>
> >  #include "inv_mpu_iio.h"
> >  #include "inv_mpu_magn.h"
> >  
> > @@ -400,26 +402,13 @@ int inv_mpu6050_set_power_itg(struct inv_mpu6050_state *st, bool power_on)
> >  {
> >        int result;
> >  
> > -     if (power_on) {
> > -             if (!st->powerup_count) {
> > -                     result = inv_mpu6050_pwr_mgmt_1_write(st, false, -1, -1);
> > -                     if (result)
> > -                             return result;
> > -                     usleep_range(INV_MPU6050_REG_UP_TIME_MIN,
> > -                                  INV_MPU6050_REG_UP_TIME_MAX);
> > -             }
> > -             st->powerup_count++;
> > -     } else {
> > -             if (st->powerup_count == 1) {
> > -                     result = inv_mpu6050_pwr_mgmt_1_write(st, true, -1, -1);
> > -                     if (result)
> > -                             return result;
> > -             }
> > -             st->powerup_count--;
> > -     }
> > +     result = inv_mpu6050_pwr_mgmt_1_write(st, !power_on, -1, -1);
> > +     if (result)
> > +             return result;
> >  
> > -     dev_dbg(regmap_get_device(st->map), "set power %d, count=%u\n",
> > -             power_on, st->powerup_count);
> > +     if (power_on)
> > +             usleep_range(INV_MPU6050_REG_UP_TIME_MIN,
> > +                          INV_MPU6050_REG_UP_TIME_MAX);
> >  
> >        return 0;
> >  }
> > @@ -563,6 +552,7 @@ static int inv_mpu6050_read_channel_data(struct iio_dev *indio_dev,
> >                                         int *val)
> >  {
> >        struct inv_mpu6050_state *st = iio_priv(indio_dev);
> > +     struct device *pdev = regmap_get_device(st->map);
> >        unsigned int freq_hz, period_us, min_sleep_us, max_sleep_us;
> >        int result;
> >        int ret;
> > @@ -571,92 +561,85 @@ static int inv_mpu6050_read_channel_data(struct iio_dev *indio_dev,
> >        freq_hz = INV_MPU6050_DIVIDER_TO_FIFO_RATE(st->chip_config.divider);
> >        period_us = 1000000 / freq_hz;
> >  
> > -     result = inv_mpu6050_set_power_itg(st, true);
> > -     if (result)
> > +     result = pm_runtime_get_sync(pdev);
> > +     if (result < 0) {
> > +             pm_runtime_put_noidle(pdev);
> >                return result;
> > +     }
> >  
> >        switch (chan->type) {
> >        case IIO_ANGL_VEL:
> > -             result = inv_mpu6050_switch_engine(st, true,
> > -                             INV_MPU6050_SENSOR_GYRO);
> > -             if (result)
> > -                     goto error_power_off;
> > -             /* need to wait 2 periods to have first valid sample */
> > -             min_sleep_us = 2 * period_us;
> > -             max_sleep_us = 2 * (period_us + period_us / 2);
> > -             usleep_range(min_sleep_us, max_sleep_us);
> > +             if (!st->chip_config.gyro_en) {
> 
> Are these changes due to runtime pm?  Or do they make sense in general?
> 
Yes, these changes are required for using efficiently the autosuspend feature
of runtime pm. We want to sample the data without turning the sensor on/off
each time. So if the sensor engine is already turned on, just read the sensor
data register. Without autosuspend it doesn't make sense, since the sensor are
turned off as soon as the data is read.

> > +                     result = inv_mpu6050_switch_engine(st, true,
> > +                                     INV_MPU6050_SENSOR_GYRO);
> > +                     if (result)
> > +                             goto error_power_off;
> > +                     /* need to wait 2 periods to have first valid sample */
> > +                     min_sleep_us = 2 * period_us;
> > +                     max_sleep_us = 2 * (period_us + period_us / 2);
> > +                     usleep_range(min_sleep_us, max_sleep_us);
> > +             }
> >                ret = inv_mpu6050_sensor_show(st, st->reg->raw_gyro,
> >                                              chan->channel2, val);
> > -             result = inv_mpu6050_switch_engine(st, false,
> > -                             INV_MPU6050_SENSOR_GYRO);
> > -             if (result)
> > -                     goto error_power_off;
> >                break;
> >        case IIO_ACCEL:
> > -             result = inv_mpu6050_switch_engine(st, true,
> > -                             INV_MPU6050_SENSOR_ACCL);
> > -             if (result)
> > -                     goto error_power_off;
> > -             /* wait 1 period for first sample availability */
> > -             min_sleep_us = period_us;
> > -             max_sleep_us = period_us + period_us / 2;
> > -             usleep_range(min_sleep_us, max_sleep_us);
> > +             if (!st->chip_config.accl_en) {
> > +                     result = inv_mpu6050_switch_engine(st, true,
> > +                                     INV_MPU6050_SENSOR_ACCL);
> > +                     if (result)
> > +                             goto error_power_off;
> > +                     /* wait 1 period for first sample availability */
> > +                     min_sleep_us = period_us;
> > +                     max_sleep_us = period_us + period_us / 2;
> > +                     usleep_range(min_sleep_us, max_sleep_us);
> > +             }
> >                ret = inv_mpu6050_sensor_show(st, st->reg->raw_accl,
> >                                              chan->channel2, val);
> > -             result = inv_mpu6050_switch_engine(st, false,
> > -                             INV_MPU6050_SENSOR_ACCL);
> > -             if (result)
> > -                     goto error_power_off;
> >                break;
> >        case IIO_TEMP:
> > -             result = inv_mpu6050_switch_engine(st, true,
> > -                             INV_MPU6050_SENSOR_TEMP);
> > -             if (result)
> > -                     goto error_power_off;
> > -             /* wait 1 period for first sample availability */
> > -             min_sleep_us = period_us;
> > -             max_sleep_us = period_us + period_us / 2;
> > -             usleep_range(min_sleep_us, max_sleep_us);
> > +             if (!st->chip_config.temp_en) {
> > +                     result = inv_mpu6050_switch_engine(st, true,
> > +                                     INV_MPU6050_SENSOR_TEMP);
> > +                     if (result)
> > +                             goto error_power_off;
> > +                     /* wait 1 period for first sample availability */
> > +                     min_sleep_us = period_us;
> > +                     max_sleep_us = period_us + period_us / 2;
> > +                     usleep_range(min_sleep_us, max_sleep_us);
> > +             }
> >                ret = inv_mpu6050_sensor_show(st, st->reg->temperature,
> >                                              IIO_MOD_X, val);
> > -             result = inv_mpu6050_switch_engine(st, false,
> > -                             INV_MPU6050_SENSOR_TEMP);
> > -             if (result)
> > -                     goto error_power_off;
> >                break;
> >        case IIO_MAGN:
> > -             result = inv_mpu6050_switch_engine(st, true,
> > -                             INV_MPU6050_SENSOR_MAGN);
> > -             if (result)
> > -                     goto error_power_off;
> > -             /* frequency is limited for magnetometer */
> > -             if (freq_hz > INV_MPU_MAGN_FREQ_HZ_MAX) {
> > -                     freq_hz = INV_MPU_MAGN_FREQ_HZ_MAX;
> > -                     period_us = 1000000 / freq_hz;
> > +             if (!st->chip_config.magn_en) {
> > +                     result = inv_mpu6050_switch_engine(st, true,
> > +                                     INV_MPU6050_SENSOR_MAGN);
> > +                     if (result)
> > +                             goto error_power_off;
> > +                     /* frequency is limited for magnetometer */
> > +                     if (freq_hz > INV_MPU_MAGN_FREQ_HZ_MAX) {
> > +                             freq_hz = INV_MPU_MAGN_FREQ_HZ_MAX;
> > +                             period_us = 1000000 / freq_hz;
> > +                     }
> > +                     /* need to wait 2 periods to have first valid sample */
> > +                     min_sleep_us = 2 * period_us;
> > +                     max_sleep_us = 2 * (period_us + period_us / 2);
> > +                     usleep_range(min_sleep_us, max_sleep_us);
> >                }
> > -             /* need to wait 2 periods to have first valid sample */
> > -             min_sleep_us = 2 * period_us;
> > -             max_sleep_us = 2 * (period_us + period_us / 2);
> > -             usleep_range(min_sleep_us, max_sleep_us);
> >                ret = inv_mpu_magn_read(st, chan->channel2, val);
> > -             result = inv_mpu6050_switch_engine(st, false,
> > -                             INV_MPU6050_SENSOR_MAGN);
> > -             if (result)
> > -                     goto error_power_off;
> >                break;
> >        default:
> >                ret = -EINVAL;
> >                break;
> >        }
> >  
> > -     result = inv_mpu6050_set_power_itg(st, false);
> > -     if (result)
> > -             goto error_power_off;
> > +     pm_runtime_mark_last_busy(pdev);
> > +     pm_runtime_put_autosuspend(pdev);
> >  
> >        return ret;
> >  
> >  error_power_off:
> > -     inv_mpu6050_set_power_itg(st, false);
> > +     pm_runtime_put_autosuspend(pdev);
> >        return result;
> >  }
> >  
> > @@ -795,6 +778,7 @@ static int inv_mpu6050_write_raw(struct iio_dev *indio_dev,
> >                                 int val, int val2, long mask)
> >  {
> >        struct inv_mpu6050_state  *st = iio_priv(indio_dev);
> > +     struct device *pdev = regmap_get_device(st->map);
> >        int result;
> >  
> >        /*
> > @@ -806,9 +790,11 @@ static int inv_mpu6050_write_raw(struct iio_dev *indio_dev,
> >                return result;
> >  
> >        mutex_lock(&st->lock);
> > -     result = inv_mpu6050_set_power_itg(st, true);
> > -     if (result)
> > +     result = pm_runtime_get_sync(pdev);
> > +     if (result < 0) {
> > +             pm_runtime_put_noidle(pdev);
> >                goto error_write_raw_unlock;
> > +     }
> >  
> >        switch (mask) {
> >        case IIO_CHAN_INFO_SCALE:
> > @@ -846,7 +832,8 @@ static int inv_mpu6050_write_raw(struct iio_dev *indio_dev,
> >                break;
> >        }
> >  
> > -     result |= inv_mpu6050_set_power_itg(st, false);
> > +     pm_runtime_mark_last_busy(pdev);
> > +     pm_runtime_put_autosuspend(pdev);
> >  error_write_raw_unlock:
> >        mutex_unlock(&st->lock);
> >        iio_device_release_direct_mode(indio_dev);
> > @@ -903,6 +890,7 @@ inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr,
> >        int result;
> >        struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> >        struct inv_mpu6050_state *st = iio_priv(indio_dev);
> > +     struct device *pdev = regmap_get_device(st->map);
> >  
> >        if (kstrtoint(buf, 10, &fifo_rate))
> >                return -EINVAL;
> > @@ -920,9 +908,11 @@ inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr,
> >                result = 0;
> >                goto fifo_rate_fail_unlock;
> >        }
> > -     result = inv_mpu6050_set_power_itg(st, true);
> > -     if (result)
> > +     result = pm_runtime_get_sync(pdev);
> > +     if (result < 0) {
> > +             pm_runtime_put_noidle(pdev);
> >                goto fifo_rate_fail_unlock;
> > +     }
> >  
> >        result = regmap_write(st->map, st->reg->sample_rate_div, d);
> >        if (result)
> > @@ -938,8 +928,9 @@ inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr,
> >        if (result)
> >                goto fifo_rate_fail_power_off;
> >  
> > +     pm_runtime_mark_last_busy(pdev);
> >  fifo_rate_fail_power_off:
> > -     result |= inv_mpu6050_set_power_itg(st, false);
> > +     pm_runtime_put_autosuspend(pdev);
> >  fifo_rate_fail_unlock:
> >        mutex_unlock(&st->lock);
> >        if (result)
> > @@ -1385,6 +1376,14 @@ static void inv_mpu_core_disable_regulator_action(void *_data)
> >        inv_mpu_core_disable_regulator_vddio(st);
> >  }
> >  
> > +static void inv_mpu_pm_disable(void *data)
> > +{
> > +     struct device *dev = data;
> > +
> > +     pm_runtime_put_sync_suspend(dev);
> > +     pm_runtime_disable(dev);
> > +}
> > +
> >  int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
> >                int (*inv_mpu_bus_setup)(struct iio_dev *), int chip_type)
> >  {
> > @@ -1409,7 +1408,6 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
> >        st = iio_priv(indio_dev);
> >        mutex_init(&st->lock);
> >        st->chip_type = chip_type;
> > -     st->powerup_count = 0;
> >        st->irq = irq;
> >        st->map = regmap;
> >  
> > @@ -1521,8 +1519,16 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
> >                        goto error_power_off;
> >        }
> >  
> > -     /* chip init is done, turning off */
> > -     result = inv_mpu6050_set_power_itg(st, false);
> > +     /* chip init is done, turning on runtime power management */
> > +     result = pm_runtime_set_active(dev);
> > +     if (result)
> > +             goto error_power_off;
> > +     pm_runtime_get_noresume(dev);
> > +     pm_runtime_enable(dev);
> > +     pm_runtime_set_autosuspend_delay(dev, INV_MPU6050_SUSPEND_DELAY_MS);
> > +     pm_runtime_use_autosuspend(dev);
> > +     pm_runtime_put(dev);
> > +     result = devm_add_action_or_reset(dev, inv_mpu_pm_disable, dev);
> >        if (result)
> >                return result;
> >  
> > @@ -1594,36 +1600,113 @@ EXPORT_SYMBOL_GPL(inv_mpu_core_probe);
> >  
> >  static int inv_mpu_resume(struct device *dev)
> >  {
> > -     struct inv_mpu6050_state *st = iio_priv(dev_get_drvdata(dev));
> > +     struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > +     struct inv_mpu6050_state *st = iio_priv(indio_dev);
> >        int result;
> >  
> >        mutex_lock(&st->lock);
> > +
> >        result = inv_mpu_core_enable_regulator_vddio(st);
> >        if (result)
> >                goto out_unlock;
> >  
> >        result = inv_mpu6050_set_power_itg(st, true);
> > +     if (result)
> > +             goto out_unlock;
> > +
> > +     result = inv_mpu6050_switch_engine(st, true, st->suspended_sensors);
> > +     if (result)
> > +             goto out_unlock;
> > +
> > +     if (iio_buffer_enabled(indio_dev))
> > +             result = inv_mpu6050_prepare_fifo(st, true);
> > +
> >  out_unlock:
> >        mutex_unlock(&st->lock);
> > -
> 
> Not keen on random white space tweaks in a complex patch like this.
> 
Oups, will be deleted in v2.

> >        return result;
> >  }
> >  
> >  static int inv_mpu_suspend(struct device *dev)
> >  {
> > -     struct inv_mpu6050_state *st = iio_priv(dev_get_drvdata(dev));
> > +     struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > +     struct inv_mpu6050_state *st = iio_priv(indio_dev);
> >        int result;
> >  
> >        mutex_lock(&st->lock);
> > +
> > +     if (iio_buffer_enabled(indio_dev)) {
> > +             result = inv_mpu6050_prepare_fifo(st, false);
> > +             goto out_unlock;
> > +     }
> > +
> > +     st->suspended_sensors = 0;
> > +     if (st->chip_config.accl_en)
> > +             st->suspended_sensors |= INV_MPU6050_SENSOR_ACCL;
> > +     if (st->chip_config.gyro_en)
> > +             st->suspended_sensors |= INV_MPU6050_SENSOR_GYRO;
> > +     if (st->chip_config.temp_en)
> > +             st->suspended_sensors |= INV_MPU6050_SENSOR_TEMP;
> > +     if (st->chip_config.magn_en)
> > +             st->suspended_sensors |= INV_MPU6050_SENSOR_MAGN;
> > +     result = inv_mpu6050_switch_engine(st, false, st->suspended_sensors);
> > +     if (result)
> > +             goto out_unlock;
> > +
> >        result = inv_mpu6050_set_power_itg(st, false);
> > +     if (result)
> > +             goto out_unlock;
> > +
> >        inv_mpu_core_disable_regulator_vddio(st);
> > -     mutex_unlock(&st->lock);
> >  
> > +out_unlock:
> > +     mutex_unlock(&st->lock);
> >        return result;
> >  }
> >  #endif /* CONFIG_PM_SLEEP */
> >  
> > -SIMPLE_DEV_PM_OPS(inv_mpu_pmops, inv_mpu_suspend, inv_mpu_resume);
> > +#ifdef CONFIG_PM
> 
> Preferred to avoid the ifdef fun and mark the functions __maybe_unused
> instead.  Avoids weird build issues that tend to happen around the
> CONFIG_PM options.  Same for the CONFIG_PM_SLEEP cases above.
> Note this preference has changed over the years since this driver
> was introduced so entirely reasonable that we might want to update
> this now.
> 
No problem. Will be fixed in v2.

> > +static int inv_mpu_runtime_suspend(struct device *dev)
> > +{
> > +     struct inv_mpu6050_state *st = iio_priv(dev_get_drvdata(dev));
> > +     unsigned int sensors;
> > +     int ret;
> > +
> > +     mutex_lock(&st->lock);
> > +
> > +     sensors = INV_MPU6050_SENSOR_ACCL | INV_MPU6050_SENSOR_GYRO |
> > +                     INV_MPU6050_SENSOR_TEMP | INV_MPU6050_SENSOR_MAGN;
> > +     ret = inv_mpu6050_switch_engine(st, false, sensors);
> > +     if (ret)
> > +             goto out_unlock;
> > +
> > +     ret = inv_mpu6050_set_power_itg(st, false);
> > +     if (ret)
> > +             goto out_unlock;
> > +
> > +     inv_mpu_core_disable_regulator_vddio(st);
> > +
> > +out_unlock:
> > +     mutex_unlock(&st->lock);
> > +     return ret;
> > +}
> > +
> > +static int inv_mpu_runtime_resume(struct device *dev)
> > +{
> > +     struct inv_mpu6050_state *st = iio_priv(dev_get_drvdata(dev));
> > +     int ret;
> > +
> > +     ret = inv_mpu_core_enable_regulator_vddio(st);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return inv_mpu6050_set_power_itg(st, true);
> > +}
> > +#endif
> > +
> > +const struct dev_pm_ops inv_mpu_pmops = {
> > +     SET_SYSTEM_SLEEP_PM_OPS(inv_mpu_suspend, inv_mpu_resume)
> > +     SET_RUNTIME_PM_OPS(inv_mpu_runtime_suspend, inv_mpu_runtime_resume, NULL)
> > +};
> >  EXPORT_SYMBOL_GPL(inv_mpu_pmops);
> >  
> >  MODULE_AUTHOR("Invensense Corporation");
> > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> > index e328c98e362c..cd38b3fccc7b 100644
> > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> > @@ -164,6 +164,7 @@ struct inv_mpu6050_hw {
> >   *  @magn_disabled:     magnetometer disabled for backward compatibility reason.
> >   *  @magn_raw_to_gauss:      coefficient to convert mag raw value to Gauss.
> >   *  @magn_orient:       magnetometer sensor chip orientation if available.
> > + *  @suspended_sensors:      sensors mask of sensors turned off for suspend
> >   */
> >  struct inv_mpu6050_state {
> >        struct mutex lock;
> > @@ -174,7 +175,6 @@ struct inv_mpu6050_state {
> >        enum   inv_devices chip_type;
> >        struct i2c_mux_core *muxc;
> >        struct i2c_client *mux_client;
> > -     unsigned int powerup_count;
> >        struct inv_mpu6050_platform_data plat_data;
> >        struct iio_mount_matrix orientation;
> >        struct regmap *map;
> > @@ -189,6 +189,7 @@ struct inv_mpu6050_state {
> >        bool magn_disabled;
> >        s32 magn_raw_to_gauss[3];
> >        struct iio_mount_matrix magn_orient;
> > +     unsigned int suspended_sensors;
> >  };
> >  
> >  /*register and associated bit definition*/
> > @@ -312,6 +313,7 @@ struct inv_mpu6050_state {
> >  #define INV_MPU6050_ACCEL_UP_TIME            20
> >  #define INV_MPU6050_GYRO_UP_TIME             35
> >  #define INV_MPU6050_GYRO_DOWN_TIME           150
> > +#define INV_MPU6050_SUSPEND_DELAY_MS         2000
> >  
> >  /* delay time in microseconds */
> >  #define INV_MPU6050_REG_UP_TIME_MIN          5000
> > @@ -439,7 +441,6 @@ int inv_mpu6050_prepare_fifo(struct inv_mpu6050_state *st, bool enable);
> >  int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en,
> >                              unsigned int mask);
> >  int inv_mpu6050_write_reg(struct inv_mpu6050_state *st, int reg, u8 val);
> > -int inv_mpu6050_set_power_itg(struct inv_mpu6050_state *st, bool power_on);
> >  int inv_mpu_acpi_create_mux_client(struct i2c_client *client);
> >  void inv_mpu_acpi_delete_mux_client(struct i2c_client *client);
> >  int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
> > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> > index f53f50d08b9e..9eab39042443 100644
> > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> > @@ -3,6 +3,7 @@
> >  * Copyright (C) 2012 Invensense, Inc.
> >  */
> >  
> > +#include <linux/pm_runtime.h>
> >  #include "inv_mpu_iio.h"
> >  
> >  static unsigned int inv_scan_query_mpu6050(struct iio_dev *indio_dev)
> > @@ -156,41 +157,39 @@ int inv_mpu6050_prepare_fifo(struct inv_mpu6050_state *st, bool enable)
> >  static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
> >  {
> >        struct inv_mpu6050_state *st = iio_priv(indio_dev);
> > +     struct device *pdev = regmap_get_device(st->map);
> >        unsigned int scan;
> >        int result;
> >  
> > -     scan = inv_scan_query(indio_dev);
> > -
> >        if (enable) {
> > -             result = inv_mpu6050_set_power_itg(st, true);
> > -             if (result)
> > +             scan = inv_scan_query(indio_dev);
> > +             result = pm_runtime_get_sync(pdev);
> > +             if (result < 0) {
> > +                     pm_runtime_put_noidle(pdev);
> >                        return result;
> > +             }
> > +             result = inv_mpu6050_switch_engine(st, false, ~scan);
> > +             if (result)
> > +                     goto error_power_off;
> 
> I'd like a comment on this block.  I guess this off / on sequence is
> now necessary as we were previously relying on the power having been
> turned off?
> 
Good guess. We need now to turn off sensor that we don't need in case
autosuspend didn't trigger. Will add a comment in v2.

> >                result = inv_mpu6050_switch_engine(st, true, scan);
> >                if (result)
> >                        goto error_power_off;
> >                st->skip_samples = inv_compute_skip_samples(st);
> >                result = inv_mpu6050_prepare_fifo(st, true);
> >                if (result)
> > -                     goto error_sensors_off;
> > +                     goto error_power_off;
> >        } else {
> >                result = inv_mpu6050_prepare_fifo(st, false);
> > -             if (result)
> > -                     goto error_sensors_off;
> > -             result = inv_mpu6050_switch_engine(st, false, scan);
> > -             if (result)
> > -                     goto error_power_off;
> > -
> > -             result = inv_mpu6050_set_power_itg(st, false);
> >                if (result)
> >                        goto error_power_off;
> > +             pm_runtime_mark_last_busy(pdev);
> > +             pm_runtime_put_autosuspend(pdev);
> >        }
> >  
> >        return 0;
> >  
> > -error_sensors_off:
> > -     inv_mpu6050_switch_engine(st, false, scan);
> >  error_power_off:
> > -     inv_mpu6050_set_power_itg(st, false);
> > +     pm_runtime_put_autosuspend(pdev);
> >        return result;
> >  }
> >  

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 13/13] iio: imu: inv_mpu6050: temperature only work with accel/gyro
  2020-02-15 18:23   ` Jonathan Cameron
@ 2020-02-18 16:46     ` Jean-Baptiste Maneyrol
  0 siblings, 0 replies; 21+ messages in thread
From: Jean-Baptiste Maneyrol @ 2020-02-18 16:46 UTC (permalink / raw)
  To: Jonathan Cameron; +Cc: linux-iio

Hello,

OK it makes sense. I will use -EBUSY in v2

JB


From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org> on behalf of Jonathan Cameron <jic23@kernel.org>

Sent: Saturday, February 15, 2020 19:23

To: Jean-Baptiste Maneyrol <JManeyrol@invensense.com>

Cc: linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>

Subject: Re: [PATCH 13/13] iio: imu: inv_mpu6050: temperature only work with accel/gyro

 


 CAUTION: This email originated from outside of the organization. Please make sure the sender is who they say they are and do not click links or open attachments unless you recognize the sender and know the content is safe.



On Wed, 12 Feb 2020 18:40:48 +0100

Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote:



> Temperature sensor works correctly only when accel and/or gyro

> is turned on. Prevent polling value if they are not running.

> Anyway it doesn't make sense to use it without sensor engines

> on.

> 

> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>

> ---

>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 5 +++++

>  1 file changed, 5 insertions(+)

> 

> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c

> index f698d2aa61f4..3212030cc083 100644

> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c

> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c

> @@ -597,6 +597,11 @@ static int inv_mpu6050_read_channel_data(struct iio_dev *indio_dev,

>                                              chan->channel2, val);

>                break;

>        case IIO_TEMP:

> +             /* temperature sensor work only with accel and/or gyro */

> +             if (!st->chip_config.accl_en && !st->chip_config.gyro_en) {

> +                     result = -ENOTSUPP;



I'd suggest -EBUSY to indicate that it might work sometime in the future.



> +                     goto error_power_off;

> +             }

>                if (!st->chip_config.temp_en) {

>                        result = inv_mpu6050_switch_engine(st, true,

>                                        INV_MPU6050_SENSOR_TEMP);




^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, back to index

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-12 17:40 [PATCH 00/13] Rework sensors engines and power management Jean-Baptiste Maneyrol
2020-02-12 17:40 ` [PATCH 01/13] iio: imu: inv_mpu6050: enable i2c aux mux bypass only once Jean-Baptiste Maneyrol
2020-02-15 17:30   ` Jonathan Cameron
2020-02-18 16:03     ` Jean-Baptiste Maneyrol
2020-02-12 17:40 ` [PATCH 02/13] iio: imu: inv_mpu6050: delete useless check Jean-Baptiste Maneyrol
2020-02-12 17:40 ` [PATCH 03/13] iio: imu: inv_mpu6050: set power on/off only once during all init Jean-Baptiste Maneyrol
2020-02-12 17:40 ` [PATCH 04/13] iio: imu: inv_mpu6050: simplify polling magnetometer Jean-Baptiste Maneyrol
2020-02-12 17:40 ` [PATCH 05/13] iio: imu: inv_mpu6050: early init of chip_config for use at setup Jean-Baptiste Maneyrol
2020-02-12 17:40 ` [PATCH 06/13] iio: imu: inv_mpu6050: add all signal path resets at init Jean-Baptiste Maneyrol
2020-02-12 17:40 ` [PATCH 07/13] iio: imu: inv_mpu6050: fix sleep time when turning regulators on Jean-Baptiste Maneyrol
2020-02-12 17:40 ` [PATCH 08/13] iio: imu: inv_mpu6050: rewrite power and engine management Jean-Baptiste Maneyrol
2020-02-15 18:09   ` Jonathan Cameron
2020-02-12 17:40 ` [PATCH 09/13] iio: imu: inv_mpu6050: fix data polling interface Jean-Baptiste Maneyrol
2020-02-12 17:40 ` [PATCH 10/13] iio: imu: inv_mpu6050: factorize fifo enable/disable Jean-Baptiste Maneyrol
2020-02-12 17:40 ` [PATCH 11/13] iio: imu: inv_mpu6050: dynamic sampling rate change Jean-Baptiste Maneyrol
2020-02-12 17:40 ` [PATCH 12/13] iio: imu: inv_mpu6050: use runtime pm with autosuspend Jean-Baptiste Maneyrol
2020-02-15 18:22   ` Jonathan Cameron
2020-02-18 16:44     ` Jean-Baptiste Maneyrol
2020-02-12 17:40 ` [PATCH 13/13] iio: imu: inv_mpu6050: temperature only work with accel/gyro Jean-Baptiste Maneyrol
2020-02-15 18:23   ` Jonathan Cameron
2020-02-18 16:46     ` Jean-Baptiste Maneyrol

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