| Message ID | 1425039885-5137-2-git-send-email-sebastian.hesselbarth@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/27/2015 05:24 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. > 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 enabled child node an I2C child bus will be created. I2C child bus > +numbers are assigned based on the order of child nodes. I think that I2C bus numbers are an internal concept for the OS. As such, we should probably remove any mention re: the bus numbers from the binding. > -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: > +There must be a corresponding pinctrl-names entry for each enabled child > +node at the position of the child node's "reg" property. Also, there can be > +an idle pinctrl state defined at the end of possible pinctrl states. If such > +a state is defined, it must be the last entry in pinctrl-names. For example: What about gaps in the numbering sequence? IIRC, in a situation with 5 nodes with reg 0, 1, 2, 3, 4 but where only the nodes with reg of 1, 3 enabled, we only want 2 entries in pinctrl-names? If so, "at the position of the child node's "reg" property" isn't correct, since "at the position" implies there must be gaps in pinctrl-names. "In the same order as the reg property values for enabled subnodes" might be a better description. Perhaps I'm misremembering and you explicitly didn't want to remove entries from pinctrl-names if child nodes were disabled? If so, then surely then in the text above, "for each enabled child" should be replaced with "for each child"? > @@ -68,6 +68,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 +80,12 @@ Example: > }; > }; > > + /* Disabled child bus 1 */ > i2c@1 { > reg = <1>; > #address-cells = <1>; > #size-cells = <0>; > + status = "disabled"; To make the example cover more cases, perhaps make child node i2c@0 disabled and i2c@1 enabled. Then, the example would show what happens to pinctrl-names when there are gaps in the reg property numbering space of enabled children?
On 02.03.2015 21:01, Stephen Warren wrote: > On 02/27/2015 05:24 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. > >> 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 enabled child node an I2C child bus will be created. I2C >> child bus >> +numbers are assigned based on the order of child nodes. > > I think that I2C bus numbers are an internal concept for the OS. As > such, we should probably remove any mention re: the bus numbers from the > binding. Stephen, yeah as you can see I am struggling to find a good documentation. I agree that we should get rid of the bus number thing above. >> -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: >> +There must be a corresponding pinctrl-names entry for each enabled child >> +node at the position of the child node's "reg" property. Also, there >> can be >> +an idle pinctrl state defined at the end of possible pinctrl states. >> If such >> +a state is defined, it must be the last entry in pinctrl-names. For >> example: > > What about gaps in the numbering sequence? IIRC, in a situation with 5 > nodes with reg 0, 1, 2, 3, 4 but where only the nodes with reg of 1, 3 > enabled, we only want 2 entries in pinctrl-names? If so, "at the > position of the child node's "reg" property" isn't correct, since "at > the position" implies there must be gaps in pinctrl-names. "In the same > order as the reg property values for enabled subnodes" might be a better > description. Good point. The idea was to _have_ gaps in pinctrl-names to allow to configure the current mux layout by status properties only. The existing implementation (and docu) suggested you have to amend pinctrl-names to achieve a specific setup. > Perhaps I'm misremembering and you explicitly didn't want to remove > entries from pinctrl-names if child nodes were disabled? If so, then > surely then in the text above, "for each enabled child" should be > replaced with "for each child"? True. >> @@ -68,6 +68,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 +80,12 @@ Example: >> }; >> }; >> >> + /* Disabled child bus 1 */ >> i2c@1 { >> reg = <1>; >> #address-cells = <1>; >> #size-cells = <0>; >> + status = "disabled"; > > To make the example cover more cases, perhaps make child node i2c@0 > disabled and i2c@1 enabled. Then, the example would show what happens to > pinctrl-names when there are gaps in the reg property numbering space of > enabled children? The idea was to make nothing happen to pinctrl-names if you enable/ disable any of the children. But I can move the disabled status to i2c@0 to make it more clear that there should still be a pinctrl-names cell for it. Sebastian
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt index ae8af1694e95..40cf6d86f935 100644 --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt @@ -28,21 +28,21 @@ 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 enabled child node 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: +There must be a corresponding pinctrl-names entry for each enabled child +node at the position of the child node's "reg" property. Also, there can be +an idle pinctrl state defined at the end of possible pinctrl states. 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) + 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) Whenever an access is made to a device on a child bus, the relevant pinctrl state will be programmed into hardware. @@ -68,6 +68,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 +80,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