[RFC 0/7] iio: inv_mpu6050: Support i2c master and external readings

[RFC 0/7] iio: inv_mpu6050: Support i2c master and external readings

[Crestez Dan Leonard]

This series attempts to implement support for external readings in i2c master
mode. I don't expect this to go in this form but I wanted to present a
functional version in order to start the discussion earlier.


The I2C master support is useful even without external readings. For example in
SPI mode this is the only way to contact auxiliary devices. Differences from
the previous version:
 - Require an explicit "inv,i2c-aux-master" bool property. Supporting dynamic
switches between bypass/master mode is extremely difficult and not really
worthwhile. Switching to bypass mode requires explicitly disabling all i2c
master functionality, including external readings! After Peter Rosin's mux
cleanup patches go in I would like to disable the mux entirely when in master
mode.
 - Describe i2c clients behind i2c@1. Maybe it should be i2c@0 like mux mode?
 - Validate parameters and return an error on unsupported ops (like read_word)
 - Wait for SLV4_DONE even on writes!
 - Fix issues when other parts of the driver write to the same registers.

My initial idea was to use regcache and regmap_update_bits to handle config
bits like in PATCH 2. It turn out that the device has several registers with a
combination of volatile "command bits" (writing triggers an action) and "config
bits" (value must normally be preserved). This requires manually storing the
state of those config bits in "chip_config". I could try to make do without
regcache support, even though it offers some advantages.


Support for external sensors is rather nasty. What it does is dynamically copy
iio_chan_spec from the slave device and register it for itself (with an updated
scan index). Then it forwards everything but IIO_CHAN_INFO_RAW to the other
driver while serving raw value requests from the MPU's own registers.

The original device is still registered with linux as a normal driver and works
normally and you can poke at it to configure stuff like sample rates and
scaling factors. You can read the same samples from the slaved device, just
slower. For example:
	cat /sys/bus/iio/devices/iio:device1/in_magn_*_raw
will be slower than:
	cat /sys/bus/iio/devices/iio:device0/in_magn_*_raw
In the first case values are read through SLV4, byte-by-byte. In the second
they are served directly from EXT_SENS_DATA_* registers.

In theory the ak8975 inside mpu9150 could be supported as a slave device but
that requires further work. Unlike the hmc5883l I've been testing with that
sensor does not update automatically by default. This means that the mpu6050
buffer code must call a function from that driver to "enable automatic updates"
somehow. For example this could be implemented as an additional bufferop?

So far this works surprisingly well without any changes in the iio core or
drivers for aux devices. But perhaps "external channels" should be listed in
struct iio_dev and a special marking should be done for iio_chan_spec's which
can accessed this way.

External/slaved channels are limited to 16bits because it would otherwise be
difficult to comply with iio alignment requirements. Support for other channel
sizes should be implemented separately. There should be a separate discussion
for how to properly support driver-specified channel offsets instead of
implicit iio rules.

Patches 1,2,3,4 and 6 are required cleanups/fixed to make the rest work. They
could go in separately.

Crestez Dan Leonard (7):
  iio: inv_mpu6050: Do burst reads using spi/i2c directly
  iio: inv_mpu6050: Initial regcache support
  iio: inv_mpu6050: Only toggle DATA_RDY_EN in inv_reset_fifo
  iio: inv_mpu6050: Cache non-volatile bits of user_ctrl
  iio: inv_mpu6050: Add support for auxiliary I2C master
  iio: inv_mpu6050: Check channel configuration on preenable
  iio: inv_mpu6050: Add support for external sensors

 .../devicetree/bindings/iio/imu/inv_mpu6050.txt    |  96 ++-
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c         | 708 ++++++++++++++++++++-
 drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c          |   5 -
 drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h          | 116 +++-
 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c         | 134 +++-
 drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c          |   5 -
 drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c      |  15 +-
 7 files changed, 1029 insertions(+), 50 deletions(-)

-- 
2.5.5

[PATCH 1/7] iio: inv_mpu6050: Do burst reads using spi/i2c directly

[Crestez Dan Leonard]

Using regmap_read_bulk is wrong because it assumes that a range of
registers is being read. In our case reading from the fifo register will
return multiple values but this is *not* auto-increment.

This currently works by accident.

Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 33 ++++++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
index d070062..8455af0 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
@@ -21,6 +21,7 @@
 #include <linux/interrupt.h>
 #include <linux/kfifo.h>
 #include <linux/poll.h>
+#include <linux/spi/spi.h>
 #include "inv_mpu_iio.h"
 
 static void inv_clear_kfifo(struct inv_mpu6050_state *st)
@@ -128,6 +129,13 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
 	u16 fifo_count;
 	s64 timestamp;
 
+	struct device *regmap_dev = regmap_get_device(st->map);
+	struct i2c_client *i2c;
+	struct spi_device *spi = NULL;
+
+	i2c = i2c_verify_client(regmap_dev);
+	spi = i2c ? NULL: to_spi_device(regmap_dev);
+
 	mutex_lock(&indio_dev->mlock);
 	if (!(st->chip_config.accl_fifo_enable |
 		st->chip_config.gyro_fifo_enable))
@@ -160,10 +168,27 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
 	    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);
-		if (result)
-			goto flush_fifo;
+		/*
+		 * We need to do a large burst read from a single register.
+		 *
+		 * regmap_read_bulk assumes that multiple registers are
+		 * involved but in our case st->reg->fifo_r_w + 1 is something
+		 * completely unrelated.
+		 */
+		if (spi) {
+			u8 cmd = st->reg->fifo_r_w | 0x80;
+			result = spi_write_then_read(spi,
+					&cmd, 1,
+					data, bytes_per_datum);
+			if (result)
+				goto flush_fifo;
+		} else {
+			result = i2c_smbus_read_i2c_block_data(i2c,
+					st->reg->fifo_r_w,
+					bytes_per_datum, data);
+			if (result != bytes_per_datum)
+				goto flush_fifo;
+		}
 
 		result = kfifo_out(&st->timestamps, &timestamp, 1);
 		/* when there is no timestamp, put timestamp as 0 */
-- 
2.5.5

[PATCH 2/7] iio: inv_mpu6050: Initial regcache support

[Crestez Dan Leonard]

Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 47 ++++++++++++++++++++++++++++++
 drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c  |  5 ----
 drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h  |  1 +
 drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c  |  5 ----
 4 files changed, 48 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index b269b37..5918c23 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -116,6 +116,53 @@ static const struct inv_mpu6050_hw hw_info[] = {
 	},
 };
 
+static bool inv_mpu6050_volatile_reg(struct device *dev, unsigned int reg)
+{
+	if (reg >= INV_MPU6050_REG_RAW_ACCEL && reg < INV_MPU6050_REG_RAW_ACCEL + 6)
+		return true;
+	if (reg >= INV_MPU6050_REG_RAW_GYRO && reg < INV_MPU6050_REG_RAW_GYRO + 6)
+		return true;
+	switch (reg) {
+	case INV_MPU6050_REG_TEMPERATURE:
+	case INV_MPU6050_REG_TEMPERATURE + 1:
+	case INV_MPU6050_REG_USER_CTRL:
+	case INV_MPU6050_REG_PWR_MGMT_1:
+	case INV_MPU6050_REG_FIFO_COUNT_H:
+	case INV_MPU6050_REG_FIFO_COUNT_H + 1:
+	case INV_MPU6050_REG_FIFO_R_W:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool inv_mpu6050_precious_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case INV_MPU6050_REG_FIFO_R_W:
+		return true;
+	default:
+		return false;
+	}
+}
+
+/*
+ * Common regmap config for inv_mpu devices
+ *
+ * The current volatile/precious registers are common among supported devices.
+ * When that changes the volatile/precious callbacks should go through the
+ * inv_mpu6050_reg_map structs.
+ */
+const struct regmap_config inv_mpu_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+
+	.cache_type = REGCACHE_RBTREE,
+	.volatile_reg = inv_mpu6050_volatile_reg,
+	.precious_reg = inv_mpu6050_precious_reg,
+};
+EXPORT_SYMBOL_GPL(inv_mpu_regmap_config);
+
 int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en, u32 mask)
 {
 	unsigned int d, mgmt_1;
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
index 1a424a6..1a8d1a5 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
@@ -20,11 +20,6 @@
 #include <linux/module.h>
 #include "inv_mpu_iio.h"
 
-static const struct regmap_config inv_mpu_regmap_config = {
-	.reg_bits = 8,
-	.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
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
index 47ca25b..297b0ef 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
@@ -291,3 +291,4 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
 int inv_mpu_core_remove(struct device *dev);
 int inv_mpu6050_set_power_itg(struct inv_mpu6050_state *st, bool power_on);
 extern const struct dev_pm_ops inv_mpu_pmops;
+extern const struct regmap_config inv_mpu_regmap_config;
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c
index 190a4a5..b3bd977 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c
@@ -17,11 +17,6 @@
 #include <linux/iio/iio.h>
 #include "inv_mpu_iio.h"
 
-static const struct regmap_config inv_mpu_regmap_config = {
-	.reg_bits = 8,
-	.val_bits = 8,
-};
-
 static int inv_mpu_i2c_disable(struct iio_dev *indio_dev)
 {
 	struct inv_mpu6050_state *st = iio_priv(indio_dev);
-- 
2.5.5

[PATCH 3/7] iio: inv_mpu6050: Only toggle DATA_RDY_EN in inv_reset_fifo

[Crestez Dan Leonard]

Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c    | 13 ++++++++-----
 drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c |  3 ++-
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
index 8455af0..3fc0b71 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
@@ -41,7 +41,8 @@ int inv_reset_fifo(struct iio_dev *indio_dev)
 	struct inv_mpu6050_state  *st = iio_priv(indio_dev);
 
 	/* disable interrupt */
-	result = regmap_write(st->map, st->reg->int_enable, 0);
+	result = regmap_update_bits(st->map, st->reg->int_enable,
+				    INV_MPU6050_BIT_DATA_RDY_EN, 0);
 	if (result) {
 		dev_err(regmap_get_device(st->map), "int_enable failed %d\n",
 			result);
@@ -68,8 +69,9 @@ int inv_reset_fifo(struct iio_dev *indio_dev)
 	/* enable interrupt */
 	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);
+		result = regmap_update_bits(st->map, st->reg->int_enable,
+					    INV_MPU6050_BIT_DATA_RDY_EN,
+					    INV_MPU6050_BIT_DATA_RDY_EN);
 		if (result)
 			return result;
 	}
@@ -92,8 +94,9 @@ 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);
+	result = regmap_update_bits(st->map, st->reg->int_enable,
+				    INV_MPU6050_BIT_DATA_RDY_EN,
+				    INV_MPU6050_BIT_DATA_RDY_EN);
 
 	return result;
 }
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
index e8818d4..1a6bad3 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
@@ -69,7 +69,8 @@ static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
 		if (result)
 			return result;
 
-		result = regmap_write(st->map, st->reg->int_enable, 0);
+		result = regmap_update_bits(st->map, st->reg->int_enable,
+					INV_MPU6050_BIT_DATA_RDY_EN, 0);
 		if (result)
 			return result;
 
-- 
2.5.5

[PATCH 4/7] iio: inv_mpu6050: Cache non-volatile bits of user_ctrl

[Crestez Dan Leonard]

Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h     | 2 ++
 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c    | 9 ++++++---
 drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c | 4 +++-
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
index 297b0ef..bd2c0fd 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
@@ -80,6 +80,7 @@ enum inv_devices {
  *  @enable:		master enable state.
  *  @accl_fifo_enable:	enable accel data output
  *  @gyro_fifo_enable:	enable gyro data output
+ *  @user_ctrl:		The non-volatile bits of user_ctrl
  *  @fifo_rate:		FIFO update rate.
  */
 struct inv_mpu6050_chip_config {
@@ -89,6 +90,7 @@ struct inv_mpu6050_chip_config {
 	unsigned int enable:1;
 	unsigned int accl_fifo_enable:1;
 	unsigned int gyro_fifo_enable:1;
+	u8 user_ctrl;
 	u16 fifo_rate;
 };
 
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
index 3fc0b71..56ee1e2 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
@@ -53,13 +53,15 @@ int inv_reset_fifo(struct iio_dev *indio_dev)
 	if (result)
 		goto reset_fifo_fail;
 	/* disable fifo reading */
-	result = regmap_write(st->map, st->reg->user_ctrl, 0);
+	st->chip_config.user_ctrl &= ~INV_MPU6050_BIT_FIFO_EN;
+	result = regmap_write(st->map, st->reg->user_ctrl,
+			      st->chip_config.user_ctrl);
 	if (result)
 		goto reset_fifo_fail;
 
 	/* reset FIFO*/
 	result = regmap_write(st->map, st->reg->user_ctrl,
-			      INV_MPU6050_BIT_FIFO_RST);
+			      st->chip_config.user_ctrl | INV_MPU6050_BIT_FIFO_RST);
 	if (result)
 		goto reset_fifo_fail;
 
@@ -76,8 +78,9 @@ int inv_reset_fifo(struct iio_dev *indio_dev)
 			return result;
 	}
 	/* enable FIFO reading and I2C master interface*/
+	st->chip_config.user_ctrl |= INV_MPU6050_BIT_FIFO_EN;
 	result = regmap_write(st->map, st->reg->user_ctrl,
-			      INV_MPU6050_BIT_FIFO_EN);
+			      st->chip_config.user_ctrl);
 	if (result)
 		goto reset_fifo_fail;
 	/* enable sensor output to FIFO */
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
index 1a6bad3..fc55923 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
@@ -74,7 +74,9 @@ static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
 		if (result)
 			return result;
 
-		result = regmap_write(st->map, st->reg->user_ctrl, 0);
+		st->chip_config.user_ctrl &= ~INV_MPU6050_BIT_FIFO_EN;
+		result = regmap_write(st->map, st->reg->user_ctrl,
+				      st->chip_config.user_ctrl);
 		if (result)
 			return result;
 
-- 
2.5.5

[RFC 5/7] iio: inv_mpu6050: Add support for auxiliary I2C master

[Crestez Dan Leonard]

The MPU has an auxiliary I2C bus for connecting external
sensors. This bus has two operating modes:
* pass-through, which connects the primary and auxiliary busses
together. This is already supported via an i2c mux.
* I2C master mode, where the mpu60x0 acts as a master to any external
connected sensors. This is implemented by this patch.

This I2C master mode also works when the MPU itself is connected via
SPI.

I2C master supports up to 5 slaves. Slaves 0-3 have a common operating
mode while slave 4 is different. This patch implements an i2c adapter
using slave 4 because it has a cleaner interface and it has an
interrupt that signals when data from slave to master arrived.

Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
---
 .../devicetree/bindings/iio/imu/inv_mpu6050.txt    |  61 +++++-
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c         | 239 ++++++++++++++++++++-
 drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h          |  46 ++++
 drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c      |   8 -
 4 files changed, 341 insertions(+), 13 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt b/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt
index a9fc11e..aaf12b4 100644
--- a/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt
+++ b/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt
@@ -1,16 +1,27 @@
 InvenSense MPU-6050 Six-Axis (Gyro + Accelerometer) MEMS MotionTracking Device
 
-http://www.invensense.com/mems/gyro/mpu6050.html
-
 Required properties:
- - compatible : should be "invensense,mpu6050"
- - reg : the I2C address of the sensor
+ - compatible : should be "invensense,mpuXXXX"
+ - reg : the I2C or SPI address of the sensor
  - interrupt-parent : should be the phandle for the interrupt controller
  - interrupts : interrupt mapping for GPIO IRQ
 
 Optional properties:
  - mount-matrix: an optional 3x3 mounting rotation matrix
+ - inv,i2c-aux-master: operate aux i2c in "master mode" (default is mux).
+
+Valid compatible strings:
+ - mpu6000
+ - mpu6050
+ - mpu6500
+ - mpu9150
+
+It is possible to attach auxiliary sensors to the MPU and have them be handled
+by linux. Those auxiliary sensors are described as an i2c bus.
+
+Devices connected in "bypass" mode must be listed behind i2c@0 with the address 0
 
+Devices connected in "master" mode must be listed behind i2c@1 with the address 1
 
 Example:
 	mpu6050@68 {
@@ -28,3 +39,45 @@ Example:
 		               "0",                   /* y2 */
 		               "0.984807753012208";   /* z2 */
 	};
+
+Example describing mpu9150 (which includes an ak9875 on chip):
+	mpu9150@68 {
+		compatible = "invensense,mpu9150";
+		reg = <0x68>;
+		interrupt-parent = <&gpio1>;
+		interrupts = <18 1>;
+
+		#address-cells = <1>;
+		#size-cells = <0>;
+		i2c@0 {
+			reg = <0>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			ak8975@0c {
+				compatible = "ak,ak8975";
+				reg = <0x0c>;
+			};
+		};
+	};
+
+Example describing a mpu6500 via SPI with an hmc5883l on auxiliary i2c:
+	mpu6500@0 {
+		compatible = "inv,mpu6500";
+		reg = <0x0>;
+		spi-max-frequency = <1000000>;
+		interrupt-parent = <&gpio1>;
+		interrupts = <31 1>;
+
+		inv,i2c-aux-master
+		i2c@1 {
+			reg = <1>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			hmc5883l@1e {
+				compatible = "honeywell,hmc5883l";
+				reg = <0x1e>;
+			};
+		};
+	};
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index 5918c23..064fc07 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -25,6 +25,8 @@
 #include <linux/iio/iio.h>
 #include <linux/i2c-mux.h>
 #include <linux/acpi.h>
