Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1383373 > unrolled thread

[PATCH 3/5] iio: inv_mpu6050: Check WHO_AM_I register on probe

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3/5] iio: inv_mpu6050: Check WHO_AM_I register on probe Crestez Dan Leonard <leonard.crestez@intel.com> - 2016-04-20 15:20 +0200
    Re: [PATCH 3/5] iio: inv_mpu6050: Check WHO_AM_I register on probe Jonathan Cameron <jic23@kernel.org> - 2016-04-24 13:20 +0200
      Re: [PATCH 3/5] iio: inv_mpu6050: Check WHO_AM_I register on probe Crestez Dan Leonard <leonard.crestez@intel.com> - 2016-04-25 13:20 +0200
        Re: [PATCH 3/5] iio: inv_mpu6050: Check WHO_AM_I register on probe Jonathan Cameron <jic23@kernel.org> - 2016-04-25 20:10 +0200

#1383373 — [PATCH 3/5] iio: inv_mpu6050: Check WHO_AM_I register on probe

This can be used to distinguish mpu6500. This is a warning rather than
an error because the differences are mostly irrelevant and it's nice to
avoid breaking users with slightly incorrect ACPI/DT.

Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 15 +++++++++++++++
 drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h  |  8 ++++++++
 2 files changed, 23 insertions(+)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index faccabafc..273b7fa7 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -91,16 +91,19 @@ static const struct inv_mpu6050_chip_config chip_config_6050 = {
 /* Indexed by enum inv_devices */
 static const struct inv_mpu6050_hw hw_info[] = {
 	{
+		.whoami = INV_MPU6050_WHOAMI_VALUE,
 		.name = "MPU6050",
 		.reg = &reg_set_6050,
 		.config = &chip_config_6050,
 	},
 	{
+		.whoami = INV_MPU6500_WHOAMI_VALUE,
 		.name = "MPU6500",
 		.reg = &reg_set_6500,
 		.config = &chip_config_6050,
 	},
 	{
+		.whoami = INV_MPU6000_WHOAMI_VALUE,
 		.name = "MPU6000",
 		.reg = &reg_set_6050,
 		.config = &chip_config_6050,
@@ -730,6 +733,7 @@ static const struct iio_info mpu_info = {
 static int inv_check_and_setup_chip(struct inv_mpu6050_state *st)
 {
 	int result;
+	unsigned int regval;
 
 	st->hw  = &hw_info[st->chip_type];
 	st->reg = hw_info[st->chip_type].reg;
@@ -740,6 +744,17 @@ static int inv_check_and_setup_chip(struct inv_mpu6050_state *st)
 	if (result)
 		return result;
 	msleep(INV_MPU6050_POWER_UP_TIME);
+
+	/* check chip self-identification */
+	result = regmap_read(st->map, INV_MPU6050_REG_WHOAMI, &regval);
+	if (result)
+		return result;
+	if (regval != st->hw->whoami) {
+		dev_warn(regmap_get_device(st->map),
+				"whoami mismatch got %#02x expected %#02hhx for %s\n",
+				regval, st->hw->whoami, st->hw->name);
+	}
+
 	/*
 	 * toggle power state. After reset, the sleep bit could be on
 	 * or off depending on the OTP settings. Toggling power would
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
index c66dbfc..564cabd 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
@@ -93,11 +93,13 @@ struct inv_mpu6050_chip_config {
 
 /**
  *  struct inv_mpu6050_hw - Other important hardware information.
+ *  @whoami:	Self identification byte from WHO_AM_I register
  *  @name:      name of the chip.
  *  @reg:   register map of the chip.
  *  @config:    configuration of the chip.
  */
 struct inv_mpu6050_hw {
+	u8 whoami;
 	u8 *name;
 	const struct inv_mpu6050_reg_map *reg;
 	const struct inv_mpu6050_chip_config *config;
@@ -213,6 +215,12 @@ struct inv_mpu6050_state {
 #define INV_MPU6050_MIN_FIFO_RATE            4
 #define INV_MPU6050_ONE_K_HZ                 1000
 
+#define INV_MPU6050_REG_WHOAMI			117
+
+#define INV_MPU6000_WHOAMI_VALUE		0x68
+#define INV_MPU6050_WHOAMI_VALUE		0x68
+#define INV_MPU6500_WHOAMI_VALUE		0x70
+
 /* scan element definition */
 enum inv_mpu6050_scan {
 	INV_MPU6050_SCAN_ACCL_X,
-- 
2.5.5

[toc] | [next] | [standalone]

#1385803

On 20/04/16 14:15, Crestez Dan Leonard wrote:
> This can be used to distinguish mpu6500. This is a warning rather than
> an error because the differences are mostly irrelevant and it's nice to
> avoid breaking users with slightly incorrect ACPI/DT.
> 
> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
Would we be better off fixing their configuration though by using the right part
if we can identify it?  So if wrong, maybe we should search the info table to 
figure out what it is?  I'm not certain on this though as then we are trying to
deal with unknown future cases - maybe what you have here is the best balance.

Jonathan
> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 15 +++++++++++++++
>  drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h  |  8 ++++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> index faccabafc..273b7fa7 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> @@ -91,16 +91,19 @@ static const struct inv_mpu6050_chip_config chip_config_6050 = {
>  /* Indexed by enum inv_devices */
>  static const struct inv_mpu6050_hw hw_info[] = {
>  	{
> +		.whoami = INV_MPU6050_WHOAMI_VALUE,
>  		.name = "MPU6050",
>  		.reg = &reg_set_6050,
>  		.config = &chip_config_6050,
>  	},
>  	{
> +		.whoami = INV_MPU6500_WHOAMI_VALUE,
>  		.name = "MPU6500",
>  		.reg = &reg_set_6500,
>  		.config = &chip_config_6050,
>  	},
>  	{
> +		.whoami = INV_MPU6000_WHOAMI_VALUE,
>  		.name = "MPU6000",
>  		.reg = &reg_set_6050,
>  		.config = &chip_config_6050,
> @@ -730,6 +733,7 @@ static const struct iio_info mpu_info = {
>  static int inv_check_and_setup_chip(struct inv_mpu6050_state *st)
>  {
>  	int result;
> +	unsigned int regval;
>  
>  	st->hw  = &hw_info[st->chip_type];
>  	st->reg = hw_info[st->chip_type].reg;
> @@ -740,6 +744,17 @@ static int inv_check_and_setup_chip(struct inv_mpu6050_state *st)
>  	if (result)
>  		return result;
>  	msleep(INV_MPU6050_POWER_UP_TIME);
> +
> +	/* check chip self-identification */
> +	result = regmap_read(st->map, INV_MPU6050_REG_WHOAMI, &regval);
> +	if (result)
> +		return result;
> +	if (regval != st->hw->whoami) {
> +		dev_warn(regmap_get_device(st->map),
> +				"whoami mismatch got %#02x expected %#02hhx for %s\n",
> +				regval, st->hw->whoami, st->hw->name);
> +	}
> +
>  	/*
>  	 * toggle power state. After reset, the sleep bit could be on
>  	 * or off depending on the OTP settings. Toggling power would
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> index c66dbfc..564cabd 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> @@ -93,11 +93,13 @@ struct inv_mpu6050_chip_config {
>  
>  /**
>   *  struct inv_mpu6050_hw - Other important hardware information.
> + *  @whoami:	Self identification byte from WHO_AM_I register
>   *  @name:      name of the chip.
>   *  @reg:   register map of the chip.
>   *  @config:    configuration of the chip.
>   */
>  struct inv_mpu6050_hw {
> +	u8 whoami;
>  	u8 *name;
>  	const struct inv_mpu6050_reg_map *reg;
>  	const struct inv_mpu6050_chip_config *config;
> @@ -213,6 +215,12 @@ struct inv_mpu6050_state {
>  #define INV_MPU6050_MIN_FIFO_RATE            4
>  #define INV_MPU6050_ONE_K_HZ                 1000
>  
> +#define INV_MPU6050_REG_WHOAMI			117
> +
> +#define INV_MPU6000_WHOAMI_VALUE		0x68
> +#define INV_MPU6050_WHOAMI_VALUE		0x68
> +#define INV_MPU6500_WHOAMI_VALUE		0x70
> +
>  /* scan element definition */
>  enum inv_mpu6050_scan {
>  	INV_MPU6050_SCAN_ACCL_X,
> 

[toc] | [prev] | [next] | [standalone]

#1386258

On 04/24/2016 02:14 PM, Jonathan Cameron wrote:
> On 20/04/16 14:15, Crestez Dan Leonard wrote:
>> This can be used to distinguish mpu6500. This is a warning rather than
>> an error because the differences are mostly irrelevant and it's nice to
>> avoid breaking users with slightly incorrect ACPI/DT.
>>
>> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
> Would we be better off fixing their configuration though by using the right part
> if we can identify it?  So if wrong, maybe we should search the info table to 
> figure out what it is?  I'm not certain on this though as then we are trying to
> deal with unknown future cases - maybe what you have here is the best balance.

I'm not sure about that. One issue is that 6000/6050/9150 have the same
WHOAMI value and can't be distinguished this way. They also seem to
identical interfaces. Models MPU6500 and MPU9250 report different WHOAMI
values.

Changing chip_type based on the WHOAMI would require some additional
refactoring. Placing that in a separate patch might be worthwhile anyway.

>> +#define INV_MPU6050_REG_WHOAMI			117
>> +
>> +#define INV_MPU6000_WHOAMI_VALUE		0x68
>> +#define INV_MPU6050_WHOAMI_VALUE		0x68
>> +#define INV_MPU6500_WHOAMI_VALUE		0x70

[toc] | [prev] | [next] | [standalone]

#1386728

On 25/04/16 12:17, Crestez Dan Leonard wrote:
> On 04/24/2016 02:14 PM, Jonathan Cameron wrote:
>> On 20/04/16 14:15, Crestez Dan Leonard wrote:
>>> This can be used to distinguish mpu6500. This is a warning rather than
>>> an error because the differences are mostly irrelevant and it's nice to
>>> avoid breaking users with slightly incorrect ACPI/DT.
>>>
>>> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
>> Would we be better off fixing their configuration though by using the right part
>> if we can identify it?  So if wrong, maybe we should search the info table to 
>> figure out what it is?  I'm not certain on this though as then we are trying to
>> deal with unknown future cases - maybe what you have here is the best balance.
> 
> I'm not sure about that. One issue is that 6000/6050/9150 have the same
> WHOAMI value and can't be distinguished this way. They also seem to
> identical interfaces. Models MPU6500 and MPU9250 report different WHOAMI
> values.
> 
> Changing chip_type based on the WHOAMI would require some additional
> refactoring. Placing that in a separate patch might be worthwhile anyway.
> 
Agreed.
>>> +#define INV_MPU6050_REG_WHOAMI			117
>>> +
>>> +#define INV_MPU6000_WHOAMI_VALUE		0x68
>>> +#define INV_MPU6050_WHOAMI_VALUE		0x68
>>> +#define INV_MPU6500_WHOAMI_VALUE		0x70

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web