Hi folks,

On Tue, May 12, 2020 at 04:41:54PM +0300, Heikki Krogerus wrote:

Hi guys,

On Mon, May 11, 2020 at 01:46:35PM -0700, Prashant Malani wrote:

quoted

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:

quoted

On Wed, Apr 22, 2020 at 03:22:39PM -0700, Prashant Malani wrote:

quoted

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 <redacted>
---
 .../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."

quoted

quoted

+          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.

quoted

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?

The mode-switch here would actually represent the "consumer" part in
the mux-control bindings. So the mux-controls would describe the
relationship between the "mode-switch" and the mux controller(s),
while the mode-switch property describes the relationship between
something like USB Type-C Port Manager (or this cros_ec function) and
the "mux consumer".

Thanks for the explanation, Heikki.

Hi Rob,

Does the above explanation help clarify the usage here?

If so, shall I upload a new patch version with the additional text
(referencing Type C connector class framework) added to the *-switch
descriptions?

Best regards,

-Prashant

quoted

quoted

quoted

+
+          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

quoted

quoted

+
+          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

thanks,

-- 
heikki