Re: [PATCH v6 1/6] dt-bindings: usb: add documentation for typec switch simple driver

[Rob Herring <robh@xxxxxxxxxx>]

On Mon, Nov 23, 2020 at 10:01:31PM +0800, Li Jun wrote:
> Some platforms need a simple driver to do some controls according to
> typec orientation, this can be extended to be a generic driver with
> compatible with "typec-orientation-switch".
> 
> Signed-off-by: Li Jun <jun.li@xxxxxxx>
> ---
> changes for v6:
> - Use general mux bindings for typec switch, one typec switch
>   may have 1 or 2 GPIOs for channel selection, if 1 GPIO, only
>   can be used to select cc1 or cc2; if 2 GPIOs, the second GPIO
>   can be used to deselect both channels or keep normal operations.
> - Add one more connection to usb controller.
>  
> No changes for v5.
> 
> changes on v4:
> - Use compatible instead of bool property for switch matching.
> - Change switch GPIO to be switch simple.
> - Change the active channel selection GPIO to be optional.
> 
> previous discussion:
> http://patchwork.ozlabs.org/patch/1054342/
> 
>  .../bindings/usb/typec-switch-simple.yaml          | 122 +++++++++++++++++++++
>  1 file changed, 122 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/typec-switch-simple.yaml b/Documentation/devicetree/bindings/usb/typec-switch-simple.yaml
> new file mode 100644
> index 0000000..030ade5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/typec-switch-simple.yaml
> @@ -0,0 +1,122 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/usb/typec-switch-simple.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Typec Orientation Switch Simple Solution Bindings
> +
> +maintainers:
> +  - Li Jun <jun.li@xxxxxxx>
> +
> +description: |-
> +  USB SuperSpeed (SS) lanes routing to which side of typec connector is
> +  decided by orientation, this maybe achieved by some simple control like
> +  GPIO toggle.
> +
> +properties:
> +  compatible:
> +    const: typec-orientation-switch
> +
> +  '#mux-control-cells':
> +    const: 0

Who is the mux-control consumer?

> +
> +  switch-gpios:
> +    description: |
> +      GPIO specifiers to select the target channel of mux.
> +      The first GPIO is for cc1 and cc2 selection, the GPIO flag use
> +      GPIO_ACTIVE_HIGH if GPIO physical state high is for cc1; or use
> +      GPIO_ACTIVE_LOW if GPIO physical state low is for cc1.
> +      The second gpio is to deselect any channles by places all channels
> +      in high-impedance state to reduce current consumption, the gpio flag
> +      use GPIO_ACTIVE_HIGH if GPIO physical state high is for
> +      high-impedance state (so low for normal operations); or Use
> +      GPIO_ACTIVE_LOW if GPIO physical state low is for high-impedance
> +      state(so high for normal operations).
> +    minItems: 1
> +    maxItems: 2
> +
> +  idle-state:
> +    description: -|
> +      For TYPEC_ORIENTATION_NONE by deselect both channels if supported.
> +    const: 2
> +
> +  # Standard properties described in
> +  # Documentation/devicetree/bindings/mux/mux-controller.txt
> +
> +  port:
> +    type: object
> +    additionalProperties: false
> +    description: -|
> +      Connection to the remote endpoint using OF graph bindings that model
> +      SS data bus to typec connector.
> +
> +    properties:
> +      '#address-cells':
> +        const: 1
> +
> +      '#size-cells':
> +        const: 0
> +
> +      endpoint@0:
> +        type: object
> +        description: Endpoint connected to typec connector.
> +        additionalProperties: false
> +
> +        properties:
> +          reg:
> +            const: 0
> +          remote-endpoint: true
> +
> +        required:
> +          - remote-endpoint
> +
> +      endpoint@1:
> +        type: object
> +        description: Endpoint connected to usb controller.
> +        additionalProperties: false
> +
> +        properties:
> +          reg:
> +            const: 1
> +          remote-endpoint: true
> +
> +        required:
> +          - remote-endpoint
> +
> +    required:
> +      - endpoint@0
> +
> +required:
> +  - compatible
> +  - port
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    typec_switch: mux-controller {
> +        compatible = "typec-orientation-switch";
> +        #mux-control-cells = <0>;
> +        pinctrl-names = "default";
> +        pinctrl-0 = <&pinctrl_ss_sel>;
> +        switch-gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>,
> +                       <&gpio2 20 GPIO_ACTIVE_HIGH>;
> +        idle-state = <2>;
> +
> +        port {
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +
> +                usb3_orien_sel: endpoint@0 {
> +                        reg = <0>;
> +                        remote-endpoint = <&typec_con_ss>;
> +                };
> +
> +                usb3_con_data: endpoint@1 {
> +                        reg = <1>;
> +                        remote-endpoint = <&dwc3_0>;
> +                };
> +        };

This still seems unnecessarily complicated. What I had in mind is just 
something like this:

mux: mux-controller {
  compatible = "gpio-mux";
  #mux-control-cells = <0>;
  mux-gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>,
              <&gpio2 20 GPIO_ACTIVE_HIGH>;
  idle-state = <2>;
};

connector {
  compatible = "usb-c-connector";
  mux-controls = <&mux>;
  ...
};

Then you aren't re-implementing the gpio-mux driver.

This should work for *any* mux implementation as long as the mux states 
always have the same meanings from the perspective of the connector.

Of course, I have little visibility into the possible h/w 
implementations for Type-C and maybe this isn't sufficient. So input 
from Type-C experts would be helpful.

Rob