Re: [PATCH v3 1/4] dt-bindings: Add cros-ec Type C port driver

[Stephen Boyd <swboyd@chromium.org>]

Don't know what happened but my MUA can't seem to thread these patches.
I wonder if something got messed up during send?

Quoting Prashant Malani (2020-02-19 16:30:55)
> Some Chrome OS devices with Embedded Controllers (EC) can read and
> modify Type C port state.
> 
> Add an entry in the DT Bindings documentation that lists out the logical
> device and describes the relevant port information, to be used by the
> corresponding driver.
> 
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
> 
> Changes in v3:
> - Fixed license identifier.
> - Renamed "port" to "connector".
> - Made "connector" be a "usb-c-connector" compatible property.
> - Updated port-number description to explain min and max values,
>   and removed $ref which was causing dt_binding_check errors.
> - Fixed power-role, data-role and try-power-role details to make
>   dt_binding_check pass.
> - Fixed example to include parent EC SPI DT Node.
> 
> Changes in v2:
> - No changes. Patch first introduced in v2 of series.
> 
>  .../bindings/chrome/google,cros-ec-typec.yaml | 86 +++++++++++++++++++
>  1 file changed, 86 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> 
> diff --git a/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> new file mode 100644
> index 00000000000000..97fd982612f120
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> @@ -0,0 +1,86 @@
> +# SPDX-License-Identifier: GPL-2.0-only

Can it be GPL or BSD 2 clause?

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/chrome/google,cros-ec-typec.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Google Chrome OS EC(Embedded Controller) Type C port driver.
> +
> +maintainers:
> +  - Benson Leung <bleung@chromium.org>
> +  - Prashant Malani <pmalani@chromium.org>
> +
> +description:
> +  Chrome OS devices have an Embedded Controller(EC) which has access to
> +  Type C port state. This node is intended to allow the host to read and
> +  control the Type C ports. The node for this device should be under a
> +  cros-ec node like google,cros-ec-spi.
> +
> +properties:
> +  compatible:
> +    const: google,cros-ec-typec
> +
> +  connector:
> +    description: A node that represents a physical Type C connector port
> +      on the device.
> +    type: object
> +    properties:
> +      compatible:
> +        const: usb-c-connector
> +      port-number:
> +        description: The number used by the Chrome OS EC to identify
> +          this type C port. Valid values are 0 - (EC_USB_PD_MAX_PORTS - 1).

What is EC_USB_PD_MAX_PORTS? Can this be done through a reg property
instead?

> +      power-role:
> +        description: Determines the power role that the Type C port will
> +          adopt.
> +        maxItems: 1

I don't think this is necessary with enum below. schema can figure out
that max is 1.

> +        contains:
> +          enum:
> +            - sink
> +            - source
> +            - dual

Other bindings have newlines between properties. Can you please add them
to improve readability?

> +      data-role:
> +        description: Determines the data role that the Type C port will
> +          adopt.
> +        maxItems: 1
> +        contains:
> +          enum:
> +            - host
> +            - device
> +            - dual
> +      try-power-role:
> +        description: Determines the preferred power role of the Type C port.

How does this interact with power-role above? Is it different?

> +        maxItems: 1
> +        contains:
> +          enum:
> +            - sink
> +            - source
> +            - dual
> +
> +    required:
> +      - port-number
> +      - power-role
> +      - data-role
> +      - try-power-role
> +
> +required:
> +  - compatible
> +  - connector
> +
> +examples:
> +  - |+
> +    cros_ec: ec@0 {
> +      compatible = "google,cros-ec-spi";
> +
> +      typec {
> +        compatible = "google,cros-ec-typec";
> +
> +        usb_con: connector {
> +          compatible = "usb-c-connector";
> +          port-number = <0>;
> +          power-role = "dual";
> +          data-role = "dual";
> +          try-power-role = "source";
> +        };

I thought that perhaps this would be done with the OF graph APIs instead
of being a child of the ec node. I don't see how the usb connector is
anything besides a child of the top-level root node because it's
typically on the board. We put board level components at the root.

Yes, the connector is intimately involved with the EC here, but I would
think that we would have an OF graph connection from the USB controller
on the SoC to the USB connector, traversing through anything that may be
in that path, such as a USB hub. Maybe the connector node itself can
point to the EC type-c controller with some property like

	connector {
		...
		type-c-manager = <&phandle_to_typec_manager>
	};

So in the end it would look like this (note that the ec doesn't have a sub-node
to make a driver probe):

	/ {
		connector@0 {
			compatible = "usb-c-connector";
			label = "left";
			reg = <0>;
			power-role = "dual";
			type-c-manager = <&cros_ec>;
			...

			ports { 
				#address-cells = <1>;
				#size-cells = <0>;

				port@0 {
					reg = <0>;
					left_ufp: endpoint {
						remote-endpoint = <&usb_controller0>;
					};
				};
			};
		};

		connector@1 {
			compatible = "usb-c-connector";
			label = "right";
			reg = <1>;
			power-role = "dual";
			type-c-manager = <&cros_ec>;
			...

			ports { 
				#address-cells = <1>;
				#size-cells = <0>;

				port@0 {
					reg = <0>;
					right_ufp: endpoint {
						remote-endpoint = <&usb_controller1>;
					};
				};
			};
		};

		soc@0 {
			#address-cells = <1>;
			#size-cells = <0>;

			spi@a000000 {
				compatible = "soc,spi-controller";
				reg = <0xa000000 0x1000>;
				cros_ec: ec@0 {
					compatible = "google,cros-ec-spi";
					reg = <0>;
				};
			};

			usb@ca00000 {
				compatible = "soc,dwc3-controller";
				reg = <0xca00000 0x1000>;

				ports {
					port@0 {
						reg = <0>;
						usb_controller0: endpoint {
							remote-endpoint = <&left_ufp>;
						};
					};
				};
			};

			usb@ea00000 {
				compatible = "soc,dwc3-controller";
				reg = <0xea00000 0x1000>;

				ports {
					port@0 {
						reg = <0>;
						usb_controller1: endpoint {
							remote-endpoint = <&right_ufp>;
						};
					};
				};
			};
		};
	};