Re: [PATCH v2 10/11] staging: typec: fusb302: Hook up mux support using tcpc_gen_mux support

From: Hans de Goede
Date: Wed Sep 13 2017 - 11:48:37 EST


On 13-09-17 17:07, Rob Herring wrote:
On Wed, Sep 13, 2017 at 9:06 AM, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:

On 13-09-17 15:38, Rob Herring wrote:

On Wed, Sep 13, 2017 at 3:56 AM, Hans de Goede <hdegoede@xxxxxxxxxx>


On 13-09-17 00:20, Rob Herring wrote:

On Tue, Sep 05, 2017 at 06:42:20PM +0200, Hans de Goede wrote:

Add mux support to the fusb302 driver, call devm_tcpc_gen_mux_create()
to let the generic tcpc_mux_dev code create a tcpc_mux_dev for us.

Also document the mux-names used by the generic tcpc_mux_dev code in
our devicetree bindings.

Cc: Rob Herring <robh+dt@xxxxxxxxxx>
Cc: Mark Rutland <mark.rutland@xxxxxxx>
Cc: devicetree@xxxxxxxxxxxxxxx
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
Documentation/devicetree/bindings/usb/fcs,fusb302.txt | 3 +++
drivers/staging/typec/fusb302/fusb302.c | 11
2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/usb/fcs,fusb302.txt
index 472facfa5a71..63d639eadacd 100644
--- a/Documentation/devicetree/bindings/usb/fcs,fusb302.txt
+++ b/Documentation/devicetree/bindings/usb/fcs,fusb302.txt
@@ -6,6 +6,9 @@ Required properties :
- interrupts : Interrupt specifier
Optional properties :
+- mux-controls : List of mux-ctrl-specifiers containing 1 or
+- mux-names : "type-c-mode-mux" when using 1 mux, or
+ "type-c-mode-mux", "usb-role-mux" when
2 muxes

I'm not sure this is the right place for this. The mux is outside the
FUSB302. In a type-C connector node or USB phy node would make more
sense to me.

The mux certainly does not belong in the USB phy node, it sits between
and the Type-C connector and can for example also mux the Type-C pins to
Port PHY.

Thinking about this some more, the mux(es) should be its own node(s).
Then the question is where to put the nodes.

Right, the mux will be its own node per
the bindings bit this patch is adding is only adding phandles pointing
to that mux-node as the fusb302 "consumes" the mux functionality.

So as such (the fusb302 is the component which should control the mux)
I do believe that the bindings this patch adds are correct.

Humm, that's not how the mux binding works. The mux controller is what
drives the mux select lines and is the provider. The consumer is the
mux device itself. What decides the mux state is determined by what
you are muxing, not which node has mux-controls property.

By putting mux-controls in fusb302 node, you are saying fusb302 is a
mux (or contains a mux).

As for putting it in a type-C connector node, currently we do not have
a node,

Well, you should. Type-C connectors are certainly complicated enough
that we'll need one. Plus we already require connector nodes for
display outputs, so what do we do once you add display muxing?

An interesting question, I'm working on this for x86 + ACPI boards actually,
not a board using DT I've been adding DT bindings docs for device-properties
I use because that seems like the right thing to do where the binding is
(which I believe it is in this case as the fusb302 is the mux consumer) and
because the device-property code should work the same on x86 + ACPI
(where some platform-specific drivers attach the device properties) and
on e.g. ARM + DT.

The rest should probably be left to be figured out when an actual DT
using device using the fusb302 or another Type-C controller shows up.

Well this is a new one (maybe, I suppose others have sneaked by). If
ACPI folks want to use DT bindings, then what do I care. But I have no
interest in reviewing ACPI properties. The whole notion of sharing
bindings between DT and ACPI beyond anything trivial is flawed IMO.
The ptifalls have been discussed multiple times before, so I'm not
going to repeat them here.

Ok, so shall I just drop the Documentation/devicetree/bindings/usb/fcs,fusb302.txt
part of this patch then ?