Re: [PATCH v5 1/9] dt-bindings: usb: Add Type-C switch binding

[Pin-yen Lin]

On Wed, Jun 29, 2022 at 10:33 PM Pin-yen Lin <treapking@xxxxxxxxxxxx> wrote:
>
> On Wed, Jun 29, 2022 at 2:23 AM Rob Herring <robh@xxxxxxxxxx> wrote:
> >
> > On Mon, Jun 27, 2022 at 02:43:39PM -0700, Prashant Malani wrote:
> > > Hello Rob,
> > >
> > > On Mon, Jun 27, 2022 at 2:04 PM Rob Herring <robh@xxxxxxxxxx> wrote:
> > > >
> > > > On Wed, Jun 22, 2022 at 05:34:30PM +0000, Prashant Malani wrote:
> > > > > Introduce a binding which represents a component that can control the
> > > > > routing of USB Type-C data lines as well as address data line
> > > > > orientation (based on CC lines' orientation).
> > > > >
> > > > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> > > > > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>
> > > > > Reviewed-by: Nícolas F. R. A. Prado <nfraprado@xxxxxxxxxxxxx>
> > > > > Tested-by: Nícolas F. R. A. Prado <nfraprado@xxxxxxxxxxxxx>
> > > > > Signed-off-by: Prashant Malani <pmalani@xxxxxxxxxxxx>
> > > > > ---
> > > > >
> > > > > Changes since v4:
> > > > > - Added Reviewed-by tags.
> > > > > - Patch moved to 1/9 position (since Patch v4 1/7 and 2/7 were
> > > > >   applied to usb-next)
> > > > >
> > > > > Changes since v3:
> > > > > - No changes.
> > > > >
> > > > > Changes since v2:
> > > > > - Added Reviewed-by and Tested-by tags.
> > > > >
> > > > > Changes since v1:
> > > > > - Removed "items" from compatible.
> > > > > - Fixed indentation in example.
> > > > >
> > > > >  .../devicetree/bindings/usb/typec-switch.yaml | 74 +++++++++++++++++++
> > > > >  1 file changed, 74 insertions(+)
> > > > >  create mode 100644 Documentation/devicetree/bindings/usb/typec-switch.yaml
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/usb/typec-switch.yaml b/Documentation/devicetree/bindings/usb/typec-switch.yaml
> > > > > new file mode 100644
> > > > > index 000000000000..78b0190c8543
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/usb/typec-switch.yaml
> > > > > @@ -0,0 +1,74 @@
> > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id: http://devicetree.org/schemas/usb/typec-switch.yaml#
> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > +
> > > > > +title: USB Type-C Switch
> > > > > +
> > > > > +maintainers:
> > > > > +  - Prashant Malani <pmalani@xxxxxxxxxxxx>
> > > > > +
> > > > > +description:
> > > > > +  A USB Type-C switch represents a component which routes USB Type-C data
> > > > > +  lines to various protocol host controllers (e.g USB, VESA DisplayPort,
> > > > > +  Thunderbolt etc.) depending on which mode the Type-C port, port partner
> > > > > +  and cable are operating in. It can also modify lane routing based on
> > > > > +  the orientation of a connected Type-C peripheral.
> > > > > +
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    const: typec-switch
> > > > > +
> > > > > +  mode-switch:
> > > > > +    type: boolean
> > > > > +    description: Specify that this switch can handle alternate mode switching.
> > > > > +
> > > > > +  orientation-switch:
> > > > > +    type: boolean
> > > > > +    description: Specify that this switch can handle orientation switching.
> > > > > +
> > > > > +  ports:
> > > > > +    $ref: /schemas/graph.yaml#/properties/ports
> > > > > +    description: OF graph binding modelling data lines to the Type-C switch.
> > > > > +
> > > > > +    properties:
> > > > > +      port@0:
> > > > > +        $ref: /schemas/graph.yaml#/properties/port
> > > > > +        description: Link between the switch and a Type-C connector.
> > > > > +
> > > > > +    required:
> > > > > +      - port@0
> > > > > +
> > > > > +required:
> > > > > +  - compatible
> > > > > +  - ports
> > > > > +
> > > > > +anyOf:
> > > > > +  - required:
> > > > > +      - mode-switch
> > > > > +  - required:
> > > > > +      - orientation-switch
> > > > > +
> > > > > +additionalProperties: true
> > > > > +
> > > > > +examples:
> > > > > +  - |
> > > > > +    drm-bridge {
> > > > > +        usb-switch {
> > > > > +            compatible = "typec-switch";
> > > >
> > > > Unless this child is supposed to represent what the parent output is
> > > > connected to, this is just wrong as, at least for the it6505 chip, it
> > > > doesn't know anything about Type-C functionality. The bridge is
> > > > just a protocol converter AFAICT.
> > >
> > > I'll let Pin-Yen comment on the specifics of the it6505 chip.
> >
> > We're all waiting...
>
> Yes it6505 is just a protocol converter. But in our use case, the output DP
> lines are connected to the Type-C ports and the chip has to know which
> port has DP Alt mode enabled. Does this justify a child node here?
>
> Does it make more sense if we we eliminate the usb-switch node here
> and list the ports in the top level?
> >
> > > > If the child node represents what the output is connected to (like a
> > > > bus), then yes that is a pattern we have used.
> > >
> > > For the anx7625 case, the child node does represent what the output is connected
> > > to (the usb-c-connector via the switch). Does that not qualify? Or do you mean
> > > the child node should be a usb-c-connector itself?
> > >
> > > > For example, a panel
> > > > represented as child node of a display controller. However, that only
> > > > works for simple cases, and is a pattern we have gotten away from in
> > > > favor of using the graph binding.
> > >
> > > The child node will still use a OF graph binding to connect to the
> > > usb-c-connector.
> > > Is that insufficient to consider a child node usage here?
> > > By "using the graph binding", do you mean "only use the top-level ports" ?
> > > I'm trying to clarify this, so that it will inform future versions and patches.
> >
> > What I want to see is block diagrams of possible h/w with different
> > scenarios and then what the binding looks like in those cases. The
> > switching/muxing could be in the SoC, a bridge chip, a Type C
> > controller, a standalone mux chip, or ????. If you want a somewhat
> > genericish binding, then you need to consider all of these.

Then, is it suitable to put the switch binding into the drivers own bindings
(i.e., the bindings for it6505 and anx7625), and introduce some helper
functions to eliminate the duplication in the code?
Though we will have two similar bindings in two drivers with this approach.

> >
> > I don't really have the b/w to work thru all this (and switch/mux is
> > just one part of dealing with Type-C). This is just one of about a
> > hundred patches I get to review a week.
> >
> > Rob