Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751517AbdIMNjT (ORCPT ); Wed, 13 Sep 2017 09:39:19 -0400 Received: from mail.kernel.org ([198.145.29.99]:45530 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750993AbdIMNjS (ORCPT ); Wed, 13 Sep 2017 09:39:18 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 3EC6821EA8 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=robh@kernel.org X-Google-Smtp-Source: AOwi7QCkNg/awK7wLpqOxv2tTQ6gaFh2ACJpdzWWjb0Utmwa0TPnC3O9VSXuLjXeDLaciZ3GBaA5Y49kyGakwOjuDZM= MIME-Version: 1.0 In-Reply-To: References: <20170905164221.11266-1-hdegoede@redhat.com> <20170905164221.11266-11-hdegoede@redhat.com> <20170912222027.hzpejmmxkwbw6seu@rob-hp-laptop> From: Rob Herring Date: Wed, 13 Sep 2017 08:38:55 -0500 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v2 10/11] staging: typec: fusb302: Hook up mux support using tcpc_gen_mux support To: Hans de Goede Cc: MyungJoo Ham , Chanwoo Choi , Guenter Roeck , Heikki Krogerus , Darren Hart , Andy Shevchenko , Peter Rosin , Mathias Nyman , "linux-kernel@vger.kernel.org" , platform-driver-x86@vger.kernel.org, devel@driverdev.osuosl.org, Kuppuswamy Sathyanarayanan , Sathyanarayanan Kuppuswamy Natarajan , Greg Kroah-Hartman , Linux USB List , Mark Rutland , "devicetree@vger.kernel.org" Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4476 Lines: 114 On Wed, Sep 13, 2017 at 3:56 AM, Hans de Goede 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 >>> Cc: Mark Rutland >>> Cc: devicetree@vger.kernel.org >>> Signed-off-by: Hans de Goede >>> --- >>> 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