| Message ID | 1424199129-22099-2-git-send-email-sebastian.hesselbarth@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? > 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. > > -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. > > Whenever an access is made to a device on a child bus, the relevant pinctrl > state will be programmed into hardware. > > -If an idle state is defined, whenever an access is not being made to a device > -on a child bus, the idle pinctrl state will be programmed into hardware. > +Also, there can be an idle pinctrl state defined at the end of possible > +pinctrl states. If an idle state is defined, whenever an access is not being > +made to a device on a child bus, the idle pinctrl state will be programmed > +into hardware.
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"; }; }; 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? Sebastian
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:-)
On 17.02.2015 22:15, Stephen Warren wrote: > On 02/17/2015 02:08 PM, Sebastian Hesselbarth wrote: >> On 17.02.2015 21:46, Stephen Warren wrote: >>> 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: [...] >> >> 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. [...] > 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. I totally agree that the binding is not affected at all. I was under the impression that the current binding doc justifies the way the driver is currently parsing the properties. So this was an attempt to reword it to make it more clear what properties should influence the way sub-busses are registered. I'll have another look at the binding doc when the driver is fine. [...] >> 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:-) Ok, I cross my fingers too and expect a Tested-by :) Sebastian
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. > > 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. Tested-by: Stephen Warren <swarren@nvidia.com> (Both the HDMI and LCD panel DDC busses on NVIDIA Tegra Springbank/Seaboard, running next-20150224)
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt index ae8af1694e95..24b9fdef8850 100644 --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt @@ -28,27 +28,24 @@ Also required are: * Standard pinctrl properties that specify the pin mux state for each child bus. See ../pinctrl/pinctrl-bindings.txt. -* Standard I2C mux properties. See mux.txt in this directory. +* Standard I2C mux properties. See i2c-mux.txt in this directory. -* I2C child bus nodes. See mux.txt in this directory. +* I2C child bus nodes. See i2c-mux.txt in this directory. -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. -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. Whenever an access is made to a device on a child bus, the relevant pinctrl state will be programmed into hardware. -If an idle state is defined, whenever an access is not being made to a device -on a child bus, the idle pinctrl state will be programmed into hardware. +Also, there can be an idle pinctrl state defined at the end of possible +pinctrl states. If an idle state is defined, whenever an access is not being +made to a device on a child bus, the idle pinctrl state will be programmed +into hardware. If an idle state is not defined, the most recently used pinctrl state will be left programmed into hardware whenever no access is being made of a device on @@ -68,6 +65,7 @@ Example: pinctrl-1 = <&state_i2cmux_pta>; pinctrl-2 = <&state_i2cmux_idle>; + /* Enabled child bus 0 */ i2c@0 { reg = <0>; #address-cells = <1>; @@ -79,10 +77,12 @@ Example: }; }; + /* Disabled child bus 1 */ i2c@1 { reg = <1>; #address-cells = <1>; #size-cells = <0>; + status = "disabled"; eeprom { compatible = "eeprom"; diff --git a/drivers/i2c/muxes/i2c-mux-pinctrl.c b/drivers/i2c/muxes/i2c-mux-pinctrl.c index b48378c4b40d..033dacfabfdf 100644 --- a/drivers/i2c/muxes/i2c-mux-pinctrl.c +++ b/drivers/i2c/muxes/i2c-mux-pinctrl.c @@ -56,9 +56,12 @@ static int i2c_mux_pinctrl_parse_dt(struct i2c_mux_pinctrl *mux, struct platform_device *pdev) { struct device_node *np = pdev->dev.of_node; - int num_names, i, ret; + struct device_node *child; + struct property *prop; + int num_names, num_children, ret; struct device_node *adapter_np; struct i2c_adapter *adapter; + const char *state; if (!np) return 0; @@ -77,32 +80,16 @@ static int i2c_mux_pinctrl_parse_dt(struct i2c_mux_pinctrl *mux, return num_names; } - mux->pdata->pinctrl_states = devm_kzalloc(&pdev->dev, - sizeof(*mux->pdata->pinctrl_states) * num_names, - GFP_KERNEL); - if (!mux->pdata->pinctrl_states) { - dev_err(mux->dev, "Cannot allocate pinctrl_states\n"); - return -ENOMEM; + num_children = of_get_available_child_count(np); + if (num_children < 0) { + dev_err(mux->dev, "Unable to count available children: %d\n", + num_children); + return num_children; } - for (i = 0; i < num_names; i++) { - ret = of_property_read_string_index(np, "pinctrl-names", i, - &mux->pdata->pinctrl_states[mux->pdata->bus_count]); - if (ret < 0) { - dev_err(mux->dev, "Cannot parse pinctrl-names: %d\n", - ret); - return ret; - } - if (!strcmp(mux->pdata->pinctrl_states[mux->pdata->bus_count], - "idle")) { - if (i != num_names - 1) { - dev_err(mux->dev, "idle state must be last\n"); - return -EINVAL; - } - mux->pdata->pinctrl_state_idle = "idle"; - } else { - mux->pdata->bus_count++; - } + if (num_names < num_children) { + dev_err(mux->dev, "Found less pinctrl states than children\n"); + return -EINVAL; } adapter_np = of_parse_phandle(np, "i2c-parent", 0); @@ -118,6 +105,39 @@ static int i2c_mux_pinctrl_parse_dt(struct i2c_mux_pinctrl *mux, mux->pdata->parent_bus_num = i2c_adapter_id(adapter); put_device(&adapter->dev); + mux->pdata->pinctrl_states = devm_kzalloc(&pdev->dev, + sizeof(*mux->pdata->pinctrl_states) * num_children, + GFP_KERNEL); + if (!mux->pdata->pinctrl_states) { + dev_err(mux->dev, "Cannot allocate pinctrl_states\n"); + return -ENOMEM; + } + + of_property_for_each_string(np, "pinctrl-names", prop, state) + if (!strcmp(state, "idle")) + mux->pdata->pinctrl_state_idle = "idle"; + + for_each_available_child_of_node(np, child) { + u32 reg; + + ret = of_property_read_u32(child, "reg", ®); + if (ret < 0) { + dev_err(mux->dev, "Missing reg property for child node: %d\n", + ret); + return ret; + } + + ret = of_property_read_string_index(np, + "pinctrl-names", reg, &state); + if (ret < 0) { + dev_err(mux->dev, "Cannot parse pinctrl-names for mux %d: %d\n", + reg, ret); + return ret; + } + + mux->pdata->pinctrl_states[mux->pdata->bus_count++] = state; + } + return 0; } #else
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. 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. Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> --- Cc: Jason Cooper <jason@lakedaemon.net> Cc: Andrew Lunn <andrew@lunn.ch> Cc: Gregory Clement <gregory.clement@free-electrons.com> Cc: Gabriel Dobato <dobatog@gmail.com> Cc: Wolfram Sang <wsa@the-dreams.de> Cc: Stephen Warren <swarren@wwwdotorg.org> Cc: linux-i2c@vger.kernel.org Cc: devicetree@vger.kernel.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-kernel@vger.kernel.org --- .../devicetree/bindings/i2c/i2c-mux-pinctrl.txt | 28 ++++----- drivers/i2c/muxes/i2c-mux-pinctrl.c | 70 ++++++++++++++-------- 2 files changed, 59 insertions(+), 39 deletions(-)