[PATCH 1/3] iio: imu: inv_mpu6050: ACPI enumeration

[PATCH 1/3] iio: imu: inv_mpu6050: ACPI enumeration

[Srinivas Pandruvada]

Added changes so that the module can be enumerated via ACPI.
Also if there is no platform data available, it will use a default
orientation data.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index df7f1e1..4a76697 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -24,8 +24,16 @@
 #include <linux/kfifo.h>
 #include <linux/spinlock.h>
 #include <linux/iio/iio.h>
+#include <linux/acpi.h>
 #include "inv_mpu_iio.h"
 
+/* Define some default platform data, if not supplied */
+static struct inv_mpu6050_platform_data inv_def_platform_data = {
+	.orientation = {-1,  0,  0,
+			0,  1,  0,
+			0,  0, -1 }
+};
+
 /*
  * this is the gyro scale translated from dynamic range plus/minus
  * {250, 500, 1000, 2000} to rad/s
@@ -662,6 +670,7 @@ static int inv_mpu_probe(struct i2c_client *client,
 	struct inv_mpu6050_state *st;
 	struct iio_dev *indio_dev;
 	int result;
+	char *name;
 
 	if (!i2c_check_functionality(client->adapter,
 		I2C_FUNC_SMBUS_I2C_BLOCK))
@@ -673,7 +682,10 @@ static int inv_mpu_probe(struct i2c_client *client,
 
 	st = iio_priv(indio_dev);
 	st->client = client;
-	st->plat_data = *(struct inv_mpu6050_platform_data
+	if (!dev_get_platdata(&client->dev))
+		st->plat_data = inv_def_platform_data;
+	else
+		st->plat_data = *(struct inv_mpu6050_platform_data
 				*)dev_get_platdata(&client->dev);
 	/* power is turned on inside check chip type*/
 	result = inv_check_and_setup_chip(st, id);
