[PATCH 1/4] iio: imu: inv_mpu6050: use i2c mux only for chip with i2c aux bus

[PATCH 1/4] iio: imu: inv_mpu6050: use i2c mux only for chip with i2c aux bus

[Jean-Baptiste Maneyrol]

icm20608 does not have i2c aux bus, so it does not make sense to
allocate an i2c mux for this chip.

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

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
index 90fdc5e..f11ae29 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
@@ -125,20 +125,27 @@ static int inv_mpu_probe(struct i2c_client *client,
 		return result;
 
 	st = iio_priv(dev_get_drvdata(&client->dev));
-	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);
-	if (!st->muxc)
-		return -ENOMEM;
-	st->muxc->priv = dev_get_drvdata(&client->dev);
-	result = i2c_mux_add_adapter(st->muxc, 0, 0, 0);
-	if (result)
-		return result;
-
-	result = inv_mpu_acpi_create_mux_client(client);
-	if (result)
-		goto out_del_mux;
+	switch (st->chip_type) {
+	case INV_ICM20608:
+		/* no i2c auxiliary bus on the chip */
+		break;
+	default:
+		/* 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);
+		if (!st->muxc)
+			return -ENOMEM;
+		st->muxc->priv = dev_get_drvdata(&client->dev);
+		result = i2c_mux_add_adapter(st->muxc, 0, 0, 0);
+		if (result)
+			return result;
+		result = inv_mpu_acpi_create_mux_client(client);
+		if (result)
+			goto out_del_mux;
+		break;
+	}
 
 	return 0;
 
@@ -152,8 +159,10 @@ static int inv_mpu_remove(struct i2c_client *client)
 	struct iio_dev *indio_dev = i2c_get_clientdata(client);
 	struct inv_mpu6050_state *st = iio_priv(indio_dev);
 
-	inv_mpu_acpi_delete_mux_client(client);
-	i2c_mux_del_adapters(st->muxc);
+	if (st->muxc) {
+		inv_mpu_acpi_delete_mux_client(client);
+		i2c_mux_del_adapters(st->muxc);
+	}
 
 	return 0;
 }
-- 
2.7.4

[PATCH 2/4] iio: imu: inv_mpu6050: fix possible deadlock between mutex and iio

[Jean-Baptiste Maneyrol]

Detected by kernel circular locking dependency checker.

We are locking iio mutex (iio_device_claim_direct_mode) after
locking our internal mutex. But when the buffer starts, iio first
locks its mutex and then we lock our internal one.

To avoid possible deadlock, we need to use the same order
everwhere. So we change the ordering by locking first iio mutex,
then our internal mutex.

Fixes: 68cd6e5b206b ("iio: imu: inv_mpu6050: fix lock issues by using our own mutex")
Cc: stable@vger.kernel.org
Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 34 ++++++++++++++----------------
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index aafa777..7358935 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -340,12 +340,9 @@ static int inv_mpu6050_read_channel_data(struct iio_dev *indio_dev,
 	int result;
 	int ret;
 
-	result = iio_device_claim_direct_mode(indio_dev);
-	if (result)
-		return result;
 	result = inv_mpu6050_set_power_itg(st, true);
 	if (result)
-		goto error_release;
+		return result;
 
 	switch (chan->type) {
 	case IIO_ANGL_VEL:
@@ -386,14 +383,11 @@ static int inv_mpu6050_read_channel_data(struct iio_dev *indio_dev,
 	result = inv_mpu6050_set_power_itg(st, false);
 	if (result)
 		goto error_power_off;
-	iio_device_release_direct_mode(indio_dev);
 
 	return ret;
 
 error_power_off:
 	inv_mpu6050_set_power_itg(st, false);
-error_release:
-	iio_device_release_direct_mode(indio_dev);
 	return result;
 }
 
@@ -407,9 +401,13 @@ inv_mpu6050_read_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
 		mutex_lock(&st->lock);
 		ret = inv_mpu6050_read_channel_data(indio_dev, chan, val);
 		mutex_unlock(&st->lock);
+		iio_device_release_direct_mode(indio_dev);
 		return ret;
 	case IIO_CHAN_INFO_SCALE:
 		switch (chan->type) {
@@ -532,17 +530,18 @@ static int inv_mpu6050_write_raw(struct iio_dev *indio_dev,
 	struct inv_mpu6050_state  *st = iio_priv(indio_dev);
 	int result;
 
-	mutex_lock(&st->lock);
 	/*
 	 * we should only update scale when the chip is disabled, i.e.
 	 * not running
 	 */
 	result = iio_device_claim_direct_mode(indio_dev);
 	if (result)
-		goto error_write_raw_unlock;
+		return result;
+
+	mutex_lock(&st->lock);
 	result = inv_mpu6050_set_power_itg(st, true);
 	if (result)
-		goto error_write_raw_release;
+		goto error_write_raw_unlock;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_SCALE:
@@ -581,10 +580,9 @@ static int inv_mpu6050_write_raw(struct iio_dev *indio_dev,
 	}
 
 	result |= inv_mpu6050_set_power_itg(st, false);
-error_write_raw_release:
-	iio_device_release_direct_mode(indio_dev);
 error_write_raw_unlock:
 	mutex_unlock(&st->lock);
+	iio_device_release_direct_mode(indio_dev);
 
 	return result;
 }
