mutiple selectable capabilities in tcpm

PATCH Superseded Kyle Tso 4 participants 9 messages Aug 7, 2023 – Dec 4, 2023 1 message not in the archive

Subsystems (5)

Device tree bindings USB Kernel janitors USB Type-C class +1 more subsystems

lore

Share & tools

lore

Kyle Tso

author

2023-08-07 17:42 UTC #0 lore raw

v2 cover mutiple selectable capabilities in tcpm

updates in v2

dt-bindings: connector: Add child nodes for multiple PD capabilities
- revised the dt-bindings

usb: typec: tcpm: Support multiple capabilities
- Added missing cleanups in the function tcpm_port_unregister_pd

---
Original cover letter:

In commit a7cff92f0635 ("usb: typec: USB Power Delivery helpers for ports and partners"), typec_operations has two new ops .pd_get and .pd_set providing selection of different PD capabilities. This commit implements these two ops in tcpm.

To support multiple capabilities, new dt-binding properties need to be defined to create a tree structure for the driver to get each set of capabilities one by one. The first tier of the child node under connector is called "capabilities". Under this child node lies several 2nd tier of child nodes whose names are in the pattern of ^caps[0-9]+$. And the source and sink capabilities are placed in these nodes.

Kyle Tso (2):
dt-bindings: connector: Add child nodes for multiple PD capabilities
usb: typec: tcpm: Support multiple capabilities

 .../bindings/connector/usb-connector.yaml     |  80 +++-
 drivers/usb/typec/tcpm/tcpm.c                 | 419 ++++++++++++++----
 2 files changed, 390 insertions(+), 109 deletions(-)

Signature · 1 line

2.41.0.585.gd2178a4bd4-goog

Kyle Tso

author

2023-08-07 17:42 UTC #1 lore raw

PATCH v2 1/2 dt-bindings: connector: Add child nodes for multiple PD capabilities

Define a new optional property "capabilities" which is a child node under connector to contain multiple USB Power Delivery capabilities.

Define a new property with pattern (e.g. caps-0, caps-1) which is a child node under "capabilities". Each node contains PDO data of a selectable Power Delivery capability.

Also define common properties for source-pdos, sink-pdos, and op-sink-microwatt that can be referenced.

Signed-off-by Kyle Tso <[email protected]>

---
v1 -> v2:
- move source/sink-pdos to $defs and reference them in properties

 .../bindings/connector/usb-connector.yaml     | 80 +++++++++++++------
 1 file changed, 57 insertions(+), 23 deletions(-)

Documentation/devicetree/bindings/connector/usb-connector.yaml +57 ~~-23~~

index 1c4d3eb87763..c6b02dbda83f 100644
@@ -117,28 +117,10 @@ properties:
   # The following are optional properties for "usb-c-connector" with power
   # delivery support.
   source-pdos:
-    description: An array of u32 with each entry providing supported power
-      source data object(PDO), the detailed bit definitions of PDO can be found
-      in "Universal Serial Bus Power Delivery Specification" chapter 6.4.1.2
-      Source_Capabilities Message, the order of each entry(PDO) should follow
-      the PD spec chapter 6.4.1. Required for power source and power dual role.
-      User can specify the source PDO array via PDO_FIXED/BATT/VAR/PPS_APDO()
-      defined in dt-bindings/usb/pd.h.
-    minItems: 1
-    maxItems: 7
-    $ref: /schemas/types.yaml#/definitions/uint32-array
+    $ref: "#/$defs/source-pdos"
 
   sink-pdos:
-    description: An array of u32 with each entry providing supported power sink
-      data object(PDO), the detailed bit definitions of PDO can be found in
-      "Universal Serial Bus Power Delivery Specification" chapter 6.4.1.3
-      Sink Capabilities Message, the order of each entry(PDO) should follow the
-      PD spec chapter 6.4.1. Required for power sink and power dual role. User
-      can specify the sink PDO array via PDO_FIXED/BATT/VAR/PPS_APDO() defined
-      in dt-bindings/usb/pd.h.
-    minItems: 1
-    maxItems: 7
-    $ref: /schemas/types.yaml#/definitions/uint32-array
+    $ref: "#/$defs/sink-pdos"
 
   sink-vdos:
     description: An array of u32 with each entry, a Vendor Defined Message Object (VDO),

@@ -164,9 +146,7 @@ properties:
     $ref: /schemas/types.yaml#/definitions/uint32-array
 
   op-sink-microwatt:
-    description: Sink required operating power in microwatt, if source can't
-      offer the power, Capability Mismatch is set. Required for power sink and
-      power dual role.
+    $ref: "#/$defs/op-sink-microwatt"
 
   port:
     $ref: /schemas/graph.yaml#/properties/port

@@ -228,6 +208,30 @@ properties:
       SNK_READY for non-pd link.
     type: boolean
 
+  capabilities:
+    description: A child node to contain all the selectable USB Power Delivery capabilities.
+    type: object
+
+    patternProperties:
+      "^caps-[0-9]+$":
+        description: Child nodes under "capabilities" node. Each node contains a selectable USB
+          Power Delivery capability.
+        type: object
+
+        properties:
+          source-pdos:
+            $ref: "#/$defs/source-pdos"
+
+          sink-pdos:
+            $ref: "#/$defs/sink-pdos"
+
+          op-sink-microwatt:
+            $ref: "#/$defs/op-sink-microwatt"
+
+        additionalProperties: false
+
+    additionalProperties: false
+
 dependencies:
   sink-vdos-v1: [ 'sink-vdos' ]
   sink-vdos: [ 'sink-vdos-v1' ]

@@ -235,6 +239,36 @@ dependencies:
 required:
   - compatible
 
+$defs:
+  source-pdos:
+    description: An array of u32 with each entry providing supported power
+      source data object(PDO), the detailed bit definitions of PDO can be found
+      in "Universal Serial Bus Power Delivery Specification" chapter 6.4.1.2
+      Source_Capabilities Message, the order of each entry(PDO) should follow
+      the PD spec chapter 6.4.1. Required for power source and power dual role.
+      User can specify the source PDO array via PDO_FIXED/BATT/VAR/PPS_APDO()
+      defined in dt-bindings/usb/pd.h.
+    minItems: 1
+    maxItems: 7
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+
+  sink-pdos:
+    description: An array of u32 with each entry providing supported power sink
+      data object(PDO), the detailed bit definitions of PDO can be found in
+      "Universal Serial Bus Power Delivery Specification" chapter 6.4.1.3
+      Sink Capabilities Message, the order of each entry(PDO) should follow the
+      PD spec chapter 6.4.1. Required for power sink and power dual role. User
+      can specify the sink PDO array via PDO_FIXED/BATT/VAR/PPS_APDO() defined
+      in dt-bindings/usb/pd.h.
+    minItems: 1
+    maxItems: 7
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+
+  op-sink-microwatt:
+    description: Sink required operating power in microwatt, if source can't
+      offer the power, Capability Mismatch is set. Required for power sink and
+      power dual role.
+
 allOf:
   - if:
       properties:

Signature · 1 line

2.41.0.585.gd2178a4bd4-goog

Rob Herring

maintainer

2023-08-21 16:25 UTC #2 lore raw

Re: [PATCH v2 1/2] dt-bindings: connector: Add child nodes for multiple PD capabilities

Quoted 4 lines · Kyle Tso

Define a new optional property "capabilities" which is a child node under connector to contain multiple USB Power Delivery capabilities.
Define a new property with pattern (e.g. caps-0, caps-1) which is a

A property in json-schema terms, but for DT it's a node not a property. 'Define a child node ...' would be clearer.

Quoted 5 lines

child node under "capabilities". Each node contains PDO data of a selectable Power Delivery capability.
Also define common properties for source-pdos, sink-pdos, and op-sink-microwatt that can be referenced.

Why do we need this? What issue does this solve? The commit message should answer those questions (always).

Quoted 86 lines

Signed-off-by Kyle Tso <[email protected]>

---
v1 -> v2:
- move source/sink-pdos to $defs and reference them in properties

 .../bindings/connector/usb-connector.yaml     | 80 +++++++++++++------
 1 file changed, 57 insertions(+), 23 deletions(-)

Documentation/devicetree/bindings/connector/usb-connector.yaml +27 ~~-23~~

index 1c4d3eb87763..c6b02dbda83f 100644
@@ -117,28 +117,10 @@ properties:
   # The following are optional properties for "usb-c-connector" with power
   # delivery support.
   source-pdos:
-    description: An array of u32 with each entry providing supported power
-      source data object(PDO), the detailed bit definitions of PDO can be found
-      in "Universal Serial Bus Power Delivery Specification" chapter 6.4.1.2
-      Source_Capabilities Message, the order of each entry(PDO) should follow
-      the PD spec chapter 6.4.1. Required for power source and power dual role.
-      User can specify the source PDO array via PDO_FIXED/BATT/VAR/PPS_APDO()
-      defined in dt-bindings/usb/pd.h.
-    minItems: 1
-    maxItems: 7
-    $ref: /schemas/types.yaml#/definitions/uint32-array
+    $ref: "#/$defs/source-pdos"
 
   sink-pdos:
-    description: An array of u32 with each entry providing supported power sink
-      data object(PDO), the detailed bit definitions of PDO can be found in
-      "Universal Serial Bus Power Delivery Specification" chapter 6.4.1.3
-      Sink Capabilities Message, the order of each entry(PDO) should follow the
-      PD spec chapter 6.4.1. Required for power sink and power dual role. User
-      can specify the sink PDO array via PDO_FIXED/BATT/VAR/PPS_APDO() defined
-      in dt-bindings/usb/pd.h.
-    minItems: 1
-    maxItems: 7
-    $ref: /schemas/types.yaml#/definitions/uint32-array
+    $ref: "#/$defs/sink-pdos"
 
   sink-vdos:
     description: An array of u32 with each entry, a Vendor Defined Message Object (VDO),

@@ -164,9 +146,7 @@ properties:
     $ref: /schemas/types.yaml#/definitions/uint32-array
 
   op-sink-microwatt:
-    description: Sink required operating power in microwatt, if source can't
-      offer the power, Capability Mismatch is set. Required for power sink and
-      power dual role.
+    $ref: "#/$defs/op-sink-microwatt"
 
   port:
     $ref: /schemas/graph.yaml#/properties/port

@@ -228,6 +208,30 @@ properties:
       SNK_READY for non-pd link.
     type: boolean
 