@@ -689,7 +701,14 @@ static int inv_mpu_probe(struct i2c_client *client,
 
 	i2c_set_clientdata(client, indio_dev);
 	indio_dev->dev.parent = &client->dev;
-	indio_dev->name = id->name;
+	if (id && id->name)
+		name = (char *)id->name;
+	else {
+		name = (char *)dev_name(&client->dev);
+		if (!name)
+			dev_err(&client->dev, "No iio dev name\n");
+	}
+	indio_dev->name = name;
 	indio_dev->channels = inv_mpu_channels;
 	indio_dev->num_channels = ARRAY_SIZE(inv_mpu_channels);
 
@@ -770,12 +789,19 @@ static const struct i2c_device_id inv_mpu_id[] = {
 
 MODULE_DEVICE_TABLE(i2c, inv_mpu_id);
 
+static const struct acpi_device_id inv_acpi_match[] = {
+	{"INVN6500", 0},
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, inv_acpi_match);
+
 static struct i2c_driver inv_mpu_driver = {
 	.probe		=	inv_mpu_probe,
 	.remove		=	inv_mpu_remove,
 	.id_table	=	inv_mpu_id,
 	.driver = {
 		.owner	=	THIS_MODULE,
+		.acpi_match_table = ACPI_PTR(inv_acpi_match),
 		.name	=	"inv-mpu6050",
 		.pm     =       INV_MPU6050_PMOPS,
 	},
-- 
1.7.11.7

[PATCH 2/3] iio: imu: inv_mpu6050: Enable default bypass mode

[Srinivas Pandruvada]

This chip has two modes to control secondary sensor attached to it.
One is master mode and another is bypass mode. In master mode
MPU6500 will directly communicates to the secondary sensor device
attached to it. This can support very few secondary devices in this
mode.
But when configured in bypass mode the i2c lines are directly connected
to host i2c bus controller.
Since in master mode it can only support few devices and they are not
implemented in this driver, set the default mode to bypass mode.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 20 +++++++++++++++++++-
 drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h  |  4 ++++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index 4a76697..6ba565a 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -61,6 +61,7 @@ static const struct inv_mpu6050_reg_map reg_set_6050 = {
 	.int_enable             = INV_MPU6050_REG_INT_ENABLE,
 	.pwr_mgmt_1             = INV_MPU6050_REG_PWR_MGMT_1,
 	.pwr_mgmt_2             = INV_MPU6050_REG_PWR_MGMT_2,
+	.int_pin_cfg		= INV_MPU6050_REG_INT_PIN_CFG,
 };
 
 static const struct inv_mpu6050_chip_config chip_config_6050 = {
@@ -616,6 +617,21 @@ static const struct iio_info mpu_info = {
 	.validate_trigger = inv_mpu6050_validate_trigger,
 };
 
+int inv_set_bypass_status(struct inv_mpu6050_state *st, bool enable)
+{
+	int ret;
+
+	if (enable)
+		ret = inv_mpu6050_write_reg(st, st->reg->int_pin_cfg,
+					st->client->irq |
+						INV_MPU6050_BIT_BYPASS_EN);
+	else
+		ret = inv_mpu6050_write_reg(st, st->reg->int_pin_cfg,
+					st->client->irq);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(inv_set_bypass_status);
+
 /**
  *  inv_check_and_setup_chip() - check and setup chip.
  */
@@ -654,7 +670,9 @@ static int inv_check_and_setup_chip(struct inv_mpu6050_state *st,
 	if (result)
 		return result;
 
-	return 0;
+	result = inv_set_bypass_status(st, true);
+
+	return result;
 }
 
 /**
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
index f383955..de5aa22 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
@@ -54,6 +54,7 @@ struct inv_mpu6050_reg_map {
 	u8 int_enable;
 	u8 pwr_mgmt_1;
 	u8 pwr_mgmt_2;
+	u8 int_pin_cfg;
 };
 
 /*device enum */
@@ -185,6 +186,9 @@ struct inv_mpu6050_state {
 #define INV_MPU6050_MIN_FIFO_RATE                         4
 #define INV_MPU6050_ONE_K_HZ                              1000
 
+#define INV_MPU6050_REG_INT_PIN_CFG		0x37
+#define INV_MPU6050_BIT_BYPASS_EN		0x2
+
 /* scan element definition */
 enum inv_mpu6050_scan {
 	INV_MPU6050_SCAN_ACCL_X,
-- 
1.7.11.7

[PATCH 3/3] iio: imu: Enable checking of presence of device

[Srinivas Pandruvada]

Added logic to check presence of MPU6050 before continuing. Currently
only i2c writes are used in the initialzation path, which don't return
any error, if some i2c device responds. In this case it continues to
create iio devices, which don't work.
This can be reproduced in a PC like platform, where ACPI definition
of this defines multiple i2c addresses. We need to check for an valid
i2c address by checking some signature before continuing.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 15 ++++++++++++++-
 drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h  |  4 ++++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index 6ba565a..e062acd 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -62,6 +62,7 @@ static const struct inv_mpu6050_reg_map reg_set_6050 = {
 	.pwr_mgmt_1             = INV_MPU6050_REG_PWR_MGMT_1,
 	.pwr_mgmt_2             = INV_MPU6050_REG_PWR_MGMT_2,
 	.int_pin_cfg		= INV_MPU6050_REG_INT_PIN_CFG,
+	.who_am_i		= INV_MPU6050_REG_WHOAMI,
 };
 
 static const struct inv_mpu6050_chip_config chip_config_6050 = {
@@ -617,7 +618,7 @@ static const struct iio_info mpu_info = {
 	.validate_trigger = inv_mpu6050_validate_trigger,
 };
 
-int inv_set_bypass_status(struct inv_mpu6050_state *st, bool enable)
+static int inv_set_bypass_status(struct inv_mpu6050_state *st, bool enable)
 {
 	int ret;
 
@@ -650,6 +651,18 @@ static int inv_check_and_setup_chip(struct inv_mpu6050_state *st,
 	if (result)
 		return result;
 	msleep(INV_MPU6050_POWER_UP_TIME);
+
+	result = i2c_smbus_read_byte_data(st->client, st->reg->who_am_i);
+	if (result < 0) {
+		dev_err(&st->client->dev, "Error reading WhoAmI\n");
+		return result;
+	}
+	if (result != INV_MPU6500_UNIQUE_ID) {
+		dev_err(&st->client->dev, "Not a valid MPU6500 device %x\n",
+								result);
+		return -ENOSYS;
+	}
+
 	/* 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
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
index de5aa22..87649bd 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
@@ -55,6 +55,7 @@ struct inv_mpu6050_reg_map {
 	u8 pwr_mgmt_1;
 	u8 pwr_mgmt_2;
 	u8 int_pin_cfg;
+	u8 who_am_i;
 };
 
 /*device enum */
@@ -189,6 +190,9 @@ struct inv_mpu6050_state {
 #define INV_MPU6050_REG_INT_PIN_CFG		0x37
 #define INV_MPU6050_BIT_BYPASS_EN		0x2
 
+#define INV_MPU6050_REG_WHOAMI			0x75
+#define INV_MPU6500_UNIQUE_ID			0x70
+
 /* scan element definition */
 enum inv_mpu6050_scan {
 	INV_MPU6050_SCAN_ACCL_X,
-- 
1.7.11.7

Re: [PATCH 1/3] iio: imu: inv_mpu6050: ACPI enumeration

[Jonathan Cameron]

On 11/03/14 21:04, Srinivas Pandruvada wrote:
> Added changes so that the module can be enumerated via ACPI.
> Also if there is no platform data available, it will use a default
> orientation data.
>
You add the mpu6500 to this driver?  If so, that needs documenting etc.
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
>   drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 30 ++++++++++++++++++++++++++++--
>   1 file changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> index df7f1e1..4a76697 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> @@ -24,8 +24,16 @@
>   #include <linux/kfifo.h>
>   #include <linux/spinlock.h>
>   #include <linux/iio/iio.h>
> +#include <linux/acpi.h>
>   #include "inv_mpu_iio.h"
>
> +/* Define some default platform data, if not supplied */
> +static struct inv_mpu6050_platform_data inv_def_platform_data = {
> +	.orientation = {-1,  0,  0,
> +			0,  1,  0,
> +			0,  0, -1 }
> +};
> +
>   /*
>    * this is the gyro scale translated from dynamic range plus/minus
>    * {250, 500, 1000, 2000} to rad/s
> @@ -662,6 +670,7 @@ static int inv_mpu_probe(struct i2c_client *client,
>   	struct inv_mpu6050_state *st;
>   	struct iio_dev *indio_dev;
>   	int result;
> +	char *name;
>
>   	if (!i2c_check_functionality(client->adapter,
>   		I2C_FUNC_SMBUS_I2C_BLOCK))
> @@ -673,7 +682,10 @@ static int inv_mpu_probe(struct i2c_client *client,
>
>   	st = iio_priv(indio_dev);
>   	st->client = client;
> -	st->plat_data = *(struct inv_mpu6050_platform_data
> +	if (!dev_get_platdata(&client->dev))
> +		st->plat_data = inv_def_platform_data;
> +	else
> +		st->plat_data = *(struct inv_mpu6050_platform_data
>   				*)dev_get_platdata(&client->dev);
>   	/* power is turned on inside check chip type*/
>   	result = inv_check_and_setup_chip(st, id);
> @@ -689,7 +701,14 @@ static int inv_mpu_probe(struct i2c_client *client,
>
>   	i2c_set_clientdata(client, indio_dev);
>   	indio_dev->dev.parent = &client->dev;
> -	indio_dev->name = id->name;
> +	if (id && id->name)
As far as I know, there isn't a way for id->name to be null is there?
> +		name = (char *)id->name;
> +	else {
> +		name = (char *)dev_name(&client->dev);
> +		if (!name)
> +			dev_err(&client->dev, "No iio dev name\n");
If it's an error, then you out to return an error from here rather than
carrying on.
> +	}
> +	indio_dev->name = name;
>   	indio_dev->channels = inv_mpu_channels;
>   	indio_dev->num_channels = ARRAY_SIZE(inv_mpu_channels);
>
> @@ -770,12 +789,19 @@ static const struct i2c_device_id inv_mpu_id[] = {
>
>   MODULE_DEVICE_TABLE(i2c, inv_mpu_id);
>
> +static const struct acpi_device_id inv_acpi_match[] = {
> +	{"INVN6500", 0},
For i2c we have the second field as INV_MPU6050. Now I'll admit it
doesn't seem to be used for anything, but it would be nice to be consistent.
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(acpi, inv_acpi_match);
> +
>   static struct i2c_driver inv_mpu_driver = {
>   	.probe		=	inv_mpu_probe,
>   	.remove		=	inv_mpu_remove,
>   	.id_table	=	inv_mpu_id,
>   	.driver = {
>   		.owner	=	THIS_MODULE,
> +		.acpi_match_table = ACPI_PTR(inv_acpi_match),
>   		.name	=	"inv-mpu6050",
>   		.pm     =       INV_MPU6050_PMOPS,
>   	},
>

Re: [PATCH 2/3] iio: imu: inv_mpu6050: Enable default bypass mode

[Jonathan Cameron]

On 11/03/14 21:04, Srinivas Pandruvada wrote:
> This chip has two modes to control secondary sensor attached to it.
> One is master mode and another is bypass mode. In master mode
> MPU6500 will directly communicates to the secondary sensor device
> attached to it. This can support very few secondary devices in this
> mode.
> But when configured in bypass mode the i2c lines are directly connected
> to host i2c bus controller.
> Since in master mode it can only support few devices and they are not
> implemented in this driver, set the default mode to bypass mode.
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
This has been proposed before by Manuel Stahl,

Please read the thread from

http://www.spinics.net/lists/linux-iio/msg11985.html

In short, the best way we have come up with for handling this cleanly
(avoiding issues with the device in between going into sleep etc) was
to implement an i2c bus multiplexer with 1 input and 1 output as part of
the driver.

Manuel, any progress on your patches?

Jonathan
> ---
>   drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 20 +++++++++++++++++++-
>   drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h  |  4 ++++
>   2 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> index 4a76697..6ba565a 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> @@ -61,6 +61,7 @@ static const struct inv_mpu6050_reg_map reg_set_6050 = {
>   	.int_enable             = INV_MPU6050_REG_INT_ENABLE,
>   	.pwr_mgmt_1             = INV_MPU6050_REG_PWR_MGMT_1,
>   	.pwr_mgmt_2             = INV_MPU6050_REG_PWR_MGMT_2,
> +	.int_pin_cfg		= INV_MPU6050_REG_INT_PIN_CFG,
>   };
>
>   static const struct inv_mpu6050_chip_config chip_config_6050 = {
> @@ -616,6 +617,21 @@ static const struct iio_info mpu_info = {
>   	.validate_trigger = inv_mpu6050_validate_trigger,
>   };
>
> +int inv_set_bypass_status(struct inv_mpu6050_state *st, bool enable)
> +{
> +	int ret;
> +
> +	if (enable)
> +		ret = inv_mpu6050_write_reg(st, st->reg->int_pin_cfg,
> +					st->client->irq |
> +						INV_MPU6050_BIT_BYPASS_EN);
> +	else
> +		ret = inv_mpu6050_write_reg(st, st->reg->int_pin_cfg,
> +					st->client->irq);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(inv_set_bypass_status);
> +
>   /**
>    *  inv_check_and_setup_chip() - check and setup chip.
>    */
> @@ -654,7 +670,9 @@ static int inv_check_and_setup_chip(struct inv_mpu6050_state *st,
>   	if (result)
>   		return result;
>
> -	return 0;
> +	result = inv_set_bypass_status(st, true);
> +
> +	return result;
>   }
>
>   /**
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> index f383955..de5aa22 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> @@ -54,6 +54,7 @@ struct inv_mpu6050_reg_map {
>   	u8 int_enable;
>   	u8 pwr_mgmt_1;
>   	u8 pwr_mgmt_2;
> +	u8 int_pin_cfg;
>   };
>
>   /*device enum */
> @@ -185,6 +186,9 @@ struct inv_mpu6050_state {
>   #define INV_MPU6050_MIN_FIFO_RATE                         4
>   #define INV_MPU6050_ONE_K_HZ                              1000
>
> +#define INV_MPU6050_REG_INT_PIN_CFG		0x37
> +#define INV_MPU6050_BIT_BYPASS_EN		0x2
> +
>   /* scan element definition */
>   enum inv_mpu6050_scan {
>   	INV_MPU6050_SCAN_ACCL_X,
>

Re: [PATCH 3/3] iio: imu: Enable checking of presence of device

[Jonathan Cameron]

On 11/03/14 21:04, Srinivas Pandruvada wrote:
> Added logic to check presence of MPU6050 before continuing. Currently
> only i2c writes are used in the initialzation path, which don't return
> any error, if some i2c device responds. In this case it continues to
> create iio devices, which don't work.
> This can be reproduced in a PC like platform, where ACPI definition
> of this defines multiple i2c addresses.
Yuk.
> We need to check for an valid
> i2c address by checking some signature before continuing.
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
This patch looks fine.  The test is a sensible, simple, debugging test
even without the ACPI fun..
> ---
>   drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 15 ++++++++++++++-
>   drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h  |  4 ++++
>   2 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> index 6ba565a..e062acd 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> @@ -62,6 +62,7 @@ static const struct inv_mpu6050_reg_map reg_set_6050 = {
>   	.pwr_mgmt_1             = INV_MPU6050_REG_PWR_MGMT_1,
>   	.pwr_mgmt_2             = INV_MPU6050_REG_PWR_MGMT_2,
>   	.int_pin_cfg		= INV_MPU6050_REG_INT_PIN_CFG,
> +	.who_am_i		= INV_MPU6050_REG_WHOAMI,
>   };
>
>   static const struct inv_mpu6050_chip_config chip_config_6050 = {
> @@ -617,7 +618,7 @@ static const struct iio_info mpu_info = {
>   	.validate_trigger = inv_mpu6050_validate_trigger,
>   };
>
> -int inv_set_bypass_status(struct inv_mpu6050_state *st, bool enable)
> +static int inv_set_bypass_status(struct inv_mpu6050_state *st, bool enable)
>   {
>   	int ret;
>
> @@ -650,6 +651,18 @@ static int inv_check_and_setup_chip(struct inv_mpu6050_state *st,
>   	if (result)
>   		return result;
>   	msleep(INV_MPU6050_POWER_UP_TIME);
> +
> +	result = i2c_smbus_read_byte_data(st->client, st->reg->who_am_i);
> +	if (result < 0) {
> +		dev_err(&st->client->dev, "Error reading WhoAmI\n");
> +		return result;
> +	}
> +	if (result != INV_MPU6500_UNIQUE_ID) {
> +		dev_err(&st->client->dev, "Not a valid MPU6500 device %x\n",
> +								result);
> +		return -ENOSYS;
> +	}
> +
>   	/* 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
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> index de5aa22..87649bd 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> @@ -55,6 +55,7 @@ struct inv_mpu6050_reg_map {
>   	u8 pwr_mgmt_1;
>   	u8 pwr_mgmt_2;
>   	u8 int_pin_cfg;
> +	u8 who_am_i;
>   };
>
>   /*device enum */
> @@ -189,6 +190,9 @@ struct inv_mpu6050_state {
>   #define INV_MPU6050_REG_INT_PIN_CFG		0x37
>   #define INV_MPU6050_BIT_BYPASS_EN		0x2
>
> +#define INV_MPU6050_REG_WHOAMI			0x75
> +#define INV_MPU6500_UNIQUE_ID			0x70
> +
>   /* scan element definition */
>   enum inv_mpu6050_scan {
>   	INV_MPU6050_SCAN_ACCL_X,
>

Re: [PATCH 2/3] iio: imu: inv_mpu6050: Enable default bypass mode

[Manuel Stahl]

Am 15.03.2014 17:01, schrieb Jonathan Cameron:
> On 11/03/14 21:04, Srinivas Pandruvada wrote:
>> This chip has two modes to control secondary sensor attached to it.
>> One is master mode and another is bypass mode. In master mode
>> MPU6500 will directly communicates to the secondary sensor device
>> attached to it. This can support very few secondary devices in this
>> mode.
>> But when configured in bypass mode the i2c lines are directly connected
>> to host i2c bus controller.
>> Since in master mode it can only support few devices and they are not
>> implemented in this driver, set the default mode to bypass mode.
>>
>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> This has been proposed before by Manuel Stahl,
>
> Please read the thread from
>
> http://www.spinics.net/lists/linux-iio/msg11985.html
>
> In short, the best way we have come up with for handling this cleanly
> (avoiding issues with the device in between going into sleep etc) was
> to implement an i2c bus multiplexer with 1 input and 1 output as part of
> the driver.
>
> Manuel, any progress on your patches?

No, I was waiting for the outcome of this discussion. Probably I will 
not be available for the next three weeks, so if someone else wants to 
start off coding, feel free to do so.

-- 
Regards,
Manuel Stahl

Re: [PATCH 2/3] iio: imu: inv_mpu6050: Enable default bypass mode

[Srinivas Pandruvada]

On 03/15/2014 09:01 AM, Jonathan Cameron wrote:
> On 11/03/14 21:04, Srinivas Pandruvada wrote:
>> This chip has two modes to control secondary sensor attached to it.
>> One is master mode and another is bypass mode. In master mode
>> MPU6500 will directly communicates to the secondary sensor device
>> attached to it. This can support very few secondary devices in this
>> mode.
>> But when configured in bypass mode the i2c lines are directly connected
>> to host i2c bus controller.
>> Since in master mode it can only support few devices and they are not
>> implemented in this driver, set the default mode to bypass mode.
>>
>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> This has been proposed before by Manuel Stahl,
>
> Please read the thread from
>
> http://www.spinics.net/lists/linux-iio/msg11985.html
>
> In short, the best way we have come up with for handling this cleanly
> (avoiding issues with the device in between going into sleep etc) was
> to implement an i2c bus multiplexer with 1 input and 1 output as part of
> the driver.
>
> Manuel, any progress on your patches?
>

I think the mux is the right solution for using master mode of MPU60X0.
But I feel that when this driver is initialized then we should disable 
the master mode at init
as currently it doesn't have any implementation for slave devices as 
Invensense typically do for Android devices.
Once a client want to use multiplexer then it can be turned on.
Invensense engineer (author of this driver) recommends not to use master 
mode.

So my patch disables at init. I am calling this function from init. I 
can remove export of symbol.

What do you think about this?

Thanks,
Srinivas


> Jonathan
>> ---
>>   drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 20 +++++++++++++++++++-
>>   drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h  |  4 ++++
>>   2 files changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c 
>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>> index 4a76697..6ba565a 100644
>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>> @@ -61,6 +61,7 @@ static const struct inv_mpu6050_reg_map 
>> reg_set_6050 = {
>>       .int_enable             = INV_MPU6050_REG_INT_ENABLE,
>>       .pwr_mgmt_1             = INV_MPU6050_REG_PWR_MGMT_1,
>>       .pwr_mgmt_2             = INV_MPU6050_REG_PWR_MGMT_2,
>> +    .int_pin_cfg        = INV_MPU6050_REG_INT_PIN_CFG,
>>   };
>>
>>   static const struct inv_mpu6050_chip_config chip_config_6050 = {
>> @@ -616,6 +617,21 @@ static const struct iio_info mpu_info = {
>>       .validate_trigger = inv_mpu6050_validate_trigger,
>>   };
>>
>> +int inv_set_bypass_status(struct inv_mpu6050_state *st, bool enable)
>> +{
>> +    int ret;
>> +
>> +    if (enable)
>> +        ret = inv_mpu6050_write_reg(st, st->reg->int_pin_cfg,
>> +                    st->client->irq |
>> +                        INV_MPU6050_BIT_BYPASS_EN);
>> +    else
>> +        ret = inv_mpu6050_write_reg(st, st->reg->int_pin_cfg,
>> +                    st->client->irq);
>> +    return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(inv_set_bypass_status);
>> +
>>   /**
>>    *  inv_check_and_setup_chip() - check and setup chip.
>>    */
>> @@ -654,7 +670,9 @@ static int inv_check_and_setup_chip(struct 
>> inv_mpu6050_state *st,
>>       if (result)
>>           return result;
>>
>> -    return 0;
>> +    result = inv_set_bypass_status(st, true);
>> +
>> +    return result;
>>   }
>>
>>   /**
>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h 
>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>> index f383955..de5aa22 100644
>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>> @@ -54,6 +54,7 @@ struct inv_mpu6050_reg_map {
>>       u8 int_enable;
>>       u8 pwr_mgmt_1;
>>       u8 pwr_mgmt_2;
>> +    u8 int_pin_cfg;
>>   };
>>
>>   /*device enum */
>> @@ -185,6 +186,9 @@ struct inv_mpu6050_state {
>>   #define INV_MPU6050_MIN_FIFO_RATE                         4
>>   #define INV_MPU6050_ONE_K_HZ                              1000
>>
>> +#define INV_MPU6050_REG_INT_PIN_CFG        0x37
>> +#define INV_MPU6050_BIT_BYPASS_EN        0x2
>> +
>>   /* scan element definition */
>>   enum inv_mpu6050_scan {
>>       INV_MPU6050_SCAN_ACCL_X,
>>
>
>

Re: [PATCH 3/3] iio: imu: Enable checking of presence of device

[Srinivas Pandruvada]

On 03/15/2014 09:04 AM, Jonathan Cameron wrote:
> On 11/03/14 21:04, Srinivas Pandruvada wrote:
>> Added logic to check presence of MPU6050 before continuing. Currently
>> only i2c writes are used in the initialzation path, which don't return
>> any error, if some i2c device responds. In this case it continues to
>> create iio devices, which don't work.
Do you think this is correct behaviour? Usually i2c devices should check 
their
identity and then only create IIO devices. Unlike PCI devices they don't 
have a common header.

Thanks,
Srinivas


>> This can be reproduced in a PC like platform, where ACPI definition
>> of this defines multiple i2c addresses.
> Yuk.
>> We need to check for an valid
>> i2c address by checking some signature before continuing.
>>
>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> This patch looks fine.  The test is a sensible, simple, debugging test
> even without the ACPI fun..
>> ---
>>   drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 15 ++++++++++++++-
>>   drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h  |  4 ++++
>>   2 files changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c 
>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>> index 6ba565a..e062acd 100644
>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>> @@ -62,6 +62,7 @@ static const struct inv_mpu6050_reg_map 
>> reg_set_6050 = {
>>       .pwr_mgmt_1             = INV_MPU6050_REG_PWR_MGMT_1,
>>       .pwr_mgmt_2             = INV_MPU6050_REG_PWR_MGMT_2,
>>       .int_pin_cfg        = INV_MPU6050_REG_INT_PIN_CFG,
>> +    .who_am_i        = INV_MPU6050_REG_WHOAMI,
>>   };
>>
>>   static const struct inv_mpu6050_chip_config chip_config_6050 = {
>> @@ -617,7 +618,7 @@ static const struct iio_info mpu_info = {
>>       .validate_trigger = inv_mpu6050_validate_trigger,
>>   };
>>
>> -int inv_set_bypass_status(struct inv_mpu6050_state *st, bool enable)
>> +static int inv_set_bypass_status(struct inv_mpu6050_state *st, bool 
>> enable)
>>   {
>>       int ret;
>>
>> @@ -650,6 +651,18 @@ static int inv_check_and_setup_chip(struct 
>> inv_mpu6050_state *st,
>>       if (result)
>>           return result;
>>       msleep(INV_MPU6050_POWER_UP_TIME);
>> +
>> +    result = i2c_smbus_read_byte_data(st->client, st->reg->who_am_i);
>> +    if (result < 0) {
>> +        dev_err(&st->client->dev, "Error reading WhoAmI\n");
>> +        return result;
>> +    }
>> +    if (result != INV_MPU6500_UNIQUE_ID) {
>> +        dev_err(&st->client->dev, "Not a valid MPU6500 device %x\n",
>> +                                result);
>> +        return -ENOSYS;
>> +    }
>> +
>>       /* 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
>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h 
>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>> index de5aa22..87649bd 100644
>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>> @@ -55,6 +55,7 @@ struct inv_mpu6050_reg_map {
>>       u8 pwr_mgmt_1;
>>       u8 pwr_mgmt_2;
>>       u8 int_pin_cfg;
>> +    u8 who_am_i;
>>   };
>>
>>   /*device enum */
>> @@ -189,6 +190,9 @@ struct inv_mpu6050_state {
>>   #define INV_MPU6050_REG_INT_PIN_CFG        0x37
>>   #define INV_MPU6050_BIT_BYPASS_EN        0x2
>>
>> +#define INV_MPU6050_REG_WHOAMI            0x75
>> +#define INV_MPU6500_UNIQUE_ID            0x70
>> +
>>   /* scan element definition */
>>   enum inv_mpu6050_scan {
>>       INV_MPU6050_SCAN_ACCL_X,
>>
>
>

Re: [PATCH 2/3] iio: imu: inv_mpu6050: Enable default bypass mode

[Jonathan Cameron]

On March 17, 2014 3:40:37 PM GMT+00:00, Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> wrote:
>On 03/15/2014 09:01 AM, Jonathan Cameron wrote:
>> On 11/03/14 21:04, Srinivas Pandruvada wrote:
>>> This chip has two modes to control secondary sensor attached to it.
>>> One is master mode and another is bypass mode. In master mode
>>> MPU6500 will directly communicates to the secondary sensor device
>>> attached to it. This can support very few secondary devices in this
>>> mode.
>>> But when configured in bypass mode the i2c lines are directly
>connected
>>> to host i2c bus controller.
>>> Since in master mode it can only support few devices and they are
>not
>>> implemented in this driver, set the default mode to bypass mode.
>>>
>>> Signed-off-by: Srinivas Pandruvada
><srinivas.pandruvada@linux.intel.com>
>> This has been proposed before by Manuel Stahl,
>>
>> Please read the thread from
>>
>> http://www.spinics.net/lists/linux-iio/msg11985.html
>>
>> In short, the best way we have come up with for handling this cleanly
>> (avoiding issues with the device in between going into sleep etc) was
>> to implement an i2c bus multiplexer with 1 input and 1 output as part
>of
>> the driver.
>>
>> Manuel, any progress on your patches?
>>
>
>I think the mux is the right solution for using master mode of MPU60X0.
>But I feel that when this driver is initialized then we should disable 
>the master mode at init
>as currently it doesn't have any implementation for slave devices as 
>Invensense typically do for Android devices.
Agreed that disabling effectively unused master mode makes sense.
>Once a client want to use multiplexer then it can be turned on.
Yes turn on bypass as needed... Or is it a case of one control for master or bypass?  If
 so then seems like bypass will do nothing unless the MUX is enabled so makes sense
 to init in bypass mode.
>Invensense engineer (author of this driver) recommends not to use
>master 
>mode.
>
>So my patch disables at init. I am calling this function from init. I 
>can remove export of symbol.

>
>What do you think about this?
All sounds sensible to me.
>
>Thanks,
>Srinivas
>
>
>> Jonathan
>>> ---
>>>   drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 20
>+++++++++++++++++++-
>>>   drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h  |  4 ++++
>>>   2 files changed, 23 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c 
>>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>>> index 4a76697..6ba565a 100644
>>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
>>> @@ -61,6 +61,7 @@ static const struct inv_mpu6050_reg_map 
>>> reg_set_6050 = {
>>>       .int_enable             = INV_MPU6050_REG_INT_ENABLE,
>>>       .pwr_mgmt_1             = INV_MPU6050_REG_PWR_MGMT_1,
>>>       .pwr_mgmt_2             = INV_MPU6050_REG_PWR_MGMT_2,
>>> +    .int_pin_cfg        = INV_MPU6050_REG_INT_PIN_CFG,
>>>   };
>>>
>>>   static const struct inv_mpu6050_chip_config chip_config_6050 = {
>>> @@ -616,6 +617,21 @@ static const struct iio_info mpu_info = {
>>>       .validate_trigger = inv_mpu6050_validate_trigger,
>>>   };
>>>
>>> +int inv_set_bypass_status(struct inv_mpu6050_state *st, bool
>enable)
>>> +{
>>> +    int ret;
>>> +
>>> +    if (enable)
>>> +        ret = inv_mpu6050_write_reg(st, st->reg->int_pin_cfg,
>>> +                    st->client->irq |
>>> +                        INV_MPU6050_BIT_BYPASS_EN);
>>> +    else
>>> +        ret = inv_mpu6050_write_reg(st, st->reg->int_pin_cfg,
>>> +                    st->client->irq);
>>> +    return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(inv_set_bypass_status);
>>> +
>>>   /**
>>>    *  inv_check_and_setup_chip() - check and setup chip.
>>>    */
>>> @@ -654,7 +670,9 @@ static int inv_check_and_setup_chip(struct 
>>> inv_mpu6050_state *st,
>>>       if (result)
>>>           return result;
>>>
>>> -    return 0;
>>> +    result = inv_set_bypass_status(st, true);
>>> +
>>> +    return result;
>>>   }
>>>
>>>   /**
>>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h 
>>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>>> index f383955..de5aa22 100644
>>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>>> @@ -54,6 +54,7 @@ struct inv_mpu6050_reg_map {
>>>       u8 int_enable;
>>>       u8 pwr_mgmt_1;
>>>       u8 pwr_mgmt_2;
>>> +    u8 int_pin_cfg;
>>>   };
>>>
>>>   /*device enum */
>>> @@ -185,6 +186,9 @@ struct inv_mpu6050_state {
>>>   #define INV_MPU6050_MIN_FIFO_RATE                         4
>>>   #define INV_MPU6050_ONE_K_HZ                              1000
>>>
>>> +#define INV_MPU6050_REG_INT_PIN_CFG        0x37
>>> +#define INV_MPU6050_BIT_BYPASS_EN        0x2
>>> +
>>>   /* scan element definition */
>>>   enum inv_mpu6050_scan {
>>>       INV_MPU6050_SCAN_ACCL_X,
>>>
>>
>>
>
>--
>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

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

[PATCH 1/3] iio: imu: inv_mpu6050: ACPI enumeration

[Srinivas Pandruvada]

Added changes so that the module can be enumerated via ACPI.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index 6d2c115..f73e60b 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -24,6 +24,7 @@
 #include <linux/spinlock.h>
 #include <linux/iio/iio.h>
 #include <linux/i2c-mux.h>
+#include <linux/acpi.h>
 #include "inv_mpu_iio.h"
 
 /*
@@ -875,6 +876,13 @@ static const struct i2c_device_id inv_mpu_id[] = {
 
 MODULE_DEVICE_TABLE(i2c, inv_mpu_id);
 
+static const struct acpi_device_id inv_acpi_match[] = {
+	{"INVN6500", 0},
+	{ },
+};
+
+MODULE_DEVICE_TABLE(acpi, inv_acpi_match);
+
 static struct i2c_driver inv_mpu_driver = {
 	.probe		=	inv_mpu_probe,
 	.remove		=	inv_mpu_remove,
@@ -883,6 +891,7 @@ static struct i2c_driver inv_mpu_driver = {
 		.owner	=	THIS_MODULE,
 		.name	=	"inv-mpu6050",
 		.pm     =       INV_MPU6050_PMOPS,
+		.acpi_match_table = ACPI_PTR(inv_acpi_match),
 	},
 };
 
-- 
1.9.3

Re: [PATCH 1/3] iio: imu: inv_mpu6050: ACPI enumeration

[Jonathan Cameron]

On 15/12/14 21:19, Srinivas Pandruvada wrote:
> Added changes so that the module can be enumerated via ACPI.
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
As this one stands on it's own.

Applied to the togreg branch of iio.git.

Thanks

Jonathan
> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> index 6d2c115..f73e60b 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> @@ -24,6 +24,7 @@
>  #include <linux/spinlock.h>
>  #include <linux/iio/iio.h>
>  #include <linux/i2c-mux.h>
> +#include <linux/acpi.h>
>  #include "inv_mpu_iio.h"
>  
>  /*
> @@ -875,6 +876,13 @@ static const struct i2c_device_id inv_mpu_id[] = {
>  
>  MODULE_DEVICE_TABLE(i2c, inv_mpu_id);
>  
> +static const struct acpi_device_id inv_acpi_match[] = {
> +	{"INVN6500", 0},
> +	{ },
> +};
> +
> +MODULE_DEVICE_TABLE(acpi, inv_acpi_match);
> +
>  static struct i2c_driver inv_mpu_driver = {
>  	.probe		=	inv_mpu_probe,
>  	.remove		=	inv_mpu_remove,
> @@ -883,6 +891,7 @@ static struct i2c_driver inv_mpu_driver = {
>  		.owner	=	THIS_MODULE,
>  		.name	=	"inv-mpu6050",
>  		.pm     =       INV_MPU6050_PMOPS,
> +		.acpi_match_table = ACPI_PTR(inv_acpi_match),
>  	},
>  };
>  
>