RE: [PATCH] dt-bindings: usb: typec-tcpci: convert to DT schema format

[Jun Li]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> Sent: Tuesday, March 28, 2023 6:43 PM
> To: Jun Li <jun.li@xxxxxxx>; Peng Fan (OSS) <peng.fan@xxxxxxxxxxx>;
> robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx;
> gregkh@xxxxxxxxxxxxxxxxxxx
> Cc: linux-usb@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; Peng Fan <peng.fan@xxxxxxx>
> Subject: Re: [PATCH] dt-bindings: usb: typec-tcpci: convert to DT schema
> format
> 
> On 28/03/2023 11:24, Jun Li wrote:
> > Hi Krzysztof,
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> >> Sent: Monday, March 27, 2023 3:41 PM
> >> To: Peng Fan (OSS) <peng.fan@xxxxxxxxxxx>; robh+dt@xxxxxxxxxx;
> >> krzysztof.kozlowski+dt@xxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; Jun Li
> >> <jun.li@xxxxxxx>
> >> Cc: linux-usb@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> >> linux-kernel@xxxxxxxxxxxxxxx; Peng Fan <peng.fan@xxxxxxx>
> >> Subject: Re: [PATCH] dt-bindings: usb: typec-tcpci: convert to DT
> >> schema format
> >>
> >> On 23/03/2023 07:58, Peng Fan (OSS) wrote:
> >>> From: Peng Fan <peng.fan@xxxxxxx>
> >>>
> >>> Convert the binding to DT schema format. The default speed is HS, so
> >>> add a dummy port@0 in the example.
> >>>
> >>> Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> >>> ---
> >>>
> >>> V1:
> >>>  The default speed is HS, so port@0 is not added for some device
> >>> tree, however the usb-c-connector requires port@0. Not sure we
> >>> should drop the required port@0 from usb-c-connector schema or add a
> >>> dummy port@0 for tcpci as what this patch does.
> >>
> >> imx8mq-librem5-devkit has full port@0 so just use similar approach.
> >>
> >>>
> >>>  .../devicetree/bindings/usb/typec-tcpci.txt   | 49 ------------
> >>>  .../devicetree/bindings/usb/typec-tcpci.yaml  | 80
> >>> +++++++++++++++++++
> >>>  2 files changed, 80 insertions(+), 49 deletions(-)  delete mode
> >>> 100644 Documentation/devicetree/bindings/usb/typec-tcpci.txt
> >>>  create mode 100644
> >>> Documentation/devicetree/bindings/usb/typec-tcpci.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/usb/typec-tcpci.txt
> >>> b/Documentation/devicetree/bindings/usb/typec-tcpci.txt
> >>> deleted file mode 100644
> >>> index 2082522b1c32..000000000000
> >>> --- a/Documentation/devicetree/bindings/usb/typec-tcpci.txt
> >>> +++ /dev/null
> >>> @@ -1,49 +0,0 @@
> >>> -TCPCI(Typec port cotroller interface) binding
> >>> ----------------------------------------------
> >>> -
> >>> -Required properties:
> >>> -- compatible:       should be set one of following:
> >>> -		    - "nxp,ptn5110" for NXP USB PD TCPC PHY IC ptn5110.
> >>> -
> >>> -- reg:              the i2c slave address of typec port controller device.
> >>> -- interrupt-parent: the phandle to the interrupt controller which
> provides
> >>> -                    the interrupt.
> >>> -- interrupts:       interrupt specification for tcpci alert.
> >>> -
> >>> -Required sub-node:
> >>> -- connector: The "usb-c-connector" attached to the tcpci chip, the
> >>> bindings
> >>> -  of connector node are specified in
> >>> -  Documentation/devicetree/bindings/connector/usb-connector.yaml
> >>> -
> >>> -Example:
> >>> -
> >>> -ptn5110@50 {
> >>> -	compatible = "nxp,ptn5110";
> >>> -	reg = <0x50>;
> >>> -	interrupt-parent = <&gpio3>;
> >>> -	interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
> >>> -
> >>> -	usb_con: connector {
> >>> -		compatible = "usb-c-connector";
> >>> -		label = "USB-C";
> >>> -		data-role = "dual";
> >>> -		power-role = "dual";
> >>> -		try-power-role = "sink";
> >>> -		source-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM)>;
> >>> -		sink-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM)
> >>> -			     PDO_VAR(5000, 12000, 2000)>;
> >>> -		op-sink-microwatt = <10000000>;
> >>> -
> >>> -		ports {
> >>> -			#address-cells = <1>;
> >>> -			#size-cells = <0>;
> >>> -
> >>> -			port@1 {
> >>> -				reg = <1>;
> >>> -				usb_con_ss: endpoint {
> >>> -					remote-endpoint = <&usb3_data_ss>;
> >>> -				};
> >>> -			};
> >>> -		};
> >>> -	};
> >>> -};
> >>> diff --git a/Documentation/devicetree/bindings/usb/typec-tcpci.yaml
> >>> b/Documentation/devicetree/bindings/usb/typec-tcpci.yaml
> >>> new file mode 100644
> >>> index 000000000000..067d3b032e3d
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/usb/typec-tcpci.yaml
> >>
> >> Is this a binding for PTN5110 or for generic tcpci? Looks like the
> >> first, thus name should be rather device specific, so nxp,ptn5110.
> >> Specially that there are other tcpci chips in separate bindings.
> >
> > This binding doc is target for generic tcpci, ptn5110 is the one
> 
> 
> Does this mean that TCPCI requires every device to have exactly one interrupt
> (no 0, no 2, exactly 1), 

Per spec below, TCPCI does need interrupt for alert, but no limit on the number.

"The TCPC uses I2C to communicate with the TCPM. The TCPC is an I2C slave
with Alert# signal for requesting attention."

> no supplies and no additional GPIOs (like reset
> GPIO)?

Those are out of scope of spec, I think it can be added as optional.

> 
> Because this is what this binding is saying.
> 
> > fully compliance with tcpci spec, if change it to be only specific to
> > nxp,ptn5110, my understanding is then other chips need duplicate a
> > binding doc even common tcpci binding and driver is enough for them.
> 
> Depends. Usually we have common schema used by actual device schemas. If
> TCPCI-compliant device cannot have additional properties,

My understanding was TCPCI-compliant device can have additional optional
properties.

> then this one here
> looks fine.
> 
> One more thing - typec-tcpci is a bit redundant "tc" means typec, so basicaly
> you said "typec-typec-pci". This shouold be then just typec-pci or tcpci.

Makes sense, "tcpci.yaml" looks better for me.

Thanks
Li Jun
> 
> Best regards,
> Krzysztof