+  capabilities:
+    description: A child node to contain all the selectable USB Power Delivery capabilities.
+    type: object
+
+    patternProperties:
+      "^caps-[0-9]+$":
+        description: Child nodes under "capabilities" node. Each node contains a selectable USB
+          Power Delivery capability.
+        type: object
+
+        properties:
+          source-pdos:
+            $ref: "#/$defs/source-pdos"
+
+          sink-pdos:
+            $ref: "#/$defs/sink-pdos"
+
+          op-sink-microwatt:
+            $ref: "#/$defs/op-sink-microwatt"
+
+        additionalProperties: false
+
+    additionalProperties: false
+
 dependencies:
   sink-vdos-v1: [ 'sink-vdos' ]
   sink-vdos: [ 'sink-vdos-v1' ]

Documentation/devicetree/bindings/connector/usb-connector.yaml dependencies: · line 242

Quoted 4 lines

Documentation/devicetree/bindings/connector/usb-connector.yaml +1 -0
@@ -235,6 +239,36 @@ dependencies:
 required:
   - compatible
 
+$defs:

Make this:

$defs:
capabilities:

    properties:
      ...

And then just reference "#/$defs/capabilities" at the top-level and in caps-[0-9] node schema.

You'll need to use unevaluatedProperties instead of additionalProperties as well.

Quoted 35 lines

Documentation/devicetree/bindings/connector/usb-connector.yaml +29 -0

+  source-pdos:
+    description: An array of u32 with each entry providing supported power
+      source data object(PDO), the detailed bit definitions of PDO can be found
+      in "Universal Serial Bus Power Delivery Specification" chapter 6.4.1.2
+      Source_Capabilities Message, the order of each entry(PDO) should follow
+      the PD spec chapter 6.4.1. Required for power source and power dual role.
+      User can specify the source PDO array via PDO_FIXED/BATT/VAR/PPS_APDO()
+      defined in dt-bindings/usb/pd.h.
+    minItems: 1
+    maxItems: 7
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+
+  sink-pdos:
+    description: An array of u32 with each entry providing supported power sink
+      data object(PDO), the detailed bit definitions of PDO can be found in
+      "Universal Serial Bus Power Delivery Specification" chapter 6.4.1.3
+      Sink Capabilities Message, the order of each entry(PDO) should follow the
+      PD spec chapter 6.4.1. Required for power sink and power dual role. User
+      can specify the sink PDO array via PDO_FIXED/BATT/VAR/PPS_APDO() defined
+      in dt-bindings/usb/pd.h.
+    minItems: 1
+    maxItems: 7
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+
+  op-sink-microwatt:
+    description: Sink required operating power in microwatt, if source can't
+      offer the power, Capability Mismatch is set. Required for power sink and
+      power dual role.
+
 allOf:
   - if:
       properties:

Signature · 1 line

2.41.0.585.gd2178a4bd4-goog

Kyle Tso

author

2023-10-18 18:19 UTC #3 lore raw

Re: [PATCH v2 1/2] dt-bindings: connector: Add child nodes for multiple PD capabilities

Quoted 1 line · Rob Herring

Sorry for the late response...

Quoted 9 lines · Kyle Tso

Quoted 4 lines
Define a new optional property "capabilities" which is a child node under connector to contain multiple USB Power Delivery capabilities.
Define a new property with pattern (e.g. caps-0, caps-1) which is a
A property in json-schema terms, but for DT it's a node not a property. 'Define a child node ...' would be clearer.

Will do in the future patch

Quoted 9 lines

Quoted 5 lines
child node under "capabilities". Each node contains PDO data of a selectable Power Delivery capability.
Also define common properties for source-pdos, sink-pdos, and op-sink-microwatt that can be referenced.
Why do we need this? What issue does this solve? The commit message should answer those questions (always).

Will state the rationale in the future patch

Quoted 95 lines

Quoted 86 lines

Signed-off-by Kyle Tso <[email protected]>

---
v1 -> v2:
- move source/sink-pdos to $defs and reference them in properties

 .../bindings/connector/usb-connector.yaml     | 80 +++++++++++++------
 1 file changed, 57 insertions(+), 23 deletions(-)

Documentation/devicetree/bindings/connector/usb-connector.yaml +27 ~~-23~~

index 1c4d3eb87763..c6b02dbda83f 100644
@@ -117,28 +117,10 @@ properties:
   # The following are optional properties for "usb-c-connector" with power
   # delivery support.
   source-pdos:
-    description: An array of u32 with each entry providing supported power
-      source data object(PDO), the detailed bit definitions of PDO can be found
-      in "Universal Serial Bus Power Delivery Specification" chapter 6.4.1.2
-      Source_Capabilities Message, the order of each entry(PDO) should follow
-      the PD spec chapter 6.4.1. Required for power source and power dual role.
-      User can specify the source PDO array via PDO_FIXED/BATT/VAR/PPS_APDO()
-      defined in dt-bindings/usb/pd.h.
-    minItems: 1
-    maxItems: 7
-    $ref: /schemas/types.yaml#/definitions/uint32-array
+    $ref: "#/$defs/source-pdos"
 
   sink-pdos:
-    description: An array of u32 with each entry providing supported power sink
-      data object(PDO), the detailed bit definitions of PDO can be found in
-      "Universal Serial Bus Power Delivery Specification" chapter 6.4.1.3
-      Sink Capabilities Message, the order of each entry(PDO) should follow the
-      PD spec chapter 6.4.1. Required for power sink and power dual role. User
-      can specify the sink PDO array via PDO_FIXED/BATT/VAR/PPS_APDO() defined
-      in dt-bindings/usb/pd.h.
-    minItems: 1
-    maxItems: 7
-    $ref: /schemas/types.yaml#/definitions/uint32-array
+    $ref: "#/$defs/sink-pdos"
 
   sink-vdos:
     description: An array of u32 with each entry, a Vendor Defined Message Object (VDO),

@@ -164,9 +146,7 @@ properties:
     $ref: /schemas/types.yaml#/definitions/uint32-array
 
   op-sink-microwatt:
-    description: Sink required operating power in microwatt, if source can't
-      offer the power, Capability Mismatch is set. Required for power sink and
-      power dual role.
+    $ref: "#/$defs/op-sink-microwatt"
 
   port:
     $ref: /schemas/graph.yaml#/properties/port

@@ -228,6 +208,30 @@ properties:
       SNK_READY for non-pd link.
     type: boolean
 
+  capabilities:
+    description: A child node to contain all the selectable USB Power Delivery capabilities.
+    type: object
+
+    patternProperties:
+      "^caps-[0-9]+$":
+        description: Child nodes under "capabilities" node. Each node contains a selectable USB
+          Power Delivery capability.
+        type: object
+
+        properties:
+          source-pdos:
+            $ref: "#/$defs/source-pdos"
+
+          sink-pdos:
+            $ref: "#/$defs/sink-pdos"
+
+          op-sink-microwatt:
+            $ref: "#/$defs/op-sink-microwatt"
+
+        additionalProperties: false
+
+    additionalProperties: false
+
 dependencies:
   sink-vdos-v1: [ 'sink-vdos' ]
   sink-vdos: [ 'sink-vdos-v1' ]

Documentation/devicetree/bindings/connector/usb-connector.yaml dependencies: · line 242

Quoted 4 lines

Documentation/devicetree/bindings/connector/usb-connector.yaml +1 -0
@@ -235,6 +239,36 @@ dependencies:
 required:
   - compatible
 
+$defs:

Make this:

$defs:
capabilities:

Quoted 9 lines

    properties:
      ...
And then just reference "#/$defs/capabilities" at the top-level and in caps-[0-9] node schema.
You'll need to use unevaluatedProperties instead of additionalProperties as well.

I am sorry I don't understand this part. Could you explain more?

In $defs, define a whole structure of "capabilities" which has patternProperties "caps-[0-9]+", and inside "caps-[0-9]+" there are properties "source/sink-pdos" ?

Reference it in the property "capabilities" under connector node? Reference it in the property "source/sink-pdos" under connector node as well?

And I don't know the usage of unevaluatedProperties. Where should it be inserted and what is the value of it?

Quoted 35 lines

Documentation/devicetree/bindings/connector/usb-connector.yaml +29 -1

+  source-pdos:
+    description: An array of u32 with each entry providing supported power
+      source data object(PDO), the detailed bit definitions of PDO can be found
+      in "Universal Serial Bus Power Delivery Specification" chapter 6.4.1.2
+      Source_Capabilities Message, the order of each entry(PDO) should follow
+      the PD spec chapter 6.4.1. Required for power source and power dual role.
+      User can specify the source PDO array via PDO_FIXED/BATT/VAR/PPS_APDO()
+      defined in dt-bindings/usb/pd.h.
+    minItems: 1
+    maxItems: 7
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+
+  sink-pdos:
+    description: An array of u32 with each entry providing supported power sink
+      data object(PDO), the detailed bit definitions of PDO can be found in
+      "Universal Serial Bus Power Delivery Specification" chapter 6.4.1.3
+      Sink Capabilities Message, the order of each entry(PDO) should follow the
+      PD spec chapter 6.4.1. Required for power sink and power dual role. User
+      can specify the sink PDO array via PDO_FIXED/BATT/VAR/PPS_APDO() defined
+      in dt-bindings/usb/pd.h.
+    minItems: 1
+    maxItems: 7
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+
+  op-sink-microwatt:
+    description: Sink required operating power in microwatt, if source can't
+      offer the power, Capability Mismatch is set. Required for power sink and
+      power dual role.
+
 allOf:
   - if:
       properties:
--

2.41.0.585.gd2178a4bd4-goog

Rob Herring

maintainer

2023-11-22 15:36 UTC #4 lore raw

Re: [PATCH v2 1/2] dt-bindings: connector: Add child nodes for multiple PD capabilities

Quoted 135 lines · Kyle Tso

Quoted 1 line · Rob Herring

Sorry for the late response...

Quoted 9 lines · Kyle Tso

Quoted 4 lines
Define a new optional property "capabilities" which is a child node under connector to contain multiple USB Power Delivery capabilities.
Define a new property with pattern (e.g. caps-0, caps-1) which is a
A property in json-schema terms, but for DT it's a node not a property. 'Define a child node ...' would be clearer.

Will do in the future patch

Quoted 9 lines

Quoted 5 lines
child node under "capabilities". Each node contains PDO data of a selectable Power Delivery capability.
Also define common properties for source-pdos, sink-pdos, and op-sink-microwatt that can be referenced.
Why do we need this? What issue does this solve? The commit message should answer those questions (always).

Will state the rationale in the future patch

Quoted 95 lines

Quoted 86 lines

Signed-off-by Kyle Tso <[email protected]>

---
v1 -> v2:
- move source/sink-pdos to $defs and reference them in properties

 .../bindings/connector/usb-connector.yaml     | 80 +++++++++++++------
 1 file changed, 57 insertions(+), 23 deletions(-)

Documentation/devicetree/bindings/connector/usb-connector.yaml +27 ~~-23~~