@@ -643,17 +641,18 @@ 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;
+
 	mutex_lock(&st->lock);
 	if (fifo_rate == st->chip_config.fifo_rate) {
 		result = 0;
 		goto fifo_rate_fail_unlock;
 	}
-	result = iio_device_claim_direct_mode(indio_dev);
-	if (result)
-		goto fifo_rate_fail_unlock;
 	result = inv_mpu6050_set_power_itg(st, true);
 	if (result)
-		goto fifo_rate_fail_release;
+		goto fifo_rate_fail_unlock;
 
 	d = INV_MPU6050_ONE_K_HZ / fifo_rate - 1;
 	result = regmap_write(st->map, st->reg->sample_rate_div, d);
@@ -667,10 +666,9 @@ inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr,
 
 fifo_rate_fail_power_off:
 	result |= inv_mpu6050_set_power_itg(st, false);
-fifo_rate_fail_release:
-	iio_device_release_direct_mode(indio_dev);
 fifo_rate_fail_unlock:
 	mutex_unlock(&st->lock);
+	iio_device_release_direct_mode(indio_dev);
 	if (result)
 		return result;
 
-- 
2.7.4

[PATCH 3/4] iio: imu: inv_mpu6050: skip first sample when gyro is on

[Jean-Baptiste Maneyrol]

Implement generic skip first samples mechanism and use it to
filter out first sample when gyro is on.

The problem for these chips is that the first sample of the gyro
is out of specs, because gyro is not completely stabilized. To
ensure all data are within sensor specs, we just skip the first
sample when turning gyro on.

Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h     | 1 +
 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c    | 7 ++++++-
 drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c | 3 +++
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
index 142a835..dfb9e4e 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
@@ -148,6 +148,7 @@ struct inv_mpu6050_state {
 	struct regmap *map;
 	int irq;
 	u8 irq_mask;
+	unsigned skip_samples;
 };
 
 /*register and associated bit definition*/
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
index e51404f..1b57354 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
@@ -185,7 +185,12 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
 		if (result == 0)
 			timestamp = 0;
 
-		iio_push_to_buffers_with_timestamp(indio_dev, data, timestamp);
+		/* skip first samples if needed */
+		if (st->skip_samples)
+			st->skip_samples--;
+		else
+			iio_push_to_buffers_with_timestamp(indio_dev, data,
+							   timestamp);
 
 		fifo_count -= bytes_per_datum;
 	}
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
index 8a9f869..0d7db27 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
@@ -49,11 +49,14 @@ static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
 		if (result)
 			return result;
 		inv_scan_query(indio_dev);
