Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751800AbdIMPse (ORCPT ); Wed, 13 Sep 2017 11:48:34 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39022 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751145AbdIMPsb (ORCPT ); Wed, 13 Sep 2017 11:48:31 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 523015D697 Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=hdegoede@redhat.com Subject: Re: [PATCH v2 10/11] staging: typec: fusb302: Hook up mux support using tcpc_gen_mux support To: Rob Herring 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" References: <20170905164221.11266-1-hdegoede@redhat.com> <20170905164221.11266-11-hdegoede@redhat.com> <20170912222027.hzpejmmxkwbw6seu@rob-hp-laptop> From: Hans de Goede Message-ID: Date: Wed, 13 Sep 2017 17:48:25 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Wed, 13 Sep 2017 15:48:31 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4653 Lines: 126 Hi, On 13-09-17 17:07, Rob Herring wrote: > On Wed, Sep 13, 2017 at 9:06 AM, Hans de Goede wrote: >> Hi, >> >> >> On 13-09-17 15:38, Rob Herring wrote: >>> >>> 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. >> >> >> Right, the mux will be its own node per >> Documentation/devicetree/bindings/mux/mux-controller.txt >> 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 >>>> 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? >> >> >> 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 >> obvious >> (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 ? Regards, Hans