index 1c4d3eb87763..c6b02dbda83f 100644
@@ -117,28 +117,10 @@ properties:
   # The following are optional properties for "usb-c-connector" with power
   # delivery support.
   source-pdos:
-    description: An array of u32 with each entry providing supported power
-      source data object(PDO), the detailed bit definitions of PDO can be found
-      in "Universal Serial Bus Power Delivery Specification" chapter 6.4.1.2
-      Source_Capabilities Message, the order of each entry(PDO) should follow
-      the PD spec chapter 6.4.1. Required for power source and power dual role.
-      User can specify the source PDO array via PDO_FIXED/BATT/VAR/PPS_APDO()
-      defined in dt-bindings/usb/pd.h.
-    minItems: 1
-    maxItems: 7
-    $ref: /schemas/types.yaml#/definitions/uint32-array
+    $ref: "#/$defs/source-pdos"
 
   sink-pdos:
-    description: An array of u32 with each entry providing supported power sink
-      data object(PDO), the detailed bit definitions of PDO can be found in
-      "Universal Serial Bus Power Delivery Specification" chapter 6.4.1.3
-      Sink Capabilities Message, the order of each entry(PDO) should follow the
-      PD spec chapter 6.4.1. Required for power sink and power dual role. User
-      can specify the sink PDO array via PDO_FIXED/BATT/VAR/PPS_APDO() defined
-      in dt-bindings/usb/pd.h.
-    minItems: 1
-    maxItems: 7
-    $ref: /schemas/types.yaml#/definitions/uint32-array
+    $ref: "#/$defs/sink-pdos"
 
   sink-vdos:
     description: An array of u32 with each entry, a Vendor Defined Message Object (VDO),

@@ -164,9 +146,7 @@ properties:
     $ref: /schemas/types.yaml#/definitions/uint32-array
 
   op-sink-microwatt:
-    description: Sink required operating power in microwatt, if source can't
-      offer the power, Capability Mismatch is set. Required for power sink and
-      power dual role.
+    $ref: "#/$defs/op-sink-microwatt"
 
   port:
     $ref: /schemas/graph.yaml#/properties/port

@@ -228,6 +208,30 @@ properties:
       SNK_READY for non-pd link.
     type: boolean
 
+  capabilities:
+    description: A child node to contain all the selectable USB Power Delivery capabilities.
+    type: object
+
+    patternProperties:
+      "^caps-[0-9]+$":
+        description: Child nodes under "capabilities" node. Each node contains a selectable USB
+          Power Delivery capability.
+        type: object
+
+        properties:
+          source-pdos:
+            $ref: "#/$defs/source-pdos"
+
+          sink-pdos:
+            $ref: "#/$defs/sink-pdos"
+
+          op-sink-microwatt:
+            $ref: "#/$defs/op-sink-microwatt"
+
+        additionalProperties: false
+
+    additionalProperties: false
+
 dependencies:
   sink-vdos-v1: [ 'sink-vdos' ]
   sink-vdos: [ 'sink-vdos-v1' ]

Documentation/devicetree/bindings/connector/usb-connector.yaml dependencies: · line 242

Quoted 4 lines

Documentation/devicetree/bindings/connector/usb-connector.yaml +1 -0
@@ -235,6 +239,36 @@ dependencies:
 required:
   - compatible
 
+$defs:

Make this:

$defs:
capabilities:

Quoted 9 lines

    properties:
      ...
And then just reference "#/$defs/capabilities" at the top-level and in caps-[0-9] node schema.
You'll need to use unevaluatedProperties instead of additionalProperties as well.

Quoted 5 lines

I am sorry I don't understand this part. Could you explain more?
In $defs, define a whole structure of "capabilities" which has patternProperties "caps-[0-9]+", and inside "caps-[0-9]+" there are properties "source/sink-pdos" ?

No, define a list of properties under a def. Here's a more complete example:

$defs:
capabilities:

    properties:
      source-pdos:
        ...

      sink-pdos:
        ...

And then add "$ref: '#/$defs/capabilities'" at the top-level of this schema doc and under "caps-[0-9]+":

patternProperties:
"^caps-[0-9]+$":
description: Child nodes under "capabilities" node. Each node
contains a selectable USB
Power Delivery capability.
type: object
$ref: "#/$defs/capabilities"
unevaluatedProperties: false

unevaluatedProperties is needed because it can "see" properties defined in a $ref to get the full list of properties for a node.

Rob

Kyle Tso

author

2023-12-04 17:24 UTC #5 lore raw

Re: [PATCH v2 1/2] dt-bindings: connector: Add child nodes for multiple PD capabilities

Quoted 166 lines · Rob Herring

Quoted 135 lines · Kyle Tso
Quoted 1 line · Rob Herring
Sorry for the late response...
Quoted 9 lines · Kyle Tso
Quoted 4 lines
Define a new optional property "capabilities" which is a child node under connector to contain multiple USB Power Delivery capabilities.
Define a new property with pattern (e.g. caps-0, caps-1) which is a
A property in json-schema terms, but for DT it's a node not a property. 'Define a child node ...' would be clearer.
Will do in the future patch
Quoted 9 lines
Quoted 5 lines
child node under "capabilities". Each node contains PDO data of a selectable Power Delivery capability.
Also define common properties for source-pdos, sink-pdos, and op-sink-microwatt that can be referenced.
Why do we need this? What issue does this solve? The commit message should answer those questions (always).
Will state the rationale in the future patch
Quoted 95 lines
Quoted 86 lines
Signed-off-by Kyle Tso <[email protected]>
---
v1 -> v2:
- move source/sink-pdos to $defs and reference them in properties
 .../bindings/connector/usb-connector.yaml     | 80 +++++++++++++------
 1 file changed, 57 insertions(+), 23 deletions(-)
Documentation/devicetree/bindings/connector/usb-connector.yaml +27 ~~-23~~
index 1c4d3eb87763..c6b02dbda83f 100644
@@ -117,28 +117,10 @@ properties:
   # The following are optional properties for "usb-c-connector" with power
   # delivery support.
   source-pdos:
-    description: An array of u32 with each entry providing supported power
-      source data object(PDO), the detailed bit definitions of PDO can be found
-      in "Universal Serial Bus Power Delivery Specification" chapter 6.4.1.2
-      Source_Capabilities Message, the order of each entry(PDO) should follow
-      the PD spec chapter 6.4.1. Required for power source and power dual role.
-      User can specify the source PDO array via PDO_FIXED/BATT/VAR/PPS_APDO()
-      defined in dt-bindings/usb/pd.h.
-    minItems: 1
-    maxItems: 7
-    $ref: /schemas/types.yaml#/definitions/uint32-array
+    $ref: "#/$defs/source-pdos"
 
   sink-pdos:
-    description: An array of u32 with each entry providing supported power sink
-      data object(PDO), the detailed bit definitions of PDO can be found in
-      "Universal Serial Bus Power Delivery Specification" chapter 6.4.1.3
-      Sink Capabilities Message, the order of each entry(PDO) should follow the
-      PD spec chapter 6.4.1. Required for power sink and power dual role. User
-      can specify the sink PDO array via PDO_FIXED/BATT/VAR/PPS_APDO() defined
-      in dt-bindings/usb/pd.h.
-    minItems: 1
-    maxItems: 7
-    $ref: /schemas/types.yaml#/definitions/uint32-array
+    $ref: "#/$defs/sink-pdos"
 
   sink-vdos:
     description: An array of u32 with each entry, a Vendor Defined Message Object (VDO),
@@ -164,9 +146,7 @@ properties:
     $ref: /schemas/types.yaml#/definitions/uint32-array
 
   op-sink-microwatt:
-    description: Sink required operating power in microwatt, if source can't
-      offer the power, Capability Mismatch is set. Required for power sink and
-      power dual role.
+    $ref: "#/$defs/op-sink-microwatt"
 
   port:
     $ref: /schemas/graph.yaml#/properties/port
@@ -228,6 +208,30 @@ properties:
       SNK_READY for non-pd link.
     type: boolean
 
+  capabilities:
+    description: A child node to contain all the selectable USB Power Delivery capabilities.
+    type: object
+
+    patternProperties:
+      "^caps-[0-9]+$":
+        description: Child nodes under "capabilities" node. Each node contains a selectable USB
+          Power Delivery capability.
+        type: object
+
+        properties:
+          source-pdos:
+            $ref: "#/$defs/source-pdos"
+
+          sink-pdos:
+            $ref: "#/$defs/sink-pdos"
+
+          op-sink-microwatt:
+            $ref: "#/$defs/op-sink-microwatt"
+
+        additionalProperties: false
+
+    additionalProperties: false
+
 dependencies:
   sink-vdos-v1: [ 'sink-vdos' ]
   sink-vdos: [ 'sink-vdos-v1' ]
Documentation/devicetree/bindings/connector/usb-connector.yaml dependencies: · line 242
Quoted 4 lines
Documentation/devicetree/bindings/connector/usb-connector.yaml +1 -0
@@ -235,6 +239,36 @@ dependencies:
 required:
   - compatible
 
+$defs:
Make this:
$defs:
capabilities:
Quoted 9 lines
    properties:
      ...
And then just reference "#/$defs/capabilities" at the top-level and in caps-[0-9] node schema.
You'll need to use unevaluatedProperties instead of additionalProperties as well.
Quoted 5 lines
I am sorry I don't understand this part. Could you explain more?
In $defs, define a whole structure of "capabilities" which has patternProperties "caps-[0-9]+", and inside "caps-[0-9]+" there are properties "source/sink-pdos" ?
No, define a list of properties under a def. Here's a more complete example:
$defs:
capabilities:
    properties:
      source-pdos:
        ...

      sink-pdos:
        ...
And then add "$ref: '#/$defs/capabilities'" at the top-level of this schema doc and under "caps-[0-9]+":
patternProperties:
"^caps-[0-9]+$":
description: Child nodes under "capabilities" node. Each node
contains a selectable USB
Power Delivery capability.
type: object
$ref: "#/$defs/capabilities"
unevaluatedProperties: false

Quoted 4 lines

unevaluatedProperties is needed because it can "see" properties defined in a $ref to get the full list of properties for a node.
Rob

Thanks! Will update in v4

Kyle

Kyle Tso

author

2023-08-07 17:42 UTC #6 lore raw

PATCH v2 2/2 usb: typec: tcpm: Support multiple capabilities

In reply to Kyle Tso

This commit refactors tcpm_fw_get_caps to support the multiple pd capabilities got from fwnode. For backward compatibility, the original single capability is still applicable. The fetched data are stored in the newly defined structure "pd_data" and there is an array "pd_list" to store the pointers to them. A dedicated array "pds" is used to store the handles of the registered usb_power_delivery instances.

This commit also implements the .pd_get and .pd_set ops which are introduced in commit a7cff92f0635 ("usb: typec: USB Power Delivery helpers for ports and partners"). Once the .pd_set is called, the current capability will be updated and state machine will re-negotiate the power contract if possible.

