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

[Heikki Krogerus]

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