+#include <linux/completion.h>
+
 #include "inv_mpu_iio.h"
 
 /*
@@ -57,6 +59,12 @@ static const struct inv_mpu6050_reg_map reg_set_6500 = {
 	.int_pin_cfg		= INV_MPU6050_REG_INT_PIN_CFG,
 	.accl_offset		= INV_MPU6500_REG_ACCEL_OFFSET,
 	.gyro_offset		= INV_MPU6050_REG_GYRO_OFFSET,
+	.slv4_addr		= INV_MPU6050_REG_I2C_SLV4_ADDR,
+	.slv4_reg		= INV_MPU6050_REG_I2C_SLV4_REG,
+	.slv4_do		= INV_MPU6050_REG_I2C_SLV4_DO,
+	.slv4_ctrl		= INV_MPU6050_REG_I2C_SLV4_CTRL,
+	.slv4_di		= INV_MPU6050_REG_I2C_SLV4_DI,
+	.mst_status		= INV_MPU6050_REG_I2C_MST_STATUS,
 };
 
 static const struct inv_mpu6050_reg_map reg_set_6050 = {
@@ -77,6 +85,12 @@ static const struct inv_mpu6050_reg_map reg_set_6050 = {
 	.int_pin_cfg		= INV_MPU6050_REG_INT_PIN_CFG,
 	.accl_offset		= INV_MPU6050_REG_ACCEL_OFFSET,
 	.gyro_offset		= INV_MPU6050_REG_GYRO_OFFSET,
+	.slv4_addr		= INV_MPU6050_REG_I2C_SLV4_ADDR,
+	.slv4_reg		= INV_MPU6050_REG_I2C_SLV4_REG,
+	.slv4_do		= INV_MPU6050_REG_I2C_SLV4_DO,
+	.slv4_ctrl		= INV_MPU6050_REG_I2C_SLV4_CTRL,
+	.slv4_di		= INV_MPU6050_REG_I2C_SLV4_DI,
+	.mst_status		= INV_MPU6050_REG_I2C_MST_STATUS,
 };
 
 static const struct inv_mpu6050_chip_config chip_config_6050 = {
@@ -130,6 +144,10 @@ static bool inv_mpu6050_volatile_reg(struct device *dev, unsigned int reg)
 	case INV_MPU6050_REG_FIFO_COUNT_H:
 	case INV_MPU6050_REG_FIFO_COUNT_H + 1:
 	case INV_MPU6050_REG_FIFO_R_W:
+	case INV_MPU6050_REG_I2C_MST_STATUS:
+	case INV_MPU6050_REG_INT_STATUS:
+	case INV_MPU6050_REG_I2C_SLV4_CTRL:
+	case INV_MPU6050_REG_I2C_SLV4_DI:
 		return true;
 	default:
 		return false;
@@ -140,6 +158,8 @@ static bool inv_mpu6050_precious_reg(struct device *dev, unsigned int reg)
 {
 	switch (reg) {
 	case INV_MPU6050_REG_FIFO_R_W:
+	case INV_MPU6050_REG_I2C_MST_STATUS:
+	case INV_MPU6050_REG_INT_STATUS:
 		return true;
 	default:
 		return false;
@@ -852,6 +872,196 @@ static int inv_check_and_setup_chip(struct inv_mpu6050_state *st)
 	return 0;
 }
 
+static irqreturn_t inv_mpu_irq_handler(int irq, void *private)
+{
+	struct inv_mpu6050_state *st = (struct inv_mpu6050_state *)private;
+
+	iio_trigger_poll(st->trig);
+
+	return IRQ_WAKE_THREAD;
+}
+
+static irqreturn_t inv_mpu_irq_thread_handler(int irq, void *private)
+{
+	struct inv_mpu6050_state *st = (struct inv_mpu6050_state *)private;
+	int ret, val;
+
+	ret = regmap_read(st->map, st->reg->mst_status, &val);
+	if (ret < 0)
+		return ret;
+
+#ifdef CONFIG_I2C
+	if (val & INV_MPU6050_BIT_I2C_SLV4_DONE)
+		complete(&st->slv4_done);
+#endif
+
+	return IRQ_HANDLED;
+}
+
+#ifdef CONFIG_I2C
+static u32 inv_mpu_i2c_functionality(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_SMBUS_BYTE_DATA;
+}
+
+static int
+inv_mpu_i2c_smbus_xfer(struct i2c_adapter *adap, u16 addr,
+		       unsigned short flags, char read_write, u8 command,
+		       int size, union i2c_smbus_data *data)
+{
+	struct inv_mpu6050_state *st = i2c_get_adapdata(adap);
+	struct iio_dev *indio_dev = iio_priv_to_dev(st);
+
+	unsigned long time_left;
+	int result, val;
+	u8 ctrl;
+
+	/*
+	 * The i2c adapter we implement is extremely limited.
+	 * Check for callers that don't check functionality bits.
+	 *
+	 * If we don't check we might succesfully return incorrect data.
+	 */
+	if (size != I2C_SMBUS_BYTE_DATA) {
+		dev_err(&adap->dev, "unsupported xfer rw=%d size=%d\n",
+			read_write, size);
+		dump_stack();
+		return -EINVAL;
+	}
+
+	mutex_lock(&indio_dev->mlock);
+	result = inv_mpu6050_set_power_itg(st, true);
+	mutex_unlock(&indio_dev->mlock);
+	if (result < 0)
+		return result;
+
+	if (read_write == I2C_SMBUS_WRITE)
+		addr |= INV_MPU6050_BIT_I2C_SLV4_W;
+	else
+		addr |= INV_MPU6050_BIT_I2C_SLV4_R;
+
+	/*
+	 * Regmap will never ignore writes but it will ignore full-register
+	 * updates to the same value.
+	 *
+	 * This avoids having to touch slv4_addr when doing consecutive
+	 * reads/writes with the same device.
+	 */
+	result = regmap_update_bits(st->map, st->reg->slv4_addr, 0xff, addr);
+	if (result < 0)
+		return result;
+
+	result = regmap_write(st->map, st->reg->slv4_reg, command);
+	if (result < 0)
+		return result;
+
+	if (read_write == I2C_SMBUS_WRITE) {
+		result = regmap_write(st->map, st->reg->slv4_do, data->byte);
+		if (result < 0)
+			return result;
+	}
+
+	ctrl = INV_MPU6050_BIT_SLV4_EN | INV_MPU6050_BIT_SLV4_INT_EN;
+	result = regmap_write(st->map, st->reg->slv4_ctrl, ctrl);
+	if (result < 0)
+		return result;
+
+	/* Wait for completion for both reads and writes */
+	time_left = wait_for_completion_timeout(&st->slv4_done, HZ);
+	if (!time_left)
+		return -ETIMEDOUT;
+
+	if (read_write == I2C_SMBUS_READ) {
+		result = regmap_read(st->map, st->reg->slv4_di, &val);
+		if (result < 0)
+			return result;
+		data->byte = val;
+	}
+
+	mutex_lock(&indio_dev->mlock);
+	result = inv_mpu6050_set_power_itg(st, false);
+	mutex_unlock(&indio_dev->mlock);
+	if (result < 0)
+		return result;
+	return 0;
+}
+
+static const struct i2c_algorithm inv_mpu_i2c_algo = {
+	.smbus_xfer	=	inv_mpu_i2c_smbus_xfer,
+	.functionality	=	inv_mpu_i2c_functionality,
+};
+
+static struct device_node *inv_mpu_get_aux_i2c_ofnode(struct device_node *parent)
+{
+	struct device_node *child;
+	int result;
+	u32 reg;
+
+	if (!parent)
+		return NULL;
+	for_each_child_of_node(parent, child) {
+		result = of_property_read_u32(child, "reg", &reg);
+		if (result)
+			continue;
+		if (reg == 1 && !strcmp(child->name, "i2c"))
+			return child;
+	}
+
+	return NULL;
+}
+
+static int inv_mpu_probe_aux_master(struct iio_dev* indio_dev)
+{
+	struct inv_mpu6050_state *st = iio_priv(indio_dev);
+	struct device *dev = regmap_get_device(st->map);
+	int result;
+
+	/* First check i2c-aux-master property */
+	st->i2c_aux_master_mode = of_property_read_bool(dev->of_node,
+							"inv,i2c-aux-master");
+	if (!st->i2c_aux_master_mode)
+		return 0;
+	dev_info(dev, "Configuring aux i2c in master mode\n");
+
+	result = inv_mpu6050_set_power_itg(st, true);
+	if (result < 0)
+		return result;
+
+	/* enable master */
+	st->chip_config.user_ctrl |= INV_MPU6050_BIT_I2C_MST_EN;
+	result = regmap_write(st->map, st->reg->user_ctrl, st->chip_config.user_ctrl);
+	if (result < 0)
+		return result;
+
+	result = regmap_update_bits(st->map, st->reg->int_enable,
+				 INV_MPU6050_BIT_MST_INT_EN,
+				 INV_MPU6050_BIT_MST_INT_EN);
+	if (result < 0)
+		return result;
+
+	result = inv_mpu6050_set_power_itg(st, false);
+	if (result < 0)
+		return result;
+
+	init_completion(&st->slv4_done);
+	st->aux_master_adapter.owner = THIS_MODULE;
+	st->aux_master_adapter.algo = &inv_mpu_i2c_algo;
+	st->aux_master_adapter.dev.parent = dev;
+	snprintf(st->aux_master_adapter.name, sizeof(st->aux_master_adapter.name),
+			"aux-master-%s", indio_dev->name);
+	st->aux_master_adapter.dev.of_node = inv_mpu_get_aux_i2c_ofnode(dev->of_node);
+	i2c_set_adapdata(&st->aux_master_adapter, st);
+	/* This will also probe aux devices so transfers must work now */
+	result = i2c_add_adapter(&st->aux_master_adapter);
+	if (result < 0) {
+		dev_err(dev, "i2x aux master register fail %d\n", result);
+		return result;
+	}
+
+	return 0;
+}
+#endif
+
 int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
 		int (*inv_mpu_bus_setup)(struct iio_dev *), int chip_type)
 {
@@ -931,16 +1141,39 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
 		goto out_unreg_ring;
 	}
 
+	/* Request interrupt for trigger and i2c master adapter */
+	result = devm_request_threaded_irq(&indio_dev->dev, st->irq,
+					   &inv_mpu_irq_handler,
+					   &inv_mpu_irq_thread_handler,
+					   IRQF_TRIGGER_RISING, "inv_mpu",
+					   st);
+	if (result) {
+		dev_err(dev, "request irq fail %d\n", result);
+		goto out_remove_trigger;
+	}
+
+#ifdef CONFIG_I2C
+	result = inv_mpu_probe_aux_master(indio_dev);
+	if (result < 0) {
+		dev_err(dev, "i2c aux master probe fail %d\n", result);
+		goto out_remove_trigger;
+	}
+#endif
+
 	INIT_KFIFO(st->timestamps);
 	spin_lock_init(&st->time_stamp_lock);
 	result = iio_device_register(indio_dev);
 	if (result) {
 		dev_err(dev, "IIO register fail %d\n", result);
-		goto out_remove_trigger;
+		goto out_del_adapter;
 	}
 
 	return 0;
 
+out_del_adapter:
+#ifdef CONFIG_I2C
+	i2c_del_adapter(&st->aux_master_adapter);
+#endif
 out_remove_trigger:
 	inv_mpu6050_remove_trigger(st);
 out_unreg_ring:
@@ -952,10 +1185,14 @@ EXPORT_SYMBOL_GPL(inv_mpu_core_probe);
 int inv_mpu_core_remove(struct device  *dev)
 {
 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct inv_mpu6050_state *st = iio_priv(indio_dev);
 
 	iio_device_unregister(indio_dev);
 	inv_mpu6050_remove_trigger(iio_priv(indio_dev));
 	iio_triggered_buffer_cleanup(indio_dev);
+#ifdef CONFIG_I2C
+	i2c_del_adapter(&st->aux_master_adapter);
+#endif
 
 	return 0;
 }
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
index bd2c0fd..9d15633 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
@@ -42,6 +42,13 @@
  *  @int_pin_cfg;	Controls interrupt pin configuration.
  *  @accl_offset:	Controls the accelerometer calibration offset.
  *  @gyro_offset:	Controls the gyroscope calibration offset.
+ *  @mst_status:	secondary I2C master interrupt source status
+ *  @slv4_addr:		I2C slave address for slave 4 transaction
+ *  @slv4_reg:		I2C register used with slave 4 transaction
+ *  @slv4_di:		I2C data in register for slave 4 transaction
+ *  @slv4_ctrl:		I2C slave 4 control register
+ *  @slv4_do:		I2C data out register for slave 4 transaction
+
  */
 struct inv_mpu6050_reg_map {
 	u8 sample_rate_div;
@@ -61,6 +68,15 @@ struct inv_mpu6050_reg_map {
 	u8 int_pin_cfg;
 	u8 accl_offset;
 	u8 gyro_offset;
+	u8 mst_status;
+
+	/* slave 4 registers */
+	u8 slv4_addr;
+	u8 slv4_reg;
+	u8 slv4_di; /* data in */
+	u8 slv4_ctrl;
+	u8 slv4_do; /* data out */
+
 };
 
 /*device enum */
@@ -139,6 +155,15 @@ struct inv_mpu6050_state {
 	DECLARE_KFIFO(timestamps, long long, TIMESTAMP_FIFO_SIZE);
 	struct regmap *map;
 	int irq;
+
+	/* if the MPU connects to aux devices as a master */
+	bool i2c_aux_master_mode;
+
+#ifdef CONFIG_I2C
+	/* I2C adapter for talking to aux sensors in "master" mode */
+	struct i2c_adapter aux_master_adapter;
+	struct completion slv4_done;
+#endif
 };
 
 /*register and associated bit definition*/
@@ -154,9 +179,30 @@ struct inv_mpu6050_state {
 #define INV_MPU6050_BIT_ACCEL_OUT           0x08
 #define INV_MPU6050_BITS_GYRO_OUT           0x70
 
+#define INV_MPU6050_REG_I2C_SLV4_ADDR       0x31
+#define INV_MPU6050_BIT_I2C_SLV4_R          0x80
+#define INV_MPU6050_BIT_I2C_SLV4_W          0x00
+
+#define INV_MPU6050_REG_I2C_SLV4_REG        0x32
+#define INV_MPU6050_REG_I2C_SLV4_DO         0x33
+#define INV_MPU6050_REG_I2C_SLV4_CTRL       0x34
+
+#define INV_MPU6050_BIT_SLV4_EN             0x80
+#define INV_MPU6050_BIT_SLV4_INT_EN         0x40
+#define INV_MPU6050_BIT_SLV4_DIS            0x20
+
+#define INV_MPU6050_REG_I2C_SLV4_DI         0x35
+
+#define INV_MPU6050_REG_I2C_MST_STATUS      0x36
+#define INV_MPU6050_BIT_I2C_SLV4_DONE       0x40
+
 #define INV_MPU6050_REG_INT_ENABLE          0x38
 #define INV_MPU6050_BIT_DATA_RDY_EN         0x01
 #define INV_MPU6050_BIT_DMP_INT_EN          0x02
+#define INV_MPU6050_BIT_MST_INT_EN          0x08
+
+#define INV_MPU6050_REG_INT_STATUS          0x3A
+#define INV_MPU6050_BIT_MST_INT             0x08
 
 #define INV_MPU6050_REG_RAW_ACCEL           0x3B
 #define INV_MPU6050_REG_TEMPERATURE         0x41
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
index fc55923..a334ed9 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
@@ -126,14 +126,6 @@ int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev)
 	if (!st->trig)
 		return -ENOMEM;
 
-	ret = devm_request_irq(&indio_dev->dev, st->irq,
-			       &iio_trigger_generic_data_rdy_poll,
-			       IRQF_TRIGGER_RISING,
-			       "inv_mpu",
-			       st->trig);
-	if (ret)
-		return ret;
-
 	st->trig->dev.parent = regmap_get_device(st->map);
 	st->trig->ops = &inv_mpu_trigger_ops;
 	iio_trigger_set_drvdata(st->trig, indio_dev);
-- 
2.5.5

[PATCH 6/7] iio: inv_mpu6050: Check channel configuration on preenable

[Crestez Dan Leonard]

Right now it is possible to only enable some of the x/y/z channels, for
example you can enable accel_z without x or y. If you actually do that
what you get is actually only the x channel.

In theory the device supports selecting gyro x/y/z channels
individually. It would also be possible to selectively enable x/y/z
accel by unpacking the data read from the hardware into a format the iio
core accepts.

It is easier to simply refuse incorrect configuration.

Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c |  2 +-
 drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h  |  4 ++++
 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 31 ++++++++++++++++++++++++++++++
 3 files changed, 36 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 064fc07..712e901 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -1130,7 +1130,7 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
 	result = iio_triggered_buffer_setup(indio_dev,
 					    inv_mpu6050_irq_handler,
 					    inv_mpu6050_read_fifo,
-					    NULL);
+					    &inv_mpu_buffer_ops);
 	if (result) {
 		dev_err(dev, "configure buffer fail %d\n", result);
 		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 9d15633..9d406df 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
@@ -284,6 +284,9 @@ enum inv_mpu6050_scan {
 	INV_MPU6050_SCAN_TIMESTAMP,
 };
 
+#define INV_MPU6050_SCAN_MASK_ACCEL	0x07
+#define INV_MPU6050_SCAN_MASK_GYRO	0x38
+
 enum inv_mpu6050_filter_e {
 	INV_MPU6050_FILTER_256HZ_NOLPF2 = 0,
 	INV_MPU6050_FILTER_188HZ,
@@ -340,3 +343,4 @@ int inv_mpu_core_remove(struct device *dev);
 int inv_mpu6050_set_power_itg(struct inv_mpu6050_state *st, bool power_on);
 extern const struct dev_pm_ops inv_mpu_pmops;
 extern const struct regmap_config inv_mpu_regmap_config;
+extern const struct iio_buffer_setup_ops inv_mpu_buffer_ops;
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
index 56ee1e2..e8bda7f 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
@@ -222,3 +222,34 @@ flush_fifo:
 
 	return IRQ_HANDLED;
 }
+
+/* Validate channels are set in a correct configuration */
+static int inv_mpu_buffer_preenable(struct iio_dev *indio_dev)
+{
+	struct inv_mpu6050_state *st = iio_priv(indio_dev);
+	unsigned long mask = *indio_dev->active_scan_mask;
+
+	if ((mask & INV_MPU6050_SCAN_MASK_GYRO) &&
+	    (mask & INV_MPU6050_SCAN_MASK_GYRO) != INV_MPU6050_SCAN_MASK_GYRO)
+	{
+		dev_warn(regmap_get_device(st->map),
+			 "Gyro channels can only be enabled together\n");
+		return -EINVAL;
+	}
+
+	if ((mask & INV_MPU6050_SCAN_MASK_ACCEL) &&
+	    (mask & INV_MPU6050_SCAN_MASK_ACCEL) != INV_MPU6050_SCAN_MASK_ACCEL)
+	{
+		dev_warn(regmap_get_device(st->map),
+			 "Accel channels can only be enabled together\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+const struct iio_buffer_setup_ops inv_mpu_buffer_ops = {
+	.preenable = inv_mpu_buffer_preenable,
+	.postenable = iio_triggered_buffer_postenable,
+	.predisable = iio_triggered_buffer_predisable,
+};
-- 
2.5.5

[RFC 7/7] iio: inv_mpu6050: Add support for external sensors

[Crestez Dan Leonard]

This works by copying their channels at probe time.

Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
---
 .../devicetree/bindings/iio/imu/inv_mpu6050.txt    |  37 +-
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c         | 420 ++++++++++++++++++++-
 drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h          |  63 +++-
 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c         |  48 ++-
 4 files changed, 555 insertions(+), 13 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt b/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt
index aaf12b4..79b8326 100644
--- a/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt
+++ b/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt
@@ -23,6 +23,28 @@ Devices connected in "bypass" mode must be listed behind i2c@0 with the address
 
 Devices connected in "master" mode must be listed behind i2c@1 with the address 1
 
+It is possible to configure the MPU to automatically fetch reading for
+devices connected on the auxiliary i2c bus without CPU intervention. When this
+is done the driver will present the channels of the slaved devices as if they
+belong to the MPU device itself.
+
+Reads and write to config values (like scaling factors) are forwarded to the
+other driver while data reads are handled from MPU registers. The channels are
+also available through the MPU's iio triggered buffer mechanism. This feature
+can allow sampling up to 24 bytes from 4 slaves at a much higher rate.
+
+This is be specified in device tree as an "inv,external-sensors" node which
+have children indexed by slave ids 0 to 3. The child node identifying each
+external sensor reading has the following properties:
+ - reg: 0 to 3 slave index
+ - inv,addr : the I2C address to read from
+ - inv,reg : the I2C register to read from
+ - inv,len : read length from the device
+ - inv,external-channels : list of slaved channels specified by config index.
+
+The sum of storagebits for external channels must equal the read length. Only
+16bit channels are currently supported.
+
 Example:
 	mpu6050@68 {
 		compatible = "invensense,mpu6050";
@@ -61,7 +83,8 @@ Example describing mpu9150 (which includes an ak9875 on chip):
 		};
 	};
 
-Example describing a mpu6500 via SPI with an hmc5883l on auxiliary i2c:
+Example describing a mpu6500 via SPI with an hmc5883l on aux i2c configured for
+automatic external readings as slave 0:
 	mpu6500@0 {
 		compatible = "inv,mpu6500";
 		reg = <0x0>;
@@ -80,4 +103,16 @@ Example describing a mpu6500 via SPI with an hmc5883l on auxiliary i2c:
 				reg = <0x1e>;
 			};
 		};
+
+		inv,external-sensors {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			hmc5833l@0 {
+				reg = <0>;
+				inv,addr = <0x1e>;
+				inv,reg = <3>;
+				inv,len = <6>;
+				inv,external-channels = <0 1 2>;
+			};
+		};
 	};
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index 712e901..224be3c 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -41,6 +41,8 @@ static const int gyro_scale_6050[] = {133090, 266181, 532362, 1064724};
  */
 static const int accel_scale[] = {598, 1196, 2392, 4785};
 
