Re: [PATCH v8 07/12] dt-bindings: i2c: i2c-mux-simple: document i2c-mux-simple bindings

[Rob Herring]

On Tue, Jan 31, 2017 at 1:36 AM, Peter Rosin <peda@xxxxxxxxxx> wrote:
> On 2017-01-30 18:20, Rob Herring wrote:
>> On Sat, Jan 28, 2017 at 4:42 PM, Peter Rosin <peda@xxxxxxxxxx> wrote:
>>> On 2017-01-27 20:39, Rob Herring wrote:
>>>> On Wed, Jan 18, 2017 at 04:57:10PM +0100, Peter Rosin wrote:
>>>>> Describe how a generic multiplexer controller is used to mux an i2c bus.
>>>>>
>>>>> Acked-by: Jonathan Cameron <jic23@xxxxxxxxxx>
>>>>> Signed-off-by: Peter Rosin <peda@xxxxxxxxxx>
>>>>> ---
>>>>>  .../devicetree/bindings/i2c/i2c-mux-simple.txt     | 81 ++++++++++++++++++++++
>>>>>  1 file changed, 81 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mux-simple.txt
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-simple.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-simple.txt
>>>>> new file mode 100644
>>>>> index 000000000000..253d5027843b
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-simple.txt
>>>>> @@ -0,0 +1,81 @@
>>>>> +Simple I2C Bus Mux
>>>>> +
>>>>> +This binding describes an I2C bus multiplexer that uses a mux controller
>>>>> +from the mux subsystem to route the I2C signals.
>>>>> +
>>>>> +                                  .-----.  .-----.
>>>>> +                                  | dev |  | dev |
>>>>> +    .------------.                '-----'  '-----'
>>>>> +    | SoC        |                   |        |
>>>>> +    |            |          .--------+--------'
>>>>> +    |   .------. |  .------+    child bus A, on MUX value set to 0
>>>>> +    |   | I2C  |-|--| Mux  |
>>>>> +    |   '------' |  '--+---+    child bus B, on MUX value set to 1
>>>>> +    |   .------. |     |    '----------+--------+--------.
>>>>> +    |   | MUX- | |     |               |        |        |
>>>>> +    |   | Ctrl |-|-----+            .-----.  .-----.  .-----.
>>>>> +    |   '------' |                  | dev |  | dev |  | dev |
>>>>> +    '------------'                  '-----'  '-----'  '-----'
>>>>> +
>>>>> +Required properties:
>>>>> +- compatible: i2c-mux-simple,mux-locked or i2c-mux-simple,parent-locked
>>>>
>>>> Not a fan of using "simple" nor the ','. Perhaps lock type should be
>>>> separate property.
>>>
>>> How about just i2c-mux for the compatible? Because i2c-mux-mux (which
>>> follows the naming of previous i2c muxes) looks really stupid. Or
>>> perhaps i2c-mux-generic?
>>
>> I like "generic" only slightly more than "simple". :)
>>
>> If the mux is gpio controlled, then it should still be called
>> i2c-gpio-mux. Let's not invent brand new bindings when current ones
>> are easily extended. We already have pretty generic names here, let's
>> not make them more generic.
>
> Ahh, but the whole point of this new driver is that it does not know
> if the mux is gpio controlled, if it is controlled by an adg792 chip
> or whatever might appear down the line. So, I think i2c-mux-gpio (and
> i2c-gpio-mux) is actively wrong.

That's fine for the driver, but the DT bindings should reflect the
h/w. Be happy that i2c-gpio-mux is allowed. I could easily say it
needs to be actual part numbers for compatible strings.

How is it okay that we now have 2 drivers for the same thing: the
i2c-gpio-mux driver and this i2c mux-controller driver with a gpio
mux-controller driver. I'm surprised Wolfram is okay with that.

> This new driver is in fact an i2c-mux driver that (in this aspect) is more
> generic than any of the existing i2c-mux drivers, hence my suggestion.

If it is more generic, then it should be able to replace existing drivers.

> If you see this new driver as something that is superseding the existing
> i2c-mux-gpio driver, I'm sad to inform you that the code is not simply
> not there. i2c-mux-gpio has acpi support and users may provide platform
> data from code. The existing i2c-mux-gpio driver also has the below
> mentioned locking heuristic. Adding all those things to the new driver
> would make it big and unwieldy and having even more unwanted tentacles
> into other subsystems. And why should it be only i2c-mux-gpio that is
> merged into this new i2c-mux driver? Why not implement a mux-pinctrl
> driver for the new mux subsustem and then merge i2c-mux-pinctrl as well?
> I say no, that way lies madness.