+		st->skip_samples = 0;
 		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;
+			/* gyro first sample is out of specs, skip it */
+			st->skip_samples = 1;
 		}
 		if (st->chip_config.accl_fifo_enable) {
 			result = inv_mpu6050_switch_engine(st, true,
-- 
2.7.4

[PATCH 4/4] iio: imu: inv_mpu6050: fix user_ctrl register overwritten

[Jean-Baptiste Maneyrol]

When in spi mode, we are setting i2c disable bit in user_ctrl
register. But the register is overwritten after when turning fifo
on. So save user_ctrl init value and always use it when updating
the register.

Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c    |  7 ++++---
 drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h     |  1 +
 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c    | 13 +++++++------
 drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c     |  5 +++--
 drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c |  3 ++-
 5 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index 7358935..50c33e2 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -88,6 +88,7 @@ static const struct inv_mpu6050_chip_config chip_config_6050 = {
 	.gyro_fifo_enable = false,
 	.accl_fifo_enable = false,
 	.accl_fs = INV_MPU6050_FS_02G,
+	.user_ctrl = 0,
 };
 
 /* Indexed by enum inv_devices */
@@ -972,15 +973,15 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
 	if (result)
 		return result;
 
-	if (inv_mpu_bus_setup)
-		inv_mpu_bus_setup(indio_dev);
-
 	result = inv_mpu6050_init_config(indio_dev);
 	if (result) {
 		dev_err(dev, "Could not initialize device.\n");
 		return result;
 	}
 
+	if (inv_mpu_bus_setup)
+		inv_mpu_bus_setup(indio_dev);
+
 	dev_set_drvdata(dev, indio_dev);
 	indio_dev->dev.parent = dev;
 	/* name will be NULL when enumerated via ACPI */
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
index dfb9e4e..c54da77 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
@@ -97,6 +97,7 @@ struct inv_mpu6050_chip_config {
 	unsigned int accl_fifo_enable:1;
 	unsigned int gyro_fifo_enable:1;
 	u16 fifo_rate;
+	u8 user_ctrl;
 };
 
 /**
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
index 1b57354..e7b23e3 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
@@ -51,13 +51,14 @@ int inv_reset_fifo(struct iio_dev *indio_dev)
 	if (result)
 		goto reset_fifo_fail;
 	/* disable fifo reading */
-	result = regmap_write(st->map, st->reg->user_ctrl, 0);
+	result = regmap_write(st->map, st->reg->user_ctrl,
+			      st->chip_config.user_ctrl);
 	if (result)
 		goto reset_fifo_fail;
 
 	/* reset FIFO*/
-	result = regmap_write(st->map, st->reg->user_ctrl,
-			      INV_MPU6050_BIT_FIFO_RST);
+	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;
 
@@ -72,9 +73,9 @@ int inv_reset_fifo(struct iio_dev *indio_dev)
 		if (result)
 			return result;
 	}
-	/* enable FIFO reading and I2C master interface*/
-	result = regmap_write(st->map, st->reg->user_ctrl,
-			      INV_MPU6050_BIT_FIFO_EN);
+	/* 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 */
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c
index fe0bf5a..227f50a 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c
@@ -31,8 +31,9 @@ static int inv_mpu_i2c_disable(struct iio_dev *indio_dev)
 	if (ret)
 		return ret;
 
-	ret = regmap_write(st->map, INV_MPU6050_REG_USER_CTRL,
-			   INV_MPU6050_BIT_I2C_IF_DIS);
+	st->chip_config.user_ctrl |= INV_MPU6050_BIT_I2C_IF_DIS;
+	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;
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
index 0d7db27..d3e26ae 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
@@ -76,7 +76,8 @@ static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
 		if (result)
 			goto error_accl_off;
 
-		result = regmap_write(st->map, st->reg->user_ctrl, 0);
+		result = regmap_write(st->map, st->reg->user_ctrl,
+				      st->chip_config.user_ctrl);
 		if (result)
 			goto error_accl_off;
 
-- 
2.7.4

Re: [PATCH 1/4] iio: imu: inv_mpu6050: use i2c mux only for chip with i2c aux bus

[Jonathan Cameron]

On Mon, 30 Apr 2018 12:14:08 +0200
Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote:

> icm20608 does not have i2c aux bus, so it does not make sense to
> allocate an i2c mux for this chip.
> 
> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
Applied to the togreg branch of iio.git and pushed out as
testing for the autobuilders to play with it.

Thanks,

Jonathan

> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c | 41 +++++++++++++++++++------------
>  1 file changed, 25 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> index 90fdc5e..f11ae29 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> @@ -125,20 +125,27 @@ static int inv_mpu_probe(struct i2c_client *client,
>  		return result;
>  
>  	st = iio_priv(dev_get_drvdata(&client->dev));
> -	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);
> -	if (!st->muxc)
> -		return -ENOMEM;
> -	st->muxc->priv = dev_get_drvdata(&client->dev);
> -	result = i2c_mux_add_adapter(st->muxc, 0, 0, 0);
> -	if (result)
> -		return result;
> -
> -	result = inv_mpu_acpi_create_mux_client(client);
> -	if (result)
> -		goto out_del_mux;
> +	switch (st->chip_type) {
> +	case INV_ICM20608:
> +		/* no i2c auxiliary bus on the chip */
> +		break;
> +	default:
> +		/* 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);
> +		if (!st->muxc)
> +			return -ENOMEM;
> +		st->muxc->priv = dev_get_drvdata(&client->dev);
> +		result = i2c_mux_add_adapter(st->muxc, 0, 0, 0);
> +		if (result)
> +			return result;
> +		result = inv_mpu_acpi_create_mux_client(client);
> +		if (result)
> +			goto out_del_mux;
> +		break;
> +	}
>  
>  	return 0;
>  
> @@ -152,8 +159,10 @@ static int inv_mpu_remove(struct i2c_client *client)
>  	struct iio_dev *indio_dev = i2c_get_clientdata(client);
>  	struct inv_mpu6050_state *st = iio_priv(indio_dev);
>  
> -	inv_mpu_acpi_delete_mux_client(client);
> -	i2c_mux_del_adapters(st->muxc);
> +	if (st->muxc) {
> +		inv_mpu_acpi_delete_mux_client(client);
> +		i2c_mux_del_adapters(st->muxc);
> +	}
>  
>  	return 0;
>  }

Re: [PATCH 2/4] iio: imu: inv_mpu6050: fix possible deadlock between mutex and iio

[Jonathan Cameron]

On Mon, 30 Apr 2018 12:14:09 +0200
Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote:

> Detected by kernel circular locking dependency checker.
> 
> We are locking iio mutex (iio_device_claim_direct_mode) after
> locking our internal mutex. But when the buffer starts, iio first
> locks its mutex and then we lock our internal one.
> 
> To avoid possible deadlock, we need to use the same order
> everwhere. So we change the ordering by locking first iio mutex,
> then our internal mutex.
> 
> Fixes: 68cd6e5b206b ("iio: imu: inv_mpu6050: fix lock issues by using our own mutex")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
Hmm.  There has been too much going on with this driver in this cycle
for this to cleanly apply to the fixes-togreg branch.
So it'll just have to wait for the next merge window and then we can
look at back porting.

Applied to the togreg branch of iio.git and pushed out as testing for the
autobuilders to play with it.

Good find.

Thanks,

Jonathan

> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 34 ++++++++++++++----------------
>  1 file changed, 16 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> index aafa777..7358935 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> @@ -340,12 +340,9 @@ static int inv_mpu6050_read_channel_data(struct iio_dev *indio_dev,
>  	int result;
>  	int ret;
>  
> -	result = iio_device_claim_direct_mode(indio_dev);
> -	if (result)
> -		return result;
>  	result = inv_mpu6050_set_power_itg(st, true);
>  	if (result)
> -		goto error_release;
> +		return result;
>  
>  	switch (chan->type) {
>  	case IIO_ANGL_VEL:
> @@ -386,14 +383,11 @@ static int inv_mpu6050_read_channel_data(struct iio_dev *indio_dev,
>  	result = inv_mpu6050_set_power_itg(st, false);
>  	if (result)
>  		goto error_power_off;
> -	iio_device_release_direct_mode(indio_dev);
>  
>  	return ret;
>  
>  error_power_off:
>  	inv_mpu6050_set_power_itg(st, false);
> -error_release:
> -	iio_device_release_direct_mode(indio_dev);
>  	return result;
>  }
>  
> @@ -407,9 +401,13 @@ inv_mpu6050_read_raw(struct iio_dev *indio_dev,
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> +		ret = iio_device_claim_direct_mode(indio_dev);
> +		if (ret)
> +			return ret;
>  		mutex_lock(&st->lock);
>  		ret = inv_mpu6050_read_channel_data(indio_dev, chan, val);
>  		mutex_unlock(&st->lock);
> +		iio_device_release_direct_mode(indio_dev);
>  		return ret;
>  	case IIO_CHAN_INFO_SCALE:
>  		switch (chan->type) {
> @@ -532,17 +530,18 @@ static int inv_mpu6050_write_raw(struct iio_dev *indio_dev,
>  	struct inv_mpu6050_state  *st = iio_priv(indio_dev);
>  	int result;
>  
> -	mutex_lock(&st->lock);
>  	/*
>  	 * we should only update scale when the chip is disabled, i.e.
>  	 * not running
>  	 */
>  	result = iio_device_claim_direct_mode(indio_dev);
>  	if (result)
> -		goto error_write_raw_unlock;
> +		return result;
> +
> +	mutex_lock(&st->lock);
>  	result = inv_mpu6050_set_power_itg(st, true);
>  	if (result)
> -		goto error_write_raw_release;
> +		goto error_write_raw_unlock;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SCALE:
> @@ -581,10 +580,9 @@ static int inv_mpu6050_write_raw(struct iio_dev *indio_dev,
>  	}
>  
>  	result |= inv_mpu6050_set_power_itg(st, false);
> -error_write_raw_release:
> -	iio_device_release_direct_mode(indio_dev);
>  error_write_raw_unlock:
>  	mutex_unlock(&st->lock);
> +	iio_device_release_direct_mode(indio_dev);
>  
>  	return result;
>  }
> @@ -643,17 +641,18 @@ 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;
> +
>  	mutex_lock(&st->lock);
>  	if (fifo_rate == st->chip_config.fifo_rate) {
>  		result = 0;
>  		goto fifo_rate_fail_unlock;
>  	}
> -	result = iio_device_claim_direct_mode(indio_dev);
> -	if (result)
> -		goto fifo_rate_fail_unlock;
>  	result = inv_mpu6050_set_power_itg(st, true);
>  	if (result)
> -		goto fifo_rate_fail_release;
> +		goto fifo_rate_fail_unlock;
>  
>  	d = INV_MPU6050_ONE_K_HZ / fifo_rate - 1;
>  	result = regmap_write(st->map, st->reg->sample_rate_div, d);
> @@ -667,10 +666,9 @@ inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr,
>  
>  fifo_rate_fail_power_off:
>  	result |= inv_mpu6050_set_power_itg(st, false);
> -fifo_rate_fail_release:
> -	iio_device_release_direct_mode(indio_dev);
>  fifo_rate_fail_unlock:
>  	mutex_unlock(&st->lock);
> +	iio_device_release_direct_mode(indio_dev);
>  	if (result)
>  		return result;
>  

Re: [PATCH 3/4] iio: imu: inv_mpu6050: skip first sample when gyro is on

[Jonathan Cameron]

On Mon, 30 Apr 2018 12:14:10 +0200
Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote:

> Implement generic skip first samples mechanism and use it to
> filter out first sample when gyro is on.
> 
> The problem for these chips is that the first sample of the gyro
> is out of specs, because gyro is not completely stabilized. To
> ensure all data are within sensor specs, we just skip the first
> sample when turning gyro on.
> 
> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
Neat and tidy solution so good.
Is there a similar issue in the read_raw path which seems to just
flick the gyro on then read from it immediately?

I may be missing a delay there which is preventing the data being
bad though.

Jonathan
> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h     | 1 +
>  drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c    | 7 ++++++-
>  drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c | 3 +++
>  3 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> index 142a835..dfb9e4e 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> @@ -148,6 +148,7 @@ struct inv_mpu6050_state {
>  	struct regmap *map;
>  	int irq;
>  	u8 irq_mask;
> +	unsigned skip_samples;
>  };
>  
>  /*register and associated bit definition*/
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> index e51404f..1b57354 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> @@ -185,7 +185,12 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
>  		if (result == 0)
>  			timestamp = 0;
>  
> -		iio_push_to_buffers_with_timestamp(indio_dev, data, timestamp);
> +		/* skip first samples if needed */
> +		if (st->skip_samples)
> +			st->skip_samples--;
> +		else
> +			iio_push_to_buffers_with_timestamp(indio_dev, data,
> +							   timestamp);
>  
>  		fifo_count -= bytes_per_datum;
>  	}
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> index 8a9f869..0d7db27 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> @@ -49,11 +49,14 @@ static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
>  		if (result)
>  			return result;
>  		inv_scan_query(indio_dev);
> +		st->skip_samples = 0;
>  		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;
> +			/* gyro first sample is out of specs, skip it */
> +			st->skip_samples = 1;
>  		}
>  		if (st->chip_config.accl_fifo_enable) {
>  			result = inv_mpu6050_switch_engine(st, true,

Re: [PATCH 4/4] iio: imu: inv_mpu6050: fix user_ctrl register overwritten

[Jonathan Cameron]

On Mon, 30 Apr 2018 12:14:11 +0200
Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote:

> When in spi mode, we are setting i2c disable bit in user_ctrl
> register. But the register is overwritten after when turning fifo
> on. So save user_ctrl init value and always use it when updating
> the register.
> 
> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
Applied, thanks.

Jonathan

> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c    |  7 ++++---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h     |  1 +
>  drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c    | 13 +++++++------
>  drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c     |  5 +++--
>  drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c |  3 ++-
>  5 files changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> index 7358935..50c33e2 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> @@ -88,6 +88,7 @@ static const struct inv_mpu6050_chip_config chip_config_6050 = {
>  	.gyro_fifo_enable = false,
>  	.accl_fifo_enable = false,
>  	.accl_fs = INV_MPU6050_FS_02G,
> +	.user_ctrl = 0,
>  };
>  
>  /* Indexed by enum inv_devices */
> @@ -972,15 +973,15 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
>  	if (result)
>  		return result;
>  
> -	if (inv_mpu_bus_setup)
> -		inv_mpu_bus_setup(indio_dev);
> -
>  	result = inv_mpu6050_init_config(indio_dev);
>  	if (result) {
>  		dev_err(dev, "Could not initialize device.\n");
>  		return result;
>  	}
>  
> +	if (inv_mpu_bus_setup)
> +		inv_mpu_bus_setup(indio_dev);
> +
>  	dev_set_drvdata(dev, indio_dev);
>  	indio_dev->dev.parent = dev;
>  	/* name will be NULL when enumerated via ACPI */
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> index dfb9e4e..c54da77 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> @@ -97,6 +97,7 @@ struct inv_mpu6050_chip_config {
>  	unsigned int accl_fifo_enable:1;
>  	unsigned int gyro_fifo_enable:1;
>  	u16 fifo_rate;
> +	u8 user_ctrl;
>  };
>  
>  /**
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> index 1b57354..e7b23e3 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> @@ -51,13 +51,14 @@ int inv_reset_fifo(struct iio_dev *indio_dev)
>  	if (result)
>  		goto reset_fifo_fail;
>  	/* disable fifo reading */
> -	result = regmap_write(st->map, st->reg->user_ctrl, 0);
> +	result = regmap_write(st->map, st->reg->user_ctrl,
> +			      st->chip_config.user_ctrl);
>  	if (result)
>  		goto reset_fifo_fail;
>  
>  	/* reset FIFO*/
> -	result = regmap_write(st->map, st->reg->user_ctrl,
> -			      INV_MPU6050_BIT_FIFO_RST);
> +	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;
>  
> @@ -72,9 +73,9 @@ int inv_reset_fifo(struct iio_dev *indio_dev)
>  		if (result)
>  			return result;
>  	}
> -	/* enable FIFO reading and I2C master interface*/
> -	result = regmap_write(st->map, st->reg->user_ctrl,
> -			      INV_MPU6050_BIT_FIFO_EN);
> +	/* 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 */
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c
> index fe0bf5a..227f50a 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c
> @@ -31,8 +31,9 @@ static int inv_mpu_i2c_disable(struct iio_dev *indio_dev)
>  	if (ret)
>  		return ret;
>  
> -	ret = regmap_write(st->map, INV_MPU6050_REG_USER_CTRL,
> -			   INV_MPU6050_BIT_I2C_IF_DIS);
> +	st->chip_config.user_ctrl |= INV_MPU6050_BIT_I2C_IF_DIS;
> +	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;
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> index 0d7db27..d3e26ae 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> @@ -76,7 +76,8 @@ static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
>  		if (result)
>  			goto error_accl_off;
>  
> -		result = regmap_write(st->map, st->reg->user_ctrl, 0);
> +		result = regmap_write(st->map, st->reg->user_ctrl,
> +				      st->chip_config.user_ctrl);
>  		if (result)
>  			goto error_accl_off;
>  

Re: [PATCH 3/4] iio: imu: inv_mpu6050: skip first sample when gyro is on

[Jean-Baptiste Maneyrol]

On 06/05/2018 19:00, Jonathan Cameron wrote:
> On Mon, 30 Apr 2018 12:14:10 +0200
> Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote:
> 
>> Implement generic skip first samples mechanism and use it to
>> filter out first sample when gyro is on.
>>
>> The problem for these chips is that the first sample of the gyro
>> is out of specs, because gyro is not completely stabilized. To
>> ensure all data are within sensor specs, we just skip the first
>> sample when turning gyro on.
>>
>> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
> Neat and tidy solution so good.
> Is there a similar issue in the read_raw path which seems to just
> flick the gyro on then read from it immediately?
> 
> I may be missing a delay there which is preventing the data being
> bad though.
> 
> Jonathan
Hello,

effectively raw reading should be affected. Since it is using direct 
sensor register reading, it should requires adding a correct sleep time. 
But direct reading like this is really not very reliable (turn chip and 
sensor on/off everytime, which is very bad for stability).

Is it possible to send later an additional patch for fixing this issue 
and accept this one? The fix for the 2 are completely unrelated since it 
doesn't use the same mechanism (FIFO reading vs register reading).

Thanks.
JB
>> ---
>>   drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h     | 1 +
>>   drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c    | 7 ++++++-
>>   drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c | 3 +++
>>   3 files changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>> index 142a835..dfb9e4e 100644
>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>> @@ -148,6 +148,7 @@ struct inv_mpu6050_state {
>>   	struct regmap *map;
>>   	int irq;
>>   	u8 irq_mask;
>> +	unsigned skip_samples;
>>   };
>>   
>>   /*register and associated bit definition*/
>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
>> index e51404f..1b57354 100644
>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
>> @@ -185,7 +185,12 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
>>   		if (result == 0)
>>   			timestamp = 0;
>>   
>> -		iio_push_to_buffers_with_timestamp(indio_dev, data, timestamp);
>> +		/* skip first samples if needed */
>> +		if (st->skip_samples)
>> +			st->skip_samples--;
>> +		else
>> +			iio_push_to_buffers_with_timestamp(indio_dev, data,
>> +							   timestamp);
>>   
>>   		fifo_count -= bytes_per_datum;
>>   	}
>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
>> index 8a9f869..0d7db27 100644
>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
>> @@ -49,11 +49,14 @@ static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
>>   		if (result)
>>   			return result;
>>   		inv_scan_query(indio_dev);
>> +		st->skip_samples = 0;
>>   		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;
>> +			/* gyro first sample is out of specs, skip it */
>> +			st->skip_samples = 1;
>>   		}
>>   		if (st->chip_config.accl_fifo_enable) {
>>   			result = inv_mpu6050_switch_engine(st, true,
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 3/4] iio: imu: inv_mpu6050: skip first sample when gyro is on

[Jonathan Cameron]

On Tue, 8 May 2018 16:40:13 +0200
Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote:

> On 06/05/2018 19:00, Jonathan Cameron wrote:
> > On Mon, 30 Apr 2018 12:14:10 +0200
> > Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote:
> >   
> >> Implement generic skip first samples mechanism and use it to
> >> filter out first sample when gyro is on.
> >>
> >> The problem for these chips is that the first sample of the gyro
> >> is out of specs, because gyro is not completely stabilized. To
> >> ensure all data are within sensor specs, we just skip the first
> >> sample when turning gyro on.
> >>
> >> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>  
> > Neat and tidy solution so good.
> > Is there a similar issue in the read_raw path which seems to just
> > flick the gyro on then read from it immediately?
> > 
> > I may be missing a delay there which is preventing the data being
> > bad though.
> > 
> > Jonathan  
> Hello,
> 
> effectively raw reading should be affected. Since it is using direct 
> sensor register reading, it should requires adding a correct sleep time. 
> But direct reading like this is really not very reliable (turn chip and 
> sensor on/off everytime, which is very bad for stability).
> 
> Is it possible to send later an additional patch for fixing this issue 
> and accept this one? The fix for the 2 are completely unrelated since it 
> doesn't use the same mechanism (FIFO reading vs register reading).
Sure, that is fine. I think I just forgot to say I had applied this.
Sorry about that.

Jonathan

> 
> Thanks.
> JB
> >> ---
> >>   drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h     | 1 +
> >>   drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c    | 7 ++++++-
> >>   drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c | 3 +++
> >>   3 files changed, 10 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> >> index 142a835..dfb9e4e 100644
> >> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> >> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> >> @@ -148,6 +148,7 @@ struct inv_mpu6050_state {
> >>   	struct regmap *map;
> >>   	int irq;
> >>   	u8 irq_mask;
> >> +	unsigned skip_samples;
> >>   };
> >>   
> >>   /*register and associated bit definition*/
> >> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> >> index e51404f..1b57354 100644
> >> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> >> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> >> @@ -185,7 +185,12 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
> >>   		if (result == 0)
> >>   			timestamp = 0;
> >>   
> >> -		iio_push_to_buffers_with_timestamp(indio_dev, data, timestamp);
> >> +		/* skip first samples if needed */
> >> +		if (st->skip_samples)
> >> +			st->skip_samples--;
> >> +		else
> >> +			iio_push_to_buffers_with_timestamp(indio_dev, data,
> >> +							   timestamp);
> >>   
> >>   		fifo_count -= bytes_per_datum;
> >>   	}
> >> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> >> index 8a9f869..0d7db27 100644
> >> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> >> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> >> @@ -49,11 +49,14 @@ static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
> >>   		if (result)
> >>   			return result;
> >>   		inv_scan_query(indio_dev);
> >> +		st->skip_samples = 0;
> >>   		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;
> >> +			/* gyro first sample is out of specs, skip it */
> >> +			st->skip_samples = 1;
> >>   		}
> >>   		if (st->chip_config.accl_fifo_enable) {
> >>   			result = inv_mpu6050_switch_engine(st, true,  
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >