Re: [PATCH] iio: inv_mpu6050: Add support for auxiliary I2C master

[Peter Rosin]

On 2016-04-29 11:29, Peter Rosin wrote:

> On 2016-04-28 12:39, Crestez Dan Leonard wrote:
>> On 04/27/2016 11:39 AM, Peter Rosin wrote:
>>> On 2016-04-23 23:32, Jonathan Cameron wrote:
 On 20/04/16 18:17, 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 
 This one needs acks from:

 Device tree maintainer (odd binding ;)
 Peter Rosin (odd binding interacting with the mux support)
 Wolfram (it has a whole i2c master driver in here).

 (just thought I'd list these for the avoidance of doubt).
>>> I spot some overlap with the questions in "[RFC] i2c: device-tree:
>>> Handling child nodes which are not i2c devices"
>>> http://marc.info/?l=linux-i2c=146073452819116=2
>>>
>>> And I think I agree with Stephen Warren that an intermediate placeholder
>>> node would make sense. I.e.
>>>
>>> mpu6050@68 {
>>> compatible = "...";
>>> reg = <0x68>;
>>> ...
>>> i2c-aux-mux {
>>> i2c@0 {
>>> #address-cells = <1>;
>>> #size-cells = <0>;
>>> reg = <0>;
>>>
>>> foo@44 {
>>> compatible = "bar";
>>> reg = <0x44>;
>>> ...
>>> }
>>> }
>>> }
>>> }
>>>
>>> Or
>>>
>>> mpu6050@68 {
>>> compatible = "...";
>>> reg = <0x68>;
>>> ...
>>> i2c-aux-master {
>>> #address-cells = <1>;
>>> #size-cells = <0>;
>>>
>>> gazonk@44 {
>>> compatible = "baz";
>>> reg = <0x44>;
>>> ...
>>> }
>>> }
>>> }
>>>
>>> depending on if you want an aux-mux or an aux-master.
>>>
>>> But I don't know if that intermediate i2c-aux-mux node causes any
>>> problems?
>> It's not clear how that would be implemented. It seems to me that right
>> now i2c_add_mux_adapter assumes that the parent device is a dedicated
>> mux device and all it's children are mux branches. Would this require
>> introducing a new "struct device" for the i2c-aux-master node?
>>
>> It might make sense to make the automatic processing of the parents
>> node's of_node optional and let the caller assign the of_node describing
>> the attached devices.
>>
>> I think the most natural solution would be to require child nodes named
>> i2c-aux-mux and i2c-aux-master to describe aux devices. For backwards
>> compatibility it would be easiest to go with i2c@0/i2c@1 (identified by
>> reg=0/1).
>>
>> But I don't know much about devicetree and I'd rather accept an external
>> suggestion.
>>
> I was thinking that with the new i2c_mux_core in place, it should be pretty 
> simple
> to add a hook to point to another node and only use dev->of_node as a default
> value for where to look for the mux child adapters?
>
Or maybe always look for an intermediate "i2c-mux" node and look there if it 
exists? Something like this (totally untested) on top of the i2c-mux-core 
cleanup already in next (should be easy to adapt to 4.5 if you want that). 
Cheers, Peter

diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index 25e9336b0e6e..ff1374f5b4f6 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -179,10 +179,15 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
 * nothing if !CONFIG_OF.
 */
if (muxc->dev->of_node) {
+   struct device_node *mux;
struct device_node *child;
u32 reg;
 
-   for_each_child_of_node(muxc->dev->of_node, child) {
+   mux = of_get_child_by_name(muxc->dev->of_node, "i2c-mux");
+   if (!mux)
+   mux = muxc->dev->of_node;
+
+   for_each_child_of_node(mux, child) {
ret = of_property_read_u32(child, "reg", );
if (ret)
continue;

Re: [PATCH] iio: inv_mpu6050: Add support for auxiliary I2C master

[Peter Rosin]

On 2016-04-29 11:29, Peter Rosin wrote:

> On 2016-04-28 12:39, Crestez Dan Leonard wrote:
>> On 04/27/2016 11:39 AM, Peter Rosin wrote:
>>> On 2016-04-23 23:32, Jonathan Cameron wrote:
 On 20/04/16 18:17, 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 
 This one needs acks from:

 Device tree maintainer (odd binding ;)
 Peter Rosin (odd binding interacting with the mux support)
 Wolfram (it has a whole i2c master driver in here).

 (just thought I'd list these for the avoidance of doubt).
>>> I spot some overlap with the questions in "[RFC] i2c: device-tree:
>>> Handling child nodes which are not i2c devices"
>>> http://marc.info/?l=linux-i2c=146073452819116=2
>>>
>>> And I think I agree with Stephen Warren that an intermediate placeholder
>>> node would make sense. I.e.
>>>
>>> mpu6050@68 {
>>> compatible = "...";
>>> reg = <0x68>;
>>> ...
>>> i2c-aux-mux {
>>> i2c@0 {
>>> #address-cells = <1>;
>>> #size-cells = <0>;
>>> reg = <0>;
>>>
>>> foo@44 {
>>> compatible = "bar";
>>> reg = <0x44>;
>>> ...
>>> }
>>> }
>>> }
>>> }
>>>
>>> Or
>>>
>>> mpu6050@68 {
>>> compatible = "...";
>>> reg = <0x68>;
>>> ...
>>> i2c-aux-master {
>>> #address-cells = <1>;
>>> #size-cells = <0>;
>>>
>>> gazonk@44 {
>>> compatible = "baz";
>>> reg = <0x44>;
>>> ...
>>> }
>>> }
>>> }
>>>
>>> depending on if you want an aux-mux or an aux-master.
>>>
>>> But I don't know if that intermediate i2c-aux-mux node causes any
>>> problems?
>> It's not clear how that would be implemented. It seems to me that right
>> now i2c_add_mux_adapter assumes that the parent device is a dedicated
>> mux device and all it's children are mux branches. Would this require
>> introducing a new "struct device" for the i2c-aux-master node?
>>
>> It might make sense to make the automatic processing of the parents
>> node's of_node optional and let the caller assign the of_node describing
>> the attached devices.
>>
>> I think the most natural solution would be to require child nodes named
>> i2c-aux-mux and i2c-aux-master to describe aux devices. For backwards
>> compatibility it would be easiest to go with i2c@0/i2c@1 (identified by
>> reg=0/1).
>>
>> But I don't know much about devicetree and I'd rather accept an external
>> suggestion.
>>
> I was thinking that with the new i2c_mux_core in place, it should be pretty 
> simple
> to add a hook to point to another node and only use dev->of_node as a default
> value for where to look for the mux child adapters?
>
Or maybe always look for an intermediate "i2c-mux" node and look there if it 
exists? Something like this (totally untested) on top of the i2c-mux-core 
cleanup already in next (should be easy to adapt to 4.5 if you want that). 
Cheers, Peter

diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index 25e9336b0e6e..ff1374f5b4f6 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -179,10 +179,15 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
 * nothing if !CONFIG_OF.
 */
if (muxc->dev->of_node) {
+   struct device_node *mux;
struct device_node *child;
u32 reg;
 
-   for_each_child_of_node(muxc->dev->of_node, child) {
+   mux = of_get_child_by_name(muxc->dev->of_node, "i2c-mux");
+   if (!mux)
+   mux = muxc->dev->of_node;
+
+   for_each_child_of_node(mux, child) {
ret = of_property_read_u32(child, "reg", );
if (ret)
continue;

Re: [PATCH] iio: inv_mpu6050: Add support for auxiliary I2C master

[Peter Rosin]

On 2016-04-28 12:39, Crestez Dan Leonard wrote:
> On 04/27/2016 11:39 AM, Peter Rosin wrote:
>> On 2016-04-23 23:32, Jonathan Cameron wrote:
>>> On 20/04/16 18:17, 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 
>>> This one needs acks from:
>>>
>>> Device tree maintainer (odd binding ;)
>>> Peter Rosin (odd binding interacting with the mux support)
>>> Wolfram (it has a whole i2c master driver in here).
>>>
>>> (just thought I'd list these for the avoidance of doubt).
>> I spot some overlap with the questions in "[RFC] i2c: device-tree:
>> Handling child nodes which are not i2c devices"
>> http://marc.info/?l=linux-i2c=146073452819116=2
>>
>> And I think I agree with Stephen Warren that an intermediate placeholder
>> node would make sense. I.e.
>>
>> mpu6050@68 {
>> compatible = "...";
>> reg = <0x68>;
>> ...
>> i2c-aux-mux {
>> i2c@0 {
>> #address-cells = <1>;
>> #size-cells = <0>;
>> reg = <0>;
>>
>> foo@44 {
>> compatible = "bar";
>> reg = <0x44>;
>> ...
>> }
>> }
>> }
>> }
>>
>> Or
>>
>> mpu6050@68 {
>> compatible = "...";
>> reg = <0x68>;
>> ...
>> i2c-aux-master {
>> #address-cells = <1>;
>> #size-cells = <0>;
>>
>> gazonk@44 {
>> compatible = "baz";
>> reg = <0x44>;
>> ...
>> }
>> }
>> }
>>
>> depending on if you want an aux-mux or an aux-master.
>>
>> But I don't know if that intermediate i2c-aux-mux node causes any
>> problems?
> It's not clear how that would be implemented. It seems to me that right
> now i2c_add_mux_adapter assumes that the parent device is a dedicated
> mux device and all it's children are mux branches. Would this require
> introducing a new "struct device" for the i2c-aux-master node?
>
> It might make sense to make the automatic processing of the parents
> node's of_node optional and let the caller assign the of_node describing
> the attached devices.
>
> I think the most natural solution would be to require child nodes named
> i2c-aux-mux and i2c-aux-master to describe aux devices. For backwards
> compatibility it would be easiest to go with i2c@0/i2c@1 (identified by
> reg=0/1).
>
> But I don't know much about devicetree and I'd rather accept an external
> suggestion.
>
I was thinking that with the new i2c_mux_core in place, it should be pretty 
simple
to add a hook to point to another node and only use dev->of_node as a default
value for where to look for the mux child adapters?

Cheers,
Peter

Re: [PATCH] iio: inv_mpu6050: Add support for auxiliary I2C master

[Peter Rosin]

On 2016-04-28 12:39, Crestez Dan Leonard wrote:
> On 04/27/2016 11:39 AM, Peter Rosin wrote:
>> On 2016-04-23 23:32, Jonathan Cameron wrote:
>>> On 20/04/16 18:17, 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 
>>> This one needs acks from:
>>>
>>> Device tree maintainer (odd binding ;)
>>> Peter Rosin (odd binding interacting with the mux support)
>>> Wolfram (it has a whole i2c master driver in here).
>>>
>>> (just thought I'd list these for the avoidance of doubt).
>> I spot some overlap with the questions in "[RFC] i2c: device-tree:
>> Handling child nodes which are not i2c devices"
>> http://marc.info/?l=linux-i2c=146073452819116=2
>>
>> And I think I agree with Stephen Warren that an intermediate placeholder
>> node would make sense. I.e.
>>
>> mpu6050@68 {
>> compatible = "...";
>> reg = <0x68>;
>> ...
>> i2c-aux-mux {
>> i2c@0 {
>> #address-cells = <1>;
>> #size-cells = <0>;
>> reg = <0>;
>>
>> foo@44 {
>> compatible = "bar";
>> reg = <0x44>;
>> ...
>> }
>> }
>> }
>> }
>>
>> Or
>>
>> mpu6050@68 {
>> compatible = "...";
>> reg = <0x68>;
>> ...
>> i2c-aux-master {
>> #address-cells = <1>;
>> #size-cells = <0>;
>>
>> gazonk@44 {
>> compatible = "baz";
>> reg = <0x44>;
>> ...
>> }
>> }
>> }
>>
>> depending on if you want an aux-mux or an aux-master.
>>
>> But I don't know if that intermediate i2c-aux-mux node causes any
>> problems?
> It's not clear how that would be implemented. It seems to me that right
> now i2c_add_mux_adapter assumes that the parent device is a dedicated
> mux device and all it's children are mux branches. Would this require
> introducing a new "struct device" for the i2c-aux-master node?
>
> It might make sense to make the automatic processing of the parents
> node's of_node optional and let the caller assign the of_node describing
> the attached devices.
>
> I think the most natural solution would be to require child nodes named
> i2c-aux-mux and i2c-aux-master to describe aux devices. For backwards
> compatibility it would be easiest to go with i2c@0/i2c@1 (identified by
> reg=0/1).
>
> But I don't know much about devicetree and I'd rather accept an external
> suggestion.
>
I was thinking that with the new i2c_mux_core in place, it should be pretty 
simple
to add a hook to point to another node and only use dev->of_node as a default
value for where to look for the mux child adapters?

Cheers,
Peter

Re: [PATCH] iio: inv_mpu6050: Add support for auxiliary I2C master

[Crestez Dan Leonard]

On 04/27/2016 11:39 AM, Peter Rosin wrote:
> On 2016-04-23 23:32, Jonathan Cameron wrote:
>> On 20/04/16 18:17, 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 
>> This one needs acks from:
>>
>> Device tree maintainer (odd binding ;)
>> Peter Rosin (odd binding interacting with the mux support)
>> Wolfram (it has a whole i2c master driver in here).
>>
>> (just thought I'd list these for the avoidance of doubt).
> 
> I spot some overlap with the questions in "[RFC] i2c: device-tree:
> Handling child nodes which are not i2c devices"
> http://marc.info/?l=linux-i2c=146073452819116=2
> 
> And I think I agree with Stephen Warren that an intermediate placeholder
> node would make sense. I.e.
> 
> mpu6050@68 {
> compatible = "...";
> reg = <0x68>;
> ...
> i2c-aux-mux {
> i2c@0 {
> #address-cells = <1>;
> #size-cells = <0>;
> reg = <0>;
> 
> foo@44 {
> compatible = "bar";
> reg = <0x44>;
> ...
> }
> }
> }
> }
> 
> Or
> 
> mpu6050@68 {
> compatible = "...";
> reg = <0x68>;
> ...
> i2c-aux-master {
> #address-cells = <1>;
> #size-cells = <0>;
> 
> gazonk@44 {
> compatible = "baz";
> reg = <0x44>;
> ...
> }
> }
> }
> 
> depending on if you want an aux-mux or an aux-master.
> 
> But I don't know if that intermediate i2c-aux-mux node causes any
> problems?

It's not clear how that would be implemented. It seems to me that right
now i2c_add_mux_adapter assumes that the parent device is a dedicated
mux device and all it's children are mux branches. Would this require
introducing a new "struct device" for the i2c-aux-master node?

It might make sense to make the automatic processing of the parents
node's of_node optional and let the caller assign the of_node describing
the attached devices.

I think the most natural solution would be to require child nodes named
i2c-aux-mux and i2c-aux-master to describe aux devices. For backwards
compatibility it would be easiest to go with i2c@0/i2c@1 (identified by
reg=0/1).

But I don't know much about devicetree and I'd rather accept an external
suggestion.

Regards,
Leonard

Re: [PATCH] iio: inv_mpu6050: Add support for auxiliary I2C master

[Crestez Dan Leonard]

On 04/27/2016 11:39 AM, Peter Rosin wrote:
> On 2016-04-23 23:32, Jonathan Cameron wrote:
>> On 20/04/16 18:17, 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 
>> This one needs acks from:
>>
>> Device tree maintainer (odd binding ;)
>> Peter Rosin (odd binding interacting with the mux support)
>> Wolfram (it has a whole i2c master driver in here).
>>
>> (just thought I'd list these for the avoidance of doubt).
> 
> I spot some overlap with the questions in "[RFC] i2c: device-tree:
> Handling child nodes which are not i2c devices"
> http://marc.info/?l=linux-i2c=146073452819116=2
> 
> And I think I agree with Stephen Warren that an intermediate placeholder
> node would make sense. I.e.
> 
> mpu6050@68 {
> compatible = "...";
> reg = <0x68>;
> ...
> i2c-aux-mux {
> i2c@0 {
> #address-cells = <1>;
> #size-cells = <0>;
> reg = <0>;
> 
> foo@44 {
> compatible = "bar";
> reg = <0x44>;
> ...
> }
> }
> }
> }
> 
> Or
> 
> mpu6050@68 {
> compatible = "...";
> reg = <0x68>;
> ...
> i2c-aux-master {
> #address-cells = <1>;
> #size-cells = <0>;
> 
> gazonk@44 {
> compatible = "baz";
> reg = <0x44>;
> ...
> }
> }
> }
> 
> depending on if you want an aux-mux or an aux-master.
> 
> But I don't know if that intermediate i2c-aux-mux node causes any
> problems?

It's not clear how that would be implemented. It seems to me that right
now i2c_add_mux_adapter assumes that the parent device is a dedicated
mux device and all it's children are mux branches. Would this require
introducing a new "struct device" for the i2c-aux-master node?

It might make sense to make the automatic processing of the parents
node's of_node optional and let the caller assign the of_node describing
the attached devices.

I think the most natural solution would be to require child nodes named
i2c-aux-mux and i2c-aux-master to describe aux devices. For backwards
compatibility it would be easiest to go with i2c@0/i2c@1 (identified by
reg=0/1).

But I don't know much about devicetree and I'd rather accept an external
suggestion.

Regards,
Leonard

Re: [PATCH] iio: inv_mpu6050: Add support for auxiliary I2C master

[Peter Rosin]

Hi!

On 2016-04-23 23:32, Jonathan Cameron wrote:
> On 20/04/16 18:17, 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 
> This one needs acks from:
>
> Device tree maintainer (odd binding ;)
> Peter Rosin (odd binding interacting with the mux support)
> Wolfram (it has a whole i2c master driver in here).
>
> (just thought I'd list these for the avoidance of doubt).

I spot some overlap with the questions in "[RFC] i2c: device-tree:
Handling child nodes which are not i2c devices"
http://marc.info/?l=linux-i2c=146073452819116=2

And I think I agree with Stephen Warren that an intermediate placeholder
node would make sense. I.e.

mpu6050@68 {
compatible = "...";
reg = <0x68>;
...
i2c-aux-mux {
i2c@0 {
#address-cells = <1>;
#size-cells = <0>;
reg = <0>;

foo@44 {
compatible = "bar";
reg = <0x44>;
...
}
}
}
}

Or

mpu6050@68 {
compatible = "...";
reg = <0x68>;
...
i2c-aux-master {
#address-cells = <1>;
#size-cells = <0>;

gazonk@44 {
compatible = "baz";
reg = <0x44>;
...
}
}
}

depending on if you want an aux-mux or an aux-master.

But I don't know if that intermediate i2c-aux-mux node causes any
problems?

Cheers,
Peter

Re: [PATCH] iio: inv_mpu6050: Add support for auxiliary I2C master

[Peter Rosin]

Hi!

On 2016-04-23 23:32, Jonathan Cameron wrote:
> On 20/04/16 18:17, 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 
> This one needs acks from:
>
> Device tree maintainer (odd binding ;)
> Peter Rosin (odd binding interacting with the mux support)
> Wolfram (it has a whole i2c master driver in here).
>
> (just thought I'd list these for the avoidance of doubt).

I spot some overlap with the questions in "[RFC] i2c: device-tree:
Handling child nodes which are not i2c devices"
http://marc.info/?l=linux-i2c=146073452819116=2

And I think I agree with Stephen Warren that an intermediate placeholder
node would make sense. I.e.

mpu6050@68 {
compatible = "...";
reg = <0x68>;
...
i2c-aux-mux {
i2c@0 {
#address-cells = <1>;
#size-cells = <0>;
reg = <0>;

foo@44 {
compatible = "bar";
reg = <0x44>;
...
}
}
}
}

Or

mpu6050@68 {
compatible = "...";
reg = <0x68>;
...
i2c-aux-master {
#address-cells = <1>;
#size-cells = <0>;

gazonk@44 {
compatible = "baz";
reg = <0x44>;
...
}
}
}

depending on if you want an aux-mux or an aux-master.

But I don't know if that intermediate i2c-aux-mux node causes any
problems?

Cheers,
Peter

Re: [PATCH] iio: inv_mpu6050: Add support for auxiliary I2C master

[Jonathan Cameron]

On 20/04/16 18:17, 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 
This one needs acks from:

Device tree maintainer (odd binding ;)
Peter Rosin (odd binding interacting with the mux support)
Wolfram (it has a whole i2c master driver in here).

(just thought I'd list these for the avoidance of doubt).

Good feature to have, so hopefully won't prove 'too hard'.

My comments are only really about function naming so far...

Jonathan
> ---
> 
> This is based on earlier work by Daniel Baluta :
> https://www.spinics.net/lists/linux-iio/msg23573.html
> 
> Changes since that version:
> * Nest the adapter in inv_mpu6050_state instead of making it static
> * Explicitly forward of_node "i2c-aux-master" to allow describing aux devices
> via devicetree.
> 
> For bypass/mux mode devicetree works automatically. The forwarding is based on
> the "chan_id" parameter to i2c_add_mux_adapter and is implemented here:
> 
> http://lxr.free-electrons.com/source/drivers/i2c/i2c-mux.c#L158
> 
> Perhaps it might be better for devices handled via master mode to be described
> via i2c@1? This would work by scanning the mpu node's children for something
> with reg == 1.
> 
> Or maybe the two busses should be called i2c-aux-master and i2c-aux-mux? Not
> sure how to deal with that on the mux side.
> 
> It is not clear how to properly handle this and suggestions are welcome. The
> way it currently works with this patch is documented immediately below.
> 
>  .../devicetree/bindings/iio/imu/inv_mpu6050.txt|  59 +++-
>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 152 
> -
>  drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h  |  41 ++
>  drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c  |   8 --
>  4 files changed, 247 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt 
> b/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt
> index e4d8f1c..9d842f2 100644
> --- a/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt
> +++ b/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt
> @@ -1,13 +1,24 @@
>  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,mpu"
> + - 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
>  
> +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 describes 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-aux-master.
> +
>  Example:
>   mpu6050@68 {
>   compatible = "invensense,mpu6050";
> @@ -15,3 +26,43 @@ Example:
>   interrupt-parent = <>;
>   interrupts = <18 1>;
>   };
> +
> +Example describing mpu9150 (which includes an ak9875 on chip):
> + mpu9150@68 {
> + compatible = "invensense,mpu9150";
> + reg = <0x68>;
> + interrupt-parent = <>;
> + 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 = <100>;
> + interrupt-parent = <>;
> + interrupts = <31 1>;
> +
> + 

Re: [PATCH] iio: inv_mpu6050: Add support for auxiliary I2C master

[Jonathan Cameron]

On 20/04/16 18:17, 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 
This one needs acks from:

Device tree maintainer (odd binding ;)
Peter Rosin (odd binding interacting with the mux support)
Wolfram (it has a whole i2c master driver in here).

(just thought I'd list these for the avoidance of doubt).

Good feature to have, so hopefully won't prove 'too hard'.

My comments are only really about function naming so far...

Jonathan
> ---
> 
> This is based on earlier work by Daniel Baluta :
> https://www.spinics.net/lists/linux-iio/msg23573.html
> 
> Changes since that version:
> * Nest the adapter in inv_mpu6050_state instead of making it static
> * Explicitly forward of_node "i2c-aux-master" to allow describing aux devices
> via devicetree.
> 
> For bypass/mux mode devicetree works automatically. The forwarding is based on
> the "chan_id" parameter to i2c_add_mux_adapter and is implemented here:
> 
> http://lxr.free-electrons.com/source/drivers/i2c/i2c-mux.c#L158
> 
> Perhaps it might be better for devices handled via master mode to be described
> via i2c@1? This would work by scanning the mpu node's children for something
> with reg == 1.
> 
> Or maybe the two busses should be called i2c-aux-master and i2c-aux-mux? Not
> sure how to deal with that on the mux side.
> 
> It is not clear how to properly handle this and suggestions are welcome. The
> way it currently works with this patch is documented immediately below.
> 
>  .../devicetree/bindings/iio/imu/inv_mpu6050.txt|  59 +++-
>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 152 
> -
>  drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h  |  41 ++
>  drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c  |   8 --
>  4 files changed, 247 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt 
> b/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt
> index e4d8f1c..9d842f2 100644
> --- a/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt
> +++ b/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt
> @@ -1,13 +1,24 @@
>  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,mpu"
> + - 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
>  
> +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 describes 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-aux-master.
> +
>  Example:
>   mpu6050@68 {
>   compatible = "invensense,mpu6050";
> @@ -15,3 +26,43 @@ Example:
>   interrupt-parent = <>;
>   interrupts = <18 1>;
>   };
> +
> +Example describing mpu9150 (which includes an ak9875 on chip):
> + mpu9150@68 {
> + compatible = "invensense,mpu9150";
> + reg = <0x68>;
> + interrupt-parent = <>;
> + 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 = <100>;
> + interrupt-parent = <>;
> + interrupts = <31 1>;
> +
> + i2c-aux-master {
> + #address-cells = <1>;
> 

Re: [PATCH] iio: inv_mpu6050: Add support for auxiliary I2C master

[Peter Rosin]

Crestez Dan Leonard wrote:
> On 04/21/2016 04:56 PM, Peter Rosin wrote:
> > Crestez Dan Leonard wrote:
> >> On 04/20/2016 11:31 PM, Peter Rosin wrote:
> >>> Crestez Dan Leonard wrote:
>  Changes since that version:
>  * Nest the adapter in inv_mpu6050_state instead of making it static
>  * Explicitly forward of_node "i2c-aux-master" to allow describing aux 
>  devices
>  via devicetree.
> 
>  For bypass/mux mode devicetree works automatically. The forwarding is 
>  based on
>  the "chan_id" parameter to i2c_add_mux_adapter and is implemented here:
> 
>  http://lxr.free-electrons.com/source/drivers/i2c/i2c-mux.c?v=4.5#L158
> 
>  Perhaps it might be better for devices handled via master mode to be 
>  described
>  via i2c@1? This would work by scanning the mpu node's children for 
>  something
>  with reg == 1.
> >>>
> >>> The 0 in i2c@0 (which is used by the mux mode) is the index of the mux 
> >>> slave
> >>> meaning that i2c@1 would be a second mux slave on the same mux, but this 
> >>> is
> >>> not a real mux as such, it is a gate which is piggybacking on the i2c mux 
> >>> infra.
> >>> So, this "mux" can't have a second slave which is why only 0 is valid.
> >>
> >> This behavior is automatic in i2c mux code and seems to assume that all
> >> the children of mux_dev are i2c muxes. This might be obviously correct
> >> and useful for dedicated i2c mux devices but in my case mux_dev is just
> >> the i2c_client for a sensor.
> >>
> >> From Documentation/devicetree/bindings/i2c/i2c-mux.txt:
> >>
> >>> An i2c bus multiplexer/switch will have several child busses that are
> >>> numbered uniquely in a device dependent manner.  The nodes for an i2c
> >>> bus multiplexer/switch will have one child node for each child bus.
> >>
> >> This seems to be written in a way that would allow me to define the
> >> "auxiliary i2c master" as bus "1". After all, the numbering is device
> >> dependent and it's not clear that all the child busses need to be
> >> accessible through muxing rather than indirect access through device
> >> registers.
> > 
> > You are correct that if you have devicetree children where reg does
> > not match the chan_id given to i2c_add_mux_adapter() those children
> > will be ignored by the i2c-mux code. So, the code would be happy with
> > a devicetree such as:
> > 
> > mpu6050@68 {
> > compatible = "...";
> > reg = <0x68>;
> > ...
> > i2c-aux-mux@0 {
> > #address-cells = <1>;
> > #size-cells = <0>;
> > reg = <0>;
> > 
> > foo@44 {
> > compatible = "bar";
> > reg = <0x44>;
> > ...
> > }
> > }
> > i2c-aux-master@1 {
> > #address-cells = <1>;
> > #size-cells = <0>;
> > reg = <1>;
> > 
> > gazonk@44 {
> > compatible = "baz";
> > reg = <0x44>;
> > ...
> > }
> > }
> > }
> > 
> > as long as you do only call i2c_add_mux_adapter() with chan_id = 0. And that
> > is what you are doing. But I think it is a bit subtle...
> 
> This kind of stuff needs to be written up in
> Documentation/devicetree/bindings anyway.

Yes, but it helps if similar things are done in similar ways. I simply
don't know devicetree stuff well enough to know how issues like this
are resolved elsewhere. So, I don't really know, but it just seemed
subtle.

> >>> I think the naming could be i2c-master0, i2c-master1 etc if it, with
> >>> future work, would be possible to add more than one master (you talked 
> >>> about
> >>> 5 i2c slaves..).
> >>
> >> The device has 5 sets of registers for controlling i2c slaves but only
> >> one physical auxiliary i2c bus. As far as I can tell slaves 0-3 are
> >> intended to be used for gathering readings for slaved sensors
> >> periodically without external intervention. Slave 4 can generate an
> >> interrupt on completion and is more suitable for general-purpose
> >> communication with any number of devices.
> > 
> > Ah, ok, so all 5 sets of slave registers are about the same physical i2c
> > bus. So, you basically cannot sanely use this physical aux i2c bus as an
> > i2c-mux and an extra i2c adapter in the same hw design. Correct?
> 
> You can access devices on the auxiliary i2c bus either through mux-ing
> or the adapter added by this patch. I think mux mode works better (lower
> latency) but is not available when the primary connection is via SPI.
> You can use both but it doesn't particularly make sense.

Right, it wouldn't make sense to use i2c-master mode for some devices
and i2c-mux mode for some devices, when all the those devices sit on the
same bus.

> > In that case, couldn't 

Re: [PATCH] iio: inv_mpu6050: Add support for auxiliary I2C master

[Peter Rosin]

Crestez Dan Leonard wrote:
> On 04/21/2016 04:56 PM, Peter Rosin wrote:
> > Crestez Dan Leonard wrote:
> >> On 04/20/2016 11:31 PM, Peter Rosin wrote:
> >>> Crestez Dan Leonard wrote:
>  Changes since that version:
>  * Nest the adapter in inv_mpu6050_state instead of making it static
>  * Explicitly forward of_node "i2c-aux-master" to allow describing aux 
>  devices
>  via devicetree.
> 
>  For bypass/mux mode devicetree works automatically. The forwarding is 
>  based on
>  the "chan_id" parameter to i2c_add_mux_adapter and is implemented here:
> 
>  http://lxr.free-electrons.com/source/drivers/i2c/i2c-mux.c?v=4.5#L158
> 
>  Perhaps it might be better for devices handled via master mode to be 
>  described
>  via i2c@1? This would work by scanning the mpu node's children for 
>  something
>  with reg == 1.
> >>>
> >>> The 0 in i2c@0 (which is used by the mux mode) is the index of the mux 
> >>> slave
> >>> meaning that i2c@1 would be a second mux slave on the same mux, but this 
> >>> is
> >>> not a real mux as such, it is a gate which is piggybacking on the i2c mux 
> >>> infra.
> >>> So, this "mux" can't have a second slave which is why only 0 is valid.
> >>
> >> This behavior is automatic in i2c mux code and seems to assume that all
> >> the children of mux_dev are i2c muxes. This might be obviously correct
> >> and useful for dedicated i2c mux devices but in my case mux_dev is just
> >> the i2c_client for a sensor.
> >>
> >> From Documentation/devicetree/bindings/i2c/i2c-mux.txt:
> >>
> >>> An i2c bus multiplexer/switch will have several child busses that are
> >>> numbered uniquely in a device dependent manner.  The nodes for an i2c
> >>> bus multiplexer/switch will have one child node for each child bus.
> >>
> >> This seems to be written in a way that would allow me to define the
> >> "auxiliary i2c master" as bus "1". After all, the numbering is device
> >> dependent and it's not clear that all the child busses need to be
> >> accessible through muxing rather than indirect access through device
> >> registers.
> > 
> > You are correct that if you have devicetree children where reg does
> > not match the chan_id given to i2c_add_mux_adapter() those children
> > will be ignored by the i2c-mux code. So, the code would be happy with
> > a devicetree such as:
> > 
> > mpu6050@68 {
> > compatible = "...";
> > reg = <0x68>;
> > ...
> > i2c-aux-mux@0 {
> > #address-cells = <1>;
> > #size-cells = <0>;
> > reg = <0>;
> > 
> > foo@44 {
> > compatible = "bar";
> > reg = <0x44>;
> > ...
> > }
> > }
> > i2c-aux-master@1 {
> > #address-cells = <1>;
> > #size-cells = <0>;
> > reg = <1>;
> > 
> > gazonk@44 {
> > compatible = "baz";
> > reg = <0x44>;
> > ...
> > }
> > }
> > }
> > 
> > as long as you do only call i2c_add_mux_adapter() with chan_id = 0. And that
> > is what you are doing. But I think it is a bit subtle...
> 
> This kind of stuff needs to be written up in
> Documentation/devicetree/bindings anyway.

Yes, but it helps if similar things are done in similar ways. I simply
don't know devicetree stuff well enough to know how issues like this
are resolved elsewhere. So, I don't really know, but it just seemed
subtle.

> >>> I think the naming could be i2c-master0, i2c-master1 etc if it, with
> >>> future work, would be possible to add more than one master (you talked 
> >>> about
> >>> 5 i2c slaves..).
> >>
> >> The device has 5 sets of registers for controlling i2c slaves but only
> >> one physical auxiliary i2c bus. As far as I can tell slaves 0-3 are
> >> intended to be used for gathering readings for slaved sensors
> >> periodically without external intervention. Slave 4 can generate an
> >> interrupt on completion and is more suitable for general-purpose
> >> communication with any number of devices.
> > 
> > Ah, ok, so all 5 sets of slave registers are about the same physical i2c
> > bus. So, you basically cannot sanely use this physical aux i2c bus as an
> > i2c-mux and an extra i2c adapter in the same hw design. Correct?
> 
> You can access devices on the auxiliary i2c bus either through mux-ing
> or the adapter added by this patch. I think mux mode works better (lower
> latency) but is not available when the primary connection is via SPI.
> You can use both but it doesn't particularly make sense.

Right, it wouldn't make sense to use i2c-master mode for some devices
and i2c-mux mode for some devices, when all the those devices sit on the
same bus.

> > In that case, couldn't 

Re: [PATCH] iio: inv_mpu6050: Add support for auxiliary I2C master

[Crestez Dan Leonard]

On 04/21/2016 04:56 PM, Peter Rosin wrote:
> Crestez Dan Leonard wrote:
>> On 04/20/2016 11:31 PM, Peter Rosin wrote:
>>> Crestez Dan Leonard wrote:
 Changes since that version:
 * Nest the adapter in inv_mpu6050_state instead of making it static
 * Explicitly forward of_node "i2c-aux-master" to allow describing aux 
 devices
 via devicetree.

 For bypass/mux mode devicetree works automatically. The forwarding is 
 based on
 the "chan_id" parameter to i2c_add_mux_adapter and is implemented here:

 http://lxr.free-electrons.com/source/drivers/i2c/i2c-mux.c?v=4.5#L158

 Perhaps it might be better for devices handled via master mode to be 
 described
 via i2c@1? This would work by scanning the mpu node's children for 
 something
 with reg == 1.
>>>
>>> The 0 in i2c@0 (which is used by the mux mode) is the index of the mux slave
>>> meaning that i2c@1 would be a second mux slave on the same mux, but this is
>>> not a real mux as such, it is a gate which is piggybacking on the i2c mux 
>>> infra.
>>> So, this "mux" can't have a second slave which is why only 0 is valid.
>>
>> This behavior is automatic in i2c mux code and seems to assume that all
>> the children of mux_dev are i2c muxes. This might be obviously correct
>> and useful for dedicated i2c mux devices but in my case mux_dev is just
>> the i2c_client for a sensor.
>>
>> From Documentation/devicetree/bindings/i2c/i2c-mux.txt:
>>
>>> An i2c bus multiplexer/switch will have several child busses that are
>>> numbered uniquely in a device dependent manner.  The nodes for an i2c
>>> bus multiplexer/switch will have one child node for each child bus.
>>
>> This seems to be written in a way that would allow me to define the
>> "auxiliary i2c master" as bus "1". After all, the numbering is device
>> dependent and it's not clear that all the child busses need to be
>> accessible through muxing rather than indirect access through device
>> registers.
> 
> You are correct that if you have devicetree children where reg does
> not match the chan_id given to i2c_add_mux_adapter() those children
> will be ignored by the i2c-mux code. So, the code would be happy with
> a devicetree such as:
> 
>   mpu6050@68 {
>   compatible = "...";
>   reg = <0x68>;
>   ...
>   i2c-aux-mux@0 {
>   #address-cells = <1>;
>   #size-cells = <0>;
>   reg = <0>;
> 
>   foo@44 {
>   compatible = "bar";
>   reg = <0x44>;
>   ...
>   }
>   }
>   i2c-aux-master@1 {
>   #address-cells = <1>;
>   #size-cells = <0>;
>   reg = <1>;
> 
>   gazonk@44 {
>   compatible = "baz";
>   reg = <0x44>;
>   ...
>   }
>   }
>   }
> 
> as long as you do only call i2c_add_mux_adapter() with chan_id = 0. And that
> is what you are doing. But I think it is a bit subtle...

This kind of stuff needs to be written up in
Documentation/devicetree/bindings anyway.

>>> I think the naming could be i2c-master0, i2c-master1 etc if it, with
>>> future work, would be possible to add more than one master (you talked about
>>> 5 i2c slaves..).
>>
>> The device has 5 sets of registers for controlling i2c slaves but only
>> one physical auxiliary i2c bus. As far as I can tell slaves 0-3 are
>> intended to be used for gathering readings for slaved sensors
>> periodically without external intervention. Slave 4 can generate an
>> interrupt on completion and is more suitable for general-purpose
>> communication with any number of devices.
> 
> Ah, ok, so all 5 sets of slave registers are about the same physical i2c
> bus. So, you basically cannot sanely use this physical aux i2c bus as an
> i2c-mux and an extra i2c adapter in the same hw design. Correct?

You can access devices on the auxiliary i2c bus either through mux-ing
or the adapter added by this patch. I think mux mode works better (lower
latency) but is not available when the primary connection is via SPI.
You can use both but it doesn't particularly make sense.

> In that case, couldn't you look at the names of any devicetree children
> and use that to decide if you should even attempt to call
> i2c_add_mux_adapter or i2c_add_adapter?

But the adapter should be added even if nothing is defined for it.
Registering i2c clients by echoing in new_device is a valid usecase.

What could be done is only register the i2c mux in i2c mode and the i2c
master in spi mode and make the bindings identical.

> (But please don't clobber stuff for my i2c-mux rework, or you will
> have to wait even longer for that deadlock to be resolved)

I won't. I should have sent this 

Re: [PATCH] iio: inv_mpu6050: Add support for auxiliary I2C master

[Crestez Dan Leonard]

On 04/21/2016 04:56 PM, Peter Rosin wrote:
> Crestez Dan Leonard wrote:
>> On 04/20/2016 11:31 PM, Peter Rosin wrote:
>>> Crestez Dan Leonard wrote:
 Changes since that version:
 * Nest the adapter in inv_mpu6050_state instead of making it static
 * Explicitly forward of_node "i2c-aux-master" to allow describing aux 
 devices
 via devicetree.

 For bypass/mux mode devicetree works automatically. The forwarding is 
 based on
 the "chan_id" parameter to i2c_add_mux_adapter and is implemented here:

 http://lxr.free-electrons.com/source/drivers/i2c/i2c-mux.c?v=4.5#L158

 Perhaps it might be better for devices handled via master mode to be 
 described
 via i2c@1? This would work by scanning the mpu node's children for 
 something
 with reg == 1.
>>>
>>> The 0 in i2c@0 (which is used by the mux mode) is the index of the mux slave
>>> meaning that i2c@1 would be a second mux slave on the same mux, but this is
>>> not a real mux as such, it is a gate which is piggybacking on the i2c mux 
>>> infra.
>>> So, this "mux" can't have a second slave which is why only 0 is valid.
>>
>> This behavior is automatic in i2c mux code and seems to assume that all
>> the children of mux_dev are i2c muxes. This might be obviously correct
>> and useful for dedicated i2c mux devices but in my case mux_dev is just
>> the i2c_client for a sensor.
>>
>> From Documentation/devicetree/bindings/i2c/i2c-mux.txt:
>>
>>> An i2c bus multiplexer/switch will have several child busses that are
>>> numbered uniquely in a device dependent manner.  The nodes for an i2c
>>> bus multiplexer/switch will have one child node for each child bus.
>>
>> This seems to be written in a way that would allow me to define the
>> "auxiliary i2c master" as bus "1". After all, the numbering is device
>> dependent and it's not clear that all the child busses need to be
>> accessible through muxing rather than indirect access through device
>> registers.
> 
> You are correct that if you have devicetree children where reg does
> not match the chan_id given to i2c_add_mux_adapter() those children
> will be ignored by the i2c-mux code. So, the code would be happy with
> a devicetree such as:
> 
>   mpu6050@68 {
>   compatible = "...";
>   reg = <0x68>;
>   ...
>   i2c-aux-mux@0 {
>   #address-cells = <1>;
>   #size-cells = <0>;
>   reg = <0>;
> 
>   foo@44 {
>   compatible = "bar";
>   reg = <0x44>;
>   ...
>   }
>   }
>   i2c-aux-master@1 {
>   #address-cells = <1>;
>   #size-cells = <0>;
>   reg = <1>;
> 
>   gazonk@44 {
>   compatible = "baz";
>   reg = <0x44>;
>   ...
>   }
>   }
>   }
> 
> as long as you do only call i2c_add_mux_adapter() with chan_id = 0. And that
> is what you are doing. But I think it is a bit subtle...

This kind of stuff needs to be written up in
Documentation/devicetree/bindings anyway.

>>> I think the naming could be i2c-master0, i2c-master1 etc if it, with
>>> future work, would be possible to add more than one master (you talked about
>>> 5 i2c slaves..).
>>
>> The device has 5 sets of registers for controlling i2c slaves but only
>> one physical auxiliary i2c bus. As far as I can tell slaves 0-3 are
>> intended to be used for gathering readings for slaved sensors
>> periodically without external intervention. Slave 4 can generate an
>> interrupt on completion and is more suitable for general-purpose
>> communication with any number of devices.
> 
> Ah, ok, so all 5 sets of slave registers are about the same physical i2c
> bus. So, you basically cannot sanely use this physical aux i2c bus as an
> i2c-mux and an extra i2c adapter in the same hw design. Correct?

You can access devices on the auxiliary i2c bus either through mux-ing
or the adapter added by this patch. I think mux mode works better (lower
latency) but is not available when the primary connection is via SPI.
You can use both but it doesn't particularly make sense.

> In that case, couldn't you look at the names of any devicetree children
> and use that to decide if you should even attempt to call
> i2c_add_mux_adapter or i2c_add_adapter?

But the adapter should be added even if nothing is defined for it.
Registering i2c clients by echoing in new_device is a valid usecase.

What could be done is only register the i2c mux in i2c mode and the i2c
master in spi mode and make the bindings identical.

> (But please don't clobber stuff for my i2c-mux rework, or you will
> have to wait even longer for that deadlock to be resolved)

I won't. I should have sent this 

Re: [PATCH] iio: inv_mpu6050: Add support for auxiliary I2C master

[Peter Rosin]

Crestez Dan Leonard wrote:
> On 04/20/2016 11:31 PM, Peter Rosin wrote:
> > Crestez Dan Leonard wrote:
> >> Changes since that version:
> >> * Nest the adapter in inv_mpu6050_state instead of making it static
> >> * Explicitly forward of_node "i2c-aux-master" to allow describing aux 
> >> devices
> >> via devicetree.
> >>
> >> For bypass/mux mode devicetree works automatically. The forwarding is 
> >> based on
> >> the "chan_id" parameter to i2c_add_mux_adapter and is implemented here:
> >>
> >> http://lxr.free-electrons.com/source/drivers/i2c/i2c-mux.c?v=4.5#L158
> >>
> >> Perhaps it might be better for devices handled via master mode to be 
> >> described
> >> via i2c@1? This would work by scanning the mpu node's children for 
> >> something
> >> with reg == 1.
> > 
> > The 0 in i2c@0 (which is used by the mux mode) is the index of the mux slave
> > meaning that i2c@1 would be a second mux slave on the same mux, but this is
> > not a real mux as such, it is a gate which is piggybacking on the i2c mux 
> > infra.
> > So, this "mux" can't have a second slave which is why only 0 is valid.
> 
> This behavior is automatic in i2c mux code and seems to assume that all
> the children of mux_dev are i2c muxes. This might be obviously correct
> and useful for dedicated i2c mux devices but in my case mux_dev is just
> the i2c_client for a sensor.
> 
> From Documentation/devicetree/bindings/i2c/i2c-mux.txt:
> 
> > An i2c bus multiplexer/switch will have several child busses that are
> > numbered uniquely in a device dependent manner.  The nodes for an i2c
> > bus multiplexer/switch will have one child node for each child bus.
> 
> This seems to be written in a way that would allow me to define the
> "auxiliary i2c master" as bus "1". After all, the numbering is device
> dependent and it's not clear that all the child busses need to be
> accessible through muxing rather than indirect access through device
> registers.

You are correct that if you have devicetree children where reg does
not match the chan_id given to i2c_add_mux_adapter() those children
will be ignored by the i2c-mux code. So, the code would be happy with
a devicetree such as:

mpu6050@68 {
compatible = "...";
reg = <0x68>;
...
i2c-aux-mux@0 {
#address-cells = <1>;
#size-cells = <0>;
reg = <0>;

foo@44 {
compatible = "bar";
reg = <0x44>;
...
}
}
i2c-aux-master@1 {
#address-cells = <1>;
#size-cells = <0>;
reg = <1>;

gazonk@44 {
compatible = "baz";
reg = <0x44>;
...
}
}
}

as long as you do only call i2c_add_mux_adapter() with chan_id = 0. And that
is what you are doing. But I think it is a bit subtle...

> >> Or maybe the two busses should be called i2c-aux-master and i2c-aux-mux? 
> >> Not
> >> sure how to deal with that on the mux side.
> > 
> > Changing i2c to i2c-aux-mux would break existing device trees, that seems
> > like a bad thing, no?
> 
> That support was not documented in mpu6050's bindings and might not be
> actually used.
> 
> >> It is not clear how to properly handle this and suggestions are welcome. 
> >> The
> >> way it currently works with this patch is documented immediately below.
> > 
> > I think the naming could be i2c-master0, i2c-master1 etc if it, with
> > future work, would be possible to add more than one master (you talked about
> > 5 i2c slaves..).
> 
> The device has 5 sets of registers for controlling i2c slaves but only
> one physical auxiliary i2c bus. As far as I can tell slaves 0-3 are
> intended to be used for gathering readings for slaved sensors
> periodically without external intervention. Slave 4 can generate an
> interrupt on completion and is more suitable for general-purpose
> communication with any number of devices.

Ah, ok, so all 5 sets of slave registers are about the same physical i2c
bus. So, you basically cannot sanely use this physical aux i2c bus as an
i2c-mux and an extra i2c adapter in the same hw design. Correct?

In that case, couldn't you look at the names of any devicetree children
and use that to decide if you should even attempt to call
i2c_add_mux_adapter or i2c_add_adapter?

(But please don't clobber stuff for my i2c-mux rework, or you will have
to wait even longer for that deadlock to be resolved)

Cheers,
Peter

Re: [PATCH] iio: inv_mpu6050: Add support for auxiliary I2C master

[Peter Rosin]

Crestez Dan Leonard wrote:
> On 04/20/2016 11:31 PM, Peter Rosin wrote:
> > Crestez Dan Leonard wrote:
> >> Changes since that version:
> >> * Nest the adapter in inv_mpu6050_state instead of making it static
> >> * Explicitly forward of_node "i2c-aux-master" to allow describing aux 
> >> devices
> >> via devicetree.
> >>
> >> For bypass/mux mode devicetree works automatically. The forwarding is 
> >> based on
> >> the "chan_id" parameter to i2c_add_mux_adapter and is implemented here:
> >>
> >> http://lxr.free-electrons.com/source/drivers/i2c/i2c-mux.c?v=4.5#L158
> >>
> >> Perhaps it might be better for devices handled via master mode to be 
> >> described
> >> via i2c@1? This would work by scanning the mpu node's children for 
> >> something
> >> with reg == 1.
> > 
> > The 0 in i2c@0 (which is used by the mux mode) is the index of the mux slave
> > meaning that i2c@1 would be a second mux slave on the same mux, but this is
> > not a real mux as such, it is a gate which is piggybacking on the i2c mux 
> > infra.
> > So, this "mux" can't have a second slave which is why only 0 is valid.
> 
> This behavior is automatic in i2c mux code and seems to assume that all
> the children of mux_dev are i2c muxes. This might be obviously correct
> and useful for dedicated i2c mux devices but in my case mux_dev is just
> the i2c_client for a sensor.
> 
> From Documentation/devicetree/bindings/i2c/i2c-mux.txt:
> 
> > An i2c bus multiplexer/switch will have several child busses that are
> > numbered uniquely in a device dependent manner.  The nodes for an i2c
> > bus multiplexer/switch will have one child node for each child bus.
> 
> This seems to be written in a way that would allow me to define the
> "auxiliary i2c master" as bus "1". After all, the numbering is device
> dependent and it's not clear that all the child busses need to be
> accessible through muxing rather than indirect access through device
> registers.

You are correct that if you have devicetree children where reg does
not match the chan_id given to i2c_add_mux_adapter() those children
will be ignored by the i2c-mux code. So, the code would be happy with
a devicetree such as:

mpu6050@68 {
compatible = "...";
reg = <0x68>;
...
i2c-aux-mux@0 {
#address-cells = <1>;
#size-cells = <0>;
reg = <0>;

foo@44 {
compatible = "bar";
reg = <0x44>;
...
}
}
i2c-aux-master@1 {
#address-cells = <1>;
#size-cells = <0>;
reg = <1>;

gazonk@44 {
compatible = "baz";
reg = <0x44>;
...
}
}
}

as long as you do only call i2c_add_mux_adapter() with chan_id = 0. And that
is what you are doing. But I think it is a bit subtle...

> >> Or maybe the two busses should be called i2c-aux-master and i2c-aux-mux? 
> >> Not
> >> sure how to deal with that on the mux side.
> > 
> > Changing i2c to i2c-aux-mux would break existing device trees, that seems
> > like a bad thing, no?
> 
> That support was not documented in mpu6050's bindings and might not be
> actually used.
> 
> >> It is not clear how to properly handle this and suggestions are welcome. 
> >> The
> >> way it currently works with this patch is documented immediately below.
> > 
> > I think the naming could be i2c-master0, i2c-master1 etc if it, with
> > future work, would be possible to add more than one master (you talked about
> > 5 i2c slaves..).
> 
> The device has 5 sets of registers for controlling i2c slaves but only
> one physical auxiliary i2c bus. As far as I can tell slaves 0-3 are
> intended to be used for gathering readings for slaved sensors
> periodically without external intervention. Slave 4 can generate an
> interrupt on completion and is more suitable for general-purpose
> communication with any number of devices.

Ah, ok, so all 5 sets of slave registers are about the same physical i2c
bus. So, you basically cannot sanely use this physical aux i2c bus as an
i2c-mux and an extra i2c adapter in the same hw design. Correct?

In that case, couldn't you look at the names of any devicetree children
and use that to decide if you should even attempt to call
i2c_add_mux_adapter or i2c_add_adapter?

(But please don't clobber stuff for my i2c-mux rework, or you will have
to wait even longer for that deadlock to be resolved)

Cheers,
Peter

Re: [PATCH] iio: inv_mpu6050: Add support for auxiliary I2C master

[Crestez Dan Leonard]

On 04/20/2016 11:31 PM, Peter Rosin wrote:
> Crestez Dan Leonard wrote:
>> Changes since that version:
>> * Nest the adapter in inv_mpu6050_state instead of making it static
>> * Explicitly forward of_node "i2c-aux-master" to allow describing aux devices
>> via devicetree.
>>
>> For bypass/mux mode devicetree works automatically. The forwarding is based 
>> on
>> the "chan_id" parameter to i2c_add_mux_adapter and is implemented here:
>>
>> http://lxr.free-electrons.com/source/drivers/i2c/i2c-mux.c?v=4.5#L158
>>
>> Perhaps it might be better for devices handled via master mode to be 
>> described
>> via i2c@1? This would work by scanning the mpu node's children for something
>> with reg == 1.
> 
> The 0 in i2c@0 (which is used by the mux mode) is the index of the mux slave
> meaning that i2c@1 would be a second mux slave on the same mux, but this is
> not a real mux as such, it is a gate which is piggybacking on the i2c mux 
> infra.
> So, this "mux" can't have a second slave which is why only 0 is valid.

This behavior is automatic in i2c mux code and seems to assume that all
the children of mux_dev are i2c muxes. This might be obviously correct
and useful for dedicated i2c mux devices but in my case mux_dev is just
the i2c_client for a sensor.

>From Documentation/devicetree/bindings/i2c/i2c-mux.txt:

> An i2c bus multiplexer/switch will have several child busses that are
> numbered uniquely in a device dependent manner.  The nodes for an i2c
> bus multiplexer/switch will have one child node for each child bus.

This seems to be written in a way that would allow me to define the
"auxiliary i2c master" as bus "1". After all, the numbering is device
dependent and it's not clear that all the child busses need to be
accessible through muxing rather than indirect access through device
registers.

>> Or maybe the two busses should be called i2c-aux-master and i2c-aux-mux? Not
>> sure how to deal with that on the mux side.
> 
> Changing i2c to i2c-aux-mux would break existing device trees, that seems
> like a bad thing, no?

That support was not documented in mpu6050's bindings and might not be
actually used.

>> It is not clear how to properly handle this and suggestions are welcome. The
>> way it currently works with this patch is documented immediately below.
> 
> I think the naming could be i2c-master0, i2c-master1 etc if it, with
> future work, would be possible to add more than one master (you talked about
> 5 i2c slaves..).

The device has 5 sets of registers for controlling i2c slaves but only
one physical auxiliary i2c bus. As far as I can tell slaves 0-3 are
intended to be used for gathering readings for slaved sensors
periodically without external intervention. Slave 4 can generate an
interrupt on completion and is more suitable for general-purpose
communication with any number of devices.

Re: [PATCH] iio: inv_mpu6050: Add support for auxiliary I2C master

[Crestez Dan Leonard]

On 04/20/2016 11:31 PM, Peter Rosin wrote:
> Crestez Dan Leonard wrote:
>> Changes since that version:
>> * Nest the adapter in inv_mpu6050_state instead of making it static
>> * Explicitly forward of_node "i2c-aux-master" to allow describing aux devices
>> via devicetree.
>>
>> For bypass/mux mode devicetree works automatically. The forwarding is based 
>> on
>> the "chan_id" parameter to i2c_add_mux_adapter and is implemented here:
>>
>> http://lxr.free-electrons.com/source/drivers/i2c/i2c-mux.c?v=4.5#L158
>>
>> Perhaps it might be better for devices handled via master mode to be 
>> described
>> via i2c@1? This would work by scanning the mpu node's children for something
>> with reg == 1.
> 
> The 0 in i2c@0 (which is used by the mux mode) is the index of the mux slave
> meaning that i2c@1 would be a second mux slave on the same mux, but this is
> not a real mux as such, it is a gate which is piggybacking on the i2c mux 
> infra.
> So, this "mux" can't have a second slave which is why only 0 is valid.

This behavior is automatic in i2c mux code and seems to assume that all
the children of mux_dev are i2c muxes. This might be obviously correct
and useful for dedicated i2c mux devices but in my case mux_dev is just
the i2c_client for a sensor.

>From Documentation/devicetree/bindings/i2c/i2c-mux.txt:

> An i2c bus multiplexer/switch will have several child busses that are
> numbered uniquely in a device dependent manner.  The nodes for an i2c
> bus multiplexer/switch will have one child node for each child bus.

This seems to be written in a way that would allow me to define the
"auxiliary i2c master" as bus "1". After all, the numbering is device
dependent and it's not clear that all the child busses need to be
accessible through muxing rather than indirect access through device
registers.

>> Or maybe the two busses should be called i2c-aux-master and i2c-aux-mux? Not
>> sure how to deal with that on the mux side.
> 
> Changing i2c to i2c-aux-mux would break existing device trees, that seems
> like a bad thing, no?

That support was not documented in mpu6050's bindings and might not be
actually used.

>> It is not clear how to properly handle this and suggestions are welcome. The
>> way it currently works with this patch is documented immediately below.
> 
> I think the naming could be i2c-master0, i2c-master1 etc if it, with
> future work, would be possible to add more than one master (you talked about
> 5 i2c slaves..).

The device has 5 sets of registers for controlling i2c slaves but only
one physical auxiliary i2c bus. As far as I can tell slaves 0-3 are
intended to be used for gathering readings for slaved sensors
periodically without external intervention. Slave 4 can generate an
interrupt on completion and is more suitable for general-purpose
communication with any number of devices.

Re: [PATCH] iio: inv_mpu6050: Add support for auxiliary I2C master

[Crestez Dan Leonard]

On 04/20/2016 09:17 PM, kbuild test robot wrote:
> Hi,
> 
> [auto build test ERROR on iio/togreg]
> [also build test ERROR on v4.6-rc4 next-20160420]
> [if your patch is applied to the wrong git tree, please drop us a note to 
> help improving the system]
> 
> url:
> https://github.com/0day-ci/linux/commits/Crestez-Dan-Leonard/iio-inv_mpu6050-Add-support-for-auxiliary-I2C-master/20160421-012042
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
> config: i386-randconfig-i1-201616 (attached as .config)
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=i386 
> 
> All errors (new ones prefixed by >>):
> 
>drivers/iio/imu/inv_mpu6050/inv_mpu_core.c: In function 
> 'inv_mpu_core_probe':
>>> drivers/iio/imu/inv_mpu6050/inv_mpu_core.c:966:11: error: implicit 
>>> declaration of function 'i2c_add_adapter' 
>>> [-Werror=implicit-function-declaration]
>  result = i2c_add_adapter(>aux_master_adapter);
>   ^
>>> drivers/iio/imu/inv_mpu6050/inv_mpu_core.c:983:2: error: implicit 
>>> declaration of function 'i2c_del_adapter' 
>>> [-Werror=implicit-function-declaration]
>  i2c_del_adapter(>aux_master_adapter);
>  ^
>cc1: some warnings being treated as errors

This error happens if you disable CONFIG_I2C and only compile with SPI
support. I will add a bunch of #ifdef CONFIG_I2C in the next version.

Re: [PATCH] iio: inv_mpu6050: Add support for auxiliary I2C master

[Crestez Dan Leonard]

On 04/20/2016 09:17 PM, kbuild test robot wrote:
> Hi,
> 
> [auto build test ERROR on iio/togreg]
> [also build test ERROR on v4.6-rc4 next-20160420]
> [if your patch is applied to the wrong git tree, please drop us a note to 
> help improving the system]
> 
> url:
> https://github.com/0day-ci/linux/commits/Crestez-Dan-Leonard/iio-inv_mpu6050-Add-support-for-auxiliary-I2C-master/20160421-012042
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
> config: i386-randconfig-i1-201616 (attached as .config)
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=i386 
> 
> All errors (new ones prefixed by >>):
> 
>drivers/iio/imu/inv_mpu6050/inv_mpu_core.c: In function 
> 'inv_mpu_core_probe':
>>> drivers/iio/imu/inv_mpu6050/inv_mpu_core.c:966:11: error: implicit 
>>> declaration of function 'i2c_add_adapter' 
>>> [-Werror=implicit-function-declaration]
>  result = i2c_add_adapter(>aux_master_adapter);
>   ^
>>> drivers/iio/imu/inv_mpu6050/inv_mpu_core.c:983:2: error: implicit 
>>> declaration of function 'i2c_del_adapter' 
>>> [-Werror=implicit-function-declaration]
>  i2c_del_adapter(>aux_master_adapter);
>  ^
>cc1: some warnings being treated as errors

This error happens if you disable CONFIG_I2C and only compile with SPI
support. I will add a bunch of #ifdef CONFIG_I2C in the next version.

Re: [PATCH] iio: inv_mpu6050: Add support for auxiliary I2C master

[Peter Rosin]

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 
> ---
> 
> This is based on earlier work by Daniel Baluta :
> https://www.spinics.net/lists/linux-iio/msg23573.html
> 
> Changes since that version:
> * Nest the adapter in inv_mpu6050_state instead of making it static
> * Explicitly forward of_node "i2c-aux-master" to allow describing aux devices
> via devicetree.
> 
> For bypass/mux mode devicetree works automatically. The forwarding is based on
> the "chan_id" parameter to i2c_add_mux_adapter and is implemented here:
> 
> http://lxr.free-electrons.com/source/drivers/i2c/i2c-mux.c#L158

With any luck [1], this reference will be stale when 4.6 is released. It
should be qulified with a version, something like .../i2c-mux.c?v=4.5#L158

> Perhaps it might be better for devices handled via master mode to be described
> via i2c@1? This would work by scanning the mpu node's children for something
> with reg == 1.

The 0 in i2c@0 (which is used by the mux mode) is the index of the mux slave
meaning that i2c@1 would be a second mux slave on the same mux, but this is
not a real mux as such, it is a gate which is piggybacking on the i2c mux infra.
So, this "mux" can't have a second slave which is why only 0 is valid.

Tl;dr i2c@1 is definitely wrong for something that is not related to i2c@0.

> Or maybe the two busses should be called i2c-aux-master and i2c-aux-mux? Not
> sure how to deal with that on the mux side.

Changing i2c to i2c-aux-mux would break existing device trees, that seems
like a bad thing, no?

> It is not clear how to properly handle this and suggestions are welcome. The
> way it currently works with this patch is documented immediately below.

I think the naming could be i2c-master0, i2c-master1 etc if it, with
future work, would be possible to add more than one master (you talked about
5 i2c slaves..).

Cheers,
Peter

[1] https://lkml.org/lkml/2016/4/20/467

Re: [PATCH] iio: inv_mpu6050: Add support for auxiliary I2C master

[Peter Rosin]

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 
> ---
> 
> This is based on earlier work by Daniel Baluta :
> https://www.spinics.net/lists/linux-iio/msg23573.html
> 
> Changes since that version:
> * Nest the adapter in inv_mpu6050_state instead of making it static
> * Explicitly forward of_node "i2c-aux-master" to allow describing aux devices
> via devicetree.
> 
> For bypass/mux mode devicetree works automatically. The forwarding is based on
> the "chan_id" parameter to i2c_add_mux_adapter and is implemented here:
> 
> http://lxr.free-electrons.com/source/drivers/i2c/i2c-mux.c#L158

With any luck [1], this reference will be stale when 4.6 is released. It
should be qulified with a version, something like .../i2c-mux.c?v=4.5#L158

> Perhaps it might be better for devices handled via master mode to be described
> via i2c@1? This would work by scanning the mpu node's children for something
> with reg == 1.

The 0 in i2c@0 (which is used by the mux mode) is the index of the mux slave
meaning that i2c@1 would be a second mux slave on the same mux, but this is
not a real mux as such, it is a gate which is piggybacking on the i2c mux infra.
So, this "mux" can't have a second slave which is why only 0 is valid.

Tl;dr i2c@1 is definitely wrong for something that is not related to i2c@0.

> Or maybe the two busses should be called i2c-aux-master and i2c-aux-mux? Not
> sure how to deal with that on the mux side.

Changing i2c to i2c-aux-mux would break existing device trees, that seems
like a bad thing, no?

> It is not clear how to properly handle this and suggestions are welcome. The
> way it currently works with this patch is documented immediately below.

I think the naming could be i2c-master0, i2c-master1 etc if it, with
future work, would be possible to add more than one master (you talked about
5 i2c slaves..).

Cheers,
Peter

[1] https://lkml.org/lkml/2016/4/20/467

Re: [PATCH] iio: inv_mpu6050: Add support for auxiliary I2C master

[kbuild test robot]

Hi,

[auto build test ERROR on iio/togreg]
[also build test ERROR on v4.6-rc4 next-20160420]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improving the system]

url:
https://github.com/0day-ci/linux/commits/Crestez-Dan-Leonard/iio-inv_mpu6050-Add-support-for-auxiliary-I2C-master/20160421-012042
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: i386-randconfig-i1-201616 (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/iio/imu/inv_mpu6050/inv_mpu_core.c: In function 'inv_mpu_core_probe':
>> drivers/iio/imu/inv_mpu6050/inv_mpu_core.c:966:11: error: implicit 
>> declaration of function 'i2c_add_adapter' 
>> [-Werror=implicit-function-declaration]
 result = i2c_add_adapter(>aux_master_adapter);
  ^
>> drivers/iio/imu/inv_mpu6050/inv_mpu_core.c:983:2: error: implicit 
>> declaration of function 'i2c_del_adapter' 
>> [-Werror=implicit-function-declaration]
 i2c_del_adapter(>aux_master_adapter);
 ^
   cc1: some warnings being treated as errors

vim +/i2c_add_adapter +966 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c

   960  snprintf(st->aux_master_adapter.name, 
sizeof(st->aux_master_adapter.name),
   961  "aux-master-%s", indio_dev->name);
   962  st->aux_master_adapter.dev.of_node = of_get_child_by_name(
   963  dev->of_node, "i2c-aux-master");
   964  i2c_set_adapdata(>aux_master_adapter, st);
   965  /* This will also probe aux devices so transfers must work now 
*/
 > 966  result = i2c_add_adapter(>aux_master_adapter);
   967  if (result < 0) {
   968  dev_err(dev, "i2x aux master register fail %d\n", 
result);
   969  goto out_remove_trigger;
   970  }
   971  
   972  INIT_KFIFO(st->timestamps);
   973  spin_lock_init(>time_stamp_lock);
   974  result = iio_device_register(indio_dev);
   975  if (result) {
   976  dev_err(dev, "IIO register fail %d\n", result);
   977  goto out_del_adapter;
   978  }
   979  
   980  return 0;
   981  
   982  out_del_adapter:
 > 983  i2c_del_adapter(>aux_master_adapter);
   984  out_remove_trigger:
   985  inv_mpu6050_remove_trigger(st);
   986  out_unreg_ring:

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

Re: [PATCH] iio: inv_mpu6050: Add support for auxiliary I2C master

[kbuild test robot]

Hi,

[auto build test ERROR on iio/togreg]
[also build test ERROR on v4.6-rc4 next-20160420]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improving the system]

url:
https://github.com/0day-ci/linux/commits/Crestez-Dan-Leonard/iio-inv_mpu6050-Add-support-for-auxiliary-I2C-master/20160421-012042
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: i386-randconfig-i1-201616 (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/iio/imu/inv_mpu6050/inv_mpu_core.c: In function 'inv_mpu_core_probe':
>> drivers/iio/imu/inv_mpu6050/inv_mpu_core.c:966:11: error: implicit 
>> declaration of function 'i2c_add_adapter' 
>> [-Werror=implicit-function-declaration]
 result = i2c_add_adapter(>aux_master_adapter);
  ^
>> drivers/iio/imu/inv_mpu6050/inv_mpu_core.c:983:2: error: implicit 
>> declaration of function 'i2c_del_adapter' 
>> [-Werror=implicit-function-declaration]
 i2c_del_adapter(>aux_master_adapter);
 ^
   cc1: some warnings being treated as errors

vim +/i2c_add_adapter +966 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c

   960  snprintf(st->aux_master_adapter.name, 
sizeof(st->aux_master_adapter.name),
   961  "aux-master-%s", indio_dev->name);
   962  st->aux_master_adapter.dev.of_node = of_get_child_by_name(
   963  dev->of_node, "i2c-aux-master");
   964  i2c_set_adapdata(>aux_master_adapter, st);
   965  /* This will also probe aux devices so transfers must work now 
*/
 > 966  result = i2c_add_adapter(>aux_master_adapter);
   967  if (result < 0) {
   968  dev_err(dev, "i2x aux master register fail %d\n", 
result);
   969  goto out_remove_trigger;
   970  }
   971  
   972  INIT_KFIFO(st->timestamps);
   973  spin_lock_init(>time_stamp_lock);
   974  result = iio_device_register(indio_dev);
   975  if (result) {
   976  dev_err(dev, "IIO register fail %d\n", result);
   977  goto out_del_adapter;
   978  }
   979  
   980  return 0;
   981  
   982  out_del_adapter:
 > 983  i2c_del_adapter(>aux_master_adapter);
   984  out_remove_trigger:
   985  inv_mpu6050_remove_trigger(st);
   986  out_unreg_ring:

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data