Sounds like a good idea to me. I'm not saying you need to merge any of
them right now though (that's Wolfram's call).

None of this has anything to do with the binding though. Compatible
strings should be specific. That's not up for debate. Whether the
driver bound to a compatible string is common or specific to that
compatible string is completely up to the OS. That decision can change
over time, but the binding should not.

>>> I'm also happy to have the lock type as a separate property. One lock
>>> type, e.g. parent-locked, could be the default and adding a 'mux-locked'
>>> property could change that. Would that be ok?
>>
>> I prefer this. Then existing bindings can use it.
>>
>>> Or should it be a requirement that one of 'mux-locked'/'parent-locked'
>>> is always present?
>>
>> I would make it boolean and make not present be the more common case.
>> Not present could also mean determined via other means as you have
>> today with existing bindings. Maybe then you need both properties.
>
> Ok. Lets define that every type of i2c-mux have a default locking.
> If no property is found that overrides this default, a heuristic may
> be applied that is only allowed to very defensively use non-default
> locking (like the current heuristics in i2c-mux-gpio/i2c-mux-pinctrl
> attempt to do, they only adjust the locking for impossible cases that
> would invariably lead to a deadlock with the default locking). Then,
> there should never exist a need to enforce the default locking with
> a property.
>
> And given that no current i2c-mux that is mux-locked by default even
> have the ability to be parent-locked, there is (currently) no need
> for more than a bool 'mux-locked' property.

Okay, good.

>>>> I'm not sure I get the mux vs. parent locked fully. How do I determine
>>>> what type I have? We already have bindings for several types of i2c
>>>> muxes. How does the locking annotation fit into those?
>>>
>>> We have briefly discussed this before [1] in the context of i2c-mux-gpio
>>> and i2c-mux-pinctrl, when I added the mux-locked/parent-locked distinction
>>> to the i2c-mux infrastructure (it wasn't named mux-locked/parent-locked
>>> way back then though). There is more detail on what the difference is
>>> between the two in Documentation/i2c/i2c-topology.
>>>
>>> Side note regarding your remark "use an I2C controlled mux instead"; it
>>> appears that I'm not alone [2] with this kind of requirement...
>>>
>>> [1] https://lkml.org/lkml/2016/1/6/437
>>> [2] http://marc.info/?t=147877959100002&r=1&w=2
>>>
>>> But, now that I have pondered on this for a year or so, I firmly
>>> believe it was a mistake to have the code in i2c-mux-gpio and
>>> i2c-mux-pinctrl automatically try to deduce if the mux should be
>>> mux-locked or parent-locked. It might be easy to make that call
>>> in some trivial cases, but it is not difficult to dream up
>>> scenarios where it would be extremely hard for the code to get
>>> this decision right. It's just fragile. But now we have code in
>>> those two muxes that has unwanted tentacles into the guts of the
>>> gpio and pinctrl subsystems. Hopefully those unwanted tentacles
>>> can be replaced with something based on device links? However, it
>>> is still not hard to come up with scenarios that will require
>>> manual intervention in order to select the right kind of i2c mux
>>> locking. So, I fear that we have inadequate code trying to make a
>>> decision automatically, and that we at some point down the line
>>> will have some impossible case needing a binding that trumps the
>>> heuristic. Why have a heuristic at all in that case? In short, it
>>> should have been a binding from the start, methinks.
>>>
>>> That was a long rant regarding i2c-mux-gpio and i2c-mux-pinctrl.
>>> I obviously think it is bad to have this new i2c mux go in the
>>> same direction as those two drivers. I.e. I think a binding that
>>> specifies the desired locking is preferable to any attempted
>>> heuristic.
>>
>> Having a separate, common property(ies) for these would solve this then.
>
> Yep, the plan is now to make this new i2c-mux parent-locked by default
> and then look for an optional mux-locked property, thus eliminating the
> comma and the suffix in the compatible string, leaving only whatever is
> eventually picked from 'i2c-mux-simple', 'i2c-mux', 'i2c-mux-generic' and
> 'i2c-mux-whatever'.
>
> Do I need to re-document the mux-locked/parent-locked difference? I.e.,
> is it enough to refer to Documentation/i2c/i2c-topology, or does the
> Documentation/devicetree subdir need to self-contained?

We do try to keep things self contained, but I'm okay with not
duplicating everything in the binding. Just try to summarize what
mux-locked means.

Rob