Re: [PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props

From: Heikki Krogerus
Date: Tue May 12 2020 - 09:42:04 EST


Hi guys,

On Mon, May 11, 2020 at 01:46:35PM -0700, Prashant Malani wrote:
> 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@xxxxxxxxxxxx>
> > > ---
> > > .../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?

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

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

thanks,

--
heikki