Signed-off-by Kyle Tso <[email protected]>

---
v1 -> v2:
- Added some missing cleanups in the function tcpm_port_unregister_pd

 drivers/usb/typec/tcpm/tcpm.c | 419 +++++++++++++++++++++++++++-------
 1 file changed, 333 insertions(+), 86 deletions(-)

drivers/usb/typec/tcpm/tcpm.c +333 ~~-86~~

index 5a7d8cc04628..dd21d593979d 100644
@@ -296,6 +296,15 @@ struct pd_pps_data {
 	bool active;
 };
 
+struct pd_data {
+	struct usb_power_delivery *pd;
+	struct usb_power_delivery_capabilities *source_cap;
+	struct usb_power_delivery_capabilities_desc source_desc;
+	struct usb_power_delivery_capabilities *sink_cap;
+	struct usb_power_delivery_capabilities_desc sink_desc;
+	unsigned int operating_snk_mw;
+};
+
 struct tcpm_port {
 	struct device *dev;

@@ -397,12 +406,14 @@ struct tcpm_port {
 	unsigned int rx_msgid;
 
 	/* USB PD objects */
-	struct usb_power_delivery *pd;
+	struct usb_power_delivery **pds;
+	struct pd_data **pd_list;
 	struct usb_power_delivery_capabilities *port_source_caps;
 	struct usb_power_delivery_capabilities *port_sink_caps;
 	struct usb_power_delivery *partner_pd;
 	struct usb_power_delivery_capabilities *partner_source_caps;
 	struct usb_power_delivery_capabilities *partner_sink_caps;
+	struct usb_power_delivery *selected_pd;
 
 	/* Partner capabilities/requests */
 	u32 sink_request;

@@ -412,6 +423,7 @@ struct tcpm_port {
 	unsigned int nr_sink_caps;
 
 	/* Local capabilities */
+	unsigned int pd_count;
 	u32 src_pdo[PDO_MAX_OBJECTS];
 	unsigned int nr_src_pdo;
 	u32 snk_pdo[PDO_MAX_OBJECTS];

@@ -5985,12 +5997,114 @@ static int tcpm_port_type_set(struct typec_port *p, enum typec_port_type type)
 	return 0;
 }
 
+static struct pd_data *tcpm_find_pd_data(struct tcpm_port *port, struct usb_power_delivery *pd)
+{
+	int i;
+
+	for (i = 0; port->pd_list[i]; i++) {
+		if (port->pd_list[i]->pd == pd)
+			return port->pd_list[i];
+	}
+
+	return ERR_PTR(-ENODATA);
+}
+
+static struct usb_power_delivery **tcpm_pd_get(struct typec_port *p)
+{
+	struct tcpm_port *port = typec_get_drvdata(p);
+
+	return port->pds;
+}
+
+static int tcpm_pd_set(struct typec_port *p, struct usb_power_delivery *pd)
+{
+	struct tcpm_port *port = typec_get_drvdata(p);
+	struct pd_data *data;
+	int i, ret = 0;
+
+	mutex_lock(&port->lock);
+
+	if (port->selected_pd == pd)
+		goto unlock;
+
+	data = tcpm_find_pd_data(port, pd);
+	if (IS_ERR_OR_NULL(data)) {
+		ret = -ENODATA;
+		goto unlock;
+	}
+
+	if (data->sink_desc.pdo[0]) {
+		for (i = 0; i < PDO_MAX_OBJECTS && data->sink_desc.pdo[i]; i++)
+			port->snk_pdo[i] = data->sink_desc.pdo[i];
+		port->nr_snk_pdo = i + 1;
+		port->operating_snk_mw = data->operating_snk_mw;
+	}
+
+	if (data->source_desc.pdo[0]) {
+		for (i = 0; i < PDO_MAX_OBJECTS && data->source_desc.pdo[i]; i++)
+			port->snk_pdo[i] = data->source_desc.pdo[i];
+		port->nr_src_pdo = i + 1;
+	}
+
+	switch (port->state) {
+	case SRC_UNATTACHED:
+	case SRC_ATTACH_WAIT:
+	case SRC_TRYWAIT:
+		tcpm_set_cc(port, tcpm_rp_cc(port));
+		break;
+	case SRC_SEND_CAPABILITIES:
+	case SRC_SEND_CAPABILITIES_TIMEOUT:
+	case SRC_NEGOTIATE_CAPABILITIES:
+	case SRC_READY:
+	case SRC_WAIT_NEW_CAPABILITIES:
+		port->caps_count = 0;
+		port->upcoming_state = SRC_SEND_CAPABILITIES;
+		ret = tcpm_ams_start(port, POWER_NEGOTIATION);
+		if (ret == -EAGAIN) {
+			port->upcoming_state = INVALID_STATE;
+			goto unlock;
+		}
+		break;
+	case SNK_NEGOTIATE_CAPABILITIES:
+	case SNK_NEGOTIATE_PPS_CAPABILITIES:
+	case SNK_READY:
+	case SNK_TRANSITION_SINK:
+	case SNK_TRANSITION_SINK_VBUS:
+		if (port->pps_data.active)
+			port->upcoming_state = SNK_NEGOTIATE_PPS_CAPABILITIES;
+		else if (port->pd_capable)
+			port->upcoming_state = SNK_NEGOTIATE_CAPABILITIES;
+		else
+			break;
+
+		port->update_sink_caps = true;
+
+		ret = tcpm_ams_start(port, POWER_NEGOTIATION);
+		if (ret == -EAGAIN) {
+			port->upcoming_state = INVALID_STATE;
+			goto unlock;
+		}
+		break;
+	default:
+		break;
+	}
+
+	port->port_source_caps = data->source_cap;
+	port->port_sink_caps = data->sink_cap;
+	port->selected_pd = pd;
+unlock:
+	mutex_unlock(&port->lock);
+	return ret;
+}
+
 static const struct typec_operations tcpm_ops = {
 	.try_role = tcpm_try_role,
 	.dr_set = tcpm_dr_set,
 	.pr_set = tcpm_pr_set,
 	.vconn_set = tcpm_vconn_set,
-	.port_type_set = tcpm_port_type_set
+	.port_type_set = tcpm_port_type_set,
+	.pd_get = tcpm_pd_get,
+	.pd_set = tcpm_pd_set
 };
 
 void tcpm_tcpc_reset(struct tcpm_port *port)

@@ -6004,58 +6118,75 @@ EXPORT_SYMBOL_GPL(tcpm_tcpc_reset);
 
 static void tcpm_port_unregister_pd(struct tcpm_port *port)
 {
-	usb_power_delivery_unregister_capabilities(port->port_sink_caps);
+	int i;
+
 	port->port_sink_caps = NULL;
-	usb_power_delivery_unregister_capabilities(port->port_source_caps);
 	port->port_source_caps = NULL;
-	usb_power_delivery_unregister(port->pd);
-	port->pd = NULL;
+	for (i = 0; i < port->pd_count; i++) {
+		if (!IS_ERR_OR_NULL(port->pd_list[i]) &&
+		    !IS_ERR_OR_NULL(port->pd_list[i]->sink_cap)) {
+			usb_power_delivery_unregister_capabilities(port->pd_list[i]->sink_cap);
+			kfree(port->pd_list[i]->sink_cap);
+			port->pd_list[i]->sink_cap = NULL;
+		}
+		if (!IS_ERR_OR_NULL(port->pd_list[i]) &&
+		    !IS_ERR_OR_NULL(port->pd_list[i]->source_cap)) {
+			usb_power_delivery_unregister_capabilities(port->pd_list[i]->source_cap);
+			kfree(port->pd_list[i]->source_cap);
+			port->pd_list[i]->source_cap = NULL;
+		}
+		if (!IS_ERR_OR_NULL(port->pd_list[i])) {
+			port->pd_list[i]->pd = NULL;
+			devm_kfree(port->dev, port->pd_list[i]);
+			port->pd_list[i] = NULL;
+		}
+		if (!IS_ERR_OR_NULL(port->pds[i]))
+			usb_power_delivery_unregister(port->pds[i]);
+		port->pds[i] = NULL;
+	}
 }
 
 static int tcpm_port_register_pd(struct tcpm_port *port)
 {
 	struct usb_power_delivery_desc desc = { port->typec_caps.pd_revision };
-	struct usb_power_delivery_capabilities_desc caps = { };
 	struct usb_power_delivery_capabilities *cap;
-	int ret;
+	int ret, i;
 
 	if (!port->nr_src_pdo && !port->nr_snk_pdo)
 		return 0;
 
-	port->pd = usb_power_delivery_register(port->dev, &desc);
-	if (IS_ERR(port->pd)) {
-		ret = PTR_ERR(port->pd);
-		goto err_unregister;
-	}
-
-	if (port->nr_src_pdo) {
-		memcpy_and_pad(caps.pdo, sizeof(caps.pdo), port->src_pdo,
-			       port->nr_src_pdo * sizeof(u32), 0);
-		caps.role = TYPEC_SOURCE;
-
-		cap = usb_power_delivery_register_capabilities(port->pd, &caps);
-		if (IS_ERR(cap)) {
-			ret = PTR_ERR(cap);
+	for (i = 0; i < port->pd_count; i++) {
+		port->pds[i] = usb_power_delivery_register(port->dev, &desc);
+		if (IS_ERR(port->pds[i])) {
+			ret = PTR_ERR(port->pds[i]);
 			goto err_unregister;
 		}
-
-		port->port_source_caps = cap;
-	}
-
-	if (port->nr_snk_pdo) {
-		memcpy_and_pad(caps.pdo, sizeof(caps.pdo), port->snk_pdo,
-			       port->nr_snk_pdo * sizeof(u32), 0);
-		caps.role = TYPEC_SINK;
-
-		cap = usb_power_delivery_register_capabilities(port->pd, &caps);
-		if (IS_ERR(cap)) {
-			ret = PTR_ERR(cap);
-			goto err_unregister;
+		port->pd_list[i]->pd = port->pds[i];
+
+		if (port->pd_list[i]->source_desc.pdo[0]) {
+			cap = usb_power_delivery_register_capabilities(port->pds[i],
+								&port->pd_list[i]->source_desc);
+			if (IS_ERR(cap)) {
+				ret = PTR_ERR(cap);
+				goto err_unregister;
+			}
+			port->pd_list[i]->source_cap = cap;
 		}
 
-		port->port_sink_caps = cap;
+		if (port->pd_list[i]->sink_desc.pdo[0]) {
+			cap = usb_power_delivery_register_capabilities(port->pds[i],
+								&port->pd_list[i]->sink_desc);
+			if (IS_ERR(cap)) {
+				ret = PTR_ERR(cap);
+				goto err_unregister;
+			}
+			port->pd_list[i]->sink_cap = cap;
+		}
 	}
 
+	port->port_source_caps = port->pd_list[0]->source_cap;
+	port->port_sink_caps = port->pd_list[0]->sink_cap;
+	port->selected_pd = port->pds[0];
 	return 0;
 
 err_unregister:

@@ -6064,12 +6195,11 @@ static int tcpm_port_register_pd(struct tcpm_port *port)
 	return ret;
 }
 
-static int tcpm_fw_get_caps(struct tcpm_port *port,
-			    struct fwnode_handle *fwnode)
+static int tcpm_fw_get_properties(struct tcpm_port *port, struct fwnode_handle *fwnode)
 {
 	const char *opmode_str;
+	u32 frs_current;
 	int ret;
-	u32 mw, frs_current;
 
 	if (!fwnode)
 		return -EINVAL;

@@ -6089,28 +6219,10 @@ static int tcpm_fw_get_caps(struct tcpm_port *port,
 
 	port->port_type = port->typec_caps.type;
 	port->pd_supported = !fwnode_property_read_bool(fwnode, "pd-disable");
-
 	port->slow_charger_loop = fwnode_property_read_bool(fwnode, "slow-charger-loop");
-	if (port->port_type == TYPEC_PORT_SNK)
-		goto sink;
-
-	/* Get Source PDOs for the PD port or Source Rp value for the non-PD port */
-	if (port->pd_supported) {
-		ret = fwnode_property_count_u32(fwnode, "source-pdos");
-		if (ret == 0)
-			return -EINVAL;
-		else if (ret < 0)
-			return ret;
+	port->self_powered = fwnode_property_read_bool(fwnode, "self-powered");
 
-		port->nr_src_pdo = min(ret, PDO_MAX_OBJECTS);
-		ret = fwnode_property_read_u32_array(fwnode, "source-pdos",
-						     port->src_pdo, port->nr_src_pdo);
-		if (ret)
-			return ret;
-		ret = tcpm_validate_caps(port, port->src_pdo, port->nr_src_pdo);
-		if (ret)
-			return ret;
-	} else {
+	if (!port->pd_supported) {
 		ret = fwnode_property_read_string(fwnode, "typec-power-opmode", &opmode_str);
 		if (ret)
 			return ret;

@@ -6120,43 +6232,174 @@ static int tcpm_fw_get_caps(struct tcpm_port *port,
 		port->src_rp = tcpm_pwr_opmode_to_rp(ret);
 	}
 
-	if (port->port_type == TYPEC_PORT_SRC)
-		return 0;
+	/* FRS can only be supported by DRP ports */
+	if (port->port_type == TYPEC_PORT_DRP) {
+		ret = fwnode_property_read_u32(fwnode, "new-source-frs-typec-current",
+					       &frs_current);
+		if (ret >= 0 && frs_current <= FRS_5V_3A)
+			port->new_source_frs_current = frs_current;
+	}
 
-sink:
-	port->self_powered = fwnode_property_read_bool(fwnode, "self-powered");
+	return 0;
+}
+
+static unsigned int tcpm_fw_count_pd(struct fwnode_handle *capabilities)
+{
+	struct fwnode_handle *child = NULL;
+	unsigned int count = 0;
+
+	do {
+		count++;
+		child = fwnode_get_next_child_node(capabilities, child);
+		fwnode_handle_put(child);
+	} while (child);
+
+	return --count;
+}
+
+static int tcpm_fw_get_caps(struct tcpm_port *port, struct fwnode_handle *fwnode)
+{
+	struct fwnode_handle *capabilities, *caps = NULL;
+	unsigned int nr_src_pdo, nr_snk_pdo;
+	u32 *src_pdo, *snk_pdo;
+	u32 uw;
+	int ret, i;
+
+	if (!fwnode)
+		return -EINVAL;
 
 	if (!port->pd_supported)
 		return 0;
 
-	/* Get sink pdos */
-	ret = fwnode_property_count_u32(fwnode, "sink-pdos");
-	if (ret <= 0)
-		return -EINVAL;
+	/* For the backward compatibility, "capabilities" node is optional. */
+	capabilities = fwnode_get_named_child_node(fwnode, "capabilities");
+	if (IS_ERR_OR_NULL(capabilities)) {
+		port->pd_count = 1;
+		capabilities = NULL;
+	} else {
+		port->pd_count = tcpm_fw_count_pd(capabilities);
+		if (!port->pd_count) {
+			fwnode_handle_put(capabilities);
+			return -ENODATA;
+		}
+	}
 
-	port->nr_snk_pdo = min(ret, PDO_MAX_OBJECTS);
-	ret = fwnode_property_read_u32_array(fwnode, "sink-pdos",
-					     port->snk_pdo, port->nr_snk_pdo);
-	if ((ret < 0) || tcpm_validate_caps(port, port->snk_pdo,
-					    port->nr_snk_pdo))
-		return -EINVAL;
+	port->pds = devm_kcalloc(port->dev, port->pd_count, sizeof(struct usb_power_delivery *),
+				 GFP_KERNEL);
+	if (IS_ERR_OR_NULL(port->pds)) {
+		fwnode_handle_put(capabilities);
+		return -ENOMEM;
+	}
 
-	if (fwnode_property_read_u32(fwnode, "op-sink-microwatt", &mw) < 0)
-		return -EINVAL;
-	port->operating_snk_mw = mw / 1000;
+	port->pd_list = devm_kcalloc(port->dev, port->pd_count, sizeof(struct pd_data *),
+				     GFP_KERNEL);
+	if (IS_ERR_OR_NULL(port->pd_list)) {
+		fwnode_handle_put(capabilities);
+		return -ENOMEM;
+	}
 
-	/* FRS can only be supported by DRP ports */
-	if (port->port_type == TYPEC_PORT_DRP) {
-		ret = fwnode_property_read_u32(fwnode, "new-source-frs-typec-current",
-					       &frs_current);
-		if (ret >= 0 && frs_current <= FRS_5V_3A)
-			port->new_source_frs_current = frs_current;
+	for (i = 0; i < port->pd_count; i++) {
+		port->pd_list[i] = devm_kzalloc(port->dev, sizeof(struct pd_data), GFP_KERNEL);
+
+		src_pdo = port->pd_list[i]->source_desc.pdo;
+		port->pd_list[i]->source_desc.role = TYPEC_SOURCE;
+		snk_pdo = port->pd_list[i]->sink_desc.pdo;
+		port->pd_list[i]->sink_desc.role = TYPEC_SINK;
+
+		/*
+		 * The last put is in "exit_fwnode_put" so forward the put to the beginning of this
+		 * "for loop". It doesn't matter for the first put because the first caps is NULL.
+		 * It won't run the second time if caps == fwnode because port->pd_count is 1 in
+		 * that case.
+		 */
+		fwnode_handle_put(caps);
+
+		/* If "capabilities" is NULL, fall back to single pd cap population. */
+		if (!capabilities)
+			caps = fwnode;
+		else
+			caps = fwnode_get_next_child_node(capabilities, caps);
+
+		if (port->port_type != TYPEC_PORT_SNK) {
+			ret = fwnode_property_count_u32(caps, "source-pdos");
+			if (ret == 0) {
+				ret = -EINVAL;
+				goto exit_fwnode_put;
+			} else if (ret < 0) {
+				goto exit_fwnode_put;
+			}
+
+			nr_src_pdo = min(ret, PDO_MAX_OBJECTS);
+			ret = fwnode_property_read_u32_array(caps, "source-pdos", src_pdo,
+							     nr_src_pdo);
+			if (ret)
+				goto exit_fwnode_put;
+
+			ret = tcpm_validate_caps(port, src_pdo, nr_src_pdo);
+			if (ret)
+				goto exit_fwnode_put;
+
+			if (i == 0) {
+				port->nr_src_pdo = nr_src_pdo;
+				memcpy_and_pad(port->src_pdo, sizeof(u32) * PDO_MAX_OBJECTS,
+					       port->pd_list[0]->source_desc.pdo,
+					       sizeof(u32) * nr_src_pdo,
+					       0);
+			}
+		}
+
+		if (port->port_type != TYPEC_PORT_SRC) {
+			ret = fwnode_property_count_u32(caps, "sink-pdos");
+			if (ret == 0) {
+				ret = -EINVAL;
+				goto exit_fwnode_put;
+			} else if (ret < 0) {
+				goto exit_fwnode_put;
+			}
+
+			nr_snk_pdo = min(ret, PDO_MAX_OBJECTS);
+			ret = fwnode_property_read_u32_array(caps, "sink-pdos", snk_pdo,
+							     nr_snk_pdo);
+			if (ret)
+				goto exit_fwnode_put;
+
+			ret = tcpm_validate_caps(port, snk_pdo, nr_snk_pdo);
+			if (ret)
+				goto exit_fwnode_put;
+
+			if (fwnode_property_read_u32(caps, "op-sink-microwatt", &uw) < 0) {
+				ret = -EINVAL;
+				goto exit_fwnode_put;
+			}
+
+			port->pd_list[i]->operating_snk_mw = uw / 1000;
+
+			if (i == 0) {
+				port->nr_snk_pdo = nr_snk_pdo;
+				memcpy_and_pad(port->snk_pdo, sizeof(u32) * PDO_MAX_OBJECTS,
+					       port->pd_list[0]->sink_desc.pdo,
+					       sizeof(u32) * nr_snk_pdo,
+					       0);
+				port->operating_snk_mw = port->pd_list[0]->operating_snk_mw;
+			}
+		}
 	}
 
+exit_fwnode_put:
+	if (caps != fwnode)
+		fwnode_handle_put(caps);
+	fwnode_handle_put(capabilities);
+	return ret;
+}
+
+static int tcpm_fw_get_snk_vdos(struct tcpm_port *port, struct fwnode_handle *fwnode)
+{
+	int ret;
+
 	/* sink-vdos is optional */
 	ret = fwnode_property_count_u32(fwnode, "sink-vdos");
 	if (ret < 0)
-		ret = 0;
+		return 0;
 
 	port->nr_snk_vdo = min(ret, VDO_MAX_OBJECTS);
 	if (port->nr_snk_vdo) {

@@ -6521,13 +6764,18 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
 	init_completion(&port->pps_complete);
 	tcpm_debugfs_init(port);
 
+	err = tcpm_fw_get_properties(port, tcpc->fwnode);
+	if (err < 0)
+		goto out_destroy_wq;
 	err = tcpm_fw_get_caps(port, tcpc->fwnode);
+	if (err < 0)
+		goto out_destroy_wq;
+	err = tcpm_fw_get_snk_vdos(port, tcpc->fwnode);
 	if (err < 0)
 		goto out_destroy_wq;
 
 	port->try_role = port->typec_caps.prefer_role;
 
-	port->typec_caps.fwnode = tcpc->fwnode;
 	port->typec_caps.revision = 0x0120;	/* Type-C spec release 1.2 */
 	port->typec_caps.pd_revision = 0x0300;	/* USB-PD spec release 3.0 */
 	port->typec_caps.svdm_version = SVDM_VER_2_0;

@@ -6536,7 +6784,6 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
 	port->typec_caps.orientation_aware = 1;
 
 	port->partner_desc.identity = &port->partner_ident;
-	port->port_type = port->typec_caps.type;
 
 	port->role_sw = usb_role_switch_get(port->dev);
 	if (!port->role_sw)

@@ -6555,7 +6802,7 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
 	if (err)
 		goto out_role_sw_put;
 
-	port->typec_caps.pd = port->pd;
+	port->typec_caps.pd = port->pds[0];
 
 	port->typec_port = typec_register_port(port->dev, &port->typec_caps);
 	if (IS_ERR(port->typec_port)) {

Signature · 1 line

2.41.0.585.gd2178a4bd4-goog

A message that is not in the archive (sent off-list or not yet received): [email protected]

Greg Kroah-Hartman

maintainer

2023-08-09 08:31 UTC #8 lore raw

Re: [PATCH v2 2/2] usb: typec: tcpm: Support multiple capabilities

Hi,

This is the semi-friendly patch-bot of Greg Kroah-Hartman.

Markus, you seem to have sent a nonsensical or otherwise pointless review comment to a patch submission on a Linux kernel developer mailing list. I strongly suggest that you not do this anymore. Please do not bother developers who are actively working to produce patches and features with comments that, in the end, are a waste of time.

Patch submitter, please ignore Markus's suggestion; you do not need to follow it at all. The person/bot/AI that sent it is being ignored by almost all Linux kernel maintainers for having a persistent pattern of behavior of producing distracting and pointless commentary, and inability to adapt to feedback. Please feel free to also ignore emails from them.

thanks,

greg k-h's patch email bot

Guenter Roeck

2023-08-09 10:27 UTC #9 lore raw

Re: [PATCH v2 2/2] usb: typec: tcpm: Support multiple capabilities

In reply to Kyle Tso

Quoted 1 line · Kyle Tso

This commit refactors tcpm_fw_get_caps to support the multiple pd

s/This commit refactors/Refactor/

Quoted 2 lines

capabilities got from fwnode. For backward compatibility, the original single capability is still applicable. The fetched data are stored in

is stored ?

Quoted 5 lines

the newly defined structure "pd_data" and there is an array "pd_list" to store the pointers to them. A dedicated array "pds" is used to store the handles of the registered usb_power_delivery instances.
This commit also implements the .pd_get and .pd_set ops which are

s/This commit also implements/Implement/

Quoted 83 lines

introduced in commit a7cff92f0635 ("usb: typec: USB Power Delivery helpers for ports and partners"). Once the .pd_set is called, the current capability will be updated and state machine will re-negotiate the power contract if possible.

Signed-off-by Kyle Tso <[email protected]>

---
v1 -> v2:
- Added some missing cleanups in the function tcpm_port_unregister_pd

 drivers/usb/typec/tcpm/tcpm.c | 419 +++++++++++++++++++++++++++-------
 1 file changed, 333 insertions(+), 86 deletions(-)

drivers/usb/typec/tcpm/tcpm.c +35 -1

index 5a7d8cc04628..dd21d593979d 100644
@@ -296,6 +296,15 @@ struct pd_pps_data {
 	bool active;
 };
 
+struct pd_data {
+	struct usb_power_delivery *pd;
+	struct usb_power_delivery_capabilities *source_cap;
+	struct usb_power_delivery_capabilities_desc source_desc;
+	struct usb_power_delivery_capabilities *sink_cap;
+	struct usb_power_delivery_capabilities_desc sink_desc;
+	unsigned int operating_snk_mw;
+};
+
 struct tcpm_port {
 	struct device *dev;

@@ -397,12 +406,14 @@ struct tcpm_port {
 	unsigned int rx_msgid;
 
 	/* USB PD objects */
-	struct usb_power_delivery *pd;
+	struct usb_power_delivery **pds;
+	struct pd_data **pd_list;
 	struct usb_power_delivery_capabilities *port_source_caps;
 	struct usb_power_delivery_capabilities *port_sink_caps;
 	struct usb_power_delivery *partner_pd;
 	struct usb_power_delivery_capabilities *partner_source_caps;
 	struct usb_power_delivery_capabilities *partner_sink_caps;
+	struct usb_power_delivery *selected_pd;
 
 	/* Partner capabilities/requests */
 	u32 sink_request;

@@ -412,6 +423,7 @@ struct tcpm_port {
 	unsigned int nr_sink_caps;
 
 	/* Local capabilities */
+	unsigned int pd_count;
 	u32 src_pdo[PDO_MAX_OBJECTS];
 	unsigned int nr_src_pdo;
 	u32 snk_pdo[PDO_MAX_OBJECTS];

@@ -5985,12 +5997,114 @@ static int tcpm_port_type_set(struct typec_port *p, enum typec_port_type type)
 	return 0;
 }
 
+static struct pd_data *tcpm_find_pd_data(struct tcpm_port *port, struct usb_power_delivery *pd)
+{
+	int i;
+
+	for (i = 0; port->pd_list[i]; i++) {
+		if (port->pd_list[i]->pd == pd)
+			return port->pd_list[i];
+	}
+
+	return ERR_PTR(-ENODATA);
+}
+
+static struct usb_power_delivery **tcpm_pd_get(struct typec_port *p)
+{
+	struct tcpm_port *port = typec_get_drvdata(p);
+
+	return port->pds;
+}
+
+static int tcpm_pd_set(struct typec_port *p, struct usb_power_delivery *pd)
+{
+	struct tcpm_port *port = typec_get_drvdata(p);

drivers/usb/typec/tcpm/tcpm.c static int tcpm_port_type_set(struct typec_port *p, enum typec_port_type type) · line 6031

Quoted 10 lines

drivers/usb/typec/tcpm/tcpm.c +10 -0

+	struct pd_data *data;
+	int i, ret = 0;
+
+	mutex_lock(&port->lock);
+
+	if (port->selected_pd == pd)
+		goto unlock;
+
+	data = tcpm_find_pd_data(port, pd);
+	if (IS_ERR_OR_NULL(data)) {

tcpm_find_pd_data() will never return NULL.

In eneral, there are way too many IS_ERR_OR_NULL() checks in this code. This makes it difficult if not impossible to review.

Quoted 85 lines

drivers/usb/typec/tcpm/tcpm.c +71 -2

+		ret = -ENODATA;
+		goto unlock;
+	}
+
+	if (data->sink_desc.pdo[0]) {
+		for (i = 0; i < PDO_MAX_OBJECTS && data->sink_desc.pdo[i]; i++)
+			port->snk_pdo[i] = data->sink_desc.pdo[i];
+		port->nr_snk_pdo = i + 1;
+		port->operating_snk_mw = data->operating_snk_mw;
+	}
+
+	if (data->source_desc.pdo[0]) {
+		for (i = 0; i < PDO_MAX_OBJECTS && data->source_desc.pdo[i]; i++)
+			port->snk_pdo[i] = data->source_desc.pdo[i];
+		port->nr_src_pdo = i + 1;
+	}
+
+	switch (port->state) {
+	case SRC_UNATTACHED:
+	case SRC_ATTACH_WAIT:
+	case SRC_TRYWAIT:
+		tcpm_set_cc(port, tcpm_rp_cc(port));
+		break;
+	case SRC_SEND_CAPABILITIES:
+	case SRC_SEND_CAPABILITIES_TIMEOUT:
+	case SRC_NEGOTIATE_CAPABILITIES:
+	case SRC_READY:
+	case SRC_WAIT_NEW_CAPABILITIES:
+		port->caps_count = 0;
+		port->upcoming_state = SRC_SEND_CAPABILITIES;
+		ret = tcpm_ams_start(port, POWER_NEGOTIATION);
+		if (ret == -EAGAIN) {
+			port->upcoming_state = INVALID_STATE;
+			goto unlock;
+		}
+		break;
+	case SNK_NEGOTIATE_CAPABILITIES:
+	case SNK_NEGOTIATE_PPS_CAPABILITIES:
+	case SNK_READY:
+	case SNK_TRANSITION_SINK:
+	case SNK_TRANSITION_SINK_VBUS:
+		if (port->pps_data.active)
+			port->upcoming_state = SNK_NEGOTIATE_PPS_CAPABILITIES;
+		else if (port->pd_capable)
+			port->upcoming_state = SNK_NEGOTIATE_CAPABILITIES;
+		else
+			break;
+
+		port->update_sink_caps = true;
+
+		ret = tcpm_ams_start(port, POWER_NEGOTIATION);
+		if (ret == -EAGAIN) {
+			port->upcoming_state = INVALID_STATE;
+			goto unlock;
+		}
+		break;
+	default:
+		break;
+	}
+
+	port->port_source_caps = data->source_cap;
+	port->port_sink_caps = data->sink_cap;
+	port->selected_pd = pd;
+unlock:
+	mutex_unlock(&port->lock);
+	return ret;
+}
+
 static const struct typec_operations tcpm_ops = {
 	.try_role = tcpm_try_role,
 	.dr_set = tcpm_dr_set,
 	.pr_set = tcpm_pr_set,
 	.vconn_set = tcpm_vconn_set,
-	.port_type_set = tcpm_port_type_set
+	.port_type_set = tcpm_port_type_set,
+	.pd_get = tcpm_pd_get,
+	.pd_set = tcpm_pd_set
 };
 
 void tcpm_tcpc_reset(struct tcpm_port *port)

@@ -6004,58 +6118,75 @@ EXPORT_SYMBOL_GPL(tcpm_tcpc_reset);
 
 static void tcpm_port_unregister_pd(struct tcpm_port *port)
 {
-	usb_power_delivery_unregister_capabilities(port->port_sink_caps);

drivers/usb/typec/tcpm/tcpm.c EXPORT_SYMBOL_GPL(tcpm_tcpc_reset); · line 6127

Quoted 10 lines

drivers/usb/typec/tcpm/tcpm.c +5 -3

+	int i;
+
 	port->port_sink_caps = NULL;
-	usb_power_delivery_unregister_capabilities(port->port_source_caps);
 	port->port_source_caps = NULL;
-	usb_power_delivery_unregister(port->pd);
-	port->pd = NULL;
+	for (i = 0; i < port->pd_count; i++) {
+		if (!IS_ERR_OR_NULL(port->pd_list[i]) &&
+		    !IS_ERR_OR_NULL(port->pd_list[i]->sink_cap)) {

I don't think either of those will ever be an error pointer.

drivers/usb/typec/tcpm/tcpm.c EXPORT_SYMBOL_GPL(tcpm_tcpc_reset); · line 6136

Quoted 9 lines

drivers/usb/typec/tcpm/tcpm.c +9 -0

+			usb_power_delivery_unregister_capabilities(port->pd_list[i]->sink_cap);
+			kfree(port->pd_list[i]->sink_cap);
+			port->pd_list[i]->sink_cap = NULL;
+		}
+		if (!IS_ERR_OR_NULL(port->pd_list[i]) &&
+		    !IS_ERR_OR_NULL(port->pd_list[i]->source_cap)) {
+			usb_power_delivery_unregister_capabilities(port->pd_list[i]->source_cap);
+			kfree(port->pd_list[i]->source_cap);
+			port->pd_list[i]->source_cap = NULL;

port->pd_list[i] is about to be released. It seems unnecessary to clear its elements.

drivers/usb/typec/tcpm/tcpm.c EXPORT_SYMBOL_GPL(tcpm_tcpc_reset); · line 6139

Quoted 3 lines

drivers/usb/typec/tcpm/tcpm.c +3 -0

+		}
+		if (!IS_ERR_OR_NULL(port->pd_list[i])) {
+			port->pd_list[i]->pd = NULL;

This has zero value since port->pd_list[i] is about to be released.

drivers/usb/typec/tcpm/tcpm.c EXPORT_SYMBOL_GPL(tcpm_tcpc_reset); · line 6140

Quoted 1 line

drivers/usb/typec/tcpm/tcpm.c +1 -0
+			devm_kfree(port->dev, port->pd_list[i]);

I am concerned about the mix of kfree() and devm_kfree(). Is that really appropriate ?

drivers/usb/typec/tcpm/tcpm.c EXPORT_SYMBOL_GPL(tcpm_tcpc_reset); · line 6143

Quoted 3 lines

drivers/usb/typec/tcpm/tcpm.c +3 -0

+			port->pd_list[i] = NULL;
+		}
+		if (!IS_ERR_OR_NULL(port->pds[i]))

Is that ever NULL or an ERR_PTR ?
tcpm_port_register_pd() fails if that happens.

Quoted 40 lines

drivers/usb/typec/tcpm/tcpm.c +8 ~~-20~~

+			usb_power_delivery_unregister(port->pds[i]);
+		port->pds[i] = NULL;
+	}
 }
 
 static int tcpm_port_register_pd(struct tcpm_port *port)
 {
 	struct usb_power_delivery_desc desc = { port->typec_caps.pd_revision };
-	struct usb_power_delivery_capabilities_desc caps = { };
 	struct usb_power_delivery_capabilities *cap;
-	int ret;
+	int ret, i;
 
 	if (!port->nr_src_pdo && !port->nr_snk_pdo)
 		return 0;
 
-	port->pd = usb_power_delivery_register(port->dev, &desc);
-	if (IS_ERR(port->pd)) {
-		ret = PTR_ERR(port->pd);
-		goto err_unregister;
-	}
-
-	if (port->nr_src_pdo) {
-		memcpy_and_pad(caps.pdo, sizeof(caps.pdo), port->src_pdo,
-			       port->nr_src_pdo * sizeof(u32), 0);
-		caps.role = TYPEC_SOURCE;
-
-		cap = usb_power_delivery_register_capabilities(port->pd, &caps);
-		if (IS_ERR(cap)) {
-			ret = PTR_ERR(cap);
+	for (i = 0; i < port->pd_count; i++) {
+		port->pds[i] = usb_power_delivery_register(port->dev, &desc);
+		if (IS_ERR(port->pds[i])) {
+			ret = PTR_ERR(port->pds[i]);
 			goto err_unregister;
 		}
-
-		port->port_source_caps = cap;
-	}
-

drivers/usb/typec/tcpm/tcpm.c EXPORT_SYMBOL_GPL(tcpm_tcpc_reset); · line 6164

Quoted 10 lines

drivers/usb/typec/tcpm/tcpm.c +1 -9

-	if (port->nr_snk_pdo) {
-		memcpy_and_pad(caps.pdo, sizeof(caps.pdo), port->snk_pdo,
-			       port->nr_snk_pdo * sizeof(u32), 0);
-		caps.role = TYPEC_SINK;
-
-		cap = usb_power_delivery_register_capabilities(port->pd, &caps);
-		if (IS_ERR(cap)) {
-			ret = PTR_ERR(cap);
-			goto err_unregister;
+		port->pd_list[i]->pd = port->pds[i];

This code assumes that port->pd_list[i] is valid for all values of i. The unregistration code above checks each element against IS_ERR_OR_NULL. How can it be guaranteed that it is valid here but not during unregistration ?

drivers/usb/typec/tcpm/tcpm.c EXPORT_SYMBOL_GPL(tcpm_tcpc_reset); · line 6173

Quoted 9 lines

drivers/usb/typec/tcpm/tcpm.c +9 -0

+
+		if (port->pd_list[i]->source_desc.pdo[0]) {
+			cap = usb_power_delivery_register_capabilities(port->pds[i],
+								&port->pd_list[i]->source_desc);
+			if (IS_ERR(cap)) {
+				ret = PTR_ERR(cap);
+				goto err_unregister;
+			}
+			port->pd_list[i]->source_cap = cap;

This suggests that port->pd_list[i]->source_cap will never be an ERR_PTR.

drivers/usb/typec/tcpm/tcpm.c EXPORT_SYMBOL_GPL(tcpm_tcpc_reset); · line 6183

Quoted 11 lines

drivers/usb/typec/tcpm/tcpm.c +8 -1

 		}
 
-		port->port_sink_caps = cap;
+		if (port->pd_list[i]->sink_desc.pdo[0]) {
+			cap = usb_power_delivery_register_capabilities(port->pds[i],
+								&port->pd_list[i]->sink_desc);
+			if (IS_ERR(cap)) {
+				ret = PTR_ERR(cap);
+				goto err_unregister;
+			}
+			port->pd_list[i]->sink_cap = cap;

Same here.

Quoted 56 lines

drivers/usb/typec/tcpm/tcpm.c +8 ~~-23~~

+		}
 	}
 
+	port->port_source_caps = port->pd_list[0]->source_cap;
+	port->port_sink_caps = port->pd_list[0]->sink_cap;
+	port->selected_pd = port->pds[0];
 	return 0;
 
 err_unregister:

@@ -6064,12 +6195,11 @@ static int tcpm_port_register_pd(struct tcpm_port *port)
 	return ret;
 }
 
-static int tcpm_fw_get_caps(struct tcpm_port *port,
-			    struct fwnode_handle *fwnode)
+static int tcpm_fw_get_properties(struct tcpm_port *port, struct fwnode_handle *fwnode)
 {
 	const char *opmode_str;
+	u32 frs_current;
 	int ret;
-	u32 mw, frs_current;
 
 	if (!fwnode)
 		return -EINVAL;

@@ -6089,28 +6219,10 @@ static int tcpm_fw_get_caps(struct tcpm_port *port,
 
 	port->port_type = port->typec_caps.type;
 	port->pd_supported = !fwnode_property_read_bool(fwnode, "pd-disable");
-
 	port->slow_charger_loop = fwnode_property_read_bool(fwnode, "slow-charger-loop");
-	if (port->port_type == TYPEC_PORT_SNK)
-		goto sink;
-
-	/* Get Source PDOs for the PD port or Source Rp value for the non-PD port */
-	if (port->pd_supported) {
-		ret = fwnode_property_count_u32(fwnode, "source-pdos");
-		if (ret == 0)
-			return -EINVAL;
-		else if (ret < 0)
-			return ret;
+	port->self_powered = fwnode_property_read_bool(fwnode, "self-powered");
 
-		port->nr_src_pdo = min(ret, PDO_MAX_OBJECTS);
-		ret = fwnode_property_read_u32_array(fwnode, "source-pdos",
-						     port->src_pdo, port->nr_src_pdo);
-		if (ret)
-			return ret;
-		ret = tcpm_validate_caps(port, port->src_pdo, port->nr_src_pdo);
-		if (ret)
-			return ret;
-	} else {
+	if (!port->pd_supported) {
 		ret = fwnode_property_read_string(fwnode, "typec-power-opmode", &opmode_str);
 		if (ret)
 			return ret;

drivers/usb/typec/tcpm/tcpm.c static int tcpm_fw_get_caps(struct tcpm_port *port, · line 6238

Quoted 9 lines

drivers/usb/typec/tcpm/tcpm.c +4 -2

@@ -6120,43 +6232,174 @@ static int tcpm_fw_get_caps(struct tcpm_port *port,
 		port->src_rp = tcpm_pwr_opmode_to_rp(ret);
 	}
 
-	if (port->port_type == TYPEC_PORT_SRC)
-		return 0;
+	/* FRS can only be supported by DRP ports */
+	if (port->port_type == TYPEC_PORT_DRP) {
+		ret = fwnode_property_read_u32(fwnode, "new-source-frs-typec-current",
+					       &frs_current);

Never returns a value > 0 if a pointer is passed.

Quoted 8 lines

drivers/usb/typec/tcpm/tcpm.c +5 -2

+		if (ret >= 0 && frs_current <= FRS_5V_3A)
+			port->new_source_frs_current = frs_current;
+	}
 
-sink:
-	port->self_powered = fwnode_property_read_bool(fwnode, "self-powered");
+	return 0;
+}

drivers/usb/typec/tcpm/tcpm.c static int tcpm_fw_get_caps(struct tcpm_port *port, · line 6254

Quoted 10 lines

drivers/usb/typec/tcpm/tcpm.c +10 -0

+
+static unsigned int tcpm_fw_count_pd(struct fwnode_handle *capabilities)
+{
+	struct fwnode_handle *child = NULL;
+	unsigned int count = 0;
+
+	do {
+		count++;
+		child = fwnode_get_next_child_node(capabilities, child);
+		fwnode_handle_put(child);

I am quite sure that fwnode_get_next_child_node() handles the put on the child node. Either case, how about the following ?

	fwnode_for_each_child_node(fwnode, child)
		count++;

Quoted 5 lines

drivers/usb/typec/tcpm/tcpm.c +5 -0
+	} while (child);
+
+	return --count;
+}
+

drivers/usb/typec/tcpm/tcpm.c static int tcpm_fw_get_caps(struct tcpm_port *port, · line 6269

Quoted 10 lines

drivers/usb/typec/tcpm/tcpm.c +10 -0

+static int tcpm_fw_get_caps(struct tcpm_port *port, struct fwnode_handle *fwnode)
+{
+	struct fwnode_handle *capabilities, *caps = NULL;
+	unsigned int nr_src_pdo, nr_snk_pdo;
+	u32 *src_pdo, *snk_pdo;
+	u32 uw;
+	int ret, i;
+
+	if (!fwnode)
+		return -EINVAL;

In practice this will never happen because the code already bailed out. It might make sense to check this in the calling code and not repeatedly in called functions.

drivers/usb/typec/tcpm/tcpm.c static int tcpm_fw_get_caps(struct tcpm_port *port, · line 6275

Quoted 10 lines

drivers/usb/typec/tcpm/tcpm.c +2 -4

 	if (!port->pd_supported)
 		return 0;
 
-	/* Get sink pdos */
-	ret = fwnode_property_count_u32(fwnode, "sink-pdos");
-	if (ret <= 0)
-		return -EINVAL;
+	/* For the backward compatibility, "capabilities" node is optional. */
+	capabilities = fwnode_get_named_child_node(fwnode, "capabilities");

I don't think this ever returns an ERR_PTR. All other callers only check for NULL.

drivers/usb/typec/tcpm/tcpm.c static int tcpm_fw_get_caps(struct tcpm_port *port, · line 6278

Quoted 3 lines

drivers/usb/typec/tcpm/tcpm.c +3 -0

+	if (IS_ERR_OR_NULL(capabilities)) {
+		port->pd_count = 1;
+		capabilities = NULL;

... making this assignment unnecessary.

Quoted 6 lines

drivers/usb/typec/tcpm/tcpm.c +6 -0

+	} else {
+		port->pd_count = tcpm_fw_count_pd(capabilities);
+		if (!port->pd_count) {
+			fwnode_handle_put(capabilities);
+			return -ENODATA;
+		}

drivers/usb/typec/tcpm/tcpm.c static int tcpm_fw_get_caps(struct tcpm_port *port, · line 6288

Quoted 10 lines

drivers/usb/typec/tcpm/tcpm.c +3 -6

+	}
 
-	port->nr_snk_pdo = min(ret, PDO_MAX_OBJECTS);
-	ret = fwnode_property_read_u32_array(fwnode, "sink-pdos",
-					     port->snk_pdo, port->nr_snk_pdo);
-	if ((ret < 0) || tcpm_validate_caps(port, port->snk_pdo,
-					    port->nr_snk_pdo))
-		return -EINVAL;
+	port->pds = devm_kcalloc(port->dev, port->pd_count, sizeof(struct usb_power_delivery *),
+				 GFP_KERNEL);

devm_kcalloc() returns NULL on error, not an ERR_PTR.

drivers/usb/typec/tcpm/tcpm.c static int tcpm_fw_get_caps(struct tcpm_port *port, · line 6298

Quoted 13 lines

drivers/usb/typec/tcpm/tcpm.c +9 -3

+	if (IS_ERR_OR_NULL(port->pds)) {
+		fwnode_handle_put(capabilities);
+		return -ENOMEM;
+	}
 
-	if (fwnode_property_read_u32(fwnode, "op-sink-microwatt", &mw) < 0)
-		return -EINVAL;
-	port->operating_snk_mw = mw / 1000;
+	port->pd_list = devm_kcalloc(port->dev, port->pd_count, sizeof(struct pd_data *),
+				     GFP_KERNEL);
+	if (IS_ERR_OR_NULL(port->pd_list)) {
+		fwnode_handle_put(capabilities);
+		return -ENOMEM;

I think something like

		ret = -ENOMEM;
		goto put_capabilities;

would be better for those error handlers.

drivers/usb/typec/tcpm/tcpm.c static int tcpm_fw_get_caps(struct tcpm_port *port, · line 6303

Quoted 11 lines

drivers/usb/typec/tcpm/tcpm.c +4 -6

+	}
 
-	/* FRS can only be supported by DRP ports */
-	if (port->port_type == TYPEC_PORT_DRP) {
-		ret = fwnode_property_read_u32(fwnode, "new-source-frs-typec-current",
-					       &frs_current);
-		if (ret >= 0 && frs_current <= FRS_5V_3A)
-			port->new_source_frs_current = frs_current;
+	for (i = 0; i < port->pd_count; i++) {
+		port->pd_list[i] = devm_kzalloc(port->dev, sizeof(struct pd_data), GFP_KERNEL);
+

Missing error check

drivers/usb/typec/tcpm/tcpm.c static int tcpm_fw_get_caps(struct tcpm_port *port, · line 6316

Quoted 13 lines

drivers/usb/typec/tcpm/tcpm.c +13 -0

+		src_pdo = port->pd_list[i]->source_desc.pdo;
+		port->pd_list[i]->source_desc.role = TYPEC_SOURCE;
+		snk_pdo = port->pd_list[i]->sink_desc.pdo;
+		port->pd_list[i]->sink_desc.role = TYPEC_SINK;
+
+		/*
+		 * The last put is in "exit_fwnode_put" so forward the put to the beginning of this
+		 * "for loop". It doesn't matter for the first put because the first caps is NULL.
+		 * It won't run the second time if caps == fwnode because port->pd_count is 1 in
+		 * that case.
+		 */
+		fwnode_handle_put(caps);
+

I am quite sure that fwnode_get_next_child_node() handles the put.

drivers/usb/typec/tcpm/tcpm.c static int tcpm_fw_get_caps(struct tcpm_port *port, · line 6328

Quoted 12 lines

drivers/usb/typec/tcpm/tcpm.c +12 -0

+		/* If "capabilities" is NULL, fall back to single pd cap population. */
+		if (!capabilities)
+			caps = fwnode;
+		else
+			caps = fwnode_get_next_child_node(capabilities, caps);
+
+		if (port->port_type != TYPEC_PORT_SNK) {
+			ret = fwnode_property_count_u32(caps, "source-pdos");
+			if (ret == 0) {
+				ret = -EINVAL;
+				goto exit_fwnode_put;
+			} else if (ret < 0) {

else after goto is pointless.

Quoted 18 lines

drivers/usb/typec/tcpm/tcpm.c +18 -0

+				goto exit_fwnode_put;
+			}
+
+			nr_src_pdo = min(ret, PDO_MAX_OBJECTS);
+			ret = fwnode_property_read_u32_array(caps, "source-pdos", src_pdo,
+							     nr_src_pdo);
+			if (ret)
+				goto exit_fwnode_put;
+
+			ret = tcpm_validate_caps(port, src_pdo, nr_src_pdo);
+			if (ret)
+				goto exit_fwnode_put;
+
+			if (i == 0) {
+				port->nr_src_pdo = nr_src_pdo;
+				memcpy_and_pad(port->src_pdo, sizeof(u32) * PDO_MAX_OBJECTS,
+					       port->pd_list[0]->source_desc.pdo,
+					       sizeof(u32) * nr_src_pdo,

drivers/usb/typec/tcpm/tcpm.c static int tcpm_fw_get_caps(struct tcpm_port *port, · line 6356

Quoted 10 lines

drivers/usb/typec/tcpm/tcpm.c +10 -0

+					       0);
+			}
+		}
+
+		if (port->port_type != TYPEC_PORT_SRC) {
+			ret = fwnode_property_count_u32(caps, "sink-pdos");
+			if (ret == 0) {
+				ret = -EINVAL;
+				goto exit_fwnode_put;
+			} else if (ret < 0) {

Another else after goto.

Quoted 90 lines

drivers/usb/typec/tcpm/tcpm.c +48 -4

+				goto exit_fwnode_put;
+			}
+
+			nr_snk_pdo = min(ret, PDO_MAX_OBJECTS);
+			ret = fwnode_property_read_u32_array(caps, "sink-pdos", snk_pdo,
+							     nr_snk_pdo);
+			if (ret)
+				goto exit_fwnode_put;
+
+			ret = tcpm_validate_caps(port, snk_pdo, nr_snk_pdo);
+			if (ret)
+				goto exit_fwnode_put;
+
+			if (fwnode_property_read_u32(caps, "op-sink-microwatt", &uw) < 0) {
+				ret = -EINVAL;
+				goto exit_fwnode_put;
+			}
+
+			port->pd_list[i]->operating_snk_mw = uw / 1000;
+
+			if (i == 0) {
+				port->nr_snk_pdo = nr_snk_pdo;
+				memcpy_and_pad(port->snk_pdo, sizeof(u32) * PDO_MAX_OBJECTS,
+					       port->pd_list[0]->sink_desc.pdo,
+					       sizeof(u32) * nr_snk_pdo,
+					       0);
+				port->operating_snk_mw = port->pd_list[0]->operating_snk_mw;
+			}
+		}
 	}
 
+exit_fwnode_put:
+	if (caps != fwnode)
+		fwnode_handle_put(caps);
+	fwnode_handle_put(capabilities);
+	return ret;
+}
+
+static int tcpm_fw_get_snk_vdos(struct tcpm_port *port, struct fwnode_handle *fwnode)
+{
+	int ret;
+
 	/* sink-vdos is optional */
 	ret = fwnode_property_count_u32(fwnode, "sink-vdos");
 	if (ret < 0)
-		ret = 0;
+		return 0;
 
 	port->nr_snk_vdo = min(ret, VDO_MAX_OBJECTS);
 	if (port->nr_snk_vdo) {

@@ -6521,13 +6764,18 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
 	init_completion(&port->pps_complete);
 	tcpm_debugfs_init(port);
 
+	err = tcpm_fw_get_properties(port, tcpc->fwnode);
+	if (err < 0)
+		goto out_destroy_wq;
 	err = tcpm_fw_get_caps(port, tcpc->fwnode);
+	if (err < 0)
+		goto out_destroy_wq;
+	err = tcpm_fw_get_snk_vdos(port, tcpc->fwnode);
 	if (err < 0)
 		goto out_destroy_wq;
 
 	port->try_role = port->typec_caps.prefer_role;
 
-	port->typec_caps.fwnode = tcpc->fwnode;
 	port->typec_caps.revision = 0x0120;	/* Type-C spec release 1.2 */
 	port->typec_caps.pd_revision = 0x0300;	/* USB-PD spec release 3.0 */
 	port->typec_caps.svdm_version = SVDM_VER_2_0;

@@ -6536,7 +6784,6 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
 	port->typec_caps.orientation_aware = 1;
 
 	port->partner_desc.identity = &port->partner_ident;
-	port->port_type = port->typec_caps.type;
 
 	port->role_sw = usb_role_switch_get(port->dev);
 	if (!port->role_sw)

@@ -6555,7 +6802,7 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
 	if (err)
 		goto out_role_sw_put;
 
-	port->typec_caps.pd = port->pd;
+	port->typec_caps.pd = port->pds[0];
 
 	port->typec_port = typec_register_port(port->dev, &port->typec_caps);
 	if (IS_ERR(port->typec_port)) {

Signature · 1 line

2.41.0.585.gd2178a4bd4-goog