Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752516AbbBQVOu (ORCPT ); Tue, 17 Feb 2015 16:14:50 -0500 Received: from avon.wwwdotorg.org ([70.85.31.133]:43897 "EHLO avon.wwwdotorg.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751063AbbBQVOs (ORCPT ); Tue, 17 Feb 2015 16:14:48 -0500 Message-ID: <54E3AF5F.3020902@wwwdotorg.org> Date: Tue, 17 Feb 2015 14:15:11 -0700 From: Stephen Warren User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Sebastian Hesselbarth CC: Jason Cooper , Andrew Lunn , Gregory Clement , Gabriel Dobato , Wolfram Sang , linux-i2c@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/8] i2c: mux-pinctrl: Rework to honor disabled child nodes References: <1424199129-22099-1-git-send-email-sebastian.hesselbarth@gmail.com> <1424199129-22099-2-git-send-email-sebastian.hesselbarth@gmail.com> <54E3A8A7.8080703@wwwdotorg.org> <54E3ADB9.1040008@gmail.com> In-Reply-To: <54E3ADB9.1040008@gmail.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6010 Lines: 157 On 02/17/2015 02:08 PM, Sebastian Hesselbarth wrote: > On 17.02.2015 21:46, Stephen Warren wrote: >> On 02/17/2015 11:52 AM, Sebastian Hesselbarth wrote: >>> I2C mux pinctrl driver currently determines the number of sub-busses by >>> counting available pinctrl-names. Unfortunately, this requires each >>> incarnation of the devicetree node with different available sub-busses >>> to be rewritten. >> >> Can you be more explicit about the problem here? Why does anything need >> to be re-written if a child node is disabled; presumably there's no need >> for the child bus numbers to be contiguous. In other words, with the >> example in the existing DT binding doc: >> >> i2cmux { >> compatible = "i2c-mux-pinctrl"; >> ... >> pinctrl-names = "ddc", "pta", "idle"; >> pinctrl-0 = <&state_i2cmux_ddc>; >> pinctrl-1 = <&state_i2cmux_pta>; >> pinctrl-2 = <&state_i2cmux_idle>; >> >> i2c@0 { >> reg = <0>; >> ... >> i2c@1 { >> reg = <1>; >> ... >> >> That would generate child busses 0 and 1. If I was to disable the i2c@0 >> node, then there would still be definitions for child busses 0 and 1 in >> the DT, it's just that child bus 0 wouldn't actually exist at run-time. >> I don't see what part of DT needs to be re-written to accomodate this? > > The way the current driver works, to disable i2c@0 you'd have to remove > the pinctrl-0 state, pinctrl-names string at position 0, and the node > itself. > > So, on Dove SoC there is three sub-busses, now consider one board A with > i2c0 and i2c1 enabled but board B with i2c0 and i2c2 enabled: > > board-A.dts: > > i2cmux { > pinctrl-names = "i2c0", "i2c1", "idle"; > pinctrl-0 = <&state_for_i2c0>; > pinctrl-1 = <&state_for_i2c1>; > }; > > but > > board-B.dts: > > i2cmux { > pinctrl-names = "i2c0", "i2c2", "idle"; > pinctrl-0 = <&state_for_i2c0>; > pinctrl-1 = <&state_for_i2c2>; > /* Note that this ^^^ is state_for_i2c2 */ > }; > > while the approach with status = "disabled" allows all properties for > both board remain the same - except you'll enable either i2c1 or i2c2 > sub-node on board level: > > i2cmux { > pinctrl-names = "i2c0", "i2c1", "i2c2", "idle"; > pinctrl-0 = <&state_for_i2c0>; > pinctrl-1 = <&state_for_i2c1>; > pinctrl-2 = <&state_for_i2c2>; > }; > > board-A.dts: > > i2cmux { > i2c@0 { status = "okay"; }; > i2c@1 { status = "okay"; }; > }; > > and > > board-B.dts: > > i2cmux { > i2c@0 { status = "okay"; }; > i2c@2 { status = "okay"; }; > }; OK, that all makes sense, but I don't think there's any change at all to the binding; this can all be fixed in the driver without affecting the definition of the binding at all. At most all that's needed in the binding is a note to the effect that if a particular child node is disabled, then this has no effect at all on the requirements for the pinctrl properties. > In general, it is less about the binding but how the driver is written: > Number of sub-busses is determined by elements in pinctrl-names not > available (enabled) sub-nodes. > >>> This patch reworks i2c-mux-pinctrl driver to count the number of >>> available sub-nodes instead. The rework should be compatible to the old >>> way of probing for sub-busses and additionally allows to disable unused >>> sub-busses with standard DT property status = "disabled". >>> >>> This also amends the corresponding devicetree binding documentation to >>> reflect the new functionality to disable unused sub-nodes. While at it, >>> also fix two references to binding documentation files that miss an >>> "i2c-" >>> prefix. >> >>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt >>> b/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt >> >>> -For each named state defined in the pinctrl-names property, an I2C >>> child bus >>> -will be created. I2C child bus numbers are assigned based on the >>> index into >>> -the pinctrl-names property. >>> +For each child node that is not disabled by a status != "okay", an I2C >>> +child bus will be created. I2C child bus numbers are assigned based >>> on the >>> +order of child nodes. >> >> I would have assumed that disabled sub-nodes was a global concept within >> DT, and so wouldn't be mentioned in the binding. It would just be a bug >> in the driver if it didn't ignore disabled sub-nodes. > > Yep, the concept is very global. It is about the current driver and this > binding changes are just to make it a little more clear that the driver > should behave different, i.e. get rid of anything that implies that > pinctrl-names has any effect on the number of sub-busses registered. > >>> -The only exception is that no bus will be created for a state named >>> "idle". If >>> -such a state is defined, it must be the last entry in pinctrl-names. >>> For >>> -example: >>> - >>> - pinctrl-names = "ddc", "pta", "idle" -> ddc = bus 0, pta = bus 1 >>> - pinctrl-names = "ddc", "idle", "pta" -> Invalid ("idle" not last) >>> - pinctrl-names = "idle", "ddc", "pta" -> Invalid ("idle" not last) >>> +There must be a corresponding pinctrl-names entry for each enabled >>> child >>> +node at the position of the child node's "reg" property. >> >> The addition there seems fine, but the existing text re: the idle state >> seems clearer in the original text. > > Ok, I'll have a look at how to preserve this section better. > > Do you still have one of the current boards available for testing? Yes, I have both Seaboard and Ventana still (the two Tegra boards that use this driver). I haven't used them in a while; I hope they still work:-) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/