+#define INV_MPU6050_NUM_INT_CHAN 8
+
 static const struct inv_mpu6050_reg_map reg_set_6500 = {
 	.sample_rate_div	= INV_MPU6050_REG_SAMPLE_RATE_DIV,
 	.lpf                    = INV_MPU6050_REG_CONFIG,
@@ -136,6 +138,9 @@ static bool inv_mpu6050_volatile_reg(struct device *dev, unsigned int reg)
 		return true;
 	if (reg >= INV_MPU6050_REG_RAW_GYRO && reg < INV_MPU6050_REG_RAW_GYRO + 6)
 		return true;
+	if (reg < INV_MPU6050_REG_EXT_SENS_DATA_00 + INV_MPU6050_CNT_EXT_SENS_DATA &&
+			reg >= INV_MPU6050_REG_EXT_SENS_DATA_00)
+		return true;
 	switch (reg) {
 	case INV_MPU6050_REG_TEMPERATURE:
 	case INV_MPU6050_REG_TEMPERATURE + 1:
@@ -340,13 +345,115 @@ static int inv_mpu6050_sensor_show(struct inv_mpu6050_state  *st, int reg,
 	return IIO_VAL_INT;
 }
 
+static bool iio_chan_needs_swab(const struct iio_chan_spec *chan)
+{
+#if defined(__LITTLE_ENDIAN)
+	return chan->scan_type.endianness == IIO_BE;
+#elif defined(__BIG_ENDIAN)
+	return chan->scan_type.endianness == IIO_LE;
+#else
+	#error undefined endianness
+#endif
+}
+
+/* Convert iio buffer format to IIO_CHAN_INFO_RAW value */
+static int iio_bufval_to_rawval(const struct iio_chan_spec *chan, void *buf, int *val)
+{
+	switch (chan->scan_type.storagebits) {
+	case 8: *val = *((u8*)buf); break;
+	case 16: *val = *(u16*)buf; break;
+	case 32: *val = *(u32*)buf; break;
+	default: return -EINVAL;
+	}
+
+	*val >>= chan->scan_type.shift;
+	*val &= (1 << chan->scan_type.realbits) - 1;
+	if (iio_chan_needs_swab(chan)) {
+		if (chan->scan_type.storagebits == 16)
+			*val = swab16(*val);
+		else if (chan->scan_type.storagebits == 32)
+			*val = swab32(*val);
+	}
+	if (chan->scan_type.sign == 's')
+		*val = sign_extend32(*val, chan->scan_type.storagebits - 1);
+
+	return 0;
+}
+
+static int
+inv_mpu6050_read_raw_external(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      int *val, int *val2, long mask)
+{
+	int result;
+	struct inv_mpu6050_state *st = iio_priv(indio_dev);
+	int chan_index = chan - indio_dev->channels;
+	struct inv_mpu6050_ext_chan_state *ext_chan_state =
+			&st->ext_chan[chan_index - INV_MPU6050_NUM_INT_CHAN];
+	struct inv_mpu6050_ext_sens_state *ext_sens_state =
+			&st->ext_sens[ext_chan_state->ext_sens_index];
+	struct iio_dev *orig_dev = ext_sens_state->orig_dev;
+	const struct iio_chan_spec *orig_chan = &orig_dev->channels[ext_chan_state->orig_chan_index];
+	int data_offset;
+	int data_length;
+	u8 buf[4];
+
+	/* Everything but raw data reads is forwarded. */
+	if (mask != IIO_CHAN_INFO_RAW)
+		return orig_dev->info->read_raw(orig_dev, orig_chan, val, val2, mask);
+
+	/* Raw reads are handled directly. */
+	data_offset = INV_MPU6050_REG_EXT_SENS_DATA_00 + ext_chan_state->data_offset;
+	data_length = chan->scan_type.storagebits / 8;
+	if (data_length > sizeof(buf)) {
+		return -EINVAL;
+	}
+
+	mutex_lock(&indio_dev->mlock);
+	if (!st->chip_config.enable) {
+		result = inv_mpu6050_set_power_itg(st, true);
+		if (result)
+			goto error_unlock;
+	}
+	result = regmap_bulk_read(st->map, data_offset, buf, data_length);
+	if (result)
+		goto error_unpower;
+	if (!st->chip_config.enable) {
+		result = inv_mpu6050_set_power_itg(st, false);
+		if (result)
+			goto error_unlock;
+	}
+	mutex_unlock(&indio_dev->mlock);
+
+	/* Convert buf to val and return success */
+	result = iio_bufval_to_rawval(chan, buf, val);
+	if (result)
+		return result;
+	return IIO_VAL_INT;
+
+error_unpower:
+	if (!st->chip_config.enable)
+		inv_mpu6050_set_power_itg(st, false);
+error_unlock:
+	mutex_unlock(&indio_dev->mlock);
+	return result;
+}
+
 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);
+	struct inv_mpu6050_state *st = iio_priv(indio_dev);
 	int ret = 0;
+	int chan_index;
+
+	if (chan < indio_dev->channels || chan > indio_dev->channels + indio_dev->num_channels) {
+		return -EINVAL;
+	}
+	chan_index = chan - indio_dev->channels;
+	if (chan_index >= INV_MPU6050_NUM_INT_CHAN)
+		return inv_mpu6050_read_raw_external(indio_dev, chan, val, val2, mask);
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
@@ -819,6 +926,309 @@ static const struct iio_info mpu_info = {
 	.validate_trigger = inv_mpu6050_validate_trigger,
 };
 
+extern struct device_type iio_device_type;
+
+static int iio_device_from_i2c_client_match(struct device *dev, void *data)
+{
+	return dev->type == &iio_device_type;
+}
+
+static struct iio_dev* iio_device_from_i2c_client(struct i2c_client* i2c)
+{
+	struct device *child;
+
+	child = device_find_child(&i2c->dev, NULL, iio_device_from_i2c_client_match);
+
+	return (child ? dev_to_iio_dev(child) : NULL);
+}
+
+static int inv_mpu_set_external_reads_enabled(struct inv_mpu6050_state *st, bool en)
+{
+	int i, result, error = 0;
+
+	/* Even if there are errors try to disable all slaves. */
+	for (i = 0; i < INV_MPU6050_MAX_EXT_SENSORS; ++i) {
+		result = regmap_update_bits(st->map, INV_MPU6050_REG_I2C_SLV_CTRL(i),
+					    INV_MPU6050_BIT_I2C_SLV_EN,
+					    en ?  INV_MPU6050_BIT_I2C_SLV_EN : 0);
+		if (result) {
+			error = result;
+		}
+	}
+
+	return error;
+}
+
+static int inv_mpu_parse_one_ext_sens(struct device *dev,
+				      struct device_node *np,
+				      struct inv_mpu6050_ext_sens_spec *spec)
+{
+	int result;
+	u32 addr, reg, len;
+
+	result = of_property_read_u32(np, "inv,addr", &addr);
+	if (result)
+		return result;
+	result = of_property_read_u32(np, "inv,reg", &reg);
+	if (result)
+		return result;
+	result = of_property_read_u32(np, "inv,len", &len);
+	if (result)
+		return result;
+
+	spec->addr = addr;
+	spec->reg = reg;
+	spec->len = len;
+
+	result = of_property_count_u32_elems(np, "inv,external-channels");
+	if (result < 0)
+		return result;
+	spec->num_ext_channels = result;
+	spec->ext_channels = devm_kmalloc(dev, spec->num_ext_channels * sizeof(*spec->ext_channels), GFP_KERNEL);
+	if (!spec->ext_channels)
+		return -ENOMEM;
+	result = of_property_read_u32_array(np, "inv,external-channels",
+					    spec->ext_channels,
+					    spec->num_ext_channels);
+	if (result)
+		return result;
+
+	return 0;
+}
+
+static int inv_mpu_parse_ext_sens(struct device *dev,
+				  struct device_node *node,
+				  struct inv_mpu6050_ext_sens_spec *specs)
+{
+	struct device_node *child;
+	int result;
+	u32 reg;
+
+	for_each_available_child_of_node(node, child) {
+		result = of_property_read_u32(child, "reg", &reg);
+		if (result)
+			return result;
+		if (reg > INV_MPU6050_MAX_EXT_SENSORS) {
+			dev_err(dev, "External sensor index %u out of range, max %d\n",
+				reg, INV_MPU6050_MAX_EXT_SENSORS);
+			return -EINVAL;
+		}
+		result = inv_mpu_parse_one_ext_sens(dev, child, &specs[reg]);
+		if (result)
+			return result;
+	}
+
+	return 0;
+}
+
+static int inv_mpu_get_ext_sens_spec(struct iio_dev *indio_dev)
+{
+	struct inv_mpu6050_state *st = iio_priv(indio_dev);
+	struct device *dev = regmap_get_device(st->map);
+	struct device_node *node;
+	int result;
+
+	node = of_get_child_by_name(dev->of_node, "inv,external-sensors");
+	if (node) {
+		result = inv_mpu_parse_ext_sens(dev, node, st->ext_sens_spec);
+		if (result)
+			dev_err(dev, "Failed to parsing external-sensors devicetree data\n");
+		return result;
+	}
+
+	/* Maybe some other way to deal with this? */
+
+	return 0;
+}
+
+/* Struct used while enumerating devices and matching them */
+struct inv_mpu_handle_ext_sensor_fnarg
+{
+	struct iio_dev *indio_dev;
+
+	/* Next scan index */
+	int scan_index;
+	/* Index among external channels */
+	int chan_index;
+	/* Offset in EXTDATA */
+	int data_offset;
+	struct iio_chan_spec *channels;
+};
+
+static int inv_mpu_config_external_read(struct inv_mpu6050_state *st, int index,
+					const struct inv_mpu6050_ext_sens_spec *spec)
+{
+	int result;
+
+	result = regmap_write(st->map, INV_MPU6050_REG_I2C_SLV_ADDR(index),
+			      INV_MPU6050_BIT_I2C_SLV_RW | spec->addr);
+	if (result)
+		return result;
+	result = regmap_write(st->map, INV_MPU6050_REG_I2C_SLV_REG(index), spec->reg);
+	if (result)
+		return result;
+	result = regmap_write(st->map, INV_MPU6050_REG_I2C_SLV_CTRL(index),
+			      INV_MPU6050_BIT_I2C_SLV_EN | spec->len);
+
+	return result;
+}
+
+static int inv_mpu_handle_ext_sensor_fn(struct device *dev, void *data)
+{
+	struct inv_mpu_handle_ext_sensor_fnarg *fnarg = data;
+	struct iio_dev *indio_dev = fnarg->indio_dev;
+	struct inv_mpu6050_state *st = iio_priv(indio_dev);
+	struct device *mydev = regmap_get_device(st->map);
+	struct i2c_client *client;
+	struct iio_dev *orig_dev;
+	int i, j;
+
+	client = i2c_verify_client(dev);
+	if (!client)
+		return 0;
+	orig_dev = iio_device_from_i2c_client(client);
+	if (!orig_dev)
+		return 0;
+	for (i = 0; i < INV_MPU6050_MAX_EXT_SENSORS; ++i) {
+		int start_data_offset;
+		struct inv_mpu6050_ext_sens_spec *ext_sens_spec = &st->ext_sens_spec[i];
+		struct inv_mpu6050_ext_sens_state *ext_sens_state = &st->ext_sens[i];
+
+		if (ext_sens_spec->addr != client->addr)
+			continue;
+		if (ext_sens_state->orig_dev) {
+			dev_warn(&indio_dev->dev, "already found slave with at addr %d\n", client->addr);
+			continue;
+		}
+		dev_info(mydev, "external sensor %d is device %s\n",
+				i, orig_dev->name ?: dev_name(&orig_dev->dev));
+		ext_sens_state->orig_dev = orig_dev;
+		ext_sens_state->scan_mask = 0;
+		ext_sens_state->data_len = ext_sens_spec->len;
+		start_data_offset = fnarg->data_offset;
+
+		/* Matched the device (by address). Now process channels: */
+		for (j = 0; j < ext_sens_spec->num_ext_channels; ++j) {
+			int orig_chan_index;
+			const struct iio_chan_spec *orig_chan_spec;
+			struct iio_chan_spec *chan_spec;
+			struct inv_mpu6050_ext_chan_state *chan_state;
+
+			orig_chan_index = ext_sens_spec->ext_channels[j];
+			orig_chan_spec = &orig_dev->channels[orig_chan_index];
+			chan_spec = &fnarg->channels[INV_MPU6050_NUM_INT_CHAN + fnarg->chan_index];
+			chan_state = &st->ext_chan[fnarg->chan_index];
+
+			chan_state->ext_sens_index = i;
+			chan_state->orig_chan_index = orig_chan_index;
+			chan_state->data_offset = fnarg->data_offset;
+			memcpy(chan_spec, orig_chan_spec, sizeof(struct iio_chan_spec));
+			chan_spec->scan_index = fnarg->scan_index;
+			ext_sens_state->scan_mask |= (1 << chan_spec->scan_index);
+			if (orig_chan_spec->scan_type.storagebits != 16) {
+				/*
+				 * Supporting other channels sizes would require data read from the
+				 * hardware fifo to comply with iio alignment.
+				 */
+				dev_err(&indio_dev->dev, "All external channels must have 16 storage bits\n");
+				return -EINVAL;
+			}
+
+			fnarg->scan_index++;
+			fnarg->chan_index++;
+			fnarg->data_offset += chan_spec->scan_type.storagebits / 8;
+			dev_info(mydev, "Reading external channel #%d scan_index %d data_offset %d"
+					" from original device %s chan #%d scan_index %d\n",
+					fnarg->chan_index, chan_spec->scan_index, chan_state->data_offset,
+					orig_dev->name ?: dev_name(&orig_dev->dev), orig_chan_index, orig_chan_spec->scan_index);
+		}
+		if (start_data_offset + ext_sens_spec->len != fnarg->data_offset) {
+			dev_warn(mydev, "length mismatch between i2c slave read length and slave channel spec\n");
+			return -EINVAL;
+		}
+
+		return inv_mpu_config_external_read(st, i, ext_sens_spec);
+	}
+	return 0;
+}
+
+static int inv_mpu6050_handle_ext_sensors(struct iio_dev *indio_dev)
+{
+	struct inv_mpu6050_state *st = iio_priv(indio_dev);
+	struct device *dev = regmap_get_device(st->map);
+	struct inv_mpu_handle_ext_sensor_fnarg fnarg = {
+		.indio_dev = indio_dev,
+		.chan_index = 0,
+		.data_offset = 0,
+		.scan_index = INV_MPU6050_SCAN_TIMESTAMP,
+	};
+	int i, result;
+	int num_ext_chan = 0, sum_data_len = 0;
+
+	inv_mpu_get_ext_sens_spec(indio_dev);
+	for (i = 0; i < INV_MPU6050_MAX_EXT_SENSORS; ++i) {
+		num_ext_chan += st->ext_sens_spec[i].num_ext_channels;
+		sum_data_len += st->ext_sens_spec[i].len;
+	}
+	if (sum_data_len > INV_MPU6050_CNT_EXT_SENS_DATA) {
+		dev_err(dev, "Too many bytes from external sensors:"
+			      " requested=%d max=%d\n",
+			      sum_data_len, INV_MPU6050_CNT_EXT_SENS_DATA);
+		return -EINVAL;
+	}
+
+	/* Zero length means nothing to do */
+	if (!sum_data_len) {
+		indio_dev->channels = inv_mpu_channels;
+		indio_dev->num_channels = INV_MPU6050_NUM_INT_CHAN;
+		BUILD_BUG_ON(ARRAY_SIZE(inv_mpu_channels) != INV_MPU6050_NUM_INT_CHAN);
+		return 0;
+	}
+
+	fnarg.channels = devm_kmalloc(indio_dev->dev.parent,
+				      (num_ext_chan + INV_MPU6050_NUM_INT_CHAN) *
+				      sizeof(struct iio_chan_spec),
+				      GFP_KERNEL);
+	if (!fnarg.channels)
+		return -ENOMEM;
+	memcpy(fnarg.channels, inv_mpu_channels, sizeof(inv_mpu_channels));
+	memset(fnarg.channels + INV_MPU6050_NUM_INT_CHAN, 0,
+	       num_ext_chan * sizeof(struct iio_chan_spec));
+
+	st->ext_chan = devm_kzalloc(indio_dev->dev.parent,
+			(num_ext_chan) * sizeof(*st->ext_chan),
+			GFP_KERNEL);
+	if (!st->ext_chan)
+		return -ENOMEM;
+
+	result = inv_mpu6050_set_power_itg(st, true);
+	if (result < 0)
+		return result;
+
+	result = device_for_each_child(&st->aux_master_adapter.dev, &fnarg,
+				       inv_mpu_handle_ext_sensor_fn);
+	if (result)
+		goto out_disable;
+	/* Timestamp channel has index 0 and last scan_index */
+	fnarg.channels[0].scan_index = fnarg.scan_index;
+
+	if (fnarg.chan_index != num_ext_chan) {
+		dev_err(&indio_dev->dev, "Failed to match all external channels!\n");
+		result = -EINVAL;
+		goto out_disable;
+	}
+
+	result = inv_mpu6050_set_power_itg(st, false);
+	indio_dev->channels = fnarg.channels;
+	indio_dev->num_channels = INV_MPU6050_NUM_INT_CHAN + num_ext_chan;
+	return result;
+
+out_disable:
+	inv_mpu6050_set_power_itg(st, false);
+	inv_mpu_set_external_reads_enabled(st, false);
+	return result;
+}
+
 /**
  *  inv_check_and_setup_chip() - check and setup chip.
  */
@@ -1121,8 +1531,6 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
 		indio_dev->name = name;
 	else
 		indio_dev->name = dev_name(dev);
-	indio_dev->channels = inv_mpu_channels;
-	indio_dev->num_channels = ARRAY_SIZE(inv_mpu_channels);
 
 	indio_dev->info = &mpu_info;
 	indio_dev->modes = INDIO_BUFFER_TRIGGERED;
@@ -1160,6 +1568,12 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
 	}
 #endif
 
+	result = inv_mpu6050_handle_ext_sensors(indio_dev);
+	if (result < 0) {
+		dev_err(dev, "failed to configure channels %d\n", result);
+		goto out_remove_trigger;
+	}
+
 	INIT_KFIFO(st->timestamps);
 	spin_lock_init(&st->time_stamp_lock);
 	result = iio_device_register(indio_dev);
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
index 9d406df..007fe75 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
@@ -124,6 +124,40 @@ struct inv_mpu6050_hw {
 	const struct inv_mpu6050_chip_config *config;
 };
 
+#define INV_MPU6050_MAX_EXT_SENSORS 4
+
+/* Specification for an external sensor */
+struct inv_mpu6050_ext_sens_spec {
+	unsigned short addr;
+	u8 reg, len;
+
+	int num_ext_channels;
+	int *ext_channels;
+};
+
+/* Driver state for each external sensor */
+struct inv_mpu6050_ext_sens_state {
+	struct iio_dev *orig_dev;
+
+	/* Mask of all channels in this sensor */
+	unsigned long scan_mask;
+
+	/* Length of reading. */
+	int data_len;
+};
+
+/* Driver state for each external channel */
+struct inv_mpu6050_ext_chan_state {
+	/* Index inside state->ext_sens */
+	int ext_sens_index;
+
+	/* Index inside orig_dev->channels */
+	int orig_chan_index;
+
+	/* Relative to EXT_SENS_DATA_00 */
+	int data_offset;
+};
+
 /*
  *  struct inv_mpu6050_state - Driver state variables.
  *  @TIMESTAMP_FIFO_SIZE: fifo size for timestamp.
@@ -164,6 +198,10 @@ struct inv_mpu6050_state {
 	struct i2c_adapter aux_master_adapter;
 	struct completion slv4_done;
 #endif
+
+	struct inv_mpu6050_ext_sens_spec ext_sens_spec[INV_MPU6050_MAX_EXT_SENSORS];
+	struct inv_mpu6050_ext_sens_state ext_sens[INV_MPU6050_MAX_EXT_SENSORS];
+	struct inv_mpu6050_ext_chan_state *ext_chan;
 };
 
 /*register and associated bit definition*/
