From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753112AbcDVLQ5 (ORCPT ); Fri, 22 Apr 2016 07:16:57 -0400 Received: from mail-db3on0103.outbound.protection.outlook.com ([157.55.234.103]:8019 "EHLO emea01-db3-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752364AbcDVLQy convert rfc822-to-8bit (ORCPT ); Fri, 22 Apr 2016 07:16:54 -0400 From: Peter Rosin To: Crestez Dan Leonard , Peter Rosin , Jonathan Cameron , "linux-iio@vger.kernel.org" CC: "linux-kernel@vger.kernel.org" , "Hartmut Knaack" , Lars-Peter Clausen , "Peter Meerwald-Stadler" , Daniel Baluta , "linux-i2c@vger.kernel.org" , Wolfram Sang , "devicetree@vger.kernel.org" , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , "Kumar Gala" Subject: Re: [PATCH] iio: inv_mpu6050: Add support for auxiliary I2C master Thread-Topic: [PATCH] iio: inv_mpu6050: Add support for auxiliary I2C master Thread-Index: AdGchhc5Bzi52Y0WRxGAXzl8IgBtxQ== Date: Fri, 22 Apr 2016 11:01:35 +0000 Message-ID: Accept-Language: en-US, sv-SE Content-Language: sv-SE X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: intel.com; dkim=none (message not signed) header.d=none;intel.com; dmarc=none action=none header.from=axentia.se; x-originating-ip: [217.210.101.82] x-ms-office365-filtering-correlation-id: 03e961c5-bbcd-48ad-d7b6-08d36a9d78bf x-microsoft-exchange-diagnostics: 1;AM4PR02MB1298;5:TL62szL0AsDx3UnKN9zpo/dnk1OLZaxlLZW5LiycU/y6BOL32lNLmknd0xjckM7kzECqnv33Ma6UocRk1hhrkgswyNz2MpAyFw5+WlJebtSl9iKiR0ft45U7uzGOQZj94DbScgFTeGSll9CXDGNd+OGvQ6FzrNoHdp6juT2NH6d49BrU6hv/8xXXf5Gkw+25;24:M3oLAbOKa1p7/T3foNH9L7Zm586xPAN/YvQAKoLtxx5XPs5hehdLX9iEm1gsgCplI2CDW8dCBVQbwOBzaetv4QG2iecRaVas5+sqmI2NhA8=;7:NAbqrdeTS3sKs5EKAoL9RfGDVG0aPwzVEr+EMy4gI1mNvH5GOJzqOEif/UbyGkFo53FPgFsk//A7Y7pvaBSUDM38qkjPf4c1TiHSeLzBs3UVKtEKYjxeTErNLh71FBjglT7x2axz5pj4xA/TGwIlppPMpurJl39BYLP7Ai0hPSrZVmCb7oeifQYC+AJJEHXfYwKW1iAvv/stsfoeFSpBiQaJPRP5LcQzRRSgflrDR38= x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:AM4PR02MB1298; x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:; x-exchange-antispam-report-cfa-test: BCL:0;PCL:0;RULEID:(9101521026)(6040130)(601004)(2401047)(5005006)(8121501046)(3002001)(10201501046)(6041072)(6043046);SRVR:AM4PR02MB1298;BCL:0;PCL:0;RULEID:;SRVR:AM4PR02MB1298; x-forefront-prvs: 0920602B08 x-forefront-antispam-report: SFV:NSPM;SFS:(10019020)(6009001)(24454002)(377454003)(74482002)(87936001)(5001770100001)(74316001)(5002640100001)(76576001)(86362001)(189998001)(66066001)(54356999)(10400500002)(92566002)(19580405001)(11100500001)(2501003)(33656002)(5003600100002)(15975445007)(2900100001)(19580395003)(81166005)(5008740100001)(9686002)(77096005)(5004730100002)(1220700001)(4326007)(122556002)(3846002)(6116002)(102836003)(3660700001)(2906002)(586003)(1096002)(3280700002)(50986999)(7059030)(42262002);DIR:OUT;SFP:1102;SCL:1;SRVR:AM4PR02MB1298;H:AM4PR02MB1299.eurprd02.prod.outlook.com;FPR:;SPF:None;MLV:sfv;LANG:en; spamdiagnosticoutput: 1:23 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 X-OriginatorOrg: axentia.se X-MS-Exchange-CrossTenant-originalarrivaltime: 22 Apr 2016 11:01:35.5901 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 4ee68585-03e1-4785-942a-df9c1871a234 X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM4PR02MB1298 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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. Oh, you misunderstand, I meant looking at the name of the children of the mpu6050 node, not the grand-children. I.e. i2c-aux-mux vs. i2c-aux-master in the above example. > 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. That is also possible of course, but if you allow i2c-master also when connected with i2c, that can be used to resolve address conflicts between the main i2c bus and the aux bus. Cheers, Peter