lkml.org 
[lkml]   [2020]   [May]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props
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
> >

\
 
 \ /
  Last update: 2020-05-11 22:47    [W:0.147 / U:0.476 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site