Re: [PATCH 1/8] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Stephen Warren <swarren@wwwdotorg.org>]

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:-)