@@ -178,6 +216,24 @@ struct inv_mpu6050_state {
 #define INV_MPU6050_REG_FIFO_EN             0x23
 #define INV_MPU6050_BIT_ACCEL_OUT           0x08
 #define INV_MPU6050_BITS_GYRO_OUT           0x70
+#define INV_MPU6050_BIT_SLV0_FIFO_EN        0x01
+#define INV_MPU6050_BIT_SLV1_FIFO_EN        0x02
+#define INV_MPU6050_BIT_SLV2_FIFO_EN        0x04
+#define INV_MPU6050_BIT_SLV2_FIFO_EN        0x04
+
+/* SLV3 fifo enabling is not in REG_FIFO_EN */
+#define INV_MPU6050_REG_MST_CTRL	    0x24
+#define INV_MPU6050_BIT_SLV3_FIFO_EN        0x10
+
+/* For slaves 0-3 */
+#define INV_MPU6050_REG_I2C_SLV_ADDR(i)     (0x25 + 3 * (i))
+#define INV_MPU6050_REG_I2C_SLV_REG(i)      (0x26 + 3 * (i))
+#define INV_MPU6050_REG_I2C_SLV_CTRL(i)     (0x27 + 3 * (i))
+#define INV_MPU6050_BIT_I2C_SLV_RW          0x80
+#define INV_MPU6050_BIT_I2C_SLV_EN          0x80
+#define INV_MPU6050_BIT_I2C_SLV_BYTE_SW     0x40
+#define INV_MPU6050_BIT_I2C_SLV_REG_DIS     0x20
+#define INV_MPU6050_BIT_I2C_SLV_REG_GRP     0x10
 
 #define INV_MPU6050_REG_I2C_SLV4_ADDR       0x31
 #define INV_MPU6050_BIT_I2C_SLV4_R          0x80
@@ -252,8 +308,8 @@ struct inv_mpu6050_state {
 #define INV_MPU6050_GYRO_CONFIG_FSR_SHIFT    3
 #define INV_MPU6050_ACCL_CONFIG_FSR_SHIFT    3
 
-/* 6 + 6 round up and plus 8 */
-#define INV_MPU6050_OUTPUT_DATA_SIZE         24
+/* max is 3*2 accel + 3*2 gyro + 24 external + 8 ts */
+#define INV_MPU6050_OUTPUT_DATA_SIZE         44
 
 #define INV_MPU6050_REG_INT_PIN_CFG	0x37
 #define INV_MPU6050_BIT_BYPASS_EN	0x2
@@ -266,6 +322,9 @@ struct inv_mpu6050_state {
 #define INV_MPU6050_MIN_FIFO_RATE            4
 #define INV_MPU6050_ONE_K_HZ                 1000
 
+#define INV_MPU6050_REG_EXT_SENS_DATA_00	0x49
+#define INV_MPU6050_CNT_EXT_SENS_DATA		24
+
 #define INV_MPU6050_REG_WHOAMI			117
 
 #define INV_MPU6000_WHOAMI_VALUE		0x68
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
index e8bda7f..8fa5c3d 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
@@ -52,6 +52,10 @@ int inv_reset_fifo(struct iio_dev *indio_dev)
 	result = regmap_write(st->map, st->reg->fifo_en, 0);
 	if (result)
 		goto reset_fifo_fail;
+	result = regmap_update_bits(st->map, INV_MPU6050_REG_MST_CTRL,
+				    INV_MPU6050_BIT_SLV3_FIFO_EN, 0);
+	if (result)
+		goto reset_fifo_fail;
 	/* disable fifo reading */
 	st->chip_config.user_ctrl &= ~INV_MPU6050_BIT_FIFO_EN;
 	result = regmap_write(st->map, st->reg->user_ctrl,
@@ -70,7 +74,8 @@ int inv_reset_fifo(struct iio_dev *indio_dev)
 
 	/* enable interrupt */
 	if (st->chip_config.accl_fifo_enable ||
-	    st->chip_config.gyro_fifo_enable) {
+	    st->chip_config.gyro_fifo_enable ||
+	    true) {
 		result = regmap_update_bits(st->map, st->reg->int_enable,
 					    INV_MPU6050_BIT_DATA_RDY_EN,
 					    INV_MPU6050_BIT_DATA_RDY_EN);
@@ -89,10 +94,23 @@ int inv_reset_fifo(struct iio_dev *indio_dev)
 		d |= INV_MPU6050_BITS_GYRO_OUT;
 	if (st->chip_config.accl_fifo_enable)
 		d |= INV_MPU6050_BIT_ACCEL_OUT;
+	if (*indio_dev->active_scan_mask & st->ext_sens[0].scan_mask)
+		d |= INV_MPU6050_BIT_SLV0_FIFO_EN;
+	if (*indio_dev->active_scan_mask & st->ext_sens[1].scan_mask)
+		d |= INV_MPU6050_BIT_SLV1_FIFO_EN;
+	if (*indio_dev->active_scan_mask & st->ext_sens[2].scan_mask)
+		d |= INV_MPU6050_BIT_SLV2_FIFO_EN;
 	result = regmap_write(st->map, st->reg->fifo_en, d);
 	if (result)
 		goto reset_fifo_fail;
-
+	if (*indio_dev->active_scan_mask & st->ext_sens[3].scan_mask)
+	{
+		result = regmap_update_bits(st->map, INV_MPU6050_REG_MST_CTRL,
+					    INV_MPU6050_BIT_SLV3_FIFO_EN,
+					    INV_MPU6050_BIT_SLV3_FIFO_EN);
+		if (result)
+			goto reset_fifo_fail;
+	}
 	return 0;
 
 reset_fifo_fail:
@@ -129,8 +147,9 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct inv_mpu6050_state *st = iio_priv(indio_dev);
-	size_t bytes_per_datum;
+	size_t bytes_per_datum = 0;
 	int result;
+	int i;
 	u8 data[INV_MPU6050_OUTPUT_DATA_SIZE];
 	u16 fifo_count;
 	s64 timestamp;
@@ -143,15 +162,18 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
 	spi = i2c ? NULL: to_spi_device(regmap_dev);
 
 	mutex_lock(&indio_dev->mlock);
-	if (!(st->chip_config.accl_fifo_enable |
-		st->chip_config.gyro_fifo_enable))
-		goto end_session;
+
+	/* Compute bytes_per_datum */
 	bytes_per_datum = 0;
 	if (st->chip_config.accl_fifo_enable)
 		bytes_per_datum += INV_MPU6050_BYTES_PER_3AXIS_SENSOR;
-
 	if (st->chip_config.gyro_fifo_enable)
 		bytes_per_datum += INV_MPU6050_BYTES_PER_3AXIS_SENSOR;
+	for (i = 0; i < INV_MPU6050_MAX_EXT_SENSORS; ++i)
+		if (st->ext_sens[i].scan_mask & *indio_dev->active_scan_mask)
+			bytes_per_datum += st->ext_sens[i].data_len;
+	if (!bytes_per_datum)
+		return 0;
 
 	/*
 	 * read fifo_count register to know how many bytes inside FIFO
@@ -228,6 +250,7 @@ static int inv_mpu_buffer_preenable(struct iio_dev *indio_dev)
 {
 	struct inv_mpu6050_state *st = iio_priv(indio_dev);
 	unsigned long mask = *indio_dev->active_scan_mask;
+	int i;
 
 	if ((mask & INV_MPU6050_SCAN_MASK_GYRO) &&
 	    (mask & INV_MPU6050_SCAN_MASK_GYRO) != INV_MPU6050_SCAN_MASK_GYRO)
@@ -245,6 +268,17 @@ static int inv_mpu_buffer_preenable(struct iio_dev *indio_dev)
 		return -EINVAL;
 	}
 
+	for (i = 0; i < INV_MPU6050_MAX_EXT_SENSORS; ++i) {
+		if ((mask & st->ext_sens[i].scan_mask) &&
+		    (mask & st->ext_sens[i].scan_mask) != st->ext_sens[i].scan_mask)
+		{
+			dev_warn(regmap_get_device(st->map),
+				 "External channels from the same reading "
+				 "can only be enabled together\n");
+			return -EINVAL;
+		}
+	}
+
 	return 0;
 }
 
-- 
2.5.5

Re: [RFC 0/7] iio: inv_mpu6050: Support i2c master and external readings

[Jonathan Cameron]

On 29/04/16 20:02, Crestez Dan Leonard wrote:
> This series attempts to implement support for external readings in i2c master
> mode. I don't expect this to go in this form but I wanted to present a
> functional version in order to start the discussion earlier.
> 
> 
> The I2C master support is useful even without external readings. For example in
> SPI mode this is the only way to contact auxiliary devices. Differences from
> the previous version:
>  - Require an explicit "inv,i2c-aux-master" bool property. Supporting dynamic
> switches between bypass/master mode is extremely difficult and not really
> worthwhile. Switching to bypass mode requires explicitly disabling all i2c
> master functionality, including external readings! After Peter Rosin's mux
> cleanup patches go in I would like to disable the mux entirely when in master
> mode.
>  - Describe i2c clients behind i2c@1. Maybe it should be i2c@0 like mux mode?
>  - Validate parameters and return an error on unsupported ops (like read_word)
>  - Wait for SLV4_DONE even on writes!
>  - Fix issues when other parts of the driver write to the same registers.
> 
> My initial idea was to use regcache and regmap_update_bits to handle config
> bits like in PATCH 2. It turn out that the device has several registers with a
> combination of volatile "command bits" (writing triggers an action) and "config
> bits" (value must normally be preserved). This requires manually storing the
> state of those config bits in "chip_config". I could try to make do without
> regcache support, even though it offers some advantages.
If you were to break these registers up into regmap fields it might solve 
this..  Regmap writes always go through whatever - whether they match the
existing state of the cache or not.  If fields are involved the write will get
built up from whatever field you change and whatever the cache has for other
elements.  I guess it only works if they volatile bits are contiguous though.
Maybe hand rolling it is cleaner here.

Mark, any clever thoughts on this?
> 
>
> Support for external sensors is rather nasty. What it does is dynamically copy
> iio_chan_spec from the slave device and register it for itself (with an updated
> scan index). Then it forwards everything but IIO_CHAN_INFO_RAW to the other
> driver while serving raw value requests from the MPU's own registers.
> 
> The original device is still registered with linux as a normal driver and works
> normally and you can poke at it to configure stuff like sample rates and
> scaling factors. You can read the same samples from the slaved device, just
> slower. For example:
> 	cat /sys/bus/iio/devices/iio:device1/in_magn_*_raw
> will be slower than:
> 	cat /sys/bus/iio/devices/iio:device0/in_magn_*_raw
> In the first case values are read through SLV4, byte-by-byte. In the second
> they are served directly from EXT_SENS_DATA_* registers.
ouch
> 
> In theory the ak8975 inside mpu9150 could be supported as a slave device but
> that requires further work. Unlike the hmc5883l I've been testing with that
> sensor does not update automatically by default. This means that the mpu6050
> buffer code must call a function from that driver to "enable automatic updates"
> somehow. For example this could be implemented as an additional bufferop?
That's hideous, but sure I guess we could have such an op.
> 
> So far this works surprisingly well without any changes in the iio core or
> drivers for aux devices. But perhaps "external channels" should be listed in
> struct iio_dev and a special marking should be done for iio_chan_spec's which
> can accessed this way.
> 
> External/slaved channels are limited to 16bits because it would otherwise be
> difficult to comply with iio alignment requirements. Support for other channel
> sizes should be implemented separately. There should be a separate discussion
> for how to properly support driver-specified channel offsets instead of
> implicit iio rules.
hmm. You'd need to give more detail on what sort of alignments we are looking at.
It might just be a case of the driver having to do a bit of memcpy magic to 
'fix up' the alignment being read from the device. 
> 
> Patches 1,2,3,4 and 6 are required cleanups/fixed to make the rest work. They
> could go in separately.
> 
> Crestez Dan Leonard (7):
>   iio: inv_mpu6050: Do burst reads using spi/i2c directly
>   iio: inv_mpu6050: Initial regcache support
>   iio: inv_mpu6050: Only toggle DATA_RDY_EN in inv_reset_fifo
>   iio: inv_mpu6050: Cache non-volatile bits of user_ctrl
>   iio: inv_mpu6050: Add support for auxiliary I2C master
>   iio: inv_mpu6050: Check channel configuration on preenable
>   iio: inv_mpu6050: Add support for external sensors
> 
>  .../devicetree/bindings/iio/imu/inv_mpu6050.txt    |  96 ++-
>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c         | 708 ++++++++++++++++++++-
>  drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c          |   5 -
>  drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h          | 116 +++-
>  drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c         | 134 +++-
>  drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c          |   5 -
>  drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c      |  15 +-
>  7 files changed, 1029 insertions(+), 50 deletions(-)
> 

Re: [PATCH 1/7] iio: inv_mpu6050: Do burst reads using spi/i2c directly

[Jonathan Cameron]

 On 29/04/16 20:02, Crestez Dan Leonard wrote:
> Using regmap_read_bulk is wrong because it assumes that a range of
> registers is being read. In our case reading from the fifo register will
> return multiple values but this is *not* auto-increment.
> 
> This currently works by accident.
Cc'd Mark again.  He's the regmap maintainer (amongst other things) so
a series doing slightly odd things with regmap should probably have been
cc'd to him in the first place.

Perhaps regmap should have a repeat read function for this sort of fifo access?
Mark, is this something you'd consider?  Easy enough to implement after all as
a variant on regmap_read_bulk...

Having the below in a driver just feels wrong to me....
> 
> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 33 ++++++++++++++++++++++++++----
>  1 file changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> index d070062..8455af0 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> @@ -21,6 +21,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/kfifo.h>
>  #include <linux/poll.h>
> +#include <linux/spi/spi.h>
>  #include "inv_mpu_iio.h"
>  
>  static void inv_clear_kfifo(struct inv_mpu6050_state *st)
> @@ -128,6 +129,13 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
>  	u16 fifo_count;
>  	s64 timestamp;
>  
> +	struct device *regmap_dev = regmap_get_device(st->map);
> +	struct i2c_client *i2c;
> +	struct spi_device *spi = NULL;
> +
> +	i2c = i2c_verify_client(regmap_dev);
> +	spi = i2c ? NULL: to_spi_device(regmap_dev);
> +
>  	mutex_lock(&indio_dev->mlock);
>  	if (!(st->chip_config.accl_fifo_enable |
>  		st->chip_config.gyro_fifo_enable))
> @@ -160,10 +168,27 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
>  	    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);
> -		if (result)
> -			goto flush_fifo;
> +		/*
> +		 * We need to do a large burst read from a single register.
> +		 *
> +		 * regmap_read_bulk assumes that multiple registers are
> +		 * involved but in our case st->reg->fifo_r_w + 1 is something
> +		 * completely unrelated.
> +		 */
> +		if (spi) {
> +			u8 cmd = st->reg->fifo_r_w | 0x80;
> +			result = spi_write_then_read(spi,
> +					&cmd, 1,
> +					data, bytes_per_datum);
> +			if (result)
> +				goto flush_fifo;
> +		} else {
> +			result = i2c_smbus_read_i2c_block_data(i2c,
> +					st->reg->fifo_r_w,
> +					bytes_per_datum, data);
> +			if (result != bytes_per_datum)
> +				goto flush_fifo;
> +		}
>  
>  		result = kfifo_out(&st->timestamps, &timestamp, 1);
>  		/* when there is no timestamp, put timestamp as 0 */
> 

Re: [PATCH 2/7] iio: inv_mpu6050: Initial regcache support

[Jonathan Cameron]

On 29/04/16 20:02, Crestez Dan Leonard wrote:
> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
Looks fine to me.

Jonathan
> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 47 ++++++++++++++++++++++++++++++
>  drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c  |  5 ----
>  drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h  |  1 +
>  drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c  |  5 ----
>  4 files changed, 48 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> index b269b37..5918c23 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> @@ -116,6 +116,53 @@ static const struct inv_mpu6050_hw hw_info[] = {
>  	},
>  };
>  
> +static bool inv_mpu6050_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	if (reg >= INV_MPU6050_REG_RAW_ACCEL && reg < INV_MPU6050_REG_RAW_ACCEL + 6)
> +		return true;
> +	if (reg >= INV_MPU6050_REG_RAW_GYRO && reg < INV_MPU6050_REG_RAW_GYRO + 6)
> +		return true;
> +	switch (reg) {
> +	case INV_MPU6050_REG_TEMPERATURE:
> +	case INV_MPU6050_REG_TEMPERATURE + 1:
> +	case INV_MPU6050_REG_USER_CTRL:
> +	case INV_MPU6050_REG_PWR_MGMT_1:
> +	case INV_MPU6050_REG_FIFO_COUNT_H:
> +	case INV_MPU6050_REG_FIFO_COUNT_H + 1:
> +	case INV_MPU6050_REG_FIFO_R_W:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static bool inv_mpu6050_precious_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case INV_MPU6050_REG_FIFO_R_W:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +/*
> + * Common regmap config for inv_mpu devices
> + *
> + * The current volatile/precious registers are common among supported devices.
> + * When that changes the volatile/precious callbacks should go through the
> + * inv_mpu6050_reg_map structs.
> + */
> +const struct regmap_config inv_mpu_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +
> +	.cache_type = REGCACHE_RBTREE,
> +	.volatile_reg = inv_mpu6050_volatile_reg,
> +	.precious_reg = inv_mpu6050_precious_reg,
> +};
> +EXPORT_SYMBOL_GPL(inv_mpu_regmap_config);
> +
>  int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en, u32 mask)
>  {
>  	unsigned int d, mgmt_1;
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> index 1a424a6..1a8d1a5 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> @@ -20,11 +20,6 @@
>  #include <linux/module.h>
>  #include "inv_mpu_iio.h"
>  
> -static const struct regmap_config inv_mpu_regmap_config = {
> -	.reg_bits = 8,
> -	.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
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> index 47ca25b..297b0ef 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> @@ -291,3 +291,4 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
>  int inv_mpu_core_remove(struct device *dev);
>  int inv_mpu6050_set_power_itg(struct inv_mpu6050_state *st, bool power_on);
>  extern const struct dev_pm_ops inv_mpu_pmops;
> +extern const struct regmap_config inv_mpu_regmap_config;
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c
> index 190a4a5..b3bd977 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c
> @@ -17,11 +17,6 @@
>  #include <linux/iio/iio.h>
>  #include "inv_mpu_iio.h"
>  
> -static const struct regmap_config inv_mpu_regmap_config = {
> -	.reg_bits = 8,
> -	.val_bits = 8,
> -};
> -
>  static int inv_mpu_i2c_disable(struct iio_dev *indio_dev)
>  {
>  	struct inv_mpu6050_state *st = iio_priv(indio_dev);
> 

Re: [PATCH 3/7] iio: inv_mpu6050: Only toggle DATA_RDY_EN in inv_reset_fifo

[Jonathan Cameron]

On 29/04/16 20:02, Crestez Dan Leonard wrote:
> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
Looks fine to me...
> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c    | 13 +++++++-----
>  drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c |  3 ++-
>  2 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> index 8455af0..3fc0b71 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> @@ -41,7 +41,8 @@ int inv_reset_fifo(struct iio_dev *indio_dev)
>  	struct inv_mpu6050_state  *st = iio_priv(indio_dev);
>  
>  	/* disable interrupt */
> -	result = regmap_write(st->map, st->reg->int_enable, 0);
> +	result = regmap_update_bits(st->map, st->reg->int_enable,
> +				    INV_MPU6050_BIT_DATA_RDY_EN, 0);
>  	if (result) {
>  		dev_err(regmap_get_device(st->map), "int_enable failed %d\n",
>  			result);
> @@ -68,8 +69,9 @@ int inv_reset_fifo(struct iio_dev *indio_dev)
>  	/* enable interrupt */
>  	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);
> +		result = regmap_update_bits(st->map, st->reg->int_enable,
> +					    INV_MPU6050_BIT_DATA_RDY_EN,
> +					    INV_MPU6050_BIT_DATA_RDY_EN);
>  		if (result)
>  			return result;
>  	}
> @@ -92,8 +94,9 @@ 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);
> +	result = regmap_update_bits(st->map, st->reg->int_enable,
> +				    INV_MPU6050_BIT_DATA_RDY_EN,
> +				    INV_MPU6050_BIT_DATA_RDY_EN);
>  
>  	return result;
>  }
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> index e8818d4..1a6bad3 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> @@ -69,7 +69,8 @@ static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
>  		if (result)
>  			return result;
>  
> -		result = regmap_write(st->map, st->reg->int_enable, 0);
> +		result = regmap_update_bits(st->map, st->reg->int_enable,
> +					INV_MPU6050_BIT_DATA_RDY_EN, 0);
>  		if (result)
>  			return result;
>  
> 

Re: [PATCH 4/7] iio: inv_mpu6050: Cache non-volatile bits of user_ctrl

[Jonathan Cameron]

On 29/04/16 20:02, Crestez Dan Leonard wrote:
> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
Again, fine but irritating that you have to do this!

Jonathan
> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h     | 2 ++
>  drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c    | 9 ++++++---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c | 4 +++-
>  3 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> index 297b0ef..bd2c0fd 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> @@ -80,6 +80,7 @@ enum inv_devices {
>   *  @enable:		master enable state.
>   *  @accl_fifo_enable:	enable accel data output
>   *  @gyro_fifo_enable:	enable gyro data output
> + *  @user_ctrl:		The non-volatile bits of user_ctrl
>   *  @fifo_rate:		FIFO update rate.
>   */
>  struct inv_mpu6050_chip_config {
> @@ -89,6 +90,7 @@ struct inv_mpu6050_chip_config {
>  	unsigned int enable:1;
>  	unsigned int accl_fifo_enable:1;
>  	unsigned int gyro_fifo_enable:1;
> +	u8 user_ctrl;
>  	u16 fifo_rate;
>  };
>  
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> index 3fc0b71..56ee1e2 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> @@ -53,13 +53,15 @@ int inv_reset_fifo(struct iio_dev *indio_dev)
>  	if (result)
>  		goto reset_fifo_fail;
>  	/* disable fifo reading */
> -	result = regmap_write(st->map, st->reg->user_ctrl, 0);
> +	st->chip_config.user_ctrl &= ~INV_MPU6050_BIT_FIFO_EN;
> +	result = regmap_write(st->map, st->reg->user_ctrl,
> +			      st->chip_config.user_ctrl);
>  	if (result)
>  		goto reset_fifo_fail;
>  
>  	/* reset FIFO*/
>  	result = regmap_write(st->map, st->reg->user_ctrl,
> -			      INV_MPU6050_BIT_FIFO_RST);
> +			      st->chip_config.user_ctrl | INV_MPU6050_BIT_FIFO_RST);
>  	if (result)
>  		goto reset_fifo_fail;
>  
> @@ -76,8 +78,9 @@ int inv_reset_fifo(struct iio_dev *indio_dev)
>  			return result;
>  	}
>  	/* enable FIFO reading and I2C master interface*/
> +	st->chip_config.user_ctrl |= INV_MPU6050_BIT_FIFO_EN;
>  	result = regmap_write(st->map, st->reg->user_ctrl,
> -			      INV_MPU6050_BIT_FIFO_EN);
> +			      st->chip_config.user_ctrl);
>  	if (result)
>  		goto reset_fifo_fail;
>  	/* enable sensor output to FIFO */
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> index 1a6bad3..fc55923 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> @@ -74,7 +74,9 @@ static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable)
>  		if (result)
>  			return result;
>  
> -		result = regmap_write(st->map, st->reg->user_ctrl, 0);
> +		st->chip_config.user_ctrl &= ~INV_MPU6050_BIT_FIFO_EN;
> +		result = regmap_write(st->map, st->reg->user_ctrl,
> +				      st->chip_config.user_ctrl);
>  		if (result)
>  			return result;
>  
> 

Re: [RFC 5/7] iio: inv_mpu6050: Add support for auxiliary I2C master

[Jonathan Cameron]

On 29/04/16 20:02, Crestez Dan Leonard wrote:
> The MPU has an auxiliary I2C bus for connecting external
> sensors. This bus has two operating modes:
> * pass-through, which connects the primary and auxiliary busses
> together. This is already supported via an i2c mux.
> * I2C master mode, where the mpu60x0 acts as a master to any external
> connected sensors. This is implemented by this patch.
> 
> This I2C master mode also works when the MPU itself is connected via
> SPI.
> 
> I2C master supports up to 5 slaves. Slaves 0-3 have a common operating
> mode while slave 4 is different. This patch implements an i2c adapter
> using slave 4 because it has a cleaner interface and it has an
> interrupt that signals when data from slave to master arrived.
> 
> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
Quite a few missing cc's here.  Device tree bindings => device tree list
and maintainers.  Master i2c device => wolfram + linux-i2c.
All these people need to be included in this sort of discussion.

The quirk used to avoid unecessary writes to one of the registers bothers
me in that it's not obviously correct.  Perhaps using a read / write pair
would be nicer?
> ---
>  .../devicetree/bindings/iio/imu/inv_mpu6050.txt    |  61 +++++-
>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c         | 239 ++++++++++++++++++++-
>  drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h          |  46 ++++
>  drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c      |   8 -
>  4 files changed, 341 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt b/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt
> index a9fc11e..aaf12b4 100644
> --- a/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt
> +++ b/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt
> @@ -1,16 +1,27 @@
>  InvenSense MPU-6050 Six-Axis (Gyro + Accelerometer) MEMS MotionTracking Device
>  
> -http://www.invensense.com/mems/gyro/mpu6050.html
If this is invalid, please add an up to date link if possible.
> -
>  Required properties:
> - - compatible : should be "invensense,mpu6050"
> - - reg : the I2C address of the sensor
> + - compatible : should be "invensense,mpuXXXX"
List them all explicitly here rather than wild cards.
> + - reg : the I2C or SPI address of the sensor
>   - interrupt-parent : should be the phandle for the interrupt controller
>   - interrupts : interrupt mapping for GPIO IRQ
>  
>  Optional properties:
>   - mount-matrix: an optional 3x3 mounting rotation matrix
> + - inv,i2c-aux-master: operate aux i2c in "master mode" (default is mux).
> +
> +Valid compatible strings:
Vendor prefix? These will work for historical reasons, but now vendor
prefix should definitely be there as well.
> + - mpu6000
> + - mpu6050
> + - mpu6500
> + - mpu9150
> +
> +It is possible to attach auxiliary sensors to the MPU and have them be handled
> +by linux. Those auxiliary sensors are described as an i2c bus.
> +
> +Devices connected in "bypass" mode must be listed behind i2c@0 with the address 0
>  
> +Devices connected in "master" mode must be listed behind i2c@1 with the address 1
>  
>  Example:
>  	mpu6050@68 {
> @@ -28,3 +39,45 @@ Example:
>  		               "0",                   /* y2 */
>  		               "0.984807753012208";   /* z2 */
>  	};
> +
> +Example describing mpu9150 (which includes an ak9875 on chip):
> +	mpu9150@68 {
> +		compatible = "invensense,mpu9150";
> +		reg = <0x68>;
> +		interrupt-parent = <&gpio1>;
> +		interrupts = <18 1>;
> +
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		i2c@0 {
> +			reg = <0>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			ak8975@0c {
> +				compatible = "ak,ak8975";
> +				reg = <0x0c>;
> +			};
> +		};
> +	};
> +
> +Example describing a mpu6500 via SPI with an hmc5883l on auxiliary i2c:
> +	mpu6500@0 {
> +		compatible = "inv,mpu6500";
> +		reg = <0x0>;
> +		spi-max-frequency = <1000000>;
> +		interrupt-parent = <&gpio1>;
> +		interrupts = <31 1>;
> +
> +		inv,i2c-aux-master
> +		i2c@1 {
> +			reg = <1>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			hmc5883l@1e {
> +				compatible = "honeywell,hmc5883l";
> +				reg = <0x1e>;
> +			};
> +		};
> +	};
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> index 5918c23..064fc07 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> @@ -25,6 +25,8 @@
>  #include <linux/iio/iio.h>
>  #include <linux/i2c-mux.h>
>  #include <linux/acpi.h>
> +#include <linux/completion.h>
> +
>  #include "inv_mpu_iio.h"
>  
>  /*
> @@ -57,6 +59,12 @@ static const struct inv_mpu6050_reg_map reg_set_6500 = {
>  	.int_pin_cfg		= INV_MPU6050_REG_INT_PIN_CFG,
>  	.accl_offset		= INV_MPU6500_REG_ACCEL_OFFSET,
>  	.gyro_offset		= INV_MPU6050_REG_GYRO_OFFSET,
> +	.slv4_addr		= INV_MPU6050_REG_I2C_SLV4_ADDR,
> +	.slv4_reg		= INV_MPU6050_REG_I2C_SLV4_REG,
> +	.slv4_do		= INV_MPU6050_REG_I2C_SLV4_DO,
> +	.slv4_ctrl		= INV_MPU6050_REG_I2C_SLV4_CTRL,
> +	.slv4_di		= INV_MPU6050_REG_I2C_SLV4_DI,
> +	.mst_status		= INV_MPU6050_REG_I2C_MST_STATUS,
>  };
>  
>  static const struct inv_mpu6050_reg_map reg_set_6050 = {
> @@ -77,6 +85,12 @@ static const struct inv_mpu6050_reg_map reg_set_6050 = {
>  	.int_pin_cfg		= INV_MPU6050_REG_INT_PIN_CFG,
>  	.accl_offset		= INV_MPU6050_REG_ACCEL_OFFSET,
>  	.gyro_offset		= INV_MPU6050_REG_GYRO_OFFSET,
> +	.slv4_addr		= INV_MPU6050_REG_I2C_SLV4_ADDR,
> +	.slv4_reg		= INV_MPU6050_REG_I2C_SLV4_REG,
> +	.slv4_do		= INV_MPU6050_REG_I2C_SLV4_DO,
> +	.slv4_ctrl		= INV_MPU6050_REG_I2C_SLV4_CTRL,
> +	.slv4_di		= INV_MPU6050_REG_I2C_SLV4_DI,
> +	.mst_status		= INV_MPU6050_REG_I2C_MST_STATUS,
>  };
>  
>  static const struct inv_mpu6050_chip_config chip_config_6050 = {
> @@ -130,6 +144,10 @@ static bool inv_mpu6050_volatile_reg(struct device *dev, unsigned int reg)
>  	case INV_MPU6050_REG_FIFO_COUNT_H:
>  	case INV_MPU6050_REG_FIFO_COUNT_H + 1:
>  	case INV_MPU6050_REG_FIFO_R_W:
> +	case INV_MPU6050_REG_I2C_MST_STATUS:
> +	case INV_MPU6050_REG_INT_STATUS:
> +	case INV_MPU6050_REG_I2C_SLV4_CTRL:
> +	case INV_MPU6050_REG_I2C_SLV4_DI:
>  		return true;
>  	default:
>  		return false;
> @@ -140,6 +158,8 @@ static bool inv_mpu6050_precious_reg(struct device *dev, unsigned int reg)
>  {
>  	switch (reg) {
>  	case INV_MPU6050_REG_FIFO_R_W:
> +	case INV_MPU6050_REG_I2C_MST_STATUS:
> +	case INV_MPU6050_REG_INT_STATUS:
>  		return true;
>  	default:
>  		return false;
> @@ -852,6 +872,196 @@ static int inv_check_and_setup_chip(struct inv_mpu6050_state *st)
>  	return 0;
>  }
>  
> +static irqreturn_t inv_mpu_irq_handler(int irq, void *private)
> +{
> +	struct inv_mpu6050_state *st = (struct inv_mpu6050_state *)private;
> +
> +	iio_trigger_poll(st->trig);
> +
> +	return IRQ_WAKE_THREAD;
> +}
> +
> +static irqreturn_t inv_mpu_irq_thread_handler(int irq, void *private)
> +{
> +	struct inv_mpu6050_state *st = (struct inv_mpu6050_state *)private;
> +	int ret, val;
> +
> +	ret = regmap_read(st->map, st->reg->mst_status, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +#ifdef CONFIG_I2C
> +	if (val & INV_MPU6050_BIT_I2C_SLV4_DONE)
> +		complete(&st->slv4_done);
> +#endif
> +
> +	return IRQ_HANDLED;
> +}
> +
> +#ifdef CONFIG_I2C
> +static u32 inv_mpu_i2c_functionality(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_SMBUS_BYTE_DATA;
> +}
> +
> +static int
> +inv_mpu_i2c_smbus_xfer(struct i2c_adapter *adap, u16 addr,
> +		       unsigned short flags, char read_write, u8 command,
> +		       int size, union i2c_smbus_data *data)
> +{
> +	struct inv_mpu6050_state *st = i2c_get_adapdata(adap);
> +	struct iio_dev *indio_dev = iio_priv_to_dev(st);
> +
> +	unsigned long time_left;
> +	int result, val;
> +	u8 ctrl;
> +
> +	/*
> +	 * The i2c adapter we implement is extremely limited.
> +	 * Check for callers that don't check functionality bits.
> +	 *
> +	 * If we don't check we might succesfully return incorrect data.
> +	 */
> +	if (size != I2C_SMBUS_BYTE_DATA) {
> +		dev_err(&adap->dev, "unsupported xfer rw=%d size=%d\n",
> +			read_write, size);
> +		dump_stack();
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&indio_dev->mlock);
> +	result = inv_mpu6050_set_power_itg(st, true);
> +	mutex_unlock(&indio_dev->mlock);
> +	if (result < 0)
> +		return result;
> +
> +	if (read_write == I2C_SMBUS_WRITE)
> +		addr |= INV_MPU6050_BIT_I2C_SLV4_W;
> +	else
> +		addr |= INV_MPU6050_BIT_I2C_SLV4_R;
> +
> +	/*
> +	 * Regmap will never ignore writes but it will ignore full-register
> +	 * updates to the same value.
Hmm. I'd missed this distinction.  Feels decidely 'interesting'... and makes
my earlier suggestion invalid as I guess the fields stuff uses update bits
internally.

> +	 *
> +	 * This avoids having to touch slv4_addr when doing consecutive
> +	 * reads/writes with the same device.
> +	 */
> +	result = regmap_update_bits(st->map, st->reg->slv4_addr, 0xff, addr);
> +	if (result < 0)
> +		return result;
> +
> +	result = regmap_write(st->map, st->reg->slv4_reg, command);
> +	if (result < 0)
> +		return result;
> +
> +	if (read_write == I2C_SMBUS_WRITE) {
> +		result = regmap_write(st->map, st->reg->slv4_do, data->byte);
> +		if (result < 0)
> +			return result;
> +	}
> +
> +	ctrl = INV_MPU6050_BIT_SLV4_EN | INV_MPU6050_BIT_SLV4_INT_EN;
> +	result = regmap_write(st->map, st->reg->slv4_ctrl, ctrl);
> +	if (result < 0)
> +		return result;
> +
> +	/* Wait for completion for both reads and writes */
> +	time_left = wait_for_completion_timeout(&st->slv4_done, HZ);
> +	if (!time_left)
> +		return -ETIMEDOUT;
> +
> +	if (read_write == I2C_SMBUS_READ) {
> +		result = regmap_read(st->map, st->reg->slv4_di, &val);
> +		if (result < 0)
> +			return result;
> +		data->byte = val;
> +	}
> +
> +	mutex_lock(&indio_dev->mlock);
> +	result = inv_mpu6050_set_power_itg(st, false);
> +	mutex_unlock(&indio_dev->mlock);
> +	if (result < 0)
> +		return result;
> +	return 0;
> +}
> +
> +static const struct i2c_algorithm inv_mpu_i2c_algo = {
> +	.smbus_xfer	=	inv_mpu_i2c_smbus_xfer,
> +	.functionality	=	inv_mpu_i2c_functionality,
> +};
> +
> +static struct device_node *inv_mpu_get_aux_i2c_ofnode(struct device_node *parent)
> +{
> +	struct device_node *child;
> +	int result;
> +	u32 reg;
> +
> +	if (!parent)
> +		return NULL;
> +	for_each_child_of_node(parent, child) {
> +		result = of_property_read_u32(child, "reg", &reg);
> +		if (result)
> +			continue;
> +		if (reg == 1 && !strcmp(child->name, "i2c"))
> +			return child;
> +	}
> +
> +	return NULL;
> +}
> +
> +static int inv_mpu_probe_aux_master(struct iio_dev* indio_dev)
> +{
> +	struct inv_mpu6050_state *st = iio_priv(indio_dev);
> +	struct device *dev = regmap_get_device(st->map);
> +	int result;
> +
> +	/* First check i2c-aux-master property */
> +	st->i2c_aux_master_mode = of_property_read_bool(dev->of_node,
> +							"inv,i2c-aux-master");
> +	if (!st->i2c_aux_master_mode)
> +		return 0;
> +	dev_info(dev, "Configuring aux i2c in master mode\n");
> +
> +	result = inv_mpu6050_set_power_itg(st, true);
> +	if (result < 0)
> +		return result;
> +
> +	/* enable master */
> +	st->chip_config.user_ctrl |= INV_MPU6050_BIT_I2C_MST_EN;
> +	result = regmap_write(st->map, st->reg->user_ctrl, st->chip_config.user_ctrl);
> +	if (result < 0)
> +		return result;
> +
> +	result = regmap_update_bits(st->map, st->reg->int_enable,
> +				 INV_MPU6050_BIT_MST_INT_EN,
> +				 INV_MPU6050_BIT_MST_INT_EN);
> +	if (result < 0)
> +		return result;
> +
> +	result = inv_mpu6050_set_power_itg(st, false);
> +	if (result < 0)
> +		return result;
> +
> +	init_completion(&st->slv4_done);
> +	st->aux_master_adapter.owner = THIS_MODULE;
> +	st->aux_master_adapter.algo = &inv_mpu_i2c_algo;
> +	st->aux_master_adapter.dev.parent = dev;
> +	snprintf(st->aux_master_adapter.name, sizeof(st->aux_master_adapter.name),
> +			"aux-master-%s", indio_dev->name);
> +	st->aux_master_adapter.dev.of_node = inv_mpu_get_aux_i2c_ofnode(dev->of_node);
> +	i2c_set_adapdata(&st->aux_master_adapter, st);
> +	/* This will also probe aux devices so transfers must work now */
> +	result = i2c_add_adapter(&st->aux_master_adapter);
> +	if (result < 0) {
> +		dev_err(dev, "i2x aux master register fail %d\n", result);
> +		return result;
> +	}
> +
> +	return 0;
> +}
> +#endif
> +
>  int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
>  		int (*inv_mpu_bus_setup)(struct iio_dev *), int chip_type)
>  {
> @@ -931,16 +1141,39 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
>  		goto out_unreg_ring;
>  	}
>  
> +	/* Request interrupt for trigger and i2c master adapter */
> +	result = devm_request_threaded_irq(&indio_dev->dev, st->irq,
> +					   &inv_mpu_irq_handler,
> +					   &inv_mpu_irq_thread_handler,
> +					   IRQF_TRIGGER_RISING, "inv_mpu",
> +					   st);
> +	if (result) {
> +		dev_err(dev, "request irq fail %d\n", result);
> +		goto out_remove_trigger;
> +	}
> +
> +#ifdef CONFIG_I2C
> +	result = inv_mpu_probe_aux_master(indio_dev);
> +	if (result < 0) {
> +		dev_err(dev, "i2c aux master probe fail %d\n", result);
> +		goto out_remove_trigger;
> +	}
> +#endif
> +
>  	INIT_KFIFO(st->timestamps);
>  	spin_lock_init(&st->time_stamp_lock);
>  	result = iio_device_register(indio_dev);
>  	if (result) {
>  		dev_err(dev, "IIO register fail %d\n", result);
> -		goto out_remove_trigger;
> +		goto out_del_adapter;
>  	}
>  
>  	return 0;
>  
> +out_del_adapter:
> +#ifdef CONFIG_I2C
> +	i2c_del_adapter(&st->aux_master_adapter);
> +#endif
>  out_remove_trigger:
>  	inv_mpu6050_remove_trigger(st);
>  out_unreg_ring:
> @@ -952,10 +1185,14 @@ EXPORT_SYMBOL_GPL(inv_mpu_core_probe);
>  int inv_mpu_core_remove(struct device  *dev)
>  {
>  	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct inv_mpu6050_state *st = iio_priv(indio_dev);
>  
>  	iio_device_unregister(indio_dev);
>  	inv_mpu6050_remove_trigger(iio_priv(indio_dev));
>  	iio_triggered_buffer_cleanup(indio_dev);
> +#ifdef CONFIG_I2C
> +	i2c_del_adapter(&st->aux_master_adapter);
> +#endif
>  
>  	return 0;
>  }
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> index bd2c0fd..9d15633 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> @@ -42,6 +42,13 @@
>   *  @int_pin_cfg;	Controls interrupt pin configuration.
>   *  @accl_offset:	Controls the accelerometer calibration offset.
>   *  @gyro_offset:	Controls the gyroscope calibration offset.
> + *  @mst_status:	secondary I2C master interrupt source status
> + *  @slv4_addr:		I2C slave address for slave 4 transaction
> + *  @slv4_reg:		I2C register used with slave 4 transaction
> + *  @slv4_di:		I2C data in register for slave 4 transaction
> + *  @slv4_ctrl:		I2C slave 4 control register
> + *  @slv4_do:		I2C data out register for slave 4 transaction
> +
>   */
>  struct inv_mpu6050_reg_map {
>  	u8 sample_rate_div;
> @@ -61,6 +68,15 @@ struct inv_mpu6050_reg_map {
>  	u8 int_pin_cfg;
>  	u8 accl_offset;
>  	u8 gyro_offset;
> +	u8 mst_status;
> +
> +	/* slave 4 registers */
> +	u8 slv4_addr;
> +	u8 slv4_reg;
> +	u8 slv4_di; /* data in */
> +	u8 slv4_ctrl;
> +	u8 slv4_do; /* data out */
> +
>  };
>  
>  /*device enum */
> @@ -139,6 +155,15 @@ struct inv_mpu6050_state {
>  	DECLARE_KFIFO(timestamps, long long, TIMESTAMP_FIFO_SIZE);
>  	struct regmap *map;
>  	int irq;
> +
> +	/* if the MPU connects to aux devices as a master */
> +	bool i2c_aux_master_mode;
> +
> +#ifdef CONFIG_I2C
> +	/* I2C adapter for talking to aux sensors in "master" mode */
> +	struct i2c_adapter aux_master_adapter;
> +	struct completion slv4_done;
> +#endif
>  };
>  
>  /*register and associated bit definition*/
> @@ -154,9 +179,30 @@ struct inv_mpu6050_state {
>  #define INV_MPU6050_BIT_ACCEL_OUT           0x08
>  #define INV_MPU6050_BITS_GYRO_OUT           0x70
>  
> +#define INV_MPU6050_REG_I2C_SLV4_ADDR       0x31
> +#define INV_MPU6050_BIT_I2C_SLV4_R          0x80
> +#define INV_MPU6050_BIT_I2C_SLV4_W          0x00
> +
> +#define INV_MPU6050_REG_I2C_SLV4_REG        0x32
> +#define INV_MPU6050_REG_I2C_SLV4_DO         0x33
> +#define INV_MPU6050_REG_I2C_SLV4_CTRL       0x34
> +
> +#define INV_MPU6050_BIT_SLV4_EN             0x80
> +#define INV_MPU6050_BIT_SLV4_INT_EN         0x40
> +#define INV_MPU6050_BIT_SLV4_DIS            0x20
> +
> +#define INV_MPU6050_REG_I2C_SLV4_DI         0x35
> +
> +#define INV_MPU6050_REG_I2C_MST_STATUS      0x36
> +#define INV_MPU6050_BIT_I2C_SLV4_DONE       0x40
> +
>  #define INV_MPU6050_REG_INT_ENABLE          0x38
>  #define INV_MPU6050_BIT_DATA_RDY_EN         0x01
>  #define INV_MPU6050_BIT_DMP_INT_EN          0x02
> +#define INV_MPU6050_BIT_MST_INT_EN          0x08
> +
> +#define INV_MPU6050_REG_INT_STATUS          0x3A
> +#define INV_MPU6050_BIT_MST_INT             0x08
>  
>  #define INV_MPU6050_REG_RAW_ACCEL           0x3B
>  #define INV_MPU6050_REG_TEMPERATURE         0x41
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> index fc55923..a334ed9 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> @@ -126,14 +126,6 @@ int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev)
>  	if (!st->trig)
>  		return -ENOMEM;
>  
> -	ret = devm_request_irq(&indio_dev->dev, st->irq,
> -			       &iio_trigger_generic_data_rdy_poll,
> -			       IRQF_TRIGGER_RISING,
> -			       "inv_mpu",
> -			       st->trig);
> -	if (ret)
> -		return ret;
> -
>  	st->trig->dev.parent = regmap_get_device(st->map);
>  	st->trig->ops = &inv_mpu_trigger_ops;
>  	iio_trigger_set_drvdata(st->trig, indio_dev);
> 

Re: [PATCH 6/7] iio: inv_mpu6050: Check channel configuration on preenable

[Jonathan Cameron]

On 29/04/16 20:02, Crestez Dan Leonard wrote:
> Right now it is possible to only enable some of the x/y/z channels, for
> example you can enable accel_z without x or y. If you actually do that
> what you get is actually only the x channel.
> 
> In theory the device supports selecting gyro x/y/z channels
> individually. It would also be possible to selectively enable x/y/z
> accel by unpacking the data read from the hardware into a format the iio
> core accepts.
> 
> It is easier to simply refuse incorrect configuration.
Or see suggestion inline. This isn't an uncommon problem!
> 
> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c |  2 +-
>  drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h  |  4 ++++
>  drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 31 ++++++++++++++++++++++++++++++
>  3 files changed, 36 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 064fc07..712e901 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> @@ -1130,7 +1130,7 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
>  	result = iio_triggered_buffer_setup(indio_dev,
>  					    inv_mpu6050_irq_handler,
>  					    inv_mpu6050_read_fifo,
> -					    NULL);
> +					    &inv_mpu_buffer_ops);
>  	if (result) {
>  		dev_err(dev, "configure buffer fail %d\n", result);
>  		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 9d15633..9d406df 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> @@ -284,6 +284,9 @@ enum inv_mpu6050_scan {
>  	INV_MPU6050_SCAN_TIMESTAMP,
>  };
>  
> +#define INV_MPU6050_SCAN_MASK_ACCEL	0x07
> +#define INV_MPU6050_SCAN_MASK_GYRO	0x38
> +
>  enum inv_mpu6050_filter_e {
>  	INV_MPU6050_FILTER_256HZ_NOLPF2 = 0,
>  	INV_MPU6050_FILTER_188HZ,
> @@ -340,3 +343,4 @@ int inv_mpu_core_remove(struct device *dev);
>  int inv_mpu6050_set_power_itg(struct inv_mpu6050_state *st, bool power_on);
>  extern const struct dev_pm_ops inv_mpu_pmops;
>  extern const struct regmap_config inv_mpu_regmap_config;
> +extern const struct iio_buffer_setup_ops inv_mpu_buffer_ops;
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> index 56ee1e2..e8bda7f 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> @@ -222,3 +222,34 @@ flush_fifo:
>  
>  	return IRQ_HANDLED;
>  }
> +
> +/* Validate channels are set in a correct configuration */
> +static int inv_mpu_buffer_preenable(struct iio_dev *indio_dev)
> +{
This should not be in the preenable.  It's perfectly possible to know that mode was invalid
earlier than this.  Use the core demux to handle this case by providing
available_scanmasks.  The the core will handle demuxing the data stream if needed to
provide the channels enabled only in the kfifo.

Not sure how we failed to pick up on this one before!  Kind of an impressively major bug
to have hiding in there.  Ah well - I guess most users always want everything!

Jonathan

> +	struct inv_mpu6050_state *st = iio_priv(indio_dev);
> +	unsigned long mask = *indio_dev->active_scan_mask;
> +
> +	if ((mask & INV_MPU6050_SCAN_MASK_GYRO) &&
> +	    (mask & INV_MPU6050_SCAN_MASK_GYRO) != INV_MPU6050_SCAN_MASK_GYRO)
> +	{
> +		dev_warn(regmap_get_device(st->map),
> +			 "Gyro channels can only be enabled together\n");
> +		return -EINVAL;
> +	}
> +
> +	if ((mask & INV_MPU6050_SCAN_MASK_ACCEL) &&
> +	    (mask & INV_MPU6050_SCAN_MASK_ACCEL) != INV_MPU6050_SCAN_MASK_ACCEL)
> +	{
> +		dev_warn(regmap_get_device(st->map),
> +			 "Accel channels can only be enabled together\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +const struct iio_buffer_setup_ops inv_mpu_buffer_ops = {
> +	.preenable = inv_mpu_buffer_preenable,
> +	.postenable = iio_triggered_buffer_postenable,
> +	.predisable = iio_triggered_buffer_predisable,
> +};
> 

Re: [RFC 7/7] iio: inv_mpu6050: Add support for external sensors

[Jonathan Cameron]

On 29/04/16 20:02, Crestez Dan Leonard wrote:
> This works by copying their channels at probe time.
> 
> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
This is some mixture of deeply nefarious and rather cool.
Great bit of work all in all.

Cc'd device tree list and maintainers - to them - this is downright mad
hardware - basically a partial i2c offload engine sitting on either i2c or
spi buses with lots of iritating restrictions but useful stuff like a fifo
between it and the rest of the world - upshot it's much much faster than
actually addressing these devices directly... 

First time I read the datasheet for this part - I thought no one would ever
figure out / go to the effort of supporting this functionality.  Turns
out I was wrong!

It is the only part I know of allowing this level of flexibility as a sensor
hub.  Most sensor hubs hide entirely what is going on behind them whereas this
one exposes what is going on...

Also cc'd the i2c list / Peter, Wolfram and Mark (just because they might be
interested!)

Jonathan
> ---
>  .../devicetree/bindings/iio/imu/inv_mpu6050.txt    |  37 +-
>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c         | 420 ++++++++++++++++++++-
>  drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h          |  63 +++-
>  drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c         |  48 ++-
>  4 files changed, 555 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt b/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt
> index aaf12b4..79b8326 100644
> --- a/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt
> +++ b/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt
> @@ -23,6 +23,28 @@ Devices connected in "bypass" mode must be listed behind i2c@0 with the address
>  
>  Devices connected in "master" mode must be listed behind i2c@1 with the address 1
>  
> +It is possible to configure the MPU to automatically fetch reading for
> +devices connected on the auxiliary i2c bus without CPU intervention. When this
> +is done the driver will present the channels of the slaved devices as if they
> +belong to the MPU device itself.
> +
> +Reads and write to config values (like scaling factors) are forwarded to the
> +other driver while data reads are handled from MPU registers. The channels are
> +also available through the MPU's iio triggered buffer mechanism. This feature
> +can allow sampling up to 24 bytes from 4 slaves at a much higher rate.
> +
> +This is be specified in device tree as an "inv,external-sensors" node which
> +have children indexed by slave ids 0 to 3. The child node identifying each
> +external sensor reading has the following properties:
> + - reg: 0 to 3 slave index
> + - inv,addr : the I2C address to read from
> + - inv,reg : the I2C register to read from
> + - inv,len : read length from the device
> + - inv,external-channels : list of slaved channels specified by config index.
I wonder if we want to try and make these generic - probably not until we have
at least one more device doing something similar...
> +
> +The sum of storagebits for external channels must equal the read length. Only
> +16bit channels are currently supported.
> +
>  Example:
>  	mpu6050@68 {
>  		compatible = "invensense,mpu6050";
> @@ -61,7 +83,8 @@ Example describing mpu9150 (which includes an ak9875 on chip):
>  		};
>  	};
>  
> -Example describing a mpu6500 via SPI with an hmc5883l on auxiliary i2c:
> +Example describing a mpu6500 via SPI with an hmc5883l on aux i2c configured for
> +automatic external readings as slave 0:
>  	mpu6500@0 {
>  		compatible = "inv,mpu6500";
>  		reg = <0x0>;
> @@ -80,4 +103,16 @@ Example describing a mpu6500 via SPI with an hmc5883l on auxiliary i2c:
>  				reg = <0x1e>;
>  			};
>  		};
> +
> +		inv,external-sensors {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			hmc5833l@0 {
> +				reg = <0>;
> +				inv,addr = <0x1e>;
> +				inv,reg = <3>;
> +				inv,len = <6>;
> +				inv,external-channels = <0 1 2>;
> +			};
> +		};
>  	};
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> index 712e901..224be3c 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> @@ -41,6 +41,8 @@ static const int gyro_scale_6050[] = {133090, 266181, 532362, 1064724};
>   */
>  static const int accel_scale[] = {598, 1196, 2392, 4785};
>  
> +#define INV_MPU6050_NUM_INT_CHAN 8
> +
>  static const struct inv_mpu6050_reg_map reg_set_6500 = {
>  	.sample_rate_div	= INV_MPU6050_REG_SAMPLE_RATE_DIV,
>  	.lpf                    = INV_MPU6050_REG_CONFIG,
> @@ -136,6 +138,9 @@ static bool inv_mpu6050_volatile_reg(struct device *dev, unsigned int reg)
>  		return true;
>  	if (reg >= INV_MPU6050_REG_RAW_GYRO && reg < INV_MPU6050_REG_RAW_GYRO + 6)
>  		return true;
> +	if (reg < INV_MPU6050_REG_EXT_SENS_DATA_00 + INV_MPU6050_CNT_EXT_SENS_DATA &&
> +			reg >= INV_MPU6050_REG_EXT_SENS_DATA_00)
> +		return true;
>  	switch (reg) {
>  	case INV_MPU6050_REG_TEMPERATURE:
>  	case INV_MPU6050_REG_TEMPERATURE + 1:
> @@ -340,13 +345,115 @@ static int inv_mpu6050_sensor_show(struct inv_mpu6050_state  *st, int reg,
>  	return IIO_VAL_INT;
>  }
>  
> +static bool iio_chan_needs_swab(const struct iio_chan_spec *chan)
> +{
> +#if defined(__LITTLE_ENDIAN)
> +	return chan->scan_type.endianness == IIO_BE;
> +#elif defined(__BIG_ENDIAN)
> +	return chan->scan_type.endianness == IIO_LE;
> +#else
> +	#error undefined endianness
> +#endif
> +}
> +
> +/* Convert iio buffer format to IIO_CHAN_INFO_RAW value */
> +static int iio_bufval_to_rawval(const struct iio_chan_spec *chan, void *buf, int *val)
> +{
> +	switch (chan->scan_type.storagebits) {
> +	case 8: *val = *((u8*)buf); break;
> +	case 16: *val = *(u16*)buf; break;
> +	case 32: *val = *(u32*)buf; break;
> +	default: return -EINVAL;
> +	}
> +
> +	*val >>= chan->scan_type.shift;
> +	*val &= (1 << chan->scan_type.realbits) - 1;
> +	if (iio_chan_needs_swab(chan)) {
> +		if (chan->scan_type.storagebits == 16)
> +			*val = swab16(*val);
> +		else if (chan->scan_type.storagebits == 32)
> +			*val = swab32(*val);
This needs some explanatory comments!
> +	}
> +	if (chan->scan_type.sign == 's')
> +		*val = sign_extend32(*val, chan->scan_type.storagebits - 1);
> +
> +	return 0;
> +}
> +
> +static int
> +inv_mpu6050_read_raw_external(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      int *val, int *val2, long mask)
> +{
> +	int result;
> +	struct inv_mpu6050_state *st = iio_priv(indio_dev);
> +	int chan_index = chan - indio_dev->channels;
> +	struct inv_mpu6050_ext_chan_state *ext_chan_state =
> +			&st->ext_chan[chan_index - INV_MPU6050_NUM_INT_CHAN];
> +	struct inv_mpu6050_ext_sens_state *ext_sens_state =
> +			&st->ext_sens[ext_chan_state->ext_sens_index];
> +	struct iio_dev *orig_dev = ext_sens_state->orig_dev;
> +	const struct iio_chan_spec *orig_chan = &orig_dev->channels[ext_chan_state->orig_chan_index];
> +	int data_offset;
> +	int data_length;
> +	u8 buf[4];
> +
> +	/* Everything but raw data reads is forwarded. */
> +	if (mask != IIO_CHAN_INFO_RAW)
> +		return orig_dev->info->read_raw(orig_dev, orig_chan, val, val2, mask);
Obviously this is useful for testing, but on a polled interface do we actually
care that it is quicker to go directly?  I'd be tempted to not bother except
for the buffered mode.
> +
> +	/* Raw reads are handled directly. */
> +	data_offset = INV_MPU6050_REG_EXT_SENS_DATA_00 + ext_chan_state->data_offset;
> +	data_length = chan->scan_type.storagebits / 8;
> +	if (data_length > sizeof(buf)) {
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&indio_dev->mlock);
> +	if (!st->chip_config.enable) {
> +		result = inv_mpu6050_set_power_itg(st, true);
> +		if (result)
> +			goto error_unlock;
> +	}
> +	result = regmap_bulk_read(st->map, data_offset, buf, data_length);
> +	if (result)
> +		goto error_unpower;
> +	if (!st->chip_config.enable) {
> +		result = inv_mpu6050_set_power_itg(st, false);
> +		if (result)
> +			goto error_unlock;
> +	}
> +	mutex_unlock(&indio_dev->mlock);
> +
> +	/* Convert buf to val and return success */
> +	result = iio_bufval_to_rawval(chan, buf, val);
> +	if (result)
> +		return result;
> +	return IIO_VAL_INT;
> +
> +error_unpower:
> +	if (!st->chip_config.enable)
> +		inv_mpu6050_set_power_itg(st, false);
> +error_unlock:
> +	mutex_unlock(&indio_dev->mlock);
> +	return result;
> +}
> +
>  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);
> +	struct inv_mpu6050_state *st = iio_priv(indio_dev);
>  	int ret = 0;
> +	int chan_index;
> +
> +	if (chan < indio_dev->channels || chan > indio_dev->channels + indio_dev->num_channels) {
> +		return -EINVAL;
> +	}
> +	chan_index = chan - indio_dev->channels;
> +	if (chan_index >= INV_MPU6050_NUM_INT_CHAN)
> +		return inv_mpu6050_read_raw_external(indio_dev, chan, val, val2, mask);
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> @@ -819,6 +926,309 @@ static const struct iio_info mpu_info = {
>  	.validate_trigger = inv_mpu6050_validate_trigger,
>  };
>  
> +extern struct device_type iio_device_type;
> +
> +static int iio_device_from_i2c_client_match(struct device *dev, void *data)
> +{
> +	return dev->type == &iio_device_type;
> +}
> +
> +static struct iio_dev* iio_device_from_i2c_client(struct i2c_client* i2c)
> +{
> +	struct device *child;
> +
> +	child = device_find_child(&i2c->dev, NULL, iio_device_from_i2c_client_match);
> +
> +	return (child ? dev_to_iio_dev(child) : NULL);
> +}
> +
> +static int inv_mpu_set_external_reads_enabled(struct inv_mpu6050_state *st, bool en)
> +{
> +	int i, result, error = 0;
> +
> +	/* Even if there are errors try to disable all slaves. */
> +	for (i = 0; i < INV_MPU6050_MAX_EXT_SENSORS; ++i) {
> +		result = regmap_update_bits(st->map, INV_MPU6050_REG_I2C_SLV_CTRL(i),
> +					    INV_MPU6050_BIT_I2C_SLV_EN,
> +					    en ?  INV_MPU6050_BIT_I2C_SLV_EN : 0);
> +		if (result) {
> +			error = result;
> +		}
> +	}
> +
> +	return error;
> +}
> +
> +static int inv_mpu_parse_one_ext_sens(struct device *dev,
> +				      struct device_node *np,
> +				      struct inv_mpu6050_ext_sens_spec *spec)
> +{
> +	int result;
> +	u32 addr, reg, len;
> +
> +	result = of_property_read_u32(np, "inv,addr", &addr);
> +	if (result)
> +		return result;
> +	result = of_property_read_u32(np, "inv,reg", &reg);
> +	if (result)
> +		return result;
> +	result = of_property_read_u32(np, "inv,len", &len);
> +	if (result)
> +		return result;
> +
> +	spec->addr = addr;
> +	spec->reg = reg;
> +	spec->len = len;
> +
> +	result = of_property_count_u32_elems(np, "inv,external-channels");
> +	if (result < 0)
> +		return result;
> +	spec->num_ext_channels = result;
> +	spec->ext_channels = devm_kmalloc(dev, spec->num_ext_channels * sizeof(*spec->ext_channels), GFP_KERNEL);
> +	if (!spec->ext_channels)
> +		return -ENOMEM;
> +	result = of_property_read_u32_array(np, "inv,external-channels",
> +					    spec->ext_channels,
> +					    spec->num_ext_channels);
> +	if (result)
> +		return result;
> +
> +	return 0;
> +}
> +
> +static int inv_mpu_parse_ext_sens(struct device *dev,
> +				  struct device_node *node,
> +				  struct inv_mpu6050_ext_sens_spec *specs)
> +{
> +	struct device_node *child;
> +	int result;
> +	u32 reg;
> +
> +	for_each_available_child_of_node(node, child) {
> +		result = of_property_read_u32(child, "reg", &reg);
> +		if (result)
> +			return result;
> +		if (reg > INV_MPU6050_MAX_EXT_SENSORS) {
> +			dev_err(dev, "External sensor index %u out of range, max %d\n",
> +				reg, INV_MPU6050_MAX_EXT_SENSORS);
> +			return -EINVAL;
> +		}
> +		result = inv_mpu_parse_one_ext_sens(dev, child, &specs[reg]);
> +		if (result)
> +			return result;
> +	}
> +
> +	return 0;
> +}
> +
> +static int inv_mpu_get_ext_sens_spec(struct iio_dev *indio_dev)
> +{
> +	struct inv_mpu6050_state *st = iio_priv(indio_dev);
> +	struct device *dev = regmap_get_device(st->map);
> +	struct device_node *node;
> +	int result;
> +
> +	node = of_get_child_by_name(dev->of_node, "inv,external-sensors");
> +	if (node) {
> +		result = inv_mpu_parse_ext_sens(dev, node, st->ext_sens_spec);
> +		if (result)
> +			dev_err(dev, "Failed to parsing external-sensors devicetree data\n");
> +		return result;
> +	}
> +
> +	/* Maybe some other way to deal with this? */
Probably not ;)
> +
> +	return 0;
> +}
> +
> +/* Struct used while enumerating devices and matching them */
> +struct inv_mpu_handle_ext_sensor_fnarg
> +{
> +	struct iio_dev *indio_dev;
> +
> +	/* Next scan index */
> +	int scan_index;
> +	/* Index among external channels */
> +	int chan_index;
> +	/* Offset in EXTDATA */
> +	int data_offset;
> +	struct iio_chan_spec *channels;
> +};
> +
> +static int inv_mpu_config_external_read(struct inv_mpu6050_state *st, int index,
> +					const struct inv_mpu6050_ext_sens_spec *spec)
> +{
> +	int result;
> +
> +	result = regmap_write(st->map, INV_MPU6050_REG_I2C_SLV_ADDR(index),
> +			      INV_MPU6050_BIT_I2C_SLV_RW | spec->addr);
> +	if (result)
> +		return result;
> +	result = regmap_write(st->map, INV_MPU6050_REG_I2C_SLV_REG(index), spec->reg);
> +	if (result)
> +		return result;
> +	result = regmap_write(st->map, INV_MPU6050_REG_I2C_SLV_CTRL(index),
> +			      INV_MPU6050_BIT_I2C_SLV_EN | spec->len);
> +
> +	return result;
> +}
> +
> +static int inv_mpu_handle_ext_sensor_fn(struct device *dev, void *data)
> +{
> +	struct inv_mpu_handle_ext_sensor_fnarg *fnarg = data;
> +	struct iio_dev *indio_dev = fnarg->indio_dev;
> +	struct inv_mpu6050_state *st = iio_priv(indio_dev);
> +	struct device *mydev = regmap_get_device(st->map);
> +	struct i2c_client *client;
> +	struct iio_dev *orig_dev;
> +	int i, j;
> +
> +	client = i2c_verify_client(dev);
> +	if (!client)
> +		return 0;
> +	orig_dev = iio_device_from_i2c_client(client);
> +	if (!orig_dev)
> +		return 0;
> +	for (i = 0; i < INV_MPU6050_MAX_EXT_SENSORS; ++i) {
> +		int start_data_offset;
> +		struct inv_mpu6050_ext_sens_spec *ext_sens_spec = &st->ext_sens_spec[i];
> +		struct inv_mpu6050_ext_sens_state *ext_sens_state = &st->ext_sens[i];
> +
> +		if (ext_sens_spec->addr != client->addr)
> +			continue;
> +		if (ext_sens_state->orig_dev) {
> +			dev_warn(&indio_dev->dev, "already found slave with at addr %d\n", client->addr);
> +			continue;
> +		}
> +		dev_info(mydev, "external sensor %d is device %s\n",
> +				i, orig_dev->name ?: dev_name(&orig_dev->dev));
> +		ext_sens_state->orig_dev = orig_dev;
> +		ext_sens_state->scan_mask = 0;
> +		ext_sens_state->data_len = ext_sens_spec->len;
> +		start_data_offset = fnarg->data_offset;
> +
> +		/* Matched the device (by address). Now process channels: */
> +		for (j = 0; j < ext_sens_spec->num_ext_channels; ++j) {
> +			int orig_chan_index;
> +			const struct iio_chan_spec *orig_chan_spec;
> +			struct iio_chan_spec *chan_spec;
> +			struct inv_mpu6050_ext_chan_state *chan_state;
> +
> +			orig_chan_index = ext_sens_spec->ext_channels[j];
> +			orig_chan_spec = &orig_dev->channels[orig_chan_index];
> +			chan_spec = &fnarg->channels[INV_MPU6050_NUM_INT_CHAN + fnarg->chan_index];
> +			chan_state = &st->ext_chan[fnarg->chan_index];
> +
> +			chan_state->ext_sens_index = i;
> +			chan_state->orig_chan_index = orig_chan_index;
> +			chan_state->data_offset = fnarg->data_offset;
> +			memcpy(chan_spec, orig_chan_spec, sizeof(struct iio_chan_spec));
> +			chan_spec->scan_index = fnarg->scan_index;
> +			ext_sens_state->scan_mask |= (1 << chan_spec->scan_index);
> +			if (orig_chan_spec->scan_type.storagebits != 16) {
> +				/*
> +				 * Supporting other channels sizes would require data read from the
> +				 * hardware fifo to comply with iio alignment.
> +				 */
Or a demux/mux before iio_push_to_buffers.
> +				dev_err(&indio_dev->dev, "All external channels must have 16 storage bits\n");
> +				return -EINVAL;
> +			}
> +
> +			fnarg->scan_index++;
> +			fnarg->chan_index++;
> +			fnarg->data_offset += chan_spec->scan_type.storagebits / 8;
> +			dev_info(mydev, "Reading external channel #%d scan_index %d data_offset %d"
> +					" from original device %s chan #%d scan_index %d\n",
> +					fnarg->chan_index, chan_spec->scan_index, chan_state->data_offset,
> +					orig_dev->name ?: dev_name(&orig_dev->dev), orig_chan_index, orig_chan_spec->scan_index);
> +		}
> +		if (start_data_offset + ext_sens_spec->len != fnarg->data_offset) {
> +			dev_warn(mydev, "length mismatch between i2c slave read length and slave channel spec\n");
> +			return -EINVAL;
> +		}
> +
> +		return inv_mpu_config_external_read(st, i, ext_sens_spec);
> +	}
> +	return 0;
> +}
> +
> +static int inv_mpu6050_handle_ext_sensors(struct iio_dev *indio_dev)
> +{
> +	struct inv_mpu6050_state *st = iio_priv(indio_dev);
> +	struct device *dev = regmap_get_device(st->map);
> +	struct inv_mpu_handle_ext_sensor_fnarg fnarg = {
> +		.indio_dev = indio_dev,
> +		.chan_index = 0,
> +		.data_offset = 0,
> +		.scan_index = INV_MPU6050_SCAN_TIMESTAMP,
> +	};
> +	int i, result;
> +	int num_ext_chan = 0, sum_data_len = 0;
> +
> +	inv_mpu_get_ext_sens_spec(indio_dev);
> +	for (i = 0; i < INV_MPU6050_MAX_EXT_SENSORS; ++i) {
> +		num_ext_chan += st->ext_sens_spec[i].num_ext_channels;
> +		sum_data_len += st->ext_sens_spec[i].len;
> +	}
> +	if (sum_data_len > INV_MPU6050_CNT_EXT_SENS_DATA) {
> +		dev_err(dev, "Too many bytes from external sensors:"
> +			      " requested=%d max=%d\n",
> +			      sum_data_len, INV_MPU6050_CNT_EXT_SENS_DATA);
> +		return -EINVAL;
> +	}
> +
> +	/* Zero length means nothing to do */
> +	if (!sum_data_len) {
> +		indio_dev->channels = inv_mpu_channels;
> +		indio_dev->num_channels = INV_MPU6050_NUM_INT_CHAN;
> +		BUILD_BUG_ON(ARRAY_SIZE(inv_mpu_channels) != INV_MPU6050_NUM_INT_CHAN);
> +		return 0;
> +	}
> +
> +	fnarg.channels = devm_kmalloc(indio_dev->dev.parent,
> +				      (num_ext_chan + INV_MPU6050_NUM_INT_CHAN) *
> +				      sizeof(struct iio_chan_spec),
> +				      GFP_KERNEL);
> +	if (!fnarg.channels)
> +		return -ENOMEM;
> +	memcpy(fnarg.channels, inv_mpu_channels, sizeof(inv_mpu_channels));
> +	memset(fnarg.channels + INV_MPU6050_NUM_INT_CHAN, 0,
> +	       num_ext_chan * sizeof(struct iio_chan_spec));
> +
> +	st->ext_chan = devm_kzalloc(indio_dev->dev.parent,
> +			(num_ext_chan) * sizeof(*st->ext_chan),
> +			GFP_KERNEL);
> +	if (!st->ext_chan)
> +		return -ENOMEM;
> +
> +	result = inv_mpu6050_set_power_itg(st, true);
> +	if (result < 0)
> +		return result;
> +
> +	result = device_for_each_child(&st->aux_master_adapter.dev, &fnarg,
> +				       inv_mpu_handle_ext_sensor_fn);
> +	if (result)
> +		goto out_disable;
> +	/* Timestamp channel has index 0 and last scan_index */
> +	fnarg.channels[0].scan_index = fnarg.scan_index;
> +
> +	if (fnarg.chan_index != num_ext_chan) {
> +		dev_err(&indio_dev->dev, "Failed to match all external channels!\n");
> +		result = -EINVAL;
> +		goto out_disable;
> +	}
> +
> +	result = inv_mpu6050_set_power_itg(st, false);
> +	indio_dev->channels = fnarg.channels;
> +	indio_dev->num_channels = INV_MPU6050_NUM_INT_CHAN + num_ext_chan;
> +	return result;
> +
> +out_disable:
> +	inv_mpu6050_set_power_itg(st, false);
> +	inv_mpu_set_external_reads_enabled(st, false);
> +	return result;
> +}
> +
>  /**
>   *  inv_check_and_setup_chip() - check and setup chip.
>   */
> @@ -1121,8 +1531,6 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
>  		indio_dev->name = name;
>  	else
>  		indio_dev->name = dev_name(dev);
> -	indio_dev->channels = inv_mpu_channels;
> -	indio_dev->num_channels = ARRAY_SIZE(inv_mpu_channels);
>  
>  	indio_dev->info = &mpu_info;
>  	indio_dev->modes = INDIO_BUFFER_TRIGGERED;
> @@ -1160,6 +1568,12 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
>  	}
>  #endif
>  
> +	result = inv_mpu6050_handle_ext_sensors(indio_dev);
> +	if (result < 0) {
> +		dev_err(dev, "failed to configure channels %d\n", result);
> +		goto out_remove_trigger;
> +	}
> +
>  	INIT_KFIFO(st->timestamps);
>  	spin_lock_init(&st->time_stamp_lock);
>  	result = iio_device_register(indio_dev);
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> index 9d406df..007fe75 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> @@ -124,6 +124,40 @@ struct inv_mpu6050_hw {
>  	const struct inv_mpu6050_chip_config *config;
>  };
>  
> +#define INV_MPU6050_MAX_EXT_SENSORS 4
> +
> +/* Specification for an external sensor */
> +struct inv_mpu6050_ext_sens_spec {
> +	unsigned short addr;
> +	u8 reg, len;
> +
> +	int num_ext_channels;
> +	int *ext_channels;
> +};
> +
> +/* Driver state for each external sensor */
> +struct inv_mpu6050_ext_sens_state {
> +	struct iio_dev *orig_dev;
> +
> +	/* Mask of all channels in this sensor */
> +	unsigned long scan_mask;
> +
> +	/* Length of reading. */
> +	int data_len;
> +};
> +
> +/* Driver state for each external channel */
> +struct inv_mpu6050_ext_chan_state {
> +	/* Index inside state->ext_sens */
> +	int ext_sens_index;
> +
> +	/* Index inside orig_dev->channels */
> +	int orig_chan_index;
> +
> +	/* Relative to EXT_SENS_DATA_00 */
> +	int data_offset;
> +};
> +
>  /*
>   *  struct inv_mpu6050_state - Driver state variables.
>   *  @TIMESTAMP_FIFO_SIZE: fifo size for timestamp.
> @@ -164,6 +198,10 @@ struct inv_mpu6050_state {
>  	struct i2c_adapter aux_master_adapter;
>  	struct completion slv4_done;
>  #endif
> +
> +	struct inv_mpu6050_ext_sens_spec ext_sens_spec[INV_MPU6050_MAX_EXT_SENSORS];
> +	struct inv_mpu6050_ext_sens_state ext_sens[INV_MPU6050_MAX_EXT_SENSORS];
> +	struct inv_mpu6050_ext_chan_state *ext_chan;
>  };
>  
>  /*register and associated bit definition*/
> @@ -178,6 +216,24 @@ struct inv_mpu6050_state {
>  #define INV_MPU6050_REG_FIFO_EN             0x23
>  #define INV_MPU6050_BIT_ACCEL_OUT           0x08
>  #define INV_MPU6050_BITS_GYRO_OUT           0x70
> +#define INV_MPU6050_BIT_SLV0_FIFO_EN        0x01
> +#define INV_MPU6050_BIT_SLV1_FIFO_EN        0x02
> +#define INV_MPU6050_BIT_SLV2_FIFO_EN        0x04
> +#define INV_MPU6050_BIT_SLV2_FIFO_EN        0x04
> +
> +/* SLV3 fifo enabling is not in REG_FIFO_EN */
> +#define INV_MPU6050_REG_MST_CTRL	    0x24
> +#define INV_MPU6050_BIT_SLV3_FIFO_EN        0x10
> +
> +/* For slaves 0-3 */
> +#define INV_MPU6050_REG_I2C_SLV_ADDR(i)     (0x25 + 3 * (i))
> +#define INV_MPU6050_REG_I2C_SLV_REG(i)      (0x26 + 3 * (i))
> +#define INV_MPU6050_REG_I2C_SLV_CTRL(i)     (0x27 + 3 * (i))
> +#define INV_MPU6050_BIT_I2C_SLV_RW          0x80
> +#define INV_MPU6050_BIT_I2C_SLV_EN          0x80
> +#define INV_MPU6050_BIT_I2C_SLV_BYTE_SW     0x40
> +#define INV_MPU6050_BIT_I2C_SLV_REG_DIS     0x20
> +#define INV_MPU6050_BIT_I2C_SLV_REG_GRP     0x10
>  
>  #define INV_MPU6050_REG_I2C_SLV4_ADDR       0x31
>  #define INV_MPU6050_BIT_I2C_SLV4_R          0x80
> @@ -252,8 +308,8 @@ struct inv_mpu6050_state {
>  #define INV_MPU6050_GYRO_CONFIG_FSR_SHIFT    3
>  #define INV_MPU6050_ACCL_CONFIG_FSR_SHIFT    3
>  
> -/* 6 + 6 round up and plus 8 */
> -#define INV_MPU6050_OUTPUT_DATA_SIZE         24
> +/* max is 3*2 accel + 3*2 gyro + 24 external + 8 ts */
> +#define INV_MPU6050_OUTPUT_DATA_SIZE         44
>  
>  #define INV_MPU6050_REG_INT_PIN_CFG	0x37
>  #define INV_MPU6050_BIT_BYPASS_EN	0x2
> @@ -266,6 +322,9 @@ struct inv_mpu6050_state {
>  #define INV_MPU6050_MIN_FIFO_RATE            4
>  #define INV_MPU6050_ONE_K_HZ                 1000
>  
> +#define INV_MPU6050_REG_EXT_SENS_DATA_00	0x49
> +#define INV_MPU6050_CNT_EXT_SENS_DATA		24
> +
>  #define INV_MPU6050_REG_WHOAMI			117
>  
>  #define INV_MPU6000_WHOAMI_VALUE		0x68
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> index e8bda7f..8fa5c3d 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> @@ -52,6 +52,10 @@ int inv_reset_fifo(struct iio_dev *indio_dev)
>  	result = regmap_write(st->map, st->reg->fifo_en, 0);
>  	if (result)
>  		goto reset_fifo_fail;
> +	result = regmap_update_bits(st->map, INV_MPU6050_REG_MST_CTRL,
> +				    INV_MPU6050_BIT_SLV3_FIFO_EN, 0);
> +	if (result)
> +		goto reset_fifo_fail;
>  	/* disable fifo reading */
>  	st->chip_config.user_ctrl &= ~INV_MPU6050_BIT_FIFO_EN;
>  	result = regmap_write(st->map, st->reg->user_ctrl,
> @@ -70,7 +74,8 @@ int inv_reset_fifo(struct iio_dev *indio_dev)
>  
>  	/* enable interrupt */
>  	if (st->chip_config.accl_fifo_enable ||
> -	    st->chip_config.gyro_fifo_enable) {
> +	    st->chip_config.gyro_fifo_enable ||
> +	    true) {
>  		result = regmap_update_bits(st->map, st->reg->int_enable,
>  					    INV_MPU6050_BIT_DATA_RDY_EN,
>  					    INV_MPU6050_BIT_DATA_RDY_EN);
> @@ -89,10 +94,23 @@ int inv_reset_fifo(struct iio_dev *indio_dev)
>  		d |= INV_MPU6050_BITS_GYRO_OUT;
>  	if (st->chip_config.accl_fifo_enable)
>  		d |= INV_MPU6050_BIT_ACCEL_OUT;
> +	if (*indio_dev->active_scan_mask & st->ext_sens[0].scan_mask)
> +		d |= INV_MPU6050_BIT_SLV0_FIFO_EN;
> +	if (*indio_dev->active_scan_mask & st->ext_sens[1].scan_mask)
> +		d |= INV_MPU6050_BIT_SLV1_FIFO_EN;
> +	if (*indio_dev->active_scan_mask & st->ext_sens[2].scan_mask)
> +		d |= INV_MPU6050_BIT_SLV2_FIFO_EN;
>  	result = regmap_write(st->map, st->reg->fifo_en, d);
>  	if (result)
>  		goto reset_fifo_fail;
> -
> +	if (*indio_dev->active_scan_mask & st->ext_sens[3].scan_mask)
> +	{
> +		result = regmap_update_bits(st->map, INV_MPU6050_REG_MST_CTRL,
> +					    INV_MPU6050_BIT_SLV3_FIFO_EN,
> +					    INV_MPU6050_BIT_SLV3_FIFO_EN);
> +		if (result)
> +			goto reset_fifo_fail;
> +	}
>  	return 0;
>  
>  reset_fifo_fail:
> @@ -129,8 +147,9 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
>  	struct iio_poll_func *pf = p;
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct inv_mpu6050_state *st = iio_priv(indio_dev);
> -	size_t bytes_per_datum;
> +	size_t bytes_per_datum = 0;
>  	int result;
> +	int i;
>  	u8 data[INV_MPU6050_OUTPUT_DATA_SIZE];
>  	u16 fifo_count;
>  	s64 timestamp;
> @@ -143,15 +162,18 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
>  	spi = i2c ? NULL: to_spi_device(regmap_dev);
>  
>  	mutex_lock(&indio_dev->mlock);
> -	if (!(st->chip_config.accl_fifo_enable |
> -		st->chip_config.gyro_fifo_enable))
> -		goto end_session;
> +
> +	/* Compute bytes_per_datum */
>  	bytes_per_datum = 0;
>  	if (st->chip_config.accl_fifo_enable)
>  		bytes_per_datum += INV_MPU6050_BYTES_PER_3AXIS_SENSOR;
> -
>  	if (st->chip_config.gyro_fifo_enable)
>  		bytes_per_datum += INV_MPU6050_BYTES_PER_3AXIS_SENSOR;
> +	for (i = 0; i < INV_MPU6050_MAX_EXT_SENSORS; ++i)
> +		if (st->ext_sens[i].scan_mask & *indio_dev->active_scan_mask)
> +			bytes_per_datum += st->ext_sens[i].data_len;
> +	if (!bytes_per_datum)
> +		return 0;
>  
>  	/*
>  	 * read fifo_count register to know how many bytes inside FIFO
> @@ -228,6 +250,7 @@ static int inv_mpu_buffer_preenable(struct iio_dev *indio_dev)
>  {
>  	struct inv_mpu6050_state *st = iio_priv(indio_dev);
>  	unsigned long mask = *indio_dev->active_scan_mask;
> +	int i;
>  
>  	if ((mask & INV_MPU6050_SCAN_MASK_GYRO) &&
>  	    (mask & INV_MPU6050_SCAN_MASK_GYRO) != INV_MPU6050_SCAN_MASK_GYRO)
> @@ -245,6 +268,17 @@ static int inv_mpu_buffer_preenable(struct iio_dev *indio_dev)
>  		return -EINVAL;
>  	}
>  
> +	for (i = 0; i < INV_MPU6050_MAX_EXT_SENSORS; ++i) {
> +		if ((mask & st->ext_sens[i].scan_mask) &&
> +		    (mask & st->ext_sens[i].scan_mask) != st->ext_sens[i].scan_mask)
> +		{
> +			dev_warn(regmap_get_device(st->map),
> +				 "External channels from the same reading "
> +				 "can only be enabled together\n");
> +			return -EINVAL;
> +		}
> +	}
> +
>  	return 0;
>  }
>  
> 

Re: [RFC 0/7] iio: inv_mpu6050: Support i2c master and external readings

[Mark Brown]

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

On Sun, May 01, 2016 at 06:04:08PM +0100, Jonathan Cameron wrote:

> If you were to break these registers up into regmap fields it might solve 
> this..  Regmap writes always go through whatever - whether they match the
> existing state of the cache or not.  If fields are involved the write will get
> built up from whatever field you change and whatever the cache has for other
> elements.  I guess it only works if they volatile bits are contiguous though.
> Maybe hand rolling it is cleaner here.

> Mark, any clever thoughts on this?

I don't have enough context here to be sure what the problem you're
trying to solve is, sorry.

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

Re: [PATCH 1/7] iio: inv_mpu6050: Do burst reads using spi/i2c directly

[Mark Brown]

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

On Sun, May 01, 2016 at 06:11:50PM +0100, Jonathan Cameron wrote:

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.

>  On 29/04/16 20:02, Crestez Dan Leonard wrote:
> > Using regmap_read_bulk is wrong because it assumes that a range of
> > registers is being read. In our case reading from the fifo register will
> > return multiple values but this is *not* auto-increment.

> > This currently works by accident.

> Cc'd Mark again.  He's the regmap maintainer (amongst other things) so
> a series doing slightly odd things with regmap should probably have been
> cc'd to him in the first place.

> Perhaps regmap should have a repeat read function for this sort of fifo access?
> Mark, is this something you'd consider?  Easy enough to implement after all as
> a variant on regmap_read_bulk...

> Having the below in a driver just feels wrong to me....

That doesn't sound like a crazy idea, please send patches for proposed
extensions.

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

Re: [RFC 5/7] iio: inv_mpu6050: Add support for auxiliary I2C master

[Peter Rosin]

On 2016-04-29 21:02, Crestez Dan Leonard wrote:
> The MPU has an auxiliary I2C bus for connecting external
> sensors. This bus has two operating modes:
> * pass-through, which connects the primary and auxiliary busses
> together. This is already supported via an i2c mux.
> * I2C master mode, where the mpu60x0 acts as a master to any external
> connected sensors. This is implemented by this patch.
> 
> This I2C master mode also works when the MPU itself is connected via
> SPI.
> 
> I2C master supports up to 5 slaves. Slaves 0-3 have a common operating
> mode while slave 4 is different. This patch implements an i2c adapter
> using slave 4 because it has a cleaner interface and it has an
> interrupt that signals when data from slave to master arrived.
> 
> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
> ---
>  .../devicetree/bindings/iio/imu/inv_mpu6050.txt    |  61 +++++-
>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c         | 239 ++++++++++++++++++++-
>  drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h          |  46 ++++
>  drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c      |   8 -
>  4 files changed, 341 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt b/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt
> index a9fc11e..aaf12b4 100644
> --- a/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt
> +++ b/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt
> @@ -1,16 +1,27 @@
>  InvenSense MPU-6050 Six-Axis (Gyro + Accelerometer) MEMS MotionTracking Device
>  
> -http://www.invensense.com/mems/gyro/mpu6050.html
> -
>  Required properties:
> - - compatible : should be "invensense,mpu6050"
> - - reg : the I2C address of the sensor
> + - compatible : should be "invensense,mpuXXXX"
> + - reg : the I2C or SPI address of the sensor
>   - interrupt-parent : should be the phandle for the interrupt controller
>   - interrupts : interrupt mapping for GPIO IRQ
>  
>  Optional properties:
>   - mount-matrix: an optional 3x3 mounting rotation matrix
> + - inv,i2c-aux-master: operate aux i2c in "master mode" (default is mux).

I've looked a bit more into this, and I don't think you need
this property...

> +
> +Valid compatible strings:
> + - mpu6000
> + - mpu6050
> + - mpu6500
> + - mpu9150
> +
> +It is possible to attach auxiliary sensors to the MPU and have them be handled
> +by linux. Those auxiliary sensors are described as an i2c bus.
> +
> +Devices connected in "bypass" mode must be listed behind i2c@0 with the address 0
>  
> +Devices connected in "master" mode must be listed behind i2c@1 with the address 1

...and just condition the new stuff on the name i2c-master
instead. If there is no i2c-master child, then fall back
to the mux code. Does that work?

>  Example:
>  	mpu6050@68 {
> @@ -28,3 +39,45 @@ Example:
>  		               "0",                   /* y2 */
>  		               "0.984807753012208";   /* z2 */
>  	};
> +
> +Example describing mpu9150 (which includes an ak9875 on chip):
> +	mpu9150@68 {
> +		compatible = "invensense,mpu9150";
> +		reg = <0x68>;
> +		interrupt-parent = <&gpio1>;
> +		interrupts = <18 1>;
> +
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		i2c@0 {
> +			reg = <0>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			ak8975@0c {
> +				compatible = "ak,ak8975";
> +				reg = <0x0c>;
> +			};
> +		};
> +	};
> +
> +Example describing a mpu6500 via SPI with an hmc5883l on auxiliary i2c:
> +	mpu6500@0 {
> +		compatible = "inv,mpu6500";
> +		reg = <0x0>;
> +		spi-max-frequency = <1000000>;
> +		interrupt-parent = <&gpio1>;
> +		interrupts = <31 1>;
> +
> +		inv,i2c-aux-master

If you keep the below as i2c@1, including the @, I think you also need
	#address-cells = <1>;
	#size-cells = <0>;

As always, I'm not a device tree expert, but I'd be glad to lose
these extra lines and change i2c@1 into i2c-master.

> +		i2c@1 {
> +			reg = <1>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			hmc5883l@1e {
> +				compatible = "honeywell,hmc5883l";
> +				reg = <0x1e>;
> +			};
> +		};
> +	};

*snip*

Cheers,
Peter

Re: [RFC 0/7] iio: inv_mpu6050: Support i2c master and external readings

[Crestez Dan Leonard]

On 05/02/2016 06:23 PM, Mark Brown wrote:
> On Sun, May 01, 2016 at 06:04:08PM +0100, Jonathan Cameron wrote:
> 
>> If you were to break these registers up into regmap fields it might solve 
>> this..  Regmap writes always go through whatever - whether they match the
>> existing state of the cache or not.  If fields are involved the write will get
>> built up from whatever field you change and whatever the cache has for other
>> elements.  I guess it only works if they volatile bits are contiguous though.
>> Maybe hand rolling it is cleaner here.
> 
>> Mark, any clever thoughts on this?
> 
> I don't have enough context here to be sure what the problem you're
> trying to solve is, sorry.
> 
This is worth explaining:

I have a device which has several registers with bits that are a mix of
"cacheable" and "volatile". For example for register SLV4_CTRL:

- Bit 7 (I2C_SLV4_EN) triggers a transaction with slave 4 when a "1" is
written. The bit is cleared when the transaction is done.
- Bits 0-4 (I2C_MST_DLY) configures the reduced access rate of I2C
slaves relative to the device sample rate. This applies to slaves 0-3 as
well.

If I2C_MST_DLY was a separate register it could be easily cached by
regmap. Because it's part of a volatile register I have to add a
private_data field caching the value and always write it when triggering
a SLV4 transfer.

Jonathan was wondering if regmap can still be used somehow instead of
custom caching.

-- 
Regards,
Leonard

Re: [RFC 0/7] iio: inv_mpu6050: Support i2c master and external readings

[Mark Brown]

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

On Tue, May 03, 2016 at 02:21:40PM +0300, Crestez Dan Leonard wrote:

> I have a device which has several registers with bits that are a mix of
> "cacheable" and "volatile". For example for register SLV4_CTRL:

> - Bit 7 (I2C_SLV4_EN) triggers a transaction with slave 4 when a "1" is
> written. The bit is cleared when the transaction is done.
> - Bits 0-4 (I2C_MST_DLY) configures the reduced access rate of I2C
> slaves relative to the device sample rate. This applies to slaves 0-3 as
> well.

> If I2C_MST_DLY was a separate register it could be easily cached by
> regmap. Because it's part of a volatile register I have to add a
> private_data field caching the value and always write it when triggering
> a SLV4 transfer.

> Jonathan was wondering if regmap can still be used somehow instead of
> custom caching.

If you want to cache in regmap just write a 0 back to the enable bit
after you've set it.

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

Re: [PATCH 6/7] iio: inv_mpu6050: Check channel configuration on preenable

[Crestez Dan Leonard]

On 05/01/2016 08:34 PM, Jonathan Cameron wrote:
> On 29/04/16 20:02, Crestez Dan Leonard wrote:
>> Right now it is possible to only enable some of the x/y/z channels, for
>> example you can enable accel_z without x or y. If you actually do that
>> what you get is actually only the x channel.
>>
>> In theory the device supports selecting gyro x/y/z channels
>> individually. It would also be possible to selectively enable x/y/z
>> accel by unpacking the data read from the hardware into a format the iio
>> core accepts.
>>
>> It is easier to simply refuse incorrect configuration.
> Or see suggestion inline. This isn't an uncommon problem!
>
>> +/* Validate channels are set in a correct configuration */
>> +static int inv_mpu_buffer_preenable(struct iio_dev *indio_dev)
>> +{
> This should not be in the preenable.  It's perfectly possible to know that mode was invalid
> earlier than this.  Use the core demux to handle this case by providing
> available_scanmasks.  The the core will handle demuxing the data stream if needed to
> provide the channels enabled only in the kfifo.
> 
But available_scanmasks is a list! Listing every valid scanmask would
work for accel/gyro but the next patch adds support for up to 4 slaves
and each slave can be enabled/disabled. This would requires generating
2^6 == 64 possible scanmasks, right?

I tried to do this same validation inside .validate_scan_mask but that
function is called for each individual scan bit. What I want is to
validate the scan mask once it's entirely configured, and preenable
seems to be the best choice.

This could be implemented with an "adjust_scan_mask" driver function
which adds bits to a trial scanmask until the configuration is suitable.

I think a better solution would be to add a .get_buffer_offsets driver
function and have the iio core handled demuxing based on offsets rather
than just scan_mask.

This would also handle alignment for external channels with storagebits
!= 16. For example if I enable accel and an external u32 channel iio
expects the following layout:
    <u16 ACCEL_X> <u16 ACCEL_Y> <u16 ACCEL_Z> <u16 PAD> <u32 external>
while my device gives the following:
    <u16 ACCEL_X> <u16 ACCEL_Y> <u16 ACCEL_Z> <u32 external>
I could add memcpy mangling in my driver but handling this belongs in
the iio core.

I'd rather avoid depending on new features in the iio core and just do
simple validations instead. This is because the feature I'm adding is
already complex.

-- 
Regards,
Leonard

Re: [PATCH 6/7] iio: inv_mpu6050: Check channel configuration on preenable

[Jonathan Cameron]

On 03/05/16 14:01, Crestez Dan Leonard wrote:
> On 05/01/2016 08:34 PM, Jonathan Cameron wrote:
>> On 29/04/16 20:02, Crestez Dan Leonard wrote:
>>> Right now it is possible to only enable some of the x/y/z channels, for
>>> example you can enable accel_z without x or y. If you actually do that
>>> what you get is actually only the x channel.
>>>
>>> In theory the device supports selecting gyro x/y/z channels
>>> individually. It would also be possible to selectively enable x/y/z
>>> accel by unpacking the data read from the hardware into a format the iio
>>> core accepts.
>>>
>>> It is easier to simply refuse incorrect configuration.
>> Or see suggestion inline. This isn't an uncommon problem!
>>
>>> +/* Validate channels are set in a correct configuration */
>>> +static int inv_mpu_buffer_preenable(struct iio_dev *indio_dev)
>>> +{
>> This should not be in the preenable.  It's perfectly possible to know that mode was invalid
>> earlier than this.  Use the core demux to handle this case by providing
>> available_scanmasks.  The the core will handle demuxing the data stream if needed to
>> provide the channels enabled only in the kfifo.
>>
> But available_scanmasks is a list! Listing every valid scanmask would
> work for accel/gyro but the next patch adds support for up to 4 slaves
> and each slave can be enabled/disabled. This would requires generating
> 2^6 == 64 possible scanmasks, right?
Not that bad (a whole 256 bytes ;), but I get your point.

> 
> I tried to do this same validation inside .validate_scan_mask but that
> function is called for each individual scan bit. What I want is to
> validate the scan mask once it's entirely configured, and preenable
> seems to be the best choice.
We want to know earlier that a given set of channels isn't possible.  
> 
> This could be implemented with an "adjust_scan_mask" driver function
> which adds bits to a trial scanmask until the configuration is suitable.
I'd call it scan_mask_available_match or similar but yes, this
would work for me.  So in iio_scan_mask_set we'd have something like:
if (indio_dev->available_scan_masks) {
		mask = iio_scan_mask_match(indio_dev->available_scan_masks,
					   indio_dev->masklength,
					   trialmask, false);
		if (!mask)
			goto err_invalid_mask;
} else if (indio_dev->ops->scan_mask_available_match) {
	mask = indio_dev->ops->scan_mask_available_match(indio_dev->masklength,
							 trialmask);
	if (!mask)
		goto err_invalid_mask;
}

Bit of complexity needed to avoid any memory issues (probably use a local
buffer in the driver private struct to point mask at once filled),

Scan mask available could then compute the minimum possible scan mask for the
device in question given the requirements of trialmask.

An easy change that effectively provides a dynamic equivalent of our constant
list of available scan masks.  I'd advocate only using it when the
device complexity make it worthwhile (much like dynamic allocation of channel
specs currently) - so not many cases.

> 
> I think a better solution would be to add a .get_buffer_offsets driver
> function and have the iio core handled demuxing based on offsets rather
> than just scan_mask.
Scan mask isn't about just this.   A common case is not that you have
to enable additional channels to end up with a valid set of readings
but rather that you can only read a (complex) subset of channels at
a time in buffered mode.  The simplest being onehot, but there are
quite a few other cases (two parallel ADCs each of which is muxed to a
subset of the channels via slow muxes for example).

> 
> This would also handle alignment for external channels with storagebits
> != 16. For example if I enable accel and an external u32 channel iio
> expects the following layout:
>     <u16 ACCEL_X> <u16 ACCEL_Y> <u16 ACCEL_Z> <u16 PAD> <u32 external>
> while my device gives the following:
>     <u16 ACCEL_X> <u16 ACCEL_Y> <u16 ACCEL_Z> <u32 external>
> I could add memcpy mangling in my driver but handling this belongs in
> the iio core.
Matter of opinion.  I'd take this as a driver requirement - much like the
common case of data that is packed tighter than this or irritatingly
interleaved.

<u4 ACCEL_X_11_8> <u4 ACCEL_Y_11_8> <u8 ACCEL_X_7_0> <u8 ACCEL_Y_7_0> is
not uncommon.

Where do we draw the line between what should be in the core and in a
driver?  I'm not against seeing a proposal for byte based packing unwinding
in the core, but I'm not convinced the added complexity (see other email
about the endian mess) is worth it without seeing code.

> 
> I'd rather avoid depending on new features in the iio core and just do
> simple validations instead. This is because the feature I'm adding is
> already complex.
> 
I think your earlier suggestion that you dismissed is a trivial extension
of the core and provides everything you need - so that's the route
that I'd advocate.

However I'm always open to having my mind changed :)

Jonathan

Re: [PATCH 6/7] iio: inv_mpu6050: Check channel configuration on preenable

[Crestez Dan Leonard]

On 05/04/2016 12:01 PM, Jonathan Cameron wrote:
> On 03/05/16 14:01, Crestez Dan Leonard wrote:
>> On 05/01/2016 08:34 PM, Jonathan Cameron wrote:
>>> On 29/04/16 20:02, Crestez Dan Leonard wrote:
>>>> Right now it is possible to only enable some of the x/y/z channels, for
>>>> example you can enable accel_z without x or y. If you actually do that
>>>> what you get is actually only the x channel.
>>>>
>>>> In theory the device supports selecting gyro x/y/z channels
>>>> individually. It would also be possible to selectively enable x/y/z
>>>> accel by unpacking the data read from the hardware into a format the iio
>>>> core accepts.
>>>>
>>>> It is easier to simply refuse incorrect configuration.
>>> Or see suggestion inline. This isn't an uncommon problem!
>>>
>>>> +/* Validate channels are set in a correct configuration */
>>>> +static int inv_mpu_buffer_preenable(struct iio_dev *indio_dev)
>>>> +{
>>> This should not be in the preenable.  It's perfectly possible to know that mode was invalid
>>> earlier than this.  Use the core demux to handle this case by providing
>>> available_scanmasks.  The the core will handle demuxing the data stream if needed to
>>> provide the channels enabled only in the kfifo.
>>>
>> But available_scanmasks is a list! Listing every valid scanmask would
>> work for accel/gyro but the next patch adds support for up to 4 slaves
>> and each slave can be enabled/disabled. This would requires generating
>> 2^6 == 64 possible scanmasks, right?
> Not that bad (a whole 256 bytes ;), but I get your point.
> 
>>
>> I tried to do this same validation inside .validate_scan_mask but that
>> function is called for each individual scan bit. What I want is to
>> validate the scan mask once it's entirely configured, and preenable
>> seems to be the best choice.
> We want to know earlier that a given set of channels isn't possible.  
>>
>> This could be implemented with an "adjust_scan_mask" driver function
>> which adds bits to a trial scanmask until the configuration is suitable.
> I'd call it scan_mask_available_match or similar but yes, this
> would work for me.  So in iio_scan_mask_set we'd have something like:
> if (indio_dev->available_scan_masks) {
> 		mask = iio_scan_mask_match(indio_dev->available_scan_masks,
> 					   indio_dev->masklength,
> 					   trialmask, false);
> 		if (!mask)
> 			goto err_invalid_mask;
> } else if (indio_dev->ops->scan_mask_available_match) {
> 	mask = indio_dev->ops->scan_mask_available_match(indio_dev->masklength,
> 							 trialmask);
> 	if (!mask)
> 		goto err_invalid_mask;
> }
> 
> Bit of complexity needed to avoid any memory issues (probably use a local
> buffer in the driver private struct to point mask at once filled),
> 
> Scan mask available could then compute the minimum possible scan mask for the
> device in question given the requirements of trialmask.
> 
> An easy change that effectively provides a dynamic equivalent of our constant
> list of available scan masks.  I'd advocate only using it when the
> device complexity make it worthwhile (much like dynamic allocation of channel
> specs currently) - so not many cases.
> 
>>
>> I think a better solution would be to add a .get_buffer_offsets driver
>> function and have the iio core handled demuxing based on offsets rather
>> than just scan_mask.
> Scan mask isn't about just this.   A common case is not that you have
> to enable additional channels to end up with a valid set of readings
> but rather that you can only read a (complex) subset of channels at
> a time in buffered mode.  The simplest being onehot, but there are
> quite a few other cases (two parallel ADCs each of which is muxed to a
> subset of the channels via slow muxes for example).
> 
>>
>> This would also handle alignment for external channels with storagebits
>> != 16. For example if I enable accel and an external u32 channel iio
>> expects the following layout:
>>     <u16 ACCEL_X> <u16 ACCEL_Y> <u16 ACCEL_Z> <u16 PAD> <u32 external>
>> while my device gives the following:
>>     <u16 ACCEL_X> <u16 ACCEL_Y> <u16 ACCEL_Z> <u32 external>
>> I could add memcpy mangling in my driver but handling this belongs in
>> the iio core.
> Matter of opinion.  I'd take this as a driver requirement - much like the
> common case of data that is packed tighter than this or irritatingly
> interleaved.
> 
> <u4 ACCEL_X_11_8> <u4 ACCEL_Y_11_8> <u8 ACCEL_X_7_0> <u8 ACCEL_Y_7_0> is
> not uncommon.
> 
> Where do we draw the line between what should be in the core and in a
> driver?  I'm not against seeing a proposal for byte based packing unwinding
> in the core, but I'm not convinced the added complexity (see other email
> about the endian mess) is worth it without seeing code.
> 
>>
>> I'd rather avoid depending on new features in the iio core and just do
>> simple validations instead. This is because the feature I'm adding is
>> already complex.
>>
> I think your earlier suggestion that you dismissed is a trivial extension
> of the core and provides everything you need - so that's the route
> that I'd advocate.
> 
Implementing some sort of "scan_mask_available_match" won't deal with
alignment issues for external channels so it only partially fixes my
current problem. Maybe for some other driver?

If you object to validations inside preenable it would be best to just
implement manual byte mangling inside the driver. I'd split it in two:
    inv_mpu_get_scan_offsets();
    iio_format_aligned_sample(&scan_offsets);

The second part would be sortof generic but implemented statically
inside the driver. Since this would implicitly skip unneeded values from
hardware it will support arbitrary scan masks.

-- 
Regards,
Leonard

Re: [PATCH 6/7] iio: inv_mpu6050: Check channel configuration on preenable

[Jonathan Cameron]

On 4 May 2016 16:34:46 BST, Crestez Dan Leonard <leonard.crestez@intel.com> wrote:
>On 05/04/2016 12:01 PM, Jonathan Cameron wrote:
>> On 03/05/16 14:01, Crestez Dan Leonard wrote:
>>> On 05/01/2016 08:34 PM, Jonathan Cameron wrote:
>>>> On 29/04/16 20:02, Crestez Dan Leonard wrote:
>>>>> Right now it is possible to only enable some of the x/y/z
>channels, for
>>>>> example you can enable accel_z without x or y. If you actually do
>that
>>>>> what you get is actually only the x channel.
>>>>>
>>>>> In theory the device supports selecting gyro x/y/z channels
>>>>> individually. It would also be possible to selectively enable
>x/y/z
>>>>> accel by unpacking the data read from the hardware into a format
>the iio
>>>>> core accepts.
>>>>>
>>>>> It is easier to simply refuse incorrect configuration.
>>>> Or see suggestion inline. This isn't an uncommon problem!
>>>>
>>>>> +/* Validate channels are set in a correct configuration */
>>>>> +static int inv_mpu_buffer_preenable(struct iio_dev *indio_dev)
>>>>> +{
>>>> This should not be in the preenable.  It's perfectly possible to
>know that mode was invalid
>>>> earlier than this.  Use the core demux to handle this case by
>providing
>>>> available_scanmasks.  The the core will handle demuxing the data
>stream if needed to
>>>> provide the channels enabled only in the kfifo.
>>>>
>>> But available_scanmasks is a list! Listing every valid scanmask
>would
>>> work for accel/gyro but the next patch adds support for up to 4
>slaves
>>> and each slave can be enabled/disabled. This would requires
>generating
>>> 2^6 == 64 possible scanmasks, right?
>> Not that bad (a whole 256 bytes ;), but I get your point.
>> 
>>>
>>> I tried to do this same validation inside .validate_scan_mask but
>that
>>> function is called for each individual scan bit. What I want is to
>>> validate the scan mask once it's entirely configured, and preenable
>>> seems to be the best choice.
>> We want to know earlier that a given set of channels isn't possible. 
>
>>>
>>> This could be implemented with an "adjust_scan_mask" driver function
>>> which adds bits to a trial scanmask until the configuration is
>suitable.
>> I'd call it scan_mask_available_match or similar but yes, this
>> would work for me.  So in iio_scan_mask_set we'd have something like:
>> if (indio_dev->available_scan_masks) {
>> 		mask = iio_scan_mask_match(indio_dev->available_scan_masks,
>> 					   indio_dev->masklength,
>> 					   trialmask, false);
>> 		if (!mask)
>> 			goto err_invalid_mask;
>> } else if (indio_dev->ops->scan_mask_available_match) {
>> 	mask =
>indio_dev->ops->scan_mask_available_match(indio_dev->masklength,
>> 							 trialmask);
>> 	if (!mask)
>> 		goto err_invalid_mask;
>> }
>> 
>> Bit of complexity needed to avoid any memory issues (probably use a
>local
>> buffer in the driver private struct to point mask at once filled),
>> 
>> Scan mask available could then compute the minimum possible scan mask
>for the
>> device in question given the requirements of trialmask.
>> 
>> An easy change that effectively provides a dynamic equivalent of our
>constant
>> list of available scan masks.  I'd advocate only using it when the
>> device complexity make it worthwhile (much like dynamic allocation of
>channel
>> specs currently) - so not many cases.
>> 
>>>
>>> I think a better solution would be to add a .get_buffer_offsets
>driver
>>> function and have the iio core handled demuxing based on offsets
>rather
>>> than just scan_mask.
>> Scan mask isn't about just this.   A common case is not that you have
>> to enable additional channels to end up with a valid set of readings
>> but rather that you can only read a (complex) subset of channels at
>> a time in buffered mode.  The simplest being onehot, but there are
>> quite a few other cases (two parallel ADCs each of which is muxed to
>a
>> subset of the channels via slow muxes for example).
>> 
>>>
>>> This would also handle alignment for external channels with
>storagebits
>>> != 16. For example if I enable accel and an external u32 channel iio
>>> expects the following layout:
>>>     <u16 ACCEL_X> <u16 ACCEL_Y> <u16 ACCEL_Z> <u16 PAD> <u32
>external>
>>> while my device gives the following:
>>>     <u16 ACCEL_X> <u16 ACCEL_Y> <u16 ACCEL_Z> <u32 external>
>>> I could add memcpy mangling in my driver but handling this belongs
>in
>>> the iio core.
>> Matter of opinion.  I'd take this as a driver requirement - much like
>the
>> common case of data that is packed tighter than this or irritatingly
>> interleaved.
>> 
>> <u4 ACCEL_X_11_8> <u4 ACCEL_Y_11_8> <u8 ACCEL_X_7_0> <u8 ACCEL_Y_7_0>
>is
>> not uncommon.
>> 
>> Where do we draw the line between what should be in the core and in a
>> driver?  I'm not against seeing a proposal for byte based packing
>unwinding
>> in the core, but I'm not convinced the added complexity (see other
>email
>> about the endian mess) is worth it without seeing code.
>> 
>>>
>>> I'd rather avoid depending on new features in the iio core and just
>do
>>> simple validations instead. This is because the feature I'm adding
>is
>>> already complex.
>>>
>> I think your earlier suggestion that you dismissed is a trivial
>extension
>> of the core and provides everything you need - so that's the route
>> that I'd advocate.
>> 
>Implementing some sort of "scan_mask_available_match" won't deal with
>alignment issues for external channels so it only partially fixes my
>current problem. Maybe for some other driver?
>
>If you object to validations inside preenable it would be best to just
>implement manual byte mangling inside the driver. I'd split it in two:
>    inv_mpu_get_scan_offsets();
>    iio_format_aligned_sample(&scan_offsets);
>
>The second part would be sortof generic but implemented statically
>inside the driver. Since this would implicitly skip unneeded values
>from
>hardware it will support arbitrary scan masks.

This would be a great first step. Will let us get a handle on how it all works.

Can always shift this put of the driver later if it makes sense.

Will be interesting to see what the code looks like.

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

Re: [RFC 5/7] iio: inv_mpu6050: Add support for auxiliary I2C master

[Crestez Dan Leonard]

On 05/01/2016 08:27 PM, Jonathan Cameron wrote:
> On 29/04/16 20:02, Crestez Dan Leonard wrote:
>> --- a/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt
>> +++ b/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt
>> @@ -1,16 +1,27 @@
>>  InvenSense MPU-6050 Six-Axis (Gyro + Accelerometer) MEMS MotionTracking Device
>>  
>> -http://www.invensense.com/mems/gyro/mpu6050.html
> If this is invalid, please add an up to date link if possible.
>> -
>>  Required properties:
>> - - compatible : should be "invensense,mpu6050"
>> - - reg : the I2C address of the sensor
>> + - compatible : should be "invensense,mpuXXXX"
> List them all explicitly here rather than wild cards.
>
But the list is a bit long. I'll just write "see below for valid
compatible strings".

>> + - reg : the I2C or SPI address of the sensor
>>   - interrupt-parent : should be the phandle for the interrupt controller
>>   - interrupts : interrupt mapping for GPIO IRQ
>>  
>>  Optional properties:
>>   - mount-matrix: an optional 3x3 mounting rotation matrix
>> + - inv,i2c-aux-master: operate aux i2c in "master mode" (default is mux).
>> +
>> +Valid compatible strings:
> Vendor prefix? These will work for historical reasons, but now vendor
> prefix should definitely be there as well.
>> + - mpu6000
>> + - mpu6050
>> + - mpu6500
>> + - mpu9150
>
The driver currently only lists i2c_device_id and this will work
ignoring the vendor string. I can prefix all these valid strings with
the vendor prefix but this is not actually a requirement. That would
require a separate unrelated patch adding of_device_id tables.

>> +	/*
>> +	 * Regmap will never ignore writes but it will ignore full-register
>> +	 * updates to the same value.
> Hmm. I'd missed this distinction.  Feels decidely 'interesting'... and makes
> my earlier suggestion invalid as I guess the fields stuff uses update bits
> internally.
> 
I will replace this with if (read() != addr) write(addr) to clarify.
Mentioning a regmap implementation quirk this way is silly.

>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>> index bd2c0fd..9d15633 100644
>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>> @@ -42,6 +42,13 @@
>>   *  @int_pin_cfg;	Controls interrupt pin configuration.
>>   *  @accl_offset:	Controls the accelerometer calibration offset.
>>   *  @gyro_offset:	Controls the gyroscope calibration offset.
>> + *  @mst_status:	secondary I2C master interrupt source status
>> + *  @slv4_addr:		I2C slave address for slave 4 transaction
>> + *  @slv4_reg:		I2C register used with slave 4 transaction
>> + *  @slv4_di:		I2C data in register for slave 4 transaction
>> + *  @slv4_ctrl:		I2C slave 4 control register
>> + *  @slv4_do:		I2C data out register for slave 4 transaction
>> +
I forgot to ask about this but this patch adds registers addresses to
struct inv_mpu6050_reg_map and not others. All the supported models have
the same registers for this functionality.

It seems to me that the simplest approach to supporting multiple models
is to only put the registers that vary in a model struct and use
constants for the rest. Is this the correct approach? If so I will use
constants for SLV4_* in the next patch.

It's not clear that adding this kind of indirection for everything is
useful for supporting new models. Different models can also move bits
around, not just registers.

-- 
Regards,
Leonard

Re: [RFC 5/7] iio: inv_mpu6050: Add support for auxiliary I2C master

[Rob Herring]

On Thu, May 5, 2016 at 7:38 AM, Crestez Dan Leonard
<leonard.crestez@intel.com> wrote:
> On 05/01/2016 08:27 PM, Jonathan Cameron wrote:
>> On 29/04/16 20:02, Crestez Dan Leonard wrote:
>>> --- a/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt
>>> +++ b/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt
>>> @@ -1,16 +1,27 @@
>>>  InvenSense MPU-6050 Six-Axis (Gyro + Accelerometer) MEMS MotionTracking Device
>>>
>>> -http://www.invensense.com/mems/gyro/mpu6050.html
>> If this is invalid, please add an up to date link if possible.
>>> -
>>>  Required properties:
>>> - - compatible : should be "invensense,mpu6050"
>>> - - reg : the I2C address of the sensor
>>> + - compatible : should be "invensense,mpuXXXX"
>> List them all explicitly here rather than wild cards.
>>
> But the list is a bit long. I'll just write "see below for valid
> compatible strings".

No, please list here. 4 is not long. A note of which ones are SPI
would be good too.

Can you add 9250 as well as it is commonly available for maker h/w.

>
>>> + - reg : the I2C or SPI address of the sensor
>>>   - interrupt-parent : should be the phandle for the interrupt controller
>>>   - interrupts : interrupt mapping for GPIO IRQ
>>>
>>>  Optional properties:
>>>   - mount-matrix: an optional 3x3 mounting rotation matrix
>>> + - inv,i2c-aux-master: operate aux i2c in "master mode" (default is mux).
>>> +
>>> +Valid compatible strings:
>> Vendor prefix? These will work for historical reasons, but now vendor
>> prefix should definitely be there as well.
>>> + - mpu6000
>>> + - mpu6050
>>> + - mpu6500
>>> + - mpu9150
>>
> The driver currently only lists i2c_device_id and this will work
> ignoring the vendor string. I can prefix all these valid strings with
> the vendor prefix but this is not actually a requirement. That would
> require a separate unrelated patch adding of_device_id tables.

What the driver happens to do is irrelevant to the binding. From a
binding standpoint, the vendor prefix is always required.

Rob