Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 11 May 2020 13:46:35 -0700
From:   Prashant Malani <pmalani@chromium.org>
To:     Rob Herring <robh@kernel.org>
Cc:     linux-kernel@vger.kernel.org, heikki.krogerus@linux.intel.com,
        twawrzynczak@chromium.org, Benson Leung <bleung@chromium.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>,
        Enric Balletbo i Serra <enric.balletbo@collabora.com>,
        Guenter Roeck <groeck@chromium.org>
Subject: Re: [PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props
Message-ID: <20200511204635.GC136540@google.com>
References: <20200422222242.241699-1-pmalani@chromium.org>
 <20200511192800.GA28762@bogus>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200511192800.GA28762@bogus>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Rob,

Thank you for reviewing the patch. Kindly see my comments inline:

On Mon, May 11, 2020 at 02:28:00PM -0500, Rob Herring wrote:
> On Wed, Apr 22, 2020 at 03:22:39PM -0700, Prashant Malani wrote:
> > Add properties for mode, orientation and USB data role switches for
> > Type C connectors. When available, these will allow the Type C connector
> > class port driver to configure the various switches according to USB PD
> > information (like orientation, alt mode etc.) provided by the Chrome OS
> > EC controller.
> > 
> > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > ---
> >  .../bindings/chrome/google,cros-ec-typec.yaml | 27 ++++++++++++++++++-
> >  1 file changed, 26 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> > index 6d7396ab8bee..b5814640aa32 100644
> > --- a/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> > +++ b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> > @@ -21,7 +21,21 @@ properties:
> >      const: google,cros-ec-typec
> >  
> >    connector:
> > -    $ref: /schemas/connector/usb-connector.yaml#
> > +    allOf:
> > +      - $ref: /schemas/connector/usb-connector.yaml#
> > +      - type: object
> > +        properties:
> 
> These don't seem CrOS EC specific, so why document them as such. 

Are you referring to the "mode-switch", "orientation-switch" and
"usb-role-switch" properties? If so, then yes, they aren't Cros EC
specific. The Type C connector class framework requires the nodes to be
named like this, and the cros-ec-typec driver uses this framework, hence
the description here (the Type C connector class framework doesn't have
any bindings).

Would it be better to add in the description string that Type Connector
class expects these switches to be named this way? :

" Reference to a DT node for the USB Type C Multiplexer controlling the
data lines routing for this connector. This switch is assumed registered
with the Type C connector class framework, which requires it to be named
this way."
> 
> > +          mode-switch:
> > +            description: Reference to a DT node for the USB Type C Multiplexer
> > +              controlling the data lines routing for this connector.
> 
> This is for alternate mode muxing I presume.

Yes, that's right.
> 
> We already have a mux-control binding. Why not use that here?

Heikki might be able to offer more insight into why this is the case,
since the connector class framework seems to expect a phandle and for
the device driver to implement a "set" command. Heikki, would you happen to know?

> 
> > +
> > +          orientation-switch:
> > +            description: Reference to a DT node for the USB Type C orientation
> > +              switch for this connector.
> 
> What's in this node?

Similar to the other "-switch", this will contain a phandle to a device
which can control orientation settings for the Type C Mux. The connector
class API assumes the switches are named this way. For example:

orientation-switch:
https://elixir.bootlin.com/linux/v5.7-rc2/source/drivers/usb/typec/mux.c#L64

mode-switch:
https://elixir.bootlin.com/linux/v5.7-rc2/source/drivers/usb/typec/mux.c#L258

> 
> > +
> > +          usb-role-switch:
> > +            description: Reference to a DT node for the USB Data role switch
> > +              for this connector.
> >  
> >  required:
> >    - compatible
> > @@ -49,6 +63,17 @@ examples:
> >              data-role = "dual";
> >              try-power-role = "source";
> >            };
> > +
> > +          connector@1 {
> > +            compatible = "usb-c-connector";
> > +            reg = <1>;
> > +            power-role = "dual";
> > +            data-role = "host";
> > +            try-power-role = "source";
> > +            mode-switch = <&typec_mux>;
> > +            orientation-switch = <&typec_orientation_switch>;
> > +            usb-role-switch = <&typec_mux>;
> > +          };
> >          };
> >        };
> >      };
> > -- 
> > 2.26.1.301.g55bc3eb7cb9-goog
> >