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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Sep 13, 2017 at 3:56 AM, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> Hi,
>
>
> 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
>>> b/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 2
>>> muxes
>>> +- mux-names              : "type-c-mode-mux" when using 1 mux, or
>>> +                           "type-c-mode-mux", "usb-role-mux" when using
>>> 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 the
> USB PHY
> and the Type-C connector and can for example also mux the Type-C pins to a
> Display
> 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.

> As for putting it in a type-C connector node, currently we do not have such
> 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?

> the closest thing we do have to a node describing it is actually the fusb302
> node
> itself. E.g. it may also contain a regulator to turn Vbus on / off (already
> there
> in the code, but I forgot to document this when I added the missing DT
> bindings
> doc for the fusb302 with a previous patch).

Either you can have a vbus-supply property in connector node or it can
be implied that the controller chip provides that. For example, HDMI
connectors have a hpd-gpios property if HPD is connected to GPIO or
they have nothing and it's implicit that the HDMI encoder handles HPD.


> Also these properties:
>
>>>   - fcs,max-sink-microvolt : Maximum voltage to negotiate when configured
>>> as sink
>>>   - fcs,max-sink-microamp  : Maximum current to negotiate when configured
>>> as sink
>>>   - fcs,max-sink-microwatt : Maximum power to negotiate when configured
>>> as sink
>
>
> Have more to do with the charger-IC used (which determines the limits) then
> with
> the fusb302 itself, but the fusb302 needs to know these as it negotiates the
> limits.

Those should probably be elsewhere and not be fusb302 specific. I did
ack that, but it was a single node for a single component which is
fine. But once we start adding more external pieces we need to pay
more attention to the overall structure.

> Likewise the fusb302 negotiates how the data pins will be used and thus to
> which pins
> on the SoC the mux should mux the data pins.
>
> TL;DR: The fusb302 does all the negotiation and ties all the Type-C
> connected
> ICs together, so this seems like the right place for it (it certainly is the
> natural place to put these from a driver code pov).

Things in DT should follow what the h/w design looks like which is not
necessarily aligned with the driver structure. If the USB PD chip
needs information from the charger, then we need a kernel interface
for that.

My concern here is not so much this binding in particular, but rather
that we handle Type-C connectors in a common way and not adhoc with
each platform doing things their own way. Otherwise, we end up with a
mess of platform specific bindings like charger/battery bindings
(though there's some work improving those).

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux