Message-ID: <20200514181655.GB121808@google.com>
Date:   Thu, 14 May 2020 11:16:55 -0700
From:   Prashant Malani <pmalani@...omium.org>
To:     Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc:     Rob Herring <robh@...nel.org>, linux-kernel@...r.kernel.org,
        twawrzynczak@...omium.org, Benson Leung <bleung@...omium.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        Enric Balletbo i Serra <enric.balletbo@...labora.com>,
        Guenter Roeck <groeck@...omium.org>
Subject: Re: [PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props

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:
> > 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@...omium.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?
> 
> 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
> > > > +
> > > > +          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

Powered by blists - more mailing lists