[RFC PATCH 0/9] iio: Fix ABBA deadlock in inv-mpu6050

[RFC PATCH 0/9] iio: Fix ABBA deadlock in inv-mpu6050

[Daniel Baluta]

Sending this as an RFC because I don't know if style fixes are appropriate
for this driver and also not sure if deadlock fix is the best solution.

I2C people should only look at patches 8/9 and 9/9.

The mpu6050 accel+gyro combo has an auxiliary I2C bus, allowing for
an auxiliary sensor to be connected (eg. a magnetometer).

The mpu can either act as an I2C master (functionality not currently
implemented in the driver) or it can use a bypass multiplexer which
directly connects the auxiliary I2C bus pins to the main I2C bus pins [1]
Currently the driver implements the bypass mode via an I2C mux.

This patchset fixes a deadlock in inv-mpu6050 IMU which happens when
magnetometer (on auxiliary I2C bus) runs in parallel with accel or gyro.

First 7 patches do some cleanup in order to make INV MPU6050 code easier
to read.

Patch number 8 allows passing NULL select callbacks to i2c_mux_master_xfer
and i2c_mux_smbus_xfer.

Patch number 9 actually fixes the deadlock.

[1] (page 28, https://www.cdiweb.com/datasheets/invensense/MPU-6050_DataSheet_V3%204.pdf)

Adriana Reus (2):
  i2c: i2c-mux: Allow for NULL select callback
  iio: imu: inv_mpu6050: Fix deadlock between i2c adapter lock and mpu
    lock

Daniel Baluta (7):
  iio: imu: inv_mpu6050: Fix multiline comments style
  iio: imu: inv_mpu6050: Fix Yoda conditions
  iio: imu: inv_mpu6050: Fix newlines to make code easier to read
  iio: imu: inv_mpu6050: Remove unnecessary parentheses
  iio: imu: inv_mpu6050: Delete space before comma
  iio: imu: inv_mpu6050: Fix code indent for if statement
  iio: imu: inv_mpu6050: Fix alignment with open parenthesis

 drivers/i2c/i2c-mux.c                         |  10 ++-
 drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c    |   6 +-
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c    | 101 ++++++++++++++------------
 drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c     |  90 ++++-------------------
 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c    |  23 +++---
 drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c |  22 +++---
 6 files changed, 102 insertions(+), 150 deletions(-)

-- 
2.5.0

[RFC PATCH 1/9] iio: imu: inv_mpu6050: Fix multiline comments style

[Daniel Baluta]

The preffered style for long (multi-line) comments is:

/*
 * this is a multiline
 * comment
 */

This also fixes checkpatch.pl warning:
WARNING: Block comments use * on subsequent lines

Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 37 +++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index 2258600..84e014c 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -79,10 +79,11 @@ int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en, u32 mask)
 {
 	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 */
+	/*
+	 * 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 (INV_MPU6050_BIT_PWR_GYRO_STBY == mask) {
 		result = regmap_read(st->map, st->reg->pwr_mgmt_1, &mgmt_1);
 		if (result)
@@ -92,8 +93,10 @@ int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en, u32 mask)
 	}
 
 	if ((INV_MPU6050_BIT_PWR_GYRO_STBY == mask) && (!en)) {
-		/* turning off gyro requires switch to internal clock first.
-		   Then turn off gyro engine */
+		/*
+		 * 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)
@@ -391,8 +394,10 @@ static int inv_mpu6050_write_raw(struct iio_dev *indio_dev,
 	int result;
 
 	mutex_lock(&indio_dev->mlock);
-	/* we should only update scale when the chip is disabled, i.e.,
-		not running */
+	/*
+	 * we should only update scale when the chip is disabled, i.e.
+	 * not running
+	 */
 	if (st->chip_config.enable) {
 		result = -EBUSY;
 		goto error_write_raw;
@@ -529,8 +534,10 @@ static ssize_t inv_attr_show(struct device *dev,
 	s8 *m;
 
 	switch (this_attr->address) {
-	/* In MPU6050, the two matrix are the same because gyro and accel
-	   are integrated in one chip */
+	/*
+	 * In MPU6050, the two matrix are the same because gyro and accel
+	 * are integrated in one chip
+	 */
 	case ATTR_GYRO_MATRIX:
 	case ATTR_ACCL_MATRIX:
 		m = st->plat_data.orientation;
@@ -654,10 +661,12 @@ static int inv_check_and_setup_chip(struct inv_mpu6050_state *st)
 	if (result)
 		return result;
 	msleep(INV_MPU6050_POWER_UP_TIME);
-	/* toggle power state. After reset, the sleep bit could be on
-		or off depending on the OTP settings. Toggling power would
-		make it in a definite state as well as making the hardware
-		state align with the software state */
+	/*
+	 * toggle power state. After reset, the sleep bit could be on
+	 * or off depending on the OTP settings. Toggling power would
+	 * make it in a definite state as well as making the hardware
+	 * state align with the software state
+	 */
 	result = inv_mpu6050_set_power_itg(st, false);
 	if (result)
 		return result;
-- 
2.5.0

Re: [RFC PATCH 1/9] iio: imu: inv_mpu6050: Fix multiline comments style

[Jonathan Cameron]

On 18/02/16 15:53, Daniel Baluta wrote:
> The preffered style for long (multi-line) comments is:
> 
> /*
>  * this is a multiline
>  * comment
>  */
> 
> This also fixes checkpatch.pl warning:
> WARNING: Block comments use * on subsequent lines
> 
> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
Applied to the togreg branch of iio.git - pushed out as testing as normal.

Jonathan
> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 37 +++++++++++++++++++-----------
>  1 file changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> index 2258600..84e014c 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> @@ -79,10 +79,11 @@ int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en, u32 mask)
>  {
>  	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 */
> +	/*
> +	 * 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 (INV_MPU6050_BIT_PWR_GYRO_STBY == mask) {
>  		result = regmap_read(st->map, st->reg->pwr_mgmt_1, &mgmt_1);
>  		if (result)
> @@ -92,8 +93,10 @@ int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en, u32 mask)
>  	}
>  
>  	if ((INV_MPU6050_BIT_PWR_GYRO_STBY == mask) && (!en)) {
> -		/* turning off gyro requires switch to internal clock first.
> -		   Then turn off gyro engine */
> +		/*
> +		 * 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)
> @@ -391,8 +394,10 @@ static int inv_mpu6050_write_raw(struct iio_dev *indio_dev,
>  	int result;
>  
>  	mutex_lock(&indio_dev->mlock);
> -	/* we should only update scale when the chip is disabled, i.e.,
> -		not running */
> +	/*
> +	 * we should only update scale when the chip is disabled, i.e.
> +	 * not running
> +	 */
>  	if (st->chip_config.enable) {
>  		result = -EBUSY;
>  		goto error_write_raw;
> @@ -529,8 +534,10 @@ static ssize_t inv_attr_show(struct device *dev,
>  	s8 *m;
>  
>  	switch (this_attr->address) {
> -	/* In MPU6050, the two matrix are the same because gyro and accel
> -	   are integrated in one chip */
> +	/*
> +	 * In MPU6050, the two matrix are the same because gyro and accel
> +	 * are integrated in one chip
> +	 */
>  	case ATTR_GYRO_MATRIX:
>  	case ATTR_ACCL_MATRIX:
>  		m = st->plat_data.orientation;
> @@ -654,10 +661,12 @@ static int inv_check_and_setup_chip(struct inv_mpu6050_state *st)
>  	if (result)
>  		return result;
>  	msleep(INV_MPU6050_POWER_UP_TIME);
> -	/* toggle power state. After reset, the sleep bit could be on
> -		or off depending on the OTP settings. Toggling power would
> -		make it in a definite state as well as making the hardware
> -		state align with the software state */
> +	/*
> +	 * toggle power state. After reset, the sleep bit could be on
> +	 * or off depending on the OTP settings. Toggling power would
> +	 * make it in a definite state as well as making the hardware
> +	 * state align with the software state
> +	 */
>  	result = inv_mpu6050_set_power_itg(st, false);
>  	if (result)
>  		return result;
> 

[RFC PATCH 2/9] iio: imu: inv_mpu6050: Fix Yoda conditions

[Daniel Baluta]

This fixes the following checkpatch warning:
	* WARNING: Comparisons should place the constant
	  on the right side of the test

Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 6 +++---
 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index 84e014c..c550ebb 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -84,7 +84,7 @@ int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en, u32 mask)
 	 * clock source be switched to gyro. Otherwise, it must be set to
 	 * internal clock
 	 */
-	if (INV_MPU6050_BIT_PWR_GYRO_STBY == mask) {
+	if (mask == INV_MPU6050_BIT_PWR_GYRO_STBY) {
 		result = regmap_read(st->map, st->reg->pwr_mgmt_1, &mgmt_1);
 		if (result)
 			return result;
@@ -92,7 +92,7 @@ int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en, u32 mask)
 		mgmt_1 &= ~INV_MPU6050_BIT_CLK_MASK;
 	}
 
-	if ((INV_MPU6050_BIT_PWR_GYRO_STBY == mask) && (!en)) {
+	if ((mask == INV_MPU6050_BIT_PWR_GYRO_STBY) && (!en)) {
 		/*
 		 * turning off gyro requires switch to internal clock first.
 		 * Then turn off gyro engine
@@ -117,7 +117,7 @@ int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en, u32 mask)
 	if (en) {
 		/* Wait for output stabilize */
 		msleep(INV_MPU6050_TEMP_UP_TIME);
-		if (INV_MPU6050_BIT_PWR_GYRO_STBY == mask) {
+		if (mask == INV_MPU6050_BIT_PWR_GYRO_STBY) {
 			/* switch internal clock to PLL */
 			mgmt_1 |= INV_CLK_PLL;
 			result = regmap_write(st->map,
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
index 1fc5fd9..441080b 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
@@ -168,7 +168,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
 
 		result = kfifo_out(&st->timestamps, &timestamp, 1);
 		/* when there is no timestamp, put timestamp as 0 */
-		if (0 == result)
+		if (result == 0)
 			timestamp = 0;
 
 		result = iio_push_to_buffers_with_timestamp(indio_dev, data,
-- 
2.5.0

Re: [RFC PATCH 2/9] iio: imu: inv_mpu6050: Fix Yoda conditions

[Crt Mori]

This is quite trivial.

Acked-by: Crt Mori <cmo@melexis.com>

On 18 February 2016 at 16:53, Daniel Baluta <daniel.baluta@intel.com> wrote:
> This fixes the following checkpatch warning:
>         * WARNING: Comparisons should place the constant
>           on the right side of the test
>
> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 6 +++---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> index 84e014c..c550ebb 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> @@ -84,7 +84,7 @@ int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en, u32 mask)
>          * clock source be switched to gyro. Otherwise, it must be set to
>          * internal clock
>          */
> -       if (INV_MPU6050_BIT_PWR_GYRO_STBY == mask) {
> +       if (mask == INV_MPU6050_BIT_PWR_GYRO_STBY) {
>                 result = regmap_read(st->map, st->reg->pwr_mgmt_1, &mgmt_1);
>                 if (result)
>                         return result;
> @@ -92,7 +92,7 @@ int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en, u32 mask)
>                 mgmt_1 &= ~INV_MPU6050_BIT_CLK_MASK;
>         }
>
> -       if ((INV_MPU6050_BIT_PWR_GYRO_STBY == mask) && (!en)) {
> +       if ((mask == INV_MPU6050_BIT_PWR_GYRO_STBY) && (!en)) {
>                 /*
>                  * turning off gyro requires switch to internal clock first.
>                  * Then turn off gyro engine
> @@ -117,7 +117,7 @@ int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en, u32 mask)
>         if (en) {
>                 /* Wait for output stabilize */
>                 msleep(INV_MPU6050_TEMP_UP_TIME);
> -               if (INV_MPU6050_BIT_PWR_GYRO_STBY == mask) {
> +               if (mask == INV_MPU6050_BIT_PWR_GYRO_STBY) {
>                         /* switch internal clock to PLL */
>                         mgmt_1 |= INV_CLK_PLL;
>                         result = regmap_write(st->map,
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> index 1fc5fd9..441080b 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> @@ -168,7 +168,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
>
>                 result = kfifo_out(&st->timestamps, &timestamp, 1);
>                 /* when there is no timestamp, put timestamp as 0 */
> -               if (0 == result)
> +               if (result == 0)
>                         timestamp = 0;
>
>                 result = iio_push_to_buffers_with_timestamp(indio_dev, data,
> --
> 2.5.0
>

Re: [RFC PATCH 2/9] iio: imu: inv_mpu6050: Fix Yoda conditions

[Jonathan Cameron]

On 19/02/16 09:09, Crt Mori wrote:
> This is quite trivial.
> 
> Acked-by: Crt Mori <cmo@melexis.com>
Applied.
> 
> On 18 February 2016 at 16:53, Daniel Baluta <daniel.baluta@intel.com> wrote:
>> This fixes the following checkpatch warning:
>>         * WARNING: Comparisons should place the constant
>>           on the right side of the test
>>
>> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
>> ---
>>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 6 +++---
>>  drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 2 +-
>>  2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>> index 84e014c..c550ebb 100644
>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>> @@ -84,7 +84,7 @@ int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en, u32 mask)
>>          * clock source be switched to gyro. Otherwise, it must be set to
>>          * internal clock
>>          */
>> -       if (INV_MPU6050_BIT_PWR_GYRO_STBY == mask) {
>> +       if (mask == INV_MPU6050_BIT_PWR_GYRO_STBY) {
>>                 result = regmap_read(st->map, st->reg->pwr_mgmt_1, &mgmt_1);
>>                 if (result)
>>                         return result;
>> @@ -92,7 +92,7 @@ int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en, u32 mask)
>>                 mgmt_1 &= ~INV_MPU6050_BIT_CLK_MASK;
>>         }
>>
>> -       if ((INV_MPU6050_BIT_PWR_GYRO_STBY == mask) && (!en)) {
>> +       if ((mask == INV_MPU6050_BIT_PWR_GYRO_STBY) && (!en)) {
>>                 /*
>>                  * turning off gyro requires switch to internal clock first.
>>                  * Then turn off gyro engine
>> @@ -117,7 +117,7 @@ int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en, u32 mask)
>>         if (en) {
>>                 /* Wait for output stabilize */
>>                 msleep(INV_MPU6050_TEMP_UP_TIME);
>> -               if (INV_MPU6050_BIT_PWR_GYRO_STBY == mask) {
>> +               if (mask == INV_MPU6050_BIT_PWR_GYRO_STBY) {
>>                         /* switch internal clock to PLL */
>>                         mgmt_1 |= INV_CLK_PLL;
>>                         result = regmap_write(st->map,
>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
>> index 1fc5fd9..441080b 100644
>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
>> @@ -168,7 +168,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
>>
>>                 result = kfifo_out(&st->timestamps, &timestamp, 1);
>>                 /* when there is no timestamp, put timestamp as 0 */
>> -               if (0 == result)
>> +               if (result == 0)
>>                         timestamp = 0;
>>
>>                 result = iio_push_to_buffers_with_timestamp(indio_dev, data,
>> --
>> 2.5.0
>>

Re: [RFC PATCH 2/9] iio: imu: inv_mpu6050: Fix Yoda conditions

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 38 bytes --]


"Yoda conditions" :D Made my day...


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[RFC PATCH 3/9] iio: imu: inv_mpu6050: Fix newlines to make code easier to read

[Daniel Baluta]

This fixes the following checkpatch.pl warnings:
	* WARNING: Missing a blank line after declarations
	* CHECK: Blank lines aren't necessary before a close brace '}'

Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c | 2 +-
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c
index 0bcfa8d..231a7af 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c
@@ -186,7 +186,6 @@ int inv_mpu_acpi_create_mux_client(struct i2c_client *client)
 		st->mux_client = i2c_new_device(st->mux_adapter, &info);
 		if (!st->mux_client)
 			return -ENODEV;
-
 	}
 
 	return 0;
@@ -195,6 +194,7 @@ int inv_mpu_acpi_create_mux_client(struct i2c_client *client)
 void inv_mpu_acpi_delete_mux_client(struct i2c_client *client)
 {
 	struct inv_mpu6050_state *st = iio_priv(dev_get_drvdata(&client->dev));
+
 	if (st->mux_client)
 		i2c_unregister_device(st->mux_client);
 }
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index c550ebb..72cc478 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -365,6 +365,7 @@ static int inv_write_raw_get_fmt(struct iio_dev *indio_dev,
 
 	return -EINVAL;
 }
+
 static int inv_mpu6050_write_accel_scale(struct inv_mpu6050_state *st, int val)
 {
 	int result, i;
-- 
2.5.0

Re: [RFC PATCH 3/9] iio: imu: inv_mpu6050: Fix newlines to make code easier to read

[Jonathan Cameron]

On 18/02/16 15:53, Daniel Baluta wrote:
> This fixes the following checkpatch.pl warnings:
> 	* WARNING: Missing a blank line after declarations
> 	* CHECK: Blank lines aren't necessary before a close brace '}'
> 
> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
Applied.
> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c | 2 +-
>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c
> index 0bcfa8d..231a7af 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c
> @@ -186,7 +186,6 @@ int inv_mpu_acpi_create_mux_client(struct i2c_client *client)
>  		st->mux_client = i2c_new_device(st->mux_adapter, &info);
>  		if (!st->mux_client)
>  			return -ENODEV;
> -
>  	}
>  
>  	return 0;
> @@ -195,6 +194,7 @@ int inv_mpu_acpi_create_mux_client(struct i2c_client *client)
>  void inv_mpu_acpi_delete_mux_client(struct i2c_client *client)
>  {
>  	struct inv_mpu6050_state *st = iio_priv(dev_get_drvdata(&client->dev));
> +
>  	if (st->mux_client)
>  		i2c_unregister_device(st->mux_client);
>  }
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> index c550ebb..72cc478 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> @@ -365,6 +365,7 @@ static int inv_write_raw_get_fmt(struct iio_dev *indio_dev,
>  
>  	return -EINVAL;
>  }
> +
>  static int inv_mpu6050_write_accel_scale(struct inv_mpu6050_state *st, int val)
>  {
>  	int result, i;
> 

[RFC PATCH 4/9] iio: imu: inv_mpu6050: Remove unnecessary parentheses

[Daniel Baluta]

Fixes the following checkpatch warning:
CHECK: Unnecessary parentheses around cpm->package.elements[i]

Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c
index 231a7af..2771106 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c
@@ -66,11 +66,11 @@ static int asus_acpi_get_sensor_info(struct acpi_device *adev,
 		union acpi_object *elem;
 		int j;
 
-		elem = &(cpm->package.elements[i]);
+		elem = &cpm->package.elements[i];
 		for (j = 0; j < elem->package.count; ++j) {
 			union acpi_object *sub_elem;
 
-			sub_elem = &(elem->package.elements[j]);
+			sub_elem = &elem->package.elements[j];
 			if (sub_elem->type == ACPI_TYPE_STRING)
 				strlcpy(info->type, sub_elem->string.pointer,
 					sizeof(info->type));
-- 
2.5.0

Re: [RFC PATCH 4/9] iio: imu: inv_mpu6050: Remove unnecessary parentheses

[Jonathan Cameron]

On 18/02/16 15:53, Daniel Baluta wrote:
> Fixes the following checkpatch warning:
> CHECK: Unnecessary parentheses around cpm->package.elements[i]
> 
> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
Applied (though I find it hard to care about this one!)

Jonathan
> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c
> index 231a7af..2771106 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_acpi.c
> @@ -66,11 +66,11 @@ static int asus_acpi_get_sensor_info(struct acpi_device *adev,
>  		union acpi_object *elem;
>  		int j;
>  
> -		elem = &(cpm->package.elements[i]);
> +		elem = &cpm->package.elements[i];
>  		for (j = 0; j < elem->package.count; ++j) {
>  			union acpi_object *sub_elem;
>  
> -			sub_elem = &(elem->package.elements[j]);
> +			sub_elem = &elem->package.elements[j];
>  			if (sub_elem->type == ACPI_TYPE_STRING)
>  				strlcpy(info->type, sub_elem->string.pointer,
>  					sizeof(info->type));
> 

[RFC PATCH 5/9] iio: imu: inv_mpu6050: Delete space before comma

[Daniel Baluta]

This fixes the following checkpatch.pl warning:

ERROR: space prohibited before that ',' (ctx:WxE)
		.shift = 0 ,

Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 2 +-
 1 file changed, 1 insertion(+), 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 72cc478..48ab575 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -582,7 +582,7 @@ static int inv_mpu6050_validate_trigger(struct iio_dev *indio_dev,
 				.sign = 's',                          \
 				.realbits = 16,                       \
 				.storagebits = 16,                    \
-				.shift = 0 ,                          \
+				.shift = 0,                           \
 				.endianness = IIO_BE,                 \
 			     },                                       \
 	}
-- 
2.5.0

[RFC PATCH 6/9] iio: imu: inv_mpu6050: Fix code indent for if statement

[Daniel Baluta]

This fixes the following checkpatch.pl warning:

WARNING: suspect code indent for conditional statements (8, 24)
+       if (kfifo_len(&st->timestamps) >
[...]
+                       goto flush_fifo;

Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
index 441080b..0bc5091 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
@@ -158,8 +158,8 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
 		goto flush_fifo;
 	/* Timestamp mismatch. */
 	if (kfifo_len(&st->timestamps) >
-		fifo_count / bytes_per_datum + INV_MPU6050_TIME_STAMP_TOR)
-			goto flush_fifo;
+	    fifo_count / bytes_per_datum + INV_MPU6050_TIME_STAMP_TOR)
+		goto flush_fifo;
 	while (fifo_count >= bytes_per_datum) {
 		result = regmap_bulk_read(st->map, st->reg->fifo_r_w,
 					  data, bytes_per_datum);
-- 
2.5.0

Re: [RFC PATCH 6/9] iio: imu: inv_mpu6050: Fix code indent for if statement

[Jonathan Cameron]

On 18/02/16 15:53, Daniel Baluta wrote:
> This fixes the following checkpatch.pl warning:
> 
> WARNING: suspect code indent for conditional statements (8, 24)
> +       if (kfifo_len(&st->timestamps) >
> [...]
> +                       goto flush_fifo;
> 
> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
Applied.
> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> index 441080b..0bc5091 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> @@ -158,8 +158,8 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
>  		goto flush_fifo;
>  	/* Timestamp mismatch. */
>  	if (kfifo_len(&st->timestamps) >
> -		fifo_count / bytes_per_datum + INV_MPU6050_TIME_STAMP_TOR)
> -			goto flush_fifo;
> +	    fifo_count / bytes_per_datum + INV_MPU6050_TIME_STAMP_TOR)
> +		goto flush_fifo;
>  	while (fifo_count >= bytes_per_datum) {
>  		result = regmap_bulk_read(st->map, st->reg->fifo_r_w,
>  					  data, bytes_per_datum);
> 

[RFC PATCH 7/9] iio: imu: inv_mpu6050: Fix alignment with open parenthesis

[Daniel Baluta]

This makes code consistent around inv_mpu6050 driver and
fixes the following checkpatch.pl warning:
CHECK: Alignment should match open parenthesis

Note that there were few cases were it was not possible to
fix this due to making the line too long, but we can live with that.

Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c    | 55 +++++++++++++--------------
 drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c     |  2 +-
 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c    | 17 ++++-----
 drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c | 22 +++++------
 4 files changed, 47 insertions(+), 49 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index 48ab575..1643cd2 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -121,7 +121,7 @@ int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en, u32 mask)
 			/* switch internal clock to PLL */
 			mgmt_1 |= INV_CLK_PLL;
 			result = regmap_write(st->map,
-					st->reg->pwr_mgmt_1, mgmt_1);
+					      st->reg->pwr_mgmt_1, mgmt_1);
 			if (result)
 				return result;
 		}
@@ -196,14 +196,14 @@ static int inv_mpu6050_init_config(struct iio_dev *indio_dev)
 		return result;
 
 	memcpy(&st->chip_config, hw_info[st->chip_type].config,
-		sizeof(struct inv_mpu6050_chip_config));
+	       sizeof(struct inv_mpu6050_chip_config));
 	result = inv_mpu6050_set_power_itg(st, false);
 
 	return result;
 }
 
 static int inv_mpu6050_sensor_show(struct inv_mpu6050_state  *st, int reg,
-				int axis, int *val)
+				   int axis, int *val)
 {
 	int ind, result;
 	__be16 d;
@@ -217,11 +217,10 @@ static int inv_mpu6050_sensor_show(struct inv_mpu6050_state  *st, int reg,
 	return IIO_VAL_INT;
 }
 
-static int inv_mpu6050_read_raw(struct iio_dev *indio_dev,
-			      struct iio_chan_spec const *chan,
-			      int *val,
-			      int *val2,
-			      long mask) {
+static int
+inv_mpu6050_read_raw(struct iio_dev *indio_dev,
+		     struct iio_chan_spec const *chan,
+		     int *val, int *val2, long mask) {
 	struct inv_mpu6050_state  *st = iio_priv(indio_dev);
 
 	switch (mask) {
@@ -241,16 +240,16 @@ static int inv_mpu6050_read_raw(struct iio_dev *indio_dev,
 		switch (chan->type) {
 		case IIO_ANGL_VEL:
 			if (!st->chip_config.gyro_fifo_enable ||
-					!st->chip_config.enable) {
+			    !st->chip_config.enable) {
 				result = inv_mpu6050_switch_engine(st, true,
 						INV_MPU6050_BIT_PWR_GYRO_STBY);
 				if (result)
 					goto error_read_raw;
 			}
 			ret =  inv_mpu6050_sensor_show(st, st->reg->raw_gyro,
-						chan->channel2, val);
+						       chan->channel2, val);
 			if (!st->chip_config.gyro_fifo_enable ||
-					!st->chip_config.enable) {
+			    !st->chip_config.enable) {
 				result = inv_mpu6050_switch_engine(st, false,
 						INV_MPU6050_BIT_PWR_GYRO_STBY);
 				if (result)
@@ -259,16 +258,16 @@ static int inv_mpu6050_read_raw(struct iio_dev *indio_dev,
 			break;
 		case IIO_ACCEL:
 			if (!st->chip_config.accl_fifo_enable ||
-					!st->chip_config.enable) {
+			    !st->chip_config.enable) {
 				result = inv_mpu6050_switch_engine(st, true,
 						INV_MPU6050_BIT_PWR_ACCL_STBY);
 				if (result)
 					goto error_read_raw;
 			}
 			ret = inv_mpu6050_sensor_show(st, st->reg->raw_accl,
-						chan->channel2, val);
+						      chan->channel2, val);
 			if (!st->chip_config.accl_fifo_enable ||
-					!st->chip_config.enable) {
+			    !st->chip_config.enable) {
 				result = inv_mpu6050_switch_engine(st, false,
 						INV_MPU6050_BIT_PWR_ACCL_STBY);
 				if (result)
@@ -279,7 +278,7 @@ static int inv_mpu6050_read_raw(struct iio_dev *indio_dev,
 			/* wait for stablization */
 			msleep(INV_MPU6050_SENSOR_UP_TIME);
 			inv_mpu6050_sensor_show(st, st->reg->temperature,
-							IIO_MOD_X, val);
+						IIO_MOD_X, val);
 			break;
 		default:
 			ret = -EINVAL;
@@ -387,10 +386,8 @@ static int inv_mpu6050_write_accel_scale(struct inv_mpu6050_state *st, int val)
 }
 
 static int inv_mpu6050_write_raw(struct iio_dev *indio_dev,
-			       struct iio_chan_spec const *chan,
-			       int val,
-			       int val2,
-			       long mask) {
+				 struct iio_chan_spec const *chan,
+				 int val, int val2, long mask) {
 	struct inv_mpu6050_state  *st = iio_priv(indio_dev);
 	int result;
 
@@ -467,8 +464,9 @@ static int inv_mpu6050_set_lpf(struct inv_mpu6050_state *st, int rate)
 /**
  * inv_mpu6050_fifo_rate_store() - Set fifo rate.
  */
-static ssize_t inv_mpu6050_fifo_rate_store(struct device *dev,
-	struct device_attribute *attr, const char *buf, size_t count)
+static ssize_t
+inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr,
+			    const char *buf, size_t count)
 {
 	s32 fifo_rate;
 	u8 d;
@@ -479,7 +477,7 @@ static ssize_t inv_mpu6050_fifo_rate_store(struct device *dev,
 	if (kstrtoint(buf, 10, &fifo_rate))
 		return -EINVAL;
 	if (fifo_rate < INV_MPU6050_MIN_FIFO_RATE ||
-				fifo_rate > INV_MPU6050_MAX_FIFO_RATE)
+	    fifo_rate > INV_MPU6050_MAX_FIFO_RATE)
 		return -EINVAL;
 	if (fifo_rate == st->chip_config.fifo_rate)
 		return count;
@@ -515,8 +513,9 @@ fifo_rate_fail:
 /**
  * inv_fifo_rate_show() - Get the current sampling rate.
  */
-static ssize_t inv_fifo_rate_show(struct device *dev,
-	struct device_attribute *attr, char *buf)
+static ssize_t
+inv_fifo_rate_show(struct device *dev, struct device_attribute *attr,
+		   char *buf)
 {
 	struct inv_mpu6050_state *st = iio_priv(dev_to_iio_dev(dev));
 
@@ -527,8 +526,8 @@ static ssize_t inv_fifo_rate_show(struct device *dev,
  * inv_attr_show() - calling this function will show current
  *                    parameters.
  */
-static ssize_t inv_attr_show(struct device *dev,
-	struct device_attribute *attr, char *buf)
+static ssize_t inv_attr_show(struct device *dev, struct device_attribute *attr,
+			     char *buf)
 {
 	struct inv_mpu6050_state *st = iio_priv(dev_to_iio_dev(dev));
 	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
@@ -676,11 +675,11 @@ static int inv_check_and_setup_chip(struct inv_mpu6050_state *st)
 		return result;
 
 	result = inv_mpu6050_switch_engine(st, false,
-					INV_MPU6050_BIT_PWR_ACCL_STBY);
+					   INV_MPU6050_BIT_PWR_ACCL_STBY);
 	if (result)
 		return result;
 	result = inv_mpu6050_switch_engine(st, false,
-					INV_MPU6050_BIT_PWR_GYRO_STBY);
+					   INV_MPU6050_BIT_PWR_GYRO_STBY);
 	if (result)
 		return result;
 
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
index af400dd..71bdaa3 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
@@ -111,7 +111,7 @@ static int inv_mpu6050_deselect_bypass(struct i2c_adapter *adap,
  *  Returns 0 on success, a negative error code otherwise.
  */
 static int inv_mpu_probe(struct i2c_client *client,
-	const struct i2c_device_id *id)
+			 const struct i2c_device_id *id)
 {
 	struct inv_mpu6050_state *st;
 	int result;
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
index 0bc5091..d070062 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
@@ -57,7 +57,7 @@ int inv_reset_fifo(struct iio_dev *indio_dev)
 
 	/* reset FIFO*/
 	result = regmap_write(st->map, st->reg->user_ctrl,
-					INV_MPU6050_BIT_FIFO_RST);
+			      INV_MPU6050_BIT_FIFO_RST);
 	if (result)
 		goto reset_fifo_fail;
 
@@ -68,13 +68,13 @@ int inv_reset_fifo(struct iio_dev *indio_dev)
 	if (st->chip_config.accl_fifo_enable ||
 	    st->chip_config.gyro_fifo_enable) {
 		result = regmap_write(st->map, st->reg->int_enable,
-					INV_MPU6050_BIT_DATA_RDY_EN);
+				      INV_MPU6050_BIT_DATA_RDY_EN);
 		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);
+			      INV_MPU6050_BIT_FIFO_EN);
 	if (result)
 		goto reset_fifo_fail;
 	/* enable sensor output to FIFO */
@@ -92,7 +92,7 @@ int inv_reset_fifo(struct iio_dev *indio_dev)
 reset_fifo_fail:
 	dev_err(regmap_get_device(st->map), "reset fifo failed %d\n", result);
 	result = regmap_write(st->map, st->reg->int_enable,
-					INV_MPU6050_BIT_DATA_RDY_EN);
+			      INV_MPU6050_BIT_DATA_RDY_EN);
 
 	return result;
 }
@@ -109,7 +109,7 @@ irqreturn_t inv_mpu6050_irq_handler(int irq, void *p)
 
 	timestamp = iio_get_time_ns();
 	kfifo_in_spinlocked(&st->timestamps, &timestamp, 1,
-				&st->time_stamp_lock);
+			    &st->time_stamp_lock);
 
 	return IRQ_WAKE_THREAD;
 }
@@ -143,9 +143,8 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
 	 * read fifo_count register to know how many bytes inside FIFO
 	 * right now
 	 */
-	result = regmap_bulk_read(st->map,
-				       st->reg->fifo_count_h,
-				       data, INV_MPU6050_FIFO_COUNT_BYTE);
+	result = regmap_bulk_read(st->map, st->reg->fifo_count_h, data,
+				  INV_MPU6050_FIFO_COUNT_BYTE);
 	if (result)
 		goto end_session;
 	fifo_count = be16_to_cpup((__be16 *)(&data[0]));
@@ -172,7 +171,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
 			timestamp = 0;
 
 		result = iio_push_to_buffers_with_timestamp(indio_dev, data,
-			timestamp);
+							    timestamp);
 		if (result)
 			goto flush_fifo;
 		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 72d6aae..e8818d4 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
@@ -19,19 +19,19 @@ static void inv_scan_query(struct iio_dev *indio_dev)
 
 	st->chip_config.gyro_fifo_enable =
 		test_bit(INV_MPU6050_SCAN_GYRO_X,
-			indio_dev->active_scan_mask) ||
-			test_bit(INV_MPU6050_SCAN_GYRO_Y,
-			indio_dev->active_scan_mask) ||
-			test_bit(INV_MPU6050_SCAN_GYRO_Z,
-			indio_dev->active_scan_mask);
+			 indio_dev->active_scan_mask) ||
+		test_bit(INV_MPU6050_SCAN_GYRO_Y,
+			 indio_dev->active_scan_mask) ||
+		test_bit(INV_MPU6050_SCAN_GYRO_Z,
+			 indio_dev->active_scan_mask);
 
 	st->chip_config.accl_fifo_enable =
 		test_bit(INV_MPU6050_SCAN_ACCL_X,
-			indio_dev->active_scan_mask) ||
-			test_bit(INV_MPU6050_SCAN_ACCL_Y,
-			indio_dev->active_scan_mask) ||
-			test_bit(INV_MPU6050_SCAN_ACCL_Z,
-			indio_dev->active_scan_mask);
+			 indio_dev->active_scan_mask) ||
+		test_bit(INV_MPU6050_SCAN_ACCL_Y,
+			 indio_dev->active_scan_mask) ||
+		test_bit(INV_MPU6050_SCAN_ACCL_Z,
+			 indio_dev->active_scan_mask);
 }
 
 /**
@@ -101,7 +101,7 @@ static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
  * @state: Desired trigger state
  */
 static int inv_mpu_data_rdy_trigger_set_state(struct iio_trigger *trig,
-						bool state)
+					      bool state)
 {
 	return inv_mpu6050_set_enable(iio_trigger_get_drvdata(trig), state);
 }
-- 
2.5.0

Re: [RFC PATCH 7/9] iio: imu: inv_mpu6050: Fix alignment with open parenthesis

[Jonathan Cameron]

On 18/02/16 15:53, Daniel Baluta wrote:
> This makes code consistent around inv_mpu6050 driver and
> fixes the following checkpatch.pl warning:
> CHECK: Alignment should match open parenthesis
> 
> Note that there were few cases were it was not possible to
> fix this due to making the line too long, but we can live with that.
> 
> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
Applied
> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c    | 55 +++++++++++++--------------
>  drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c     |  2 +-
>  drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c    | 17 ++++-----
>  drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c | 22 +++++------
>  4 files changed, 47 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> index 48ab575..1643cd2 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> @@ -121,7 +121,7 @@ int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en, u32 mask)
>  			/* switch internal clock to PLL */
>  			mgmt_1 |= INV_CLK_PLL;
>  			result = regmap_write(st->map,
> -					st->reg->pwr_mgmt_1, mgmt_1);
> +					      st->reg->pwr_mgmt_1, mgmt_1);
>  			if (result)
>  				return result;
>  		}
> @@ -196,14 +196,14 @@ static int inv_mpu6050_init_config(struct iio_dev *indio_dev)
>  		return result;
>  
>  	memcpy(&st->chip_config, hw_info[st->chip_type].config,
> -		sizeof(struct inv_mpu6050_chip_config));
> +	       sizeof(struct inv_mpu6050_chip_config));
>  	result = inv_mpu6050_set_power_itg(st, false);
>  
>  	return result;
>  }
>  
>  static int inv_mpu6050_sensor_show(struct inv_mpu6050_state  *st, int reg,
> -				int axis, int *val)
> +				   int axis, int *val)
>  {
>  	int ind, result;
>  	__be16 d;
> @@ -217,11 +217,10 @@ static int inv_mpu6050_sensor_show(struct inv_mpu6050_state  *st, int reg,
>  	return IIO_VAL_INT;
>  }
>  
> -static int inv_mpu6050_read_raw(struct iio_dev *indio_dev,
> -			      struct iio_chan_spec const *chan,
> -			      int *val,
> -			      int *val2,
> -			      long mask) {
> +static int
> +inv_mpu6050_read_raw(struct iio_dev *indio_dev,
> +		     struct iio_chan_spec const *chan,
> +		     int *val, int *val2, long mask) {
>  	struct inv_mpu6050_state  *st = iio_priv(indio_dev);
>  
>  	switch (mask) {
> @@ -241,16 +240,16 @@ static int inv_mpu6050_read_raw(struct iio_dev *indio_dev,
>  		switch (chan->type) {
>  		case IIO_ANGL_VEL:
>  			if (!st->chip_config.gyro_fifo_enable ||
> -					!st->chip_config.enable) {
> +			    !st->chip_config.enable) {
>  				result = inv_mpu6050_switch_engine(st, true,
>  						INV_MPU6050_BIT_PWR_GYRO_STBY);
>  				if (result)
>  					goto error_read_raw;
>  			}
>  			ret =  inv_mpu6050_sensor_show(st, st->reg->raw_gyro,
> -						chan->channel2, val);
> +						       chan->channel2, val);
>  			if (!st->chip_config.gyro_fifo_enable ||
> -					!st->chip_config.enable) {
> +			    !st->chip_config.enable) {
>  				result = inv_mpu6050_switch_engine(st, false,
>  						INV_MPU6050_BIT_PWR_GYRO_STBY);
>  				if (result)
> @@ -259,16 +258,16 @@ static int inv_mpu6050_read_raw(struct iio_dev *indio_dev,
>  			break;
>  		case IIO_ACCEL:
>  			if (!st->chip_config.accl_fifo_enable ||
> -					!st->chip_config.enable) {
> +			    !st->chip_config.enable) {
>  				result = inv_mpu6050_switch_engine(st, true,
>  						INV_MPU6050_BIT_PWR_ACCL_STBY);
>  				if (result)
>  					goto error_read_raw;
>  			}
>  			ret = inv_mpu6050_sensor_show(st, st->reg->raw_accl,
> -						chan->channel2, val);
> +						      chan->channel2, val);
>  			if (!st->chip_config.accl_fifo_enable ||
> -					!st->chip_config.enable) {
> +			    !st->chip_config.enable) {
>  				result = inv_mpu6050_switch_engine(st, false,
>  						INV_MPU6050_BIT_PWR_ACCL_STBY);
>  				if (result)
> @@ -279,7 +278,7 @@ static int inv_mpu6050_read_raw(struct iio_dev *indio_dev,
>  			/* wait for stablization */
>  			msleep(INV_MPU6050_SENSOR_UP_TIME);
>  			inv_mpu6050_sensor_show(st, st->reg->temperature,
> -							IIO_MOD_X, val);
> +						IIO_MOD_X, val);
>  			break;
>  		default:
>  			ret = -EINVAL;
> @@ -387,10 +386,8 @@ static int inv_mpu6050_write_accel_scale(struct inv_mpu6050_state *st, int val)
>  }
>  
>  static int inv_mpu6050_write_raw(struct iio_dev *indio_dev,
> -			       struct iio_chan_spec const *chan,
> -			       int val,
> -			       int val2,
> -			       long mask) {
> +				 struct iio_chan_spec const *chan,
> +				 int val, int val2, long mask) {
>  	struct inv_mpu6050_state  *st = iio_priv(indio_dev);
>  	int result;
>  
> @@ -467,8 +464,9 @@ static int inv_mpu6050_set_lpf(struct inv_mpu6050_state *st, int rate)
>  /**
>   * inv_mpu6050_fifo_rate_store() - Set fifo rate.
>   */
> -static ssize_t inv_mpu6050_fifo_rate_store(struct device *dev,
> -	struct device_attribute *attr, const char *buf, size_t count)
> +static ssize_t
> +inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr,
> +			    const char *buf, size_t count)
>  {
>  	s32 fifo_rate;
>  	u8 d;
> @@ -479,7 +477,7 @@ static ssize_t inv_mpu6050_fifo_rate_store(struct device *dev,
>  	if (kstrtoint(buf, 10, &fifo_rate))
>  		return -EINVAL;
>  	if (fifo_rate < INV_MPU6050_MIN_FIFO_RATE ||
> -				fifo_rate > INV_MPU6050_MAX_FIFO_RATE)
> +	    fifo_rate > INV_MPU6050_MAX_FIFO_RATE)
>  		return -EINVAL;
>  	if (fifo_rate == st->chip_config.fifo_rate)
>  		return count;
> @@ -515,8 +513,9 @@ fifo_rate_fail:
>  /**
>   * inv_fifo_rate_show() - Get the current sampling rate.
>   */
> -static ssize_t inv_fifo_rate_show(struct device *dev,
> -	struct device_attribute *attr, char *buf)
> +static ssize_t
> +inv_fifo_rate_show(struct device *dev, struct device_attribute *attr,
> +		   char *buf)
>  {
>  	struct inv_mpu6050_state *st = iio_priv(dev_to_iio_dev(dev));
>  
> @@ -527,8 +526,8 @@ static ssize_t inv_fifo_rate_show(struct device *dev,
>   * inv_attr_show() - calling this function will show current
>   *                    parameters.
>   */
> -static ssize_t inv_attr_show(struct device *dev,
> -	struct device_attribute *attr, char *buf)
> +static ssize_t inv_attr_show(struct device *dev, struct device_attribute *attr,
> +			     char *buf)
>  {
>  	struct inv_mpu6050_state *st = iio_priv(dev_to_iio_dev(dev));
>  	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> @@ -676,11 +675,11 @@ static int inv_check_and_setup_chip(struct inv_mpu6050_state *st)
>  		return result;
>  
>  	result = inv_mpu6050_switch_engine(st, false,
> -					INV_MPU6050_BIT_PWR_ACCL_STBY);
> +					   INV_MPU6050_BIT_PWR_ACCL_STBY);
>  	if (result)
>  		return result;
>  	result = inv_mpu6050_switch_engine(st, false,
> -					INV_MPU6050_BIT_PWR_GYRO_STBY);
> +					   INV_MPU6050_BIT_PWR_GYRO_STBY);
>  	if (result)
>  		return result;
>  
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> index af400dd..71bdaa3 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> @@ -111,7 +111,7 @@ static int inv_mpu6050_deselect_bypass(struct i2c_adapter *adap,
>   *  Returns 0 on success, a negative error code otherwise.
>   */
>  static int inv_mpu_probe(struct i2c_client *client,
> -	const struct i2c_device_id *id)
> +			 const struct i2c_device_id *id)
>  {
>  	struct inv_mpu6050_state *st;
>  	int result;
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> index 0bc5091..d070062 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> @@ -57,7 +57,7 @@ int inv_reset_fifo(struct iio_dev *indio_dev)
>  
>  	/* reset FIFO*/
>  	result = regmap_write(st->map, st->reg->user_ctrl,
> -					INV_MPU6050_BIT_FIFO_RST);
> +			      INV_MPU6050_BIT_FIFO_RST);
>  	if (result)
>  		goto reset_fifo_fail;
>  
> @@ -68,13 +68,13 @@ int inv_reset_fifo(struct iio_dev *indio_dev)
>  	if (st->chip_config.accl_fifo_enable ||
>  	    st->chip_config.gyro_fifo_enable) {
>  		result = regmap_write(st->map, st->reg->int_enable,
> -					INV_MPU6050_BIT_DATA_RDY_EN);
> +				      INV_MPU6050_BIT_DATA_RDY_EN);
>  		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);
> +			      INV_MPU6050_BIT_FIFO_EN);
>  	if (result)
>  		goto reset_fifo_fail;
>  	/* enable sensor output to FIFO */
> @@ -92,7 +92,7 @@ int inv_reset_fifo(struct iio_dev *indio_dev)
>  reset_fifo_fail:
>  	dev_err(regmap_get_device(st->map), "reset fifo failed %d\n", result);
>  	result = regmap_write(st->map, st->reg->int_enable,
> -					INV_MPU6050_BIT_DATA_RDY_EN);
> +			      INV_MPU6050_BIT_DATA_RDY_EN);
>  
>  	return result;
>  }
> @@ -109,7 +109,7 @@ irqreturn_t inv_mpu6050_irq_handler(int irq, void *p)
>  
>  	timestamp = iio_get_time_ns();
>  	kfifo_in_spinlocked(&st->timestamps, &timestamp, 1,
> -				&st->time_stamp_lock);
> +			    &st->time_stamp_lock);
>  
>  	return IRQ_WAKE_THREAD;
>  }
> @@ -143,9 +143,8 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
>  	 * read fifo_count register to know how many bytes inside FIFO
>  	 * right now
>  	 */
> -	result = regmap_bulk_read(st->map,
> -				       st->reg->fifo_count_h,
> -				       data, INV_MPU6050_FIFO_COUNT_BYTE);
> +	result = regmap_bulk_read(st->map, st->reg->fifo_count_h, data,
> +				  INV_MPU6050_FIFO_COUNT_BYTE);
>  	if (result)
>  		goto end_session;
>  	fifo_count = be16_to_cpup((__be16 *)(&data[0]));
> @@ -172,7 +171,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
>  			timestamp = 0;
>  
>  		result = iio_push_to_buffers_with_timestamp(indio_dev, data,
> -			timestamp);
> +							    timestamp);
>  		if (result)
>  			goto flush_fifo;
>  		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 72d6aae..e8818d4 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> @@ -19,19 +19,19 @@ static void inv_scan_query(struct iio_dev *indio_dev)
>  
>  	st->chip_config.gyro_fifo_enable =
>  		test_bit(INV_MPU6050_SCAN_GYRO_X,
> -			indio_dev->active_scan_mask) ||
> -			test_bit(INV_MPU6050_SCAN_GYRO_Y,
> -			indio_dev->active_scan_mask) ||
> -			test_bit(INV_MPU6050_SCAN_GYRO_Z,
> -			indio_dev->active_scan_mask);
> +			 indio_dev->active_scan_mask) ||
> +		test_bit(INV_MPU6050_SCAN_GYRO_Y,
> +			 indio_dev->active_scan_mask) ||
> +		test_bit(INV_MPU6050_SCAN_GYRO_Z,
> +			 indio_dev->active_scan_mask);
>  
>  	st->chip_config.accl_fifo_enable =
>  		test_bit(INV_MPU6050_SCAN_ACCL_X,
> -			indio_dev->active_scan_mask) ||
> -			test_bit(INV_MPU6050_SCAN_ACCL_Y,
> -			indio_dev->active_scan_mask) ||
> -			test_bit(INV_MPU6050_SCAN_ACCL_Z,
> -			indio_dev->active_scan_mask);
> +			 indio_dev->active_scan_mask) ||
> +		test_bit(INV_MPU6050_SCAN_ACCL_Y,
> +			 indio_dev->active_scan_mask) ||
> +		test_bit(INV_MPU6050_SCAN_ACCL_Z,
> +			 indio_dev->active_scan_mask);
>  }
>  
>  /**
> @@ -101,7 +101,7 @@ static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
>   * @state: Desired trigger state
>   */
>  static int inv_mpu_data_rdy_trigger_set_state(struct iio_trigger *trig,
> -						bool state)
> +					      bool state)
>  {
>  	return inv_mpu6050_set_enable(iio_trigger_get_drvdata(trig), state);
>  }
> 

Re: [RFC PATCH 7/9] iio: imu: inv_mpu6050: Fix alignment with open parenthesis

[Joe Perches]

On Sun, 2016-02-21 at 20:41 +0000, Jonathan Cameron wrote:
> On 18/02/16 15:53, Daniel Baluta wrote:
> > This makes code consistent around inv_mpu6050 driver and
> > fixes the following checkpatch.pl warning:
> > CHECK: Alignment should match open parenthesis
> > 
> > Note that there were few cases were it was not possible to
> > fix this due to making the line too long, but we can live with that.
> > 
> > Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
> Applied
[]
> > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
[]
> > @@ -217,11 +217,10 @@ static int inv_mpu6050_sensor_show(struct inv_mpu6050_state  *st, int reg,
> >  	return IIO_VAL_INT;
> >  }
> >  
> > -static int inv_mpu6050_read_raw(struct iio_dev *indio_dev,
> > -			      struct iio_chan_spec const *chan,
> > -			      int *val,
> > -			      int *val2,
> > -			      long mask) {
> > +static int
> > +inv_mpu6050_read_raw(struct iio_dev *indio_dev,
> > +		     struct iio_chan_spec const *chan,
> > +		     int *val, int *val2, long mask) {

Ideally, you'd also convert this form to use
the open brace on a new line like:

static int
inv_mpu6050_read_raw(struct iio_dev *indio_dev,
		     struct iio_chan_spec const *chan,
		     int *val, int *val2, long mask)
{

Re: [RFC PATCH 7/9] iio: imu: inv_mpu6050: Fix alignment with open parenthesis

[Jonathan Cameron]

On 21/02/16 20:59, Joe Perches wrote:
> On Sun, 2016-02-21 at 20:41 +0000, Jonathan Cameron wrote:
>> On 18/02/16 15:53, Daniel Baluta wrote:
>>> This makes code consistent around inv_mpu6050 driver and
>>> fixes the following checkpatch.pl warning:
>>> CHECK: Alignment should match open parenthesis
>>>
>>> Note that there were few cases were it was not possible to
>>> fix this due to making the line too long, but we can live with that.
>>>
>>> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
>> Applied
> []
>>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> []
>>> @@ -217,11 +217,10 @@ static int inv_mpu6050_sensor_show(struct inv_mpu6050_state  *st, int reg,
>>>  	return IIO_VAL_INT;
>>>  }
>>>  
>>> -static int inv_mpu6050_read_raw(struct iio_dev *indio_dev,
>>> -			      struct iio_chan_spec const *chan,
>>> -			      int *val,
>>> -			      int *val2,
>>> -			      long mask) {
>>> +static int
>>> +inv_mpu6050_read_raw(struct iio_dev *indio_dev,
>>> +		     struct iio_chan_spec const *chan,
>>> +		     int *val, int *val2, long mask) {
> 
> Ideally, you'd also convert this form to use
> the open brace on a new line like:
> 
> static int
> inv_mpu6050_read_raw(struct iio_dev *indio_dev,
> 		     struct iio_chan_spec const *chan,
> 		     int *val, int *val2, long mask)
> {
> 
Good point - as it fitted nicely in with Daniel's patch I applied a quick fixup
doing this one and the write_raw.

Thanks Joe,

Jonathan

[RFC PATCH 8/9] i2c: i2c-mux: Allow for NULL select callback

[Daniel Baluta]

From: Adriana Reus <adriana.reus@intel.com>

Add a check in i2c_mux_master_xfer before calling the select callback.
This is necessary so that NULL callbacks can be safely registered.

Signed-off-by: Adriana Reus <adriana.reus@intel.com>
Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
---
 drivers/i2c/i2c-mux.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index 00fc5b1..74d1700 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -46,11 +46,12 @@ static int i2c_mux_master_xfer(struct i2c_adapter *adap,
 {
 	struct i2c_mux_priv *priv = adap->algo_data;
 	struct i2c_adapter *parent = priv->parent;
-	int ret;
+	int ret = 0;
 
 	/* Switch to the right mux port and perform the transfer. */
 
-	ret = priv->select(parent, priv->mux_priv, priv->chan_id);
+	if (priv->select)
+		ret = priv->select(parent, priv->mux_priv, priv->chan_id);
 	if (ret >= 0)
 		ret = __i2c_transfer(parent, msgs, num);
 	if (priv->deselect)
@@ -66,11 +67,12 @@ static int i2c_mux_smbus_xfer(struct i2c_adapter *adap,
 {
 	struct i2c_mux_priv *priv = adap->algo_data;
 	struct i2c_adapter *parent = priv->parent;
-	int ret;
+	int ret = 0;
 
 	/* Select the right mux port and perform the transfer. */
 
-	ret = priv->select(parent, priv->mux_priv, priv->chan_id);
+	if (priv->select)
+		ret = priv->select(parent, priv->mux_priv, priv->chan_id);
 	if (ret >= 0)
 		ret = parent->algo->smbus_xfer(parent, addr, flags,
 					read_write, command, size, data);
-- 
2.5.0

Re: [RFC PATCH 8/9] i2c: i2c-mux: Allow for NULL select callback

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 569 bytes --]

On Thu, Feb 18, 2016 at 05:53:13PM +0200, Daniel Baluta wrote:
> From: Adriana Reus <adriana.reus@intel.com>
> 
> Add a check in i2c_mux_master_xfer before calling the select callback.
> This is necessary so that NULL callbacks can be safely registered.
> 
> Signed-off-by: Adriana Reus <adriana.reus@intel.com>
> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>

Hmm, rather than supporting that in the core, I'd prefer to have the
driver pass an empty function instead. Then, in the driver, we can have
a comment explaining the special situation.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC PATCH 8/9] i2c: i2c-mux: Allow for NULL select callback

[Daniel Baluta]

On Tue, Mar 1, 2016 at 10:30 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> On Thu, Feb 18, 2016 at 05:53:13PM +0200, Daniel Baluta wrote:
>> From: Adriana Reus <adriana.reus@intel.com>
>>
>> Add a check in i2c_mux_master_xfer before calling the select callback.
>> This is necessary so that NULL callbacks can be safely registered.
>>
>> Signed-off-by: Adriana Reus <adriana.reus@intel.com>
>> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
>
> Hmm, rather than supporting that in the core, I'd prefer to have the
> driver pass an empty function instead. Then, in the driver, we can have
> a comment explaining the special situation.

Agree. This seems a better idea forcing the user to explain the situation :).

[RFC PATCH 9/9] iio: imu: inv_mpu6050: Fix deadlock between i2c adapter lock and mpu lock

[Daniel Baluta]

From: Adriana Reus <adriana.reus@intel.com>

This deadlock occurs if the accel/gyro and the sensor on the auxiliary
I2C (in my setup it's an ak8975) are working at the same time.

Scenario:

      T1					T2
     ====				       ====
inv_mpu6050_read_fifo                  aux sensor op (eg. ak8975_read_raw)
        |                                     |
mutex_lock(&indio_dev->mlock)           i2c_transfer
        |                                     |
i2c transaction                         i2c adapter lock
        |                                     |
i2c adapter lock                        i2c_mux_master_xfer
                                              |
                                        inv_mpu6050_select_bypass
                                              |
                                        mutex_lock(&indio_dev->mlock)

When we operate on an mpu sensor the order of locking is mpu lock
followed by the i2c adapter lock. However, when we operate the auxiliary
sensor the order of locking is the other way around.

In order to avoid this enable the bypass mux bit once in the beginning
and remove the select/deselect_bypass functions.

This patch moves the bypass enabling in a separate function
that is called once at probe and removes the functionality from
inv_mpu_select/deselect_bypass.

Another advantage of this approach is that power-wise the mpu chip isn't
powered up at each auxiliary sensor i2c transaction so if only the
compass is used this would be more power efficient.

Signed-off-by: Adriana Reus <adriana.reus@intel.com>
Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c | 88 ++++++-------------------------
 1 file changed, 15 insertions(+), 73 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
index 71bdaa3..8065355 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
@@ -25,82 +25,25 @@ static const struct regmap_config inv_mpu_regmap_config = {
 	.val_bits = 8,
 };
 
-/*
- * The i2c read/write needs to happen in unlocked mode. As the parent
- * adapter is common. If we use locked versions, it will fail as
- * the mux adapter will lock the parent i2c adapter, while calling
- * select/deselect functions.
- */
-static int inv_mpu6050_write_reg_unlocked(struct i2c_client *client,
-					  u8 reg, u8 d)
+static int inv_mpu6050_bypass_en(struct iio_dev *indio_dev)
 {
-	int ret;
-	u8 buf[2] = {reg, d};
-	struct i2c_msg msg[1] = {
-		{
-			.addr = client->addr,
-			.flags = 0,
-			.len = sizeof(buf),
-			.buf = buf,
-		}
-	};
-
-	ret = __i2c_transfer(client->adapter, msg, 1);
-	if (ret != 1)
-		return ret;
-
-	return 0;
-}
-
-static int inv_mpu6050_select_bypass(struct i2c_adapter *adap, void *mux_priv,
-				     u32 chan_id)
-{
-	struct i2c_client *client = mux_priv;
-	struct iio_dev *indio_dev = dev_get_drvdata(&client->dev);
 	struct inv_mpu6050_state *st = iio_priv(indio_dev);
 	int ret = 0;
 
-	/* Use the same mutex which was used everywhere to protect power-op */
-	mutex_lock(&indio_dev->mlock);
-	if (!st->powerup_count) {
-		ret = inv_mpu6050_write_reg_unlocked(client,
-						     st->reg->pwr_mgmt_1, 0);
-		if (ret)
-			goto write_error;
+	ret = inv_mpu6050_set_power_itg(st, true);
+	if (ret)
+		return ret;
 
-		msleep(INV_MPU6050_REG_UP_TIME);
-	}
-	if (!ret) {
-		st->powerup_count++;
-		ret = inv_mpu6050_write_reg_unlocked(client,
-						     st->reg->int_pin_cfg,
-						     INV_MPU6050_INT_PIN_CFG |
-						     INV_MPU6050_BIT_BYPASS_EN);
+	ret = regmap_write(st->map,
+			   st->reg->int_pin_cfg,
+			   INV_MPU6050_INT_PIN_CFG |
+			   INV_MPU6050_BIT_BYPASS_EN);
+	if (ret) {
+		inv_mpu6050_set_power_itg(st, false);
+		return ret;
 	}
-write_error:
-	mutex_unlock(&indio_dev->mlock);
 
-	return ret;
-}
-
-static int inv_mpu6050_deselect_bypass(struct i2c_adapter *adap,
-				       void *mux_priv, u32 chan_id)
-{
-	struct i2c_client *client = mux_priv;
-	struct iio_dev *indio_dev = dev_get_drvdata(&client->dev);
-	struct inv_mpu6050_state *st = iio_priv(indio_dev);
-
-	mutex_lock(&indio_dev->mlock);
-	/* It doesn't really mattter, if any of the calls fails */
-	inv_mpu6050_write_reg_unlocked(client, st->reg->int_pin_cfg,
-				       INV_MPU6050_INT_PIN_CFG);
-	st->powerup_count--;
-	if (!st->powerup_count)
-		inv_mpu6050_write_reg_unlocked(client, st->reg->pwr_mgmt_1,
-					       INV_MPU6050_BIT_SLEEP);
-	mutex_unlock(&indio_dev->mlock);
-
-	return 0;
+	return inv_mpu6050_set_power_itg(st, false);
 }
 
 /**
@@ -129,7 +72,8 @@ static int inv_mpu_probe(struct i2c_client *client,
 		return PTR_ERR(regmap);
 	}
 
-	result = inv_mpu_core_probe(regmap, client->irq, name, NULL);
+	result = inv_mpu_core_probe(regmap, client->irq, name,
+				    inv_mpu6050_bypass_en);
 	if (result < 0)
 		return result;
 
@@ -137,9 +81,7 @@ static int inv_mpu_probe(struct i2c_client *client,
 	st->mux_adapter = i2c_add_mux_adapter(client->adapter,
 					      &client->dev,
 					      client,
-					      0, 0, 0,
-					      inv_mpu6050_select_bypass,
-					      inv_mpu6050_deselect_bypass);
+					      0, 0, 0, NULL, NULL);
 	if (!st->mux_adapter) {
 		result = -ENODEV;
 		goto out_unreg_device;
-- 
2.5.0

Re: [RFC PATCH 9/9] iio: imu: inv_mpu6050: Fix deadlock between i2c adapter lock and mpu lock

[Srinivas Pandruvada]

On Thu, 2016-02-18 at 17:53 +0200, Daniel Baluta wrote:
> From: Adriana Reus <adriana.reus@intel.com>
> 
> This deadlock occurs if the accel/gyro and the sensor on the
> auxiliary
> I2C (in my setup it's an ak8975) are working at the same time.
> 
> Scenario:
> 
>       T1					T2
>      ====				       ====
> inv_mpu6050_read_fifo                  aux sensor op (eg.
> ak8975_read_raw)
>         |                                     |
> mutex_lock(&indio_dev->mlock)           i2c_transfer
>         |                                     |
> i2c transaction                         i2c adapter lock
>         |                                     |
> i2c adapter lock                        i2c_mux_master_xfer
>                                               |
>                                         inv_mpu6050_select_bypass
>                                               |
>                                         mutex_lock(&indio_dev->mlock)
> 
> When we operate on an mpu sensor the order of locking is mpu lock
> followed by the i2c adapter lock. However, when we operate the
> auxiliary
> sensor the order of locking is the other way around.
> 
> In order to avoid this enable the bypass mux bit once in the
> beginning
> and remove the select/deselect_bypass functions.
> 
> This patch moves the bypass enabling in a separate function
> that is called once at probe and removes the functionality from
> inv_mpu_select/deselect_bypass.
> 
> Another advantage of this approach is that power-wise the mpu chip
> isn't
> powered up at each auxiliary sensor i2c transaction so if only the
> compass is used this would be more power efficient.
There was one comment before that power off will cause the the bypass
i2c lines to disable for some MPU chips which this driver can support.
So get some confirmation from Invensense folks that this is OK for all
MPU chips.

Thanks,
Srinivas

> 
> Signed-off-by: Adriana Reus <adriana.reus@intel.com>
> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c | 88 ++++++---------------
> ----------
>  1 file changed, 15 insertions(+), 73 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> index 71bdaa3..8065355 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> @@ -25,82 +25,25 @@ static const struct regmap_config
> inv_mpu_regmap_config = {
>  	.val_bits = 8,
>  };
>  
> -/*
> - * The i2c read/write needs to happen in unlocked mode. As the
> parent
> - * adapter is common. If we use locked versions, it will fail as
> - * the mux adapter will lock the parent i2c adapter, while calling
> - * select/deselect functions.
> - */
> -static int inv_mpu6050_write_reg_unlocked(struct i2c_client *client,
> -					  u8 reg, u8 d)
> +static int inv_mpu6050_bypass_en(struct iio_dev *indio_dev)
>  {
> -	int ret;
> -	u8 buf[2] = {reg, d};
> -	struct i2c_msg msg[1] = {
> -		{
> -			.addr = client->addr,
> -			.flags = 0,
> -			.len = sizeof(buf),
> -			.buf = buf,
> -		}
> -	};
> -
> -	ret = __i2c_transfer(client->adapter, msg, 1);
> -	if (ret != 1)
> -		return ret;
> -
> -	return 0;
> -}
> -
> -static int inv_mpu6050_select_bypass(struct i2c_adapter *adap, void
> *mux_priv,
> -				     u32 chan_id)
> -{
> -	struct i2c_client *client = mux_priv;
> -	struct iio_dev *indio_dev = dev_get_drvdata(&client->dev);
>  	struct inv_mpu6050_state *st = iio_priv(indio_dev);
>  	int ret = 0;
>  
> -	/* Use the same mutex which was used everywhere to protect
> power-op */
> -	mutex_lock(&indio_dev->mlock);
> -	if (!st->powerup_count) {
> -		ret = inv_mpu6050_write_reg_unlocked(client,
> -						     st->reg-
> >pwr_mgmt_1, 0);
> -		if (ret)
> -			goto write_error;
> +	ret = inv_mpu6050_set_power_itg(st, true);
> +	if (ret)
> +		return ret;
>  
> -		msleep(INV_MPU6050_REG_UP_TIME);
> -	}
> -	if (!ret) {
> -		st->powerup_count++;
> -		ret = inv_mpu6050_write_reg_unlocked(client,
> -						     st->reg-
> >int_pin_cfg,
> -						     INV_MPU6050_INT
> _PIN_CFG |
> -						     INV_MPU6050_BIT
> _BYPASS_EN);
> +	ret = regmap_write(st->map,
> +			   st->reg->int_pin_cfg,
> +			   INV_MPU6050_INT_PIN_CFG |
> +			   INV_MPU6050_BIT_BYPASS_EN);
> +	if (ret) {
> +		inv_mpu6050_set_power_itg(st, false);
> +		return ret;
>  	}
> -write_error:
> -	mutex_unlock(&indio_dev->mlock);
>  
> -	return ret;
> -}
> -
> -static int inv_mpu6050_deselect_bypass(struct i2c_adapter *adap,
> -				       void *mux_priv, u32 chan_id)
> -{
> -	struct i2c_client *client = mux_priv;
> -	struct iio_dev *indio_dev = dev_get_drvdata(&client->dev);
> -	struct inv_mpu6050_state *st = iio_priv(indio_dev);
> -
> -	mutex_lock(&indio_dev->mlock);
> -	/* It doesn't really mattter, if any of the calls fails */
> -	inv_mpu6050_write_reg_unlocked(client, st->reg->int_pin_cfg,
> -				       INV_MPU6050_INT_PIN_CFG);
> -	st->powerup_count--;
> -	if (!st->powerup_count)
> -		inv_mpu6050_write_reg_unlocked(client, st->reg-
> >pwr_mgmt_1,
> -					       INV_MPU6050_BIT_SLEEP
> );
> -	mutex_unlock(&indio_dev->mlock);
> -
> -	return 0;
> +	return inv_mpu6050_set_power_itg(st, false);
>  }
>  
>  /**
> @@ -129,7 +72,8 @@ static int inv_mpu_probe(struct i2c_client
> *client,
>  		return PTR_ERR(regmap);
>  	}
>  
> -	result = inv_mpu_core_probe(regmap, client->irq, name,
> NULL);
> +	result = inv_mpu_core_probe(regmap, client->irq, name,
> +				    inv_mpu6050_bypass_en);
>  	if (result < 0)
>  		return result;
>  
> @@ -137,9 +81,7 @@ static int inv_mpu_probe(struct i2c_client
> *client,
>  	st->mux_adapter = i2c_add_mux_adapter(client->adapter,
>  					      &client->dev,
>  					      client,
> -					      0, 0, 0,
> -					      inv_mpu6050_select_byp
> ass,
> -					      inv_mpu6050_deselect_b
> ypass);
> +					      0, 0, 0, NULL, NULL);
>  	if (!st->mux_adapter) {
>  		result = -ENODEV;
>  		goto out_unreg_device;

RE: [RFC PATCH 9/9] iio: imu: inv_mpu6050: Fix deadlock between i2c adapter lock and mpu lock

[Ge Gao]

"there was one comment before that power off will cause the the bypass i2c lines to disable for some MPU chips which this driver can support."
This is right. I remember there was some code about it earlier trying to take advantage of the secondary bus without enabling the MPU chip itself. This is impossible. The secondary bus interface is part of the chip and it will not function without enabling the chip. Without enabling gyro/accel engine, enabling the chip alone takes very little power. It is not worth the trouble to do that.

Thanks,

Ge



-----Original Message-----
From: Srinivas Pandruvada [mailto:srinivas.pandruvada@linux.intel.com] 
Sent: Thursday, February 18, 2016 10:17 AM
To: Daniel Baluta; jic23@kernel.org
Cc: knaack.h@gmx.de; lars@metafoo.de; pmeerw@pmeerw.net; linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org; wsa@the-dreams.de; linux-i2c@vger.kernel.org; lucas.demarchi@intel.com; Ge Gao; adi.reus@gmail.com; cmo@melexis.com; mwelling@ieee.org
Subject: Re: [RFC PATCH 9/9] iio: imu: inv_mpu6050: Fix deadlock between i2c adapter lock and mpu lock

On Thu, 2016-02-18 at 17:53 +0200, Daniel Baluta wrote:
> From: Adriana Reus <adriana.reus@intel.com>
> 
> This deadlock occurs if the accel/gyro and the sensor on the auxiliary 
> I2C (in my setup it's an ak8975) are working at the same time.
> 
> Scenario:
> 
>       T1					T2
>      ====				       ====
> inv_mpu6050_read_fifo                  aux sensor op (eg.
> ak8975_read_raw)
>         |                                     |
> mutex_lock(&indio_dev->mlock)           i2c_transfer
>         |                                     | i2c transaction                         
> i2c adapter lock
>         |                                     | i2c adapter lock                        
> i2c_mux_master_xfer
>                                               |
>                                         inv_mpu6050_select_bypass
>                                               |
>                                         mutex_lock(&indio_dev->mlock)
> 
> When we operate on an mpu sensor the order of locking is mpu lock 
> followed by the i2c adapter lock. However, when we operate the 
> auxiliary sensor the order of locking is the other way around.
> 
> In order to avoid this enable the bypass mux bit once in the beginning 
> and remove the select/deselect_bypass functions.
> 
> This patch moves the bypass enabling in a separate function that is 
> called once at probe and removes the functionality from 
> inv_mpu_select/deselect_bypass.
> 
> Another advantage of this approach is that power-wise the mpu chip 
> isn't powered up at each auxiliary sensor i2c transaction so if only 
> the compass is used this would be more power efficient.
There was one comment before that power off will cause the the bypass i2c lines to disable for some MPU chips which this driver can support.
So get some confirmation from Invensense folks that this is OK for all MPU chips.


Thanks,
Srinivas

> 
> Signed-off-by: Adriana Reus <adriana.reus@intel.com>
> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c | 88 ++++++---------------
> ----------
>  1 file changed, 15 insertions(+), 73 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> index 71bdaa3..8065355 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> @@ -25,82 +25,25 @@ static const struct regmap_config 
> inv_mpu_regmap_config = {
>  	.val_bits = 8,
>  };
>  
> -/*
> - * The i2c read/write needs to happen in unlocked mode. As the parent
> - * adapter is common. If we use locked versions, it will fail as
> - * the mux adapter will lock the parent i2c adapter, while calling
> - * select/deselect functions.
> - */
> -static int inv_mpu6050_write_reg_unlocked(struct i2c_client *client,
> -					  u8 reg, u8 d)
> +static int inv_mpu6050_bypass_en(struct iio_dev *indio_dev)
>  {
> -	int ret;
> -	u8 buf[2] = {reg, d};
> -	struct i2c_msg msg[1] = {
> -		{
> -			.addr = client->addr,
> -			.flags = 0,
> -			.len = sizeof(buf),
> -			.buf = buf,
> -		}
> -	};
> -
> -	ret = __i2c_transfer(client->adapter, msg, 1);
> -	if (ret != 1)
> -		return ret;
> -
> -	return 0;
> -}
> -
> -static int inv_mpu6050_select_bypass(struct i2c_adapter *adap, void 
> *mux_priv,
> -				     u32 chan_id)
> -{
> -	struct i2c_client *client = mux_priv;
> -	struct iio_dev *indio_dev = dev_get_drvdata(&client->dev);
>  	struct inv_mpu6050_state *st = iio_priv(indio_dev);
>  	int ret = 0;
>  
> -	/* Use the same mutex which was used everywhere to protect
> power-op */
> -	mutex_lock(&indio_dev->mlock);
> -	if (!st->powerup_count) {
> -		ret = inv_mpu6050_write_reg_unlocked(client,
> -						     st->reg-
> >pwr_mgmt_1, 0);
> -		if (ret)
> -			goto write_error;
> +	ret = inv_mpu6050_set_power_itg(st, true);
> +	if (ret)
> +		return ret;
>  
> -		msleep(INV_MPU6050_REG_UP_TIME);
> -	}
> -	if (!ret) {
> -		st->powerup_count++;
> -		ret = inv_mpu6050_write_reg_unlocked(client,
> -						     st->reg-
> >int_pin_cfg,
> -						     INV_MPU6050_INT
> _PIN_CFG |
> -						     INV_MPU6050_BIT
> _BYPASS_EN);
> +	ret = regmap_write(st->map,
> +			   st->reg->int_pin_cfg,
> +			   INV_MPU6050_INT_PIN_CFG |
> +			   INV_MPU6050_BIT_BYPASS_EN);
> +	if (ret) {
> +		inv_mpu6050_set_power_itg(st, false);
> +		return ret;
>  	}
> -write_error:
> -	mutex_unlock(&indio_dev->mlock);
>  
> -	return ret;
> -}
> -
> -static int inv_mpu6050_deselect_bypass(struct i2c_adapter *adap,
> -				       void *mux_priv, u32 chan_id)
> -{
> -	struct i2c_client *client = mux_priv;
> -	struct iio_dev *indio_dev = dev_get_drvdata(&client->dev);
> -	struct inv_mpu6050_state *st = iio_priv(indio_dev);
> -
> -	mutex_lock(&indio_dev->mlock);
> -	/* It doesn't really mattter, if any of the calls fails */
> -	inv_mpu6050_write_reg_unlocked(client, st->reg->int_pin_cfg,
> -				       INV_MPU6050_INT_PIN_CFG);
> -	st->powerup_count--;
> -	if (!st->powerup_count)
> -		inv_mpu6050_write_reg_unlocked(client, st->reg-
> >pwr_mgmt_1,
> -					       INV_MPU6050_BIT_SLEEP
> );
> -	mutex_unlock(&indio_dev->mlock);
> -
> -	return 0;
> +	return inv_mpu6050_set_power_itg(st, false);
>  }
>  
>  /**
> @@ -129,7 +72,8 @@ static int inv_mpu_probe(struct i2c_client *client,
>  		return PTR_ERR(regmap);
>  	}
>  
> -	result = inv_mpu_core_probe(regmap, client->irq, name,
> NULL);
> +	result = inv_mpu_core_probe(regmap, client->irq, name,
> +				    inv_mpu6050_bypass_en);
>  	if (result < 0)
>  		return result;
>  
> @@ -137,9 +81,7 @@ static int inv_mpu_probe(struct i2c_client *client,
>  	st->mux_adapter = i2c_add_mux_adapter(client->adapter,
>  					      &client->dev,
>  					      client,
> -					      0, 0, 0,
> -					      inv_mpu6050_select_byp
> ass,
> -					      inv_mpu6050_deselect_b
> ypass);
> +					      0, 0, 0, NULL, NULL);
>  	if (!st->mux_adapter) {
>  		result = -ENODEV;
>  		goto out_unreg_device;

Re: [RFC PATCH 9/9] iio: imu: inv_mpu6050: Fix deadlock between i2c adapter lock and mpu lock

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1901 bytes --]

On Thu, Feb 18, 2016 at 05:53:14PM +0200, Daniel Baluta wrote:
> From: Adriana Reus <adriana.reus@intel.com>
> 
> This deadlock occurs if the accel/gyro and the sensor on the auxiliary
> I2C (in my setup it's an ak8975) are working at the same time.
> 
> Scenario:
> 
>       T1					T2
>      ====				       ====
> inv_mpu6050_read_fifo                  aux sensor op (eg. ak8975_read_raw)
>         |                                     |
> mutex_lock(&indio_dev->mlock)           i2c_transfer
>         |                                     |
> i2c transaction                         i2c adapter lock
>         |                                     |
> i2c adapter lock                        i2c_mux_master_xfer
>                                               |
>                                         inv_mpu6050_select_bypass
>                                               |
>                                         mutex_lock(&indio_dev->mlock)
> 
> When we operate on an mpu sensor the order of locking is mpu lock
> followed by the i2c adapter lock. However, when we operate the auxiliary
> sensor the order of locking is the other way around.
> 
> In order to avoid this enable the bypass mux bit once in the beginning
> and remove the select/deselect_bypass functions.
> 
> This patch moves the bypass enabling in a separate function
> that is called once at probe and removes the functionality from
> inv_mpu_select/deselect_bypass.
> 
> Another advantage of this approach is that power-wise the mpu chip isn't
> powered up at each auxiliary sensor i2c transaction so if only the
> compass is used this would be more power efficient.

I think the comments (power must be enabled on select) rendered this
solution not acceptable. What about using mutex_trylock in
inv_mpu6050_select_bypass() and returning -EAGAIN if the lock is already
held?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC PATCH 9/9] iio: imu: inv_mpu6050: Fix deadlock between i2c adapter lock and mpu lock

[Daniel Baluta]

On Tue, Mar 1, 2016 at 10:50 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> On Thu, Feb 18, 2016 at 05:53:14PM +0200, Daniel Baluta wrote:
>> From: Adriana Reus <adriana.reus@intel.com>
>>
>> This deadlock occurs if the accel/gyro and the sensor on the auxiliary
>> I2C (in my setup it's an ak8975) are working at the same time.
>>
>> Scenario:
>>
>>       T1                                      T2
>>      ====                                    ====
>> inv_mpu6050_read_fifo                  aux sensor op (eg. ak8975_read_raw)
>>         |                                     |
>> mutex_lock(&indio_dev->mlock)           i2c_transfer
>>         |                                     |
>> i2c transaction                         i2c adapter lock
>>         |                                     |
>> i2c adapter lock                        i2c_mux_master_xfer
>>                                               |
>>                                         inv_mpu6050_select_bypass
>>                                               |
>>                                         mutex_lock(&indio_dev->mlock)
>>
>> When we operate on an mpu sensor the order of locking is mpu lock
>> followed by the i2c adapter lock. However, when we operate the auxiliary
>> sensor the order of locking is the other way around.
>>
>> In order to avoid this enable the bypass mux bit once in the beginning
>> and remove the select/deselect_bypass functions.
>>
>> This patch moves the bypass enabling in a separate function
>> that is called once at probe and removes the functionality from
>> inv_mpu_select/deselect_bypass.
>>
>> Another advantage of this approach is that power-wise the mpu chip isn't
>> powered up at each auxiliary sensor i2c transaction so if only the
>> compass is used this would be more power efficient.
>
> I think the comments (power must be enabled on select) rendered this
> solution not acceptable. What about using mutex_trylock in
> inv_mpu6050_select_bypass() and returning -EAGAIN if the lock is already
> held?

Well, yes this would be a very good temporary solution. I'm afraid tough
that reading at high rates accel/gyro data will starve the aux sensor readings.

I looked into the I2C adapter / mux code but I got lost rapidly :). It
feels like
the natural solution would be for the I2C core to not hold the adapter lock
while doing transactions on the muxed child adapter.

Anyhow, sometimes starving is better than deadlocking so I will send a patch
with your suggestion.

thanks,
Daniel.

Re: [RFC PATCH 9/9] iio: imu: inv_mpu6050: Fix deadlock between i2c adapter lock and mpu lock

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 716 bytes --]


> I looked into the I2C adapter / mux code but I got lost rapidly :). It
> feels like the natural solution would be for the I2C core to not hold
> the adapter lock while doing transactions on the muxed child adapter.

The patch series I mentioned to you does exactly that. It locks only the
mux side of the muxed bus, not the whole parent adapter. It didn't work
for you because the mux driver maybe needed some adaptions as well?

However, I am still undecided if that series should go upstream because
it makes the mux code another magnitude more complex. And while this
seems to be the second issue which could be fixed by that series, both
issues are corner cases, so I am not sure it is worth the complexity.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC PATCH 0/9] iio: Fix ABBA deadlock in inv-mpu6050

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1362 bytes --]

On Thu, Feb 18, 2016 at 05:53:05PM +0200, Daniel Baluta wrote:
> Sending this as an RFC because I don't know if style fixes are appropriate
> for this driver and also not sure if deadlock fix is the best solution.
> 
> I2C people should only look at patches 8/9 and 9/9.
> 
> The mpu6050 accel+gyro combo has an auxiliary I2C bus, allowing for
> an auxiliary sensor to be connected (eg. a magnetometer).
> 
> The mpu can either act as an I2C master (functionality not currently
> implemented in the driver) or it can use a bypass multiplexer which
> directly connects the auxiliary I2C bus pins to the main I2C bus pins [1]
> Currently the driver implements the bypass mode via an I2C mux.
> 
> This patchset fixes a deadlock in inv-mpu6050 IMU which happens when
> magnetometer (on auxiliary I2C bus) runs in parallel with accel or gyro.
> 
> First 7 patches do some cleanup in order to make INV MPU6050 code easier
> to read.
> 
> Patch number 8 allows passing NULL select callbacks to i2c_mux_master_xfer
> and i2c_mux_smbus_xfer.
> 
> Patch number 9 actually fixes the deadlock.

We recently had a bigger patch series fixing locking problems related to
muxes. I sadly didn't have the time to review it. Can you have a look if
it helps your case?

http://thread.gmane.org/gmane.linux.drivers.i2c/26169

Thanks,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC PATCH 0/9] iio: Fix ABBA deadlock in inv-mpu6050

[Daniel Baluta]

On Mon, Feb 22, 2016 at 1:17 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
> On Thu, Feb 18, 2016 at 05:53:05PM +0200, Daniel Baluta wrote:
>> Sending this as an RFC because I don't know if style fixes are appropriate
>> for this driver and also not sure if deadlock fix is the best solution.
>>
>> I2C people should only look at patches 8/9 and 9/9.
>>
>> The mpu6050 accel+gyro combo has an auxiliary I2C bus, allowing for
>> an auxiliary sensor to be connected (eg. a magnetometer).
>>
>> The mpu can either act as an I2C master (functionality not currently
>> implemented in the driver) or it can use a bypass multiplexer which
>> directly connects the auxiliary I2C bus pins to the main I2C bus pins [1]
>> Currently the driver implements the bypass mode via an I2C mux.
>>
>> This patchset fixes a deadlock in inv-mpu6050 IMU which happens when
>> magnetometer (on auxiliary I2C bus) runs in parallel with accel or gyro.
>>
>> First 7 patches do some cleanup in order to make INV MPU6050 code easier
>> to read.
>>
>> Patch number 8 allows passing NULL select callbacks to i2c_mux_master_xfer
>> and i2c_mux_smbus_xfer.
>>
>> Patch number 9 actually fixes the deadlock.
>
> We recently had a bigger patch series fixing locking problems related to
> muxes. I sadly didn't have the time to review it. Can you have a look if
> it helps your case?
>
> http://thread.gmane.org/gmane.linux.drivers.i2c/26169

Thanks Wolfram,

Will look into it.

Re: [RFC PATCH 0/9] iio: Fix ABBA deadlock in inv-mpu6050

[Daniel Baluta]

On Mon, Feb 22, 2016 at 1:17 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
> On Thu, Feb 18, 2016 at 05:53:05PM +0200, Daniel Baluta wrote:
>> Sending this as an RFC because I don't know if style fixes are appropriate
>> for this driver and also not sure if deadlock fix is the best solution.
>>
>> I2C people should only look at patches 8/9 and 9/9.
>>
>> The mpu6050 accel+gyro combo has an auxiliary I2C bus, allowing for
>> an auxiliary sensor to be connected (eg. a magnetometer).
>>
>> The mpu can either act as an I2C master (functionality not currently
>> implemented in the driver) or it can use a bypass multiplexer which
>> directly connects the auxiliary I2C bus pins to the main I2C bus pins [1]
>> Currently the driver implements the bypass mode via an I2C mux.
>>
>> This patchset fixes a deadlock in inv-mpu6050 IMU which happens when
>> magnetometer (on auxiliary I2C bus) runs in parallel with accel or gyro.
>>
>> First 7 patches do some cleanup in order to make INV MPU6050 code easier
>> to read.
>>
>> Patch number 8 allows passing NULL select callbacks to i2c_mux_master_xfer
>> and i2c_mux_smbus_xfer.
>>
>> Patch number 9 actually fixes the deadlock.
>
> We recently had a bigger patch series fixing locking problems related to
> muxes. I sadly didn't have the time to review it. Can you have a look if
> it helps your case?
>
> http://thread.gmane.org/gmane.linux.drivers.i2c/26169


Hi Wolfram,

Tested this and the deadlock is still there :(.

It can be easily reproduced with following patch which enlarges
the race window:

iff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index a3f5070..279d6e0 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -232,6 +232,7 @@ inv_mpu6050_read_raw(struct iio_dev *indio_dev,
                ret = IIO_VAL_INT;
                result = 0;
                mutex_lock(&indio_dev->mlock);
+               msleep(12000);
                if (!st->chip_config.enable) {
                        result = inv_mpu6050_set_power_itg(st, true);
                        if (result)


using the following commands:

$ cat /sys/bus/iio/devices/iio:device0/in_accel_x_raw
$ cat /sys/bus/iio/devices/iio:device1/in_magn_x_raw

Off topic: I'm very curious why I didn't get a lockdep splat while running this,
might be because adapter lock is a rt_mutex.

thanks,
Daniel.

Re: [RFC PATCH 0/9] iio: Fix ABBA deadlock in inv-mpu6050

[Peter Rosin]

From: Peter Rosin <peda@axentia.se>

Hi Daniel,

Daniel Baluta wrote:
>On Mon, Feb 22, 2016 at 1:17 AM, Wolfram Sang wrote:
>> On Thu, Feb 18, 2016 at 05:53:05PM +0200, Daniel Baluta wrote:
>>> Sending this as an RFC because I don't know if style fixes are appropriate
>>> for this driver and also not sure if deadlock fix is the best solution.

*snip*

>> We recently had a bigger patch series fixing locking problems related to
>> muxes. I sadly didn't have the time to review it. Can you have a look if
>> it helps your case?
>>
>> http://thread.gmane.org/gmane.linux.drivers.i2c/26169
> 
> Tested this and the deadlock is still there :(.

I assume that when you tested v3 of my series, you did nothing
to actually make use of the new stuff available in the mux-core?
If you didn't do anything to make use of the new stuff, the
driver should behave as before.

So, please try this patch on top of my recently posted v4 of the
"i2c mux cleanup and locking update" series [1]. I have not tested
this patch at all, but I have the feeling it could do the trick...

Cheers,
Peter

[1] https://marc.info/?l=linux-iio&m=145704469628330&w=3

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index 642f22013d17..02b56e631973 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -79,35 +79,6 @@ int inv_mpu6050_write_reg(struct inv_mpu6050_state *st, int reg, u8 d)
 	return i2c_smbus_write_i2c_block_data(st->client, reg, 1, &d);
 }
 
-/*
- * The i2c read/write needs to happen in unlocked mode. As the parent
- * adapter is common. If we use locked versions, it will fail as
- * the mux adapter will lock the parent i2c adapter, while calling
- * select/deselect functions.
- */
-static int inv_mpu6050_write_reg_unlocked(struct inv_mpu6050_state *st,
-					  u8 reg, u8 d)
-{
-	int ret;
-	u8 buf[2];
-	struct i2c_msg msg[1] = {
-		{
-			.addr = st->client->addr,
-			.flags = 0,
-			.len = sizeof(buf),
-			.buf = buf,
-		}
-	};
-
-	buf[0] = reg;
-	buf[1] = d;
-	ret = __i2c_transfer(st->client->adapter, msg, 1);
-	if (ret != 1)
-		return ret;
-
-	return 0;
-}
-
 static int inv_mpu6050_select_bypass(struct i2c_mux_core *muxc, u32 chan_id)
 {
 	struct iio_dev *indio_dev = i2c_mux_priv(muxc);
@@ -117,8 +88,7 @@ static int inv_mpu6050_select_bypass(struct i2c_mux_core *muxc, u32 chan_id)
 	/* Use the same mutex which was used everywhere to protect power-op */
 	mutex_lock(&indio_dev->mlock);
 	if (!st->powerup_count) {
-		ret = inv_mpu6050_write_reg_unlocked(st, st->reg->pwr_mgmt_1,
-						     0);
+		ret = inv_mpu6050_write_reg(st, st->reg->pwr_mgmt_1, 0);
 		if (ret)
 			goto write_error;
 
@@ -126,9 +96,9 @@ static int inv_mpu6050_select_bypass(struct i2c_mux_core *muxc, u32 chan_id)
 	}
 	if (!ret) {
 		st->powerup_count++;
-		ret = inv_mpu6050_write_reg_unlocked(st, st->reg->int_pin_cfg,
-						     st->client->irq |
-						     INV_MPU6050_BIT_BYPASS_EN);
+		ret = inv_mpu6050_write_reg(st, st->reg->int_pin_cfg,
+					    st->client->irq |
+					    INV_MPU6050_BIT_BYPASS_EN);
 	}
 write_error:
 	mutex_unlock(&indio_dev->mlock);
@@ -143,12 +113,11 @@ static int inv_mpu6050_deselect_bypass(struct i2c_mux_core *muxc, u32 chan_id)
 
 	mutex_lock(&indio_dev->mlock);
 	/* It doesn't really mattter, if any of the calls fails */
-	inv_mpu6050_write_reg_unlocked(st, st->reg->int_pin_cfg,
-				       st->client->irq);
+	inv_mpu6050_write_reg(st, st->reg->int_pin_cfg, st->client->irq);
 	st->powerup_count--;
 	if (!st->powerup_count)
-		inv_mpu6050_write_reg_unlocked(st, st->reg->pwr_mgmt_1,
-					       INV_MPU6050_BIT_SLEEP);
+		inv_mpu6050_write_reg(st, st->reg->pwr_mgmt_1,
+				      INV_MPU6050_BIT_SLEEP);
 	mutex_unlock(&indio_dev->mlock);
 
 	return 0;
@@ -839,8 +808,8 @@ static int inv_mpu_probe(struct i2c_client *client,
 		goto out_remove_trigger;
 	}
 
-	st->muxc = i2c_mux_one_adapter(client->adapter, &client->dev, 0, 0,
-				       0, 0, 0,
+	st->muxc = i2c_mux_one_adapter(client->adapter, &client->dev, 0,
+				       I2C_CONTROLLED_MUX, 0, 0, 0,
 				       inv_mpu6050_select_bypass,
 				       inv_mpu6050_deselect_bypass);
 	if (IS_ERR(st->muxc)) {
-- 
2.1.4

Re: [RFC PATCH 0/9] iio: Fix ABBA deadlock in inv-mpu6050

[Daniel Baluta]

On Fri, Mar 4, 2016 at 1:09 AM, Peter Rosin <peda@lysator.liu.se> wrote:
> From: Peter Rosin <peda@axentia.se>
>
> Hi Daniel,
>
> Daniel Baluta wrote:
>>On Mon, Feb 22, 2016 at 1:17 AM, Wolfram Sang wrote:
>>> On Thu, Feb 18, 2016 at 05:53:05PM +0200, Daniel Baluta wrote:
>>>> Sending this as an RFC because I don't know if style fixes are appropriate
>>>> for this driver and also not sure if deadlock fix is the best solution.
>
> *snip*
>
>>> We recently had a bigger patch series fixing locking problems related to
>>> muxes. I sadly didn't have the time to review it. Can you have a look if
>>> it helps your case?
>>>
>>> http://thread.gmane.org/gmane.linux.drivers.i2c/26169
>>
>> Tested this and the deadlock is still there :(.
>
> I assume that when you tested v3 of my series, you did nothing
> to actually make use of the new stuff available in the mux-core?
> If you didn't do anything to make use of the new stuff, the
> driver should behave as before.

You are right. I used v3 as it is.
>
> So, please try this patch on top of my recently posted v4 of the
> "i2c mux cleanup and locking update" series [1]. I have not tested
> this patch at all, but I have the feeling it could do the trick...

This patch fixes the problem! :P

> Cheers,
> Peter
>
> [1] https://marc.info/?l=linux-iio&m=145704469628330&w=3
>
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> index 642f22013d17..02b56e631973 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> @@ -79,35 +79,6 @@ int inv_mpu6050_write_reg(struct inv_mpu6050_state *st, int reg, u8 d)
>         return i2c_smbus_write_i2c_block_data(st->client, reg, 1, &d);
>  }
>
> -/*
> - * The i2c read/write needs to happen in unlocked mode. As the parent
> - * adapter is common. If we use locked versions, it will fail as
> - * the mux adapter will lock the parent i2c adapter, while calling
> - * select/deselect functions.
> - */
> -static int inv_mpu6050_write_reg_unlocked(struct inv_mpu6050_state *st,
> -                                         u8 reg, u8 d)
> -{
> -       int ret;
> -       u8 buf[2];
> -       struct i2c_msg msg[1] = {
> -               {
> -                       .addr = st->client->addr,
> -                       .flags = 0,
> -                       .len = sizeof(buf),
> -                       .buf = buf,
> -               }
> -       };
> -
> -       buf[0] = reg;
> -       buf[1] = d;
> -       ret = __i2c_transfer(st->client->adapter, msg, 1);
> -       if (ret != 1)
> -               return ret;
> -
> -       return 0;
> -}
> -
>  static int inv_mpu6050_select_bypass(struct i2c_mux_core *muxc, u32 chan_id)
>  {
>         struct iio_dev *indio_dev = i2c_mux_priv(muxc);
> @@ -117,8 +88,7 @@ static int inv_mpu6050_select_bypass(struct i2c_mux_core *muxc, u32 chan_id)
>         /* Use the same mutex which was used everywhere to protect power-op */
>         mutex_lock(&indio_dev->mlock);
>         if (!st->powerup_count) {
> -               ret = inv_mpu6050_write_reg_unlocked(st, st->reg->pwr_mgmt_1,
> -                                                    0);
> +               ret = inv_mpu6050_write_reg(st, st->reg->pwr_mgmt_1, 0);
>                 if (ret)
>                         goto write_error;
>
> @@ -126,9 +96,9 @@ static int inv_mpu6050_select_bypass(struct i2c_mux_core *muxc, u32 chan_id)
>         }
>         if (!ret) {
>                 st->powerup_count++;
> -               ret = inv_mpu6050_write_reg_unlocked(st, st->reg->int_pin_cfg,
> -                                                    st->client->irq |
> -                                                    INV_MPU6050_BIT_BYPASS_EN);
> +               ret = inv_mpu6050_write_reg(st, st->reg->int_pin_cfg,
> +                                           st->client->irq |
> +                                           INV_MPU6050_BIT_BYPASS_EN);
>         }
>  write_error:
>         mutex_unlock(&indio_dev->mlock);
> @@ -143,12 +113,11 @@ static int inv_mpu6050_deselect_bypass(struct i2c_mux_core *muxc, u32 chan_id)
>
>         mutex_lock(&indio_dev->mlock);
>         /* It doesn't really mattter, if any of the calls fails */
> -       inv_mpu6050_write_reg_unlocked(st, st->reg->int_pin_cfg,
> -                                      st->client->irq);
> +       inv_mpu6050_write_reg(st, st->reg->int_pin_cfg, st->client->irq);
>         st->powerup_count--;
>         if (!st->powerup_count)
> -               inv_mpu6050_write_reg_unlocked(st, st->reg->pwr_mgmt_1,
> -                                              INV_MPU6050_BIT_SLEEP);
> +               inv_mpu6050_write_reg(st, st->reg->pwr_mgmt_1,
> +                                     INV_MPU6050_BIT_SLEEP);
>         mutex_unlock(&indio_dev->mlock);
>
>         return 0;
> @@ -839,8 +808,8 @@ static int inv_mpu_probe(struct i2c_client *client,
>                 goto out_remove_trigger;
>         }
>
> -       st->muxc = i2c_mux_one_adapter(client->adapter, &client->dev, 0, 0,
> -                                      0, 0, 0,
> +       st->muxc = i2c_mux_one_adapter(client->adapter, &client->dev, 0,
> +                                      I2C_CONTROLLED_MUX, 0, 0, 0,
>                                        inv_mpu6050_select_bypass,
>                                        inv_mpu6050_deselect_bypass);
>         if (IS_ERR(st->muxc)) {
> --
> 2.1.4
>
> --
> 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