[PATCH v2 0/7] usb: typec: Introduce typec-switch binding

[PATCH v2 0/7] usb: typec: Introduce typec-switch binding

[Prashant Malani]

This series introduces a binding for Type-C data lane switches. These
control the routing and operating modes of USB Type-C data lanes based
on the PD messaging from the Type-C port driver regarding connected
peripherals.

The first patch introduces a change to the Type-C mux class mode-switch
matching code, while the second adds a config guard to a Type-C header.
The next couple of patches introduce the new "typec-switch" binding as
well as one user of it (the ANX7625 drm bridge).

The remaining patches add functionality to the anx7625 driver to
register the mode-switches, as well as program its crosspoint
switch depending on which Type-C port has a DisplayPort (DP) peripheral
connected to it.

v1: https://lore.kernel.org/linux-usb/20220607190131.1647511-1-pmalani@chromium.org/

Changes since v1:
- Fixed function signature error in "else case" of typec_mux.h
- typec-switch.yaml: Fixed indentation, compatible, and node names in examples.
- anx7625.yaml:
    + Introduced patternProperties for "switch" children (suggested by Krzysztof Kozlowski).
    + Added unevaluatedProperties descriptor (suggested by Krzysztof Kozlowski).
    + Added "address-cells" and "size-cells" properties to "switches".

Pin-Yen Lin (1):
  drm/bridge: anx7625: Add typec_mux_set callback function

Prashant Malani (6):
  usb: typec: mux: Allow muxes to specify mode-switch
  usb: typec: mux: Add CONFIG guards for functions
  dt-bindings: usb: Add Type-C switch binding
  dt-bindings: drm/bridge: anx7625: Add mode-switch support
  drm/bridge: anx7625: Register number of Type C switches
  drm/bridge: anx7625: Register Type-C mode switches

 .../display/bridge/analogix,anx7625.yaml      |  63 ++++++++
 .../devicetree/bindings/usb/typec-switch.yaml |  74 +++++++++
 drivers/gpu/drm/bridge/analogix/anx7625.c     | 151 ++++++++++++++++++
 drivers/gpu/drm/bridge/analogix/anx7625.h     |  20 +++
 drivers/usb/typec/mux.c                       |   8 +-
 include/linux/usb/typec_mux.h                 |  38 +++++
 6 files changed, 352 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/typec-switch.yaml

-- 
2.36.1.476.g0c4daa206d-goog

[PATCH v2 0/7] usb: typec: Introduce typec-switch binding

[Prashant Malani]

This series introduces a binding for Type-C data lane switches. These
control the routing and operating modes of USB Type-C data lanes based
on the PD messaging from the Type-C port driver regarding connected
peripherals.

The first patch introduces a change to the Type-C mux class mode-switch
matching code, while the second adds a config guard to a Type-C header.
The next couple of patches introduce the new "typec-switch" binding as
well as one user of it (the ANX7625 drm bridge).

The remaining patches add functionality to the anx7625 driver to
register the mode-switches, as well as program its crosspoint
switch depending on which Type-C port has a DisplayPort (DP) peripheral
connected to it.

v1: https://lore.kernel.org/linux-usb/20220607190131.1647511-1-pmalani@chromium.org/

Changes since v1:
- Fixed function signature error in "else case" of typec_mux.h
- typec-switch.yaml: Fixed indentation, compatible, and node names in examples.
- anx7625.yaml:
    + Introduced patternProperties for "switch" children (suggested by Krzysztof Kozlowski).
    + Added unevaluatedProperties descriptor (suggested by Krzysztof Kozlowski).
    + Added "address-cells" and "size-cells" properties to "switches".

Pin-Yen Lin (1):
  drm/bridge: anx7625: Add typec_mux_set callback function

Prashant Malani (6):
  usb: typec: mux: Allow muxes to specify mode-switch
  usb: typec: mux: Add CONFIG guards for functions
  dt-bindings: usb: Add Type-C switch binding
  dt-bindings: drm/bridge: anx7625: Add mode-switch support
  drm/bridge: anx7625: Register number of Type C switches
  drm/bridge: anx7625: Register Type-C mode switches

 .../display/bridge/analogix,anx7625.yaml      |  63 ++++++++
 .../devicetree/bindings/usb/typec-switch.yaml |  74 +++++++++
 drivers/gpu/drm/bridge/analogix/anx7625.c     | 151 ++++++++++++++++++
 drivers/gpu/drm/bridge/analogix/anx7625.h     |  20 +++
 drivers/usb/typec/mux.c                       |   8 +-
 include/linux/usb/typec_mux.h                 |  38 +++++
 6 files changed, 352 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/typec-switch.yaml

-- 
2.36.1.476.g0c4daa206d-goog

[PATCH v2 1/7] usb: typec: mux: Allow muxes to specify mode-switch

[Prashant Malani]

Loosen the typec_mux_match() requirements so that searches where an
alt mode is not specified, but the target mux device lists the
"mode-switch" property, return a success.

This is helpful in Type C port drivers which would like to get a pointer
to the mux switch associated with a Type C port, but don't want to
specify a particular alt mode.

Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

Changes since v1:
- No changes.

 drivers/usb/typec/mux.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c
index fd55c2c516a5..464330776cd6 100644
--- a/drivers/usb/typec/mux.c
+++ b/drivers/usb/typec/mux.c
@@ -281,9 +281,13 @@ static void *typec_mux_match(struct fwnode_handle *fwnode, const char *id,
 	if (match)
 		goto find_mux;
 
-	/* Accessory Mode muxes */
 	if (!desc) {
-		match = fwnode_property_present(fwnode, "accessory");
+		/*
+		 * Accessory Mode muxes & muxes which explicitly specify
+		 * the required identifier can avoid SVID matching.
+		 */
+		match = fwnode_property_present(fwnode, "accessory") ||
+			fwnode_property_present(fwnode, id);
 		if (match)
 			goto find_mux;
 		return NULL;
-- 
2.36.1.476.g0c4daa206d-goog

[PATCH v2 1/7] usb: typec: mux: Allow muxes to specify mode-switch

[Prashant Malani]

Loosen the typec_mux_match() requirements so that searches where an
alt mode is not specified, but the target mux device lists the
"mode-switch" property, return a success.

This is helpful in Type C port drivers which would like to get a pointer
to the mux switch associated with a Type C port, but don't want to
specify a particular alt mode.

Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

Changes since v1:
- No changes.

 drivers/usb/typec/mux.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c
index fd55c2c516a5..464330776cd6 100644
--- a/drivers/usb/typec/mux.c
+++ b/drivers/usb/typec/mux.c
@@ -281,9 +281,13 @@ static void *typec_mux_match(struct fwnode_handle *fwnode, const char *id,
 	if (match)
 		goto find_mux;
 
-	/* Accessory Mode muxes */
 	if (!desc) {
-		match = fwnode_property_present(fwnode, "accessory");
+		/*
+		 * Accessory Mode muxes & muxes which explicitly specify
+		 * the required identifier can avoid SVID matching.
+		 */
+		match = fwnode_property_present(fwnode, "accessory") ||
+			fwnode_property_present(fwnode, id);
 		if (match)
 			goto find_mux;
 		return NULL;
-- 
2.36.1.476.g0c4daa206d-goog

[PATCH v2 2/7] usb: typec: mux: Add CONFIG guards for functions

[Prashant Malani]

There are some drivers that can use the Type C mux API, but don't have
to. Introduce CONFIG guards for the mux functions so that drivers can
include the header file and not run into compilation errors on systems
which don't have CONFIG_TYPEC enabled. When CONFIG_TYPEC is not enabled,
the Type C mux functions will be stub versions of the original calls.

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

Changes since v1:
- Added static inline to stub functions.
- Updated function signature of stub functions from "struct typec_mux"
  to "struct typec_mux_dev" in accordance with updates from commit
  713fd49b430c ("usb: typec: mux: Introduce indirection")

 include/linux/usb/typec_mux.h | 38 +++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/include/linux/usb/typec_mux.h b/include/linux/usb/typec_mux.h
index ee57781dcf28..9eda6146fd26 100644
--- a/include/linux/usb/typec_mux.h
+++ b/include/linux/usb/typec_mux.h
@@ -58,6 +58,8 @@ struct typec_mux_desc {
 	void *drvdata;
 };
 
+#if IS_ENABLED(CONFIG_TYPEC) || IS_MODULE(CONFIG_TYPEC)
+
 struct typec_mux *fwnode_typec_mux_get(struct fwnode_handle *fwnode,
 				       const struct typec_altmode_desc *desc);
 void typec_mux_put(struct typec_mux *mux);
@@ -76,4 +78,40 @@ void typec_mux_unregister(struct typec_mux_dev *mux);
 void typec_mux_set_drvdata(struct typec_mux_dev *mux, void *data);
 void *typec_mux_get_drvdata(struct typec_mux_dev *mux);
 
+#else
+
+static inline struct typec_mux *fwnode_typec_mux_get(struct fwnode_handle *fwnode,
+				       const struct typec_altmode_desc *desc)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
+
+static inline void typec_mux_put(struct typec_mux *mux) {}
+
+static inline int typec_mux_set(struct typec_mux *mux, struct typec_mux_state *state)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline struct typec_mux *
+typec_mux_get(struct device *dev, const struct typec_altmode_desc *desc)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
+
+static inline struct typec_mux_dev *
+typec_mux_register(struct device *parent, const struct typec_mux_desc *desc)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
+static inline void typec_mux_unregister(struct typec_mux_dev *mux) {}
+
+static inline void typec_mux_set_drvdata(struct typec_mux_dev *mux, void *data) {}
+static inline void *typec_mux_get_drvdata(struct typec_mux_dev *mux)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
+
+#endif /* CONFIG_TYPEC */
+
 #endif /* __USB_TYPEC_MUX */
-- 
2.36.1.476.g0c4daa206d-goog

[PATCH v2 2/7] usb: typec: mux: Add CONFIG guards for functions

[Prashant Malani]

There are some drivers that can use the Type C mux API, but don't have
to. Introduce CONFIG guards for the mux functions so that drivers can
include the header file and not run into compilation errors on systems
which don't have CONFIG_TYPEC enabled. When CONFIG_TYPEC is not enabled,
the Type C mux functions will be stub versions of the original calls.

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

Changes since v1:
- Added static inline to stub functions.
- Updated function signature of stub functions from "struct typec_mux"
  to "struct typec_mux_dev" in accordance with updates from commit
  713fd49b430c ("usb: typec: mux: Introduce indirection")

 include/linux/usb/typec_mux.h | 38 +++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/include/linux/usb/typec_mux.h b/include/linux/usb/typec_mux.h
index ee57781dcf28..9eda6146fd26 100644
--- a/include/linux/usb/typec_mux.h
+++ b/include/linux/usb/typec_mux.h
@@ -58,6 +58,8 @@ struct typec_mux_desc {
 	void *drvdata;
 };
 
+#if IS_ENABLED(CONFIG_TYPEC) || IS_MODULE(CONFIG_TYPEC)
+
 struct typec_mux *fwnode_typec_mux_get(struct fwnode_handle *fwnode,
 				       const struct typec_altmode_desc *desc);
 void typec_mux_put(struct typec_mux *mux);
@@ -76,4 +78,40 @@ void typec_mux_unregister(struct typec_mux_dev *mux);
 void typec_mux_set_drvdata(struct typec_mux_dev *mux, void *data);
 void *typec_mux_get_drvdata(struct typec_mux_dev *mux);
 
+#else
+
+static inline struct typec_mux *fwnode_typec_mux_get(struct fwnode_handle *fwnode,
+				       const struct typec_altmode_desc *desc)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
+
+static inline void typec_mux_put(struct typec_mux *mux) {}
+
+static inline int typec_mux_set(struct typec_mux *mux, struct typec_mux_state *state)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline struct typec_mux *
+typec_mux_get(struct device *dev, const struct typec_altmode_desc *desc)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
+
+static inline struct typec_mux_dev *
+typec_mux_register(struct device *parent, const struct typec_mux_desc *desc)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
+static inline void typec_mux_unregister(struct typec_mux_dev *mux) {}
+
+static inline void typec_mux_set_drvdata(struct typec_mux_dev *mux, void *data) {}
+static inline void *typec_mux_get_drvdata(struct typec_mux_dev *mux)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
+
+#endif /* CONFIG_TYPEC */
+
 #endif /* __USB_TYPEC_MUX */
-- 
2.36.1.476.g0c4daa206d-goog

[PATCH v2 3/7] dt-bindings: usb: Add Type-C switch binding

[Prashant Malani]

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).

Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

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@chromium.org>
+
+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";
+            mode-switch;
+            orientation-switch;
+            ports {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                port@0 {
+                    reg = <0>;
+                    anx_ep: endpoint {
+                        remote-endpoint = <&typec_controller>;
+                    };
+                };
+            };
+        };
+    };
-- 
2.36.1.476.g0c4daa206d-goog

[PATCH v2 3/7] dt-bindings: usb: Add Type-C switch binding

[Prashant Malani]

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).

Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

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@chromium.org>
+
+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";
+            mode-switch;
+            orientation-switch;
+            ports {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                port@0 {
+                    reg = <0>;
+                    anx_ep: endpoint {
+                        remote-endpoint = <&typec_controller>;
+                    };
+                };
+            };
+        };
+    };
-- 
2.36.1.476.g0c4daa206d-goog

[PATCH v2 4/7] dt-bindings: drm/bridge: anx7625: Add mode-switch support

[Prashant Malani]

Analogix 7625 can be used in systems to switch USB Type-C DisplayPort
alternate mode lane traffic between 2 Type-C ports.

Update the binding to accommodate this usage by introducing a switch
property.

Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

Changes since v1:
- Introduced patternProperties for "switch" children (suggested by Krzysztof Kozlowski).
- Added unevaluatedProperties descriptor (suggested by Krzysztof Kozlowski).
- Added "address-cells" and "size-cells" properties to "switches".

 .../display/bridge/analogix,anx7625.yaml      | 63 +++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
index 35a48515836e..cb4a23391244 100644
--- a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
@@ -105,6 +105,33 @@ properties:
       - port@0
       - port@1
 
+  switches:
+    type: object
+    description: Set of switches controlling DisplayPort traffic on
+      outgoing RX/TX lanes to Type C ports.
+    unevaluatedProperties: false
+
+    properties:
+      "#address-cells":
+        const: 1
+
+      "#size-cells":
+        const: 0
+
+    patternProperties:
+      'switch@[01]':
+        $ref: /schemas/usb/typec-switch.yaml#
+
+        properties:
+          reg:
+            maxItems: 1
+
+        required:
+          - reg
+
+    required:
+      - switch@0
+
 required:
   - compatible
   - reg
@@ -167,5 +194,41 @@ examples:
                     };
                 };
             };
+            switches {
+                #address-cells = <1>;
+                #size-cells = <0>;
+                switch@0 {
+                    compatible = "typec-switch";
+                    reg = <0>;
+                    mode-switch;
+
+                    ports {
+                        #address-cells = <1>;
+                        #size-cells = <0>;
+                        port@0 {
+                            reg = <0>;
+                            anx_typec0: endpoint {
+                                remote-endpoint = <&typec_port0>;
+                            };
+                        };
+                    };
+                };
+                switch@1 {
+                    compatible = "typec-switch";
+                    reg = <1>;
+                    mode-switch;
+
+                    ports {
+                        #address-cells = <1>;
+                        #size-cells = <0>;
+                        port@0 {
+                            reg = <0>;
+                            anx_typec1: endpoint {
+                                remote-endpoint = <&typec_port1>;
+                            };
+                        };
+                    };
+                };
+            };
         };
     };
-- 
2.36.1.476.g0c4daa206d-goog

[PATCH v2 4/7] dt-bindings: drm/bridge: anx7625: Add mode-switch support

[Prashant Malani]

Analogix 7625 can be used in systems to switch USB Type-C DisplayPort
alternate mode lane traffic between 2 Type-C ports.

Update the binding to accommodate this usage by introducing a switch
property.

Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

Changes since v1:
- Introduced patternProperties for "switch" children (suggested by Krzysztof Kozlowski).
- Added unevaluatedProperties descriptor (suggested by Krzysztof Kozlowski).
- Added "address-cells" and "size-cells" properties to "switches".

 .../display/bridge/analogix,anx7625.yaml      | 63 +++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
index 35a48515836e..cb4a23391244 100644
--- a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
@@ -105,6 +105,33 @@ properties:
       - port@0
       - port@1
 
+  switches:
+    type: object
+    description: Set of switches controlling DisplayPort traffic on
+      outgoing RX/TX lanes to Type C ports.
+    unevaluatedProperties: false
+
+    properties:
+      "#address-cells":
+        const: 1
+
+      "#size-cells":
+        const: 0
+
+    patternProperties:
+      'switch@[01]':
+        $ref: /schemas/usb/typec-switch.yaml#
+
+        properties:
+          reg:
+            maxItems: 1
+
+        required:
+          - reg
+
+    required:
+      - switch@0
+
 required:
   - compatible
   - reg
@@ -167,5 +194,41 @@ examples:
                     };
                 };
             };
+            switches {
+                #address-cells = <1>;
+                #size-cells = <0>;
+                switch@0 {
+                    compatible = "typec-switch";
+                    reg = <0>;
+                    mode-switch;
+
+                    ports {
+                        #address-cells = <1>;
+                        #size-cells = <0>;
+                        port@0 {
+                            reg = <0>;
+                            anx_typec0: endpoint {
+                                remote-endpoint = <&typec_port0>;
+                            };
+                        };
+                    };
+                };
+                switch@1 {
+                    compatible = "typec-switch";
+                    reg = <1>;
+                    mode-switch;
+
+                    ports {
+                        #address-cells = <1>;
+                        #size-cells = <0>;
+                        port@0 {
+                            reg = <0>;
+                            anx_typec1: endpoint {
+                                remote-endpoint = <&typec_port1>;
+                            };
+                        };
+                    };
+                };
+            };
         };
     };
-- 
2.36.1.476.g0c4daa206d-goog

[PATCH v2 5/7] drm/bridge: anx7625: Register number of Type C switches

[Prashant Malani]

Parse the "switches" node, if available, and count and store the number
of Type-C switches within it. Since we currently don't do anything with
this info, no functional changes are expected from this change.

This patch sets a foundation for the actual registering of Type-C
switches with the Type-C connector class framework.

Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

Changes since v1:
- No changes.

 drivers/gpu/drm/bridge/analogix/anx7625.c | 20 ++++++++++++++++++++
 drivers/gpu/drm/bridge/analogix/anx7625.h |  1 +
 2 files changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
index 53a5da6c49dd..07ed44c6b839 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.c
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
@@ -2581,6 +2581,22 @@ static void anx7625_runtime_disable(void *data)
 	pm_runtime_disable(data);
 }
 
+static int anx7625_register_typec_switches(struct device *device, struct anx7625_data *ctx)
+{
+	struct device_node *of = NULL;
+	int ret = 0;
+
+	of = of_get_child_by_name(device->of_node, "switches");
+	if (!of)
+		return -ENODEV;
+
+	ctx->num_typec_switches = of_get_child_count(of);
+	if (ctx->num_typec_switches <= 0)
+		return -ENODEV;
+
+	return ret;
+}
+
 static int anx7625_i2c_probe(struct i2c_client *client,
 			     const struct i2c_device_id *id)
 {
@@ -2686,6 +2702,10 @@ static int anx7625_i2c_probe(struct i2c_client *client,
 	if (platform->pdata.intp_irq)
 		queue_work(platform->workqueue, &platform->work);
 
+	ret = anx7625_register_typec_switches(dev, platform);
+	if (ret)
+		dev_info(dev, "Didn't register Type C switches, err: %d\n", ret);
+
 	platform->bridge.funcs = &anx7625_bridge_funcs;
 	platform->bridge.of_node = client->dev.of_node;
 	if (!anx7625_of_panel_on_aux_bus(&client->dev))
diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h b/drivers/gpu/drm/bridge/analogix/anx7625.h
index e257a84db962..d5cbca708842 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.h
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.h
@@ -473,6 +473,7 @@ struct anx7625_data {
 	struct drm_connector *connector;
 	struct mipi_dsi_device *dsi;
 	struct drm_dp_aux aux;
+	int num_typec_switches;
 };
 
 #endif  /* __ANX7625_H__ */
-- 
2.36.1.476.g0c4daa206d-goog

[PATCH v2 5/7] drm/bridge: anx7625: Register number of Type C switches

[Prashant Malani]

Parse the "switches" node, if available, and count and store the number
of Type-C switches within it. Since we currently don't do anything with
this info, no functional changes are expected from this change.

This patch sets a foundation for the actual registering of Type-C
switches with the Type-C connector class framework.

Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

Changes since v1:
- No changes.

 drivers/gpu/drm/bridge/analogix/anx7625.c | 20 ++++++++++++++++++++
 drivers/gpu/drm/bridge/analogix/anx7625.h |  1 +
 2 files changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
index 53a5da6c49dd..07ed44c6b839 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.c
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
@@ -2581,6 +2581,22 @@ static void anx7625_runtime_disable(void *data)
 	pm_runtime_disable(data);
 }
 
+static int anx7625_register_typec_switches(struct device *device, struct anx7625_data *ctx)
+{
+	struct device_node *of = NULL;
+	int ret = 0;
+
+	of = of_get_child_by_name(device->of_node, "switches");
+	if (!of)
+		return -ENODEV;
+
+	ctx->num_typec_switches = of_get_child_count(of);
+	if (ctx->num_typec_switches <= 0)
+		return -ENODEV;
+
+	return ret;
+}
+
 static int anx7625_i2c_probe(struct i2c_client *client,
 			     const struct i2c_device_id *id)
 {
@@ -2686,6 +2702,10 @@ static int anx7625_i2c_probe(struct i2c_client *client,
 	if (platform->pdata.intp_irq)
 		queue_work(platform->workqueue, &platform->work);
 
+	ret = anx7625_register_typec_switches(dev, platform);
+	if (ret)
+		dev_info(dev, "Didn't register Type C switches, err: %d\n", ret);
+
 	platform->bridge.funcs = &anx7625_bridge_funcs;
 	platform->bridge.of_node = client->dev.of_node;
 	if (!anx7625_of_panel_on_aux_bus(&client->dev))
diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h b/drivers/gpu/drm/bridge/analogix/anx7625.h
index e257a84db962..d5cbca708842 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.h
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.h
@@ -473,6 +473,7 @@ struct anx7625_data {
 	struct drm_connector *connector;
 	struct mipi_dsi_device *dsi;
 	struct drm_dp_aux aux;
+	int num_typec_switches;
 };
 
 #endif  /* __ANX7625_H__ */
-- 
2.36.1.476.g0c4daa206d-goog

[PATCH v2 6/7] drm/bridge: anx7625: Register Type-C mode switches

[Prashant Malani]

When the DT node has "switches" available, register a Type-C mode-switch
for each listed "switch". This allows the driver to receive state
information about what operating mode a Type-C port and its connected
peripherals are in, as well as status information (like VDOs) related to
that state.

The callback function is currently a stub, but subsequent patches will
implement the required functionality.

Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

Changes since v2:
- No changes.

 drivers/gpu/drm/bridge/analogix/anx7625.c | 73 +++++++++++++++++++++++
 drivers/gpu/drm/bridge/analogix/anx7625.h |  6 ++
 2 files changed, 79 insertions(+)

diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
index 07ed44c6b839..d41a21103bd3 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.c
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
@@ -15,6 +15,7 @@
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <linux/types.h>
+#include <linux/usb/typec_mux.h>
 #include <linux/workqueue.h>
 
 #include <linux/of_gpio.h>
@@ -2581,9 +2582,59 @@ static void anx7625_runtime_disable(void *data)
 	pm_runtime_disable(data);
 }
 
+static int anx7625_typec_mux_set(struct typec_mux_dev *mux,
+				 struct typec_mux_state *state)
+{
+	return 0;
+}
+
+static int anx7625_register_mode_switch(struct device *dev, struct device_node *node,
+					struct anx7625_data *ctx)
+{
+	struct anx7625_port_data *port_data;
+	struct typec_mux_desc mux_desc = {};
+	char name[32];
+	u32 port_num;
+	int ret;
+
+	ret = of_property_read_u32(node, "reg", &port_num);
+	if (ret)
+		return ret;
+
+	if (port_num >= ctx->num_typec_switches) {
+		dev_err(dev, "Invalid port number specified: %d\n", port_num);
+		return -EINVAL;
+	}
+
+	port_data = &ctx->typec_ports[port_num];
+	port_data->ctx = ctx;
+	mux_desc.fwnode = &node->fwnode;
+	mux_desc.drvdata = port_data;
+	snprintf(name, sizeof(name), "%s-%u", node->name, port_num);
+	mux_desc.name = name;
+	mux_desc.set = anx7625_typec_mux_set;
+
+	port_data->typec_mux = typec_mux_register(dev, &mux_desc);
+	if (IS_ERR(port_data->typec_mux)) {
+		ret = PTR_ERR(port_data->typec_mux);
+		dev_err(dev, "Mode switch register for port %d failed: %d", port_num, ret);
+	}
+
+	return ret;
+}
+
+static void anx7625_unregister_typec_switches(struct anx7625_data *ctx)
+{
+	int i;
+
+	for (i = 0; i < ctx->num_typec_switches; i++)
+		typec_mux_unregister(ctx->typec_ports[i].typec_mux);
+}
+
 static int anx7625_register_typec_switches(struct device *device, struct anx7625_data *ctx)
 {
 	struct device_node *of = NULL;
+	struct device_node *sw;
 	int ret = 0;
 
 	of = of_get_child_by_name(device->of_node, "switches");
@@ -2594,6 +2645,26 @@ static int anx7625_register_typec_switches(struct device *device, struct anx7625
 	if (ctx->num_typec_switches <= 0)
 		return -ENODEV;
 
+	ctx->typec_ports = devm_kzalloc(device,
+					ctx->num_typec_switches * sizeof(struct anx7625_port_data),
+					GFP_KERNEL);
+	if (!ctx->typec_ports)
+		return -ENOMEM;
+
+	/* Register switches for each connector. */
+	for_each_available_child_of_node(of, sw) {
+		if (!of_property_read_bool(sw, "mode-switch"))
+			continue;
+		ret = anx7625_register_mode_switch(device, sw, ctx);
+		if (ret) {
+			dev_err(device, "Failed to register mode switch: %d\n", ret);
+			break;
+		}
+	}
+
+	if (ret)
+		anx7625_unregister_typec_switches(ctx);
+
 	return ret;
 }
 
@@ -2759,6 +2830,8 @@ static int anx7625_i2c_remove(struct i2c_client *client)
 
 	drm_bridge_remove(&platform->bridge);
 
+	anx7625_unregister_typec_switches(platform);
+
 	if (platform->pdata.intp_irq)
 		destroy_workqueue(platform->workqueue);
 
diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h b/drivers/gpu/drm/bridge/analogix/anx7625.h
index d5cbca708842..76cfc64f7574 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.h
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.h
@@ -443,6 +443,11 @@ struct anx7625_i2c_client {
 	struct i2c_client *tcpc_client;
 };
 
+struct anx7625_port_data {
+	struct typec_mux_dev *typec_mux;
+	struct anx7625_data *ctx;
+};
+
 struct anx7625_data {
 	struct anx7625_platform_data pdata;
 	struct platform_device *audio_pdev;
@@ -474,6 +479,7 @@ struct anx7625_data {
 	struct mipi_dsi_device *dsi;
 	struct drm_dp_aux aux;
 	int num_typec_switches;
+	struct anx7625_port_data *typec_ports;
 };
 
 #endif  /* __ANX7625_H__ */
-- 
2.36.1.476.g0c4daa206d-goog

[PATCH v2 6/7] drm/bridge: anx7625: Register Type-C mode switches

[Prashant Malani]

When the DT node has "switches" available, register a Type-C mode-switch
for each listed "switch". This allows the driver to receive state
information about what operating mode a Type-C port and its connected
peripherals are in, as well as status information (like VDOs) related to
that state.

The callback function is currently a stub, but subsequent patches will
implement the required functionality.

Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

Changes since v2:
- No changes.

 drivers/gpu/drm/bridge/analogix/anx7625.c | 73 +++++++++++++++++++++++
 drivers/gpu/drm/bridge/analogix/anx7625.h |  6 ++
 2 files changed, 79 insertions(+)

diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
index 07ed44c6b839..d41a21103bd3 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.c
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
@@ -15,6 +15,7 @@
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <linux/types.h>
+#include <linux/usb/typec_mux.h>
 #include <linux/workqueue.h>
 
 #include <linux/of_gpio.h>
@@ -2581,9 +2582,59 @@ static void anx7625_runtime_disable(void *data)
 	pm_runtime_disable(data);
 }
 
+static int anx7625_typec_mux_set(struct typec_mux_dev *mux,
+				 struct typec_mux_state *state)
+{
+	return 0;
+}
+
+static int anx7625_register_mode_switch(struct device *dev, struct device_node *node,
+					struct anx7625_data *ctx)
+{
+	struct anx7625_port_data *port_data;
+	struct typec_mux_desc mux_desc = {};
+	char name[32];
+	u32 port_num;
+	int ret;
+
+	ret = of_property_read_u32(node, "reg", &port_num);
+	if (ret)
+		return ret;
+
+	if (port_num >= ctx->num_typec_switches) {
+		dev_err(dev, "Invalid port number specified: %d\n", port_num);
+		return -EINVAL;
+	}
+
+	port_data = &ctx->typec_ports[port_num];
+	port_data->ctx = ctx;
+	mux_desc.fwnode = &node->fwnode;
+	mux_desc.drvdata = port_data;
+	snprintf(name, sizeof(name), "%s-%u", node->name, port_num);
+	mux_desc.name = name;
+	mux_desc.set = anx7625_typec_mux_set;
+
+	port_data->typec_mux = typec_mux_register(dev, &mux_desc);
+	if (IS_ERR(port_data->typec_mux)) {
+		ret = PTR_ERR(port_data->typec_mux);
+		dev_err(dev, "Mode switch register for port %d failed: %d", port_num, ret);
+	}
+
+	return ret;
+}
+
+static void anx7625_unregister_typec_switches(struct anx7625_data *ctx)
+{
+	int i;
+
+	for (i = 0; i < ctx->num_typec_switches; i++)
+		typec_mux_unregister(ctx->typec_ports[i].typec_mux);
+}
+
 static int anx7625_register_typec_switches(struct device *device, struct anx7625_data *ctx)
 {
 	struct device_node *of = NULL;
+	struct device_node *sw;
 	int ret = 0;
 
 	of = of_get_child_by_name(device->of_node, "switches");
@@ -2594,6 +2645,26 @@ static int anx7625_register_typec_switches(struct device *device, struct anx7625
 	if (ctx->num_typec_switches <= 0)
 		return -ENODEV;
 
+	ctx->typec_ports = devm_kzalloc(device,
+					ctx->num_typec_switches * sizeof(struct anx7625_port_data),
+					GFP_KERNEL);
+	if (!ctx->typec_ports)
+		return -ENOMEM;
+
+	/* Register switches for each connector. */
+	for_each_available_child_of_node(of, sw) {
+		if (!of_property_read_bool(sw, "mode-switch"))
+			continue;
+		ret = anx7625_register_mode_switch(device, sw, ctx);
+		if (ret) {
+			dev_err(device, "Failed to register mode switch: %d\n", ret);
+			break;
+		}
+	}
+
+	if (ret)
+		anx7625_unregister_typec_switches(ctx);
+
 	return ret;
 }
 
@@ -2759,6 +2830,8 @@ static int anx7625_i2c_remove(struct i2c_client *client)
 
 	drm_bridge_remove(&platform->bridge);
 
+	anx7625_unregister_typec_switches(platform);
+
 	if (platform->pdata.intp_irq)
 		destroy_workqueue(platform->workqueue);
 
diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h b/drivers/gpu/drm/bridge/analogix/anx7625.h
index d5cbca708842..76cfc64f7574 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.h
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.h
@@ -443,6 +443,11 @@ struct anx7625_i2c_client {
 	struct i2c_client *tcpc_client;
 };
 
+struct anx7625_port_data {
+	struct typec_mux_dev *typec_mux;
+	struct anx7625_data *ctx;
+};
+
 struct anx7625_data {
 	struct anx7625_platform_data pdata;
 	struct platform_device *audio_pdev;
@@ -474,6 +479,7 @@ struct anx7625_data {
 	struct mipi_dsi_device *dsi;
 	struct drm_dp_aux aux;
 	int num_typec_switches;
+	struct anx7625_port_data *typec_ports;
 };
 
 #endif  /* __ANX7625_H__ */
-- 
2.36.1.476.g0c4daa206d-goog

[PATCH v2 7/7] drm/bridge: anx7625: Add typec_mux_set callback function

[Prashant Malani]

From: Pin-Yen Lin <treapking@chromium.org>

Add the callback function when the driver receives state
changes of the Type-C port. The callback function configures the
crosspoint switch of the anx7625 bridge chip, which can change the
output pins of the signals according to the port state.

Signed-off-by: Pin-Yen Lin <treapking@chromium.org>
Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

Changes since v2:
- No changes.

 drivers/gpu/drm/bridge/analogix/anx7625.c | 58 +++++++++++++++++++++++
 drivers/gpu/drm/bridge/analogix/anx7625.h | 13 +++++
 2 files changed, 71 insertions(+)

diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
index d41a21103bd3..2c308d12fab2 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.c
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
@@ -15,6 +15,7 @@
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <linux/types.h>
+#include <linux/usb/typec_dp.h>
 #include <linux/usb/typec_mux.h>
 #include <linux/workqueue.h>
 
@@ -2582,9 +2583,66 @@ static void anx7625_runtime_disable(void *data)
 	pm_runtime_disable(data);
 }
 
+static void anx7625_set_crosspoint_switch(struct anx7625_data *ctx,
+					  enum typec_orientation orientation)
+{
+	if (orientation == TYPEC_ORIENTATION_NORMAL) {
+		anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0,
+				  SW_SEL1_SSRX_RX1 | SW_SEL1_DPTX0_RX2);
+		anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1,
+				  SW_SEL2_SSTX_TX1 | SW_SEL2_DPTX1_TX2);
+	} else if (orientation == TYPEC_ORIENTATION_REVERSE) {
+		anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0,
+				  SW_SEL1_SSRX_RX2 | SW_SEL1_DPTX0_RX1);
+		anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1,
+				  SW_SEL2_SSTX_TX2 | SW_SEL2_DPTX1_TX1);
+	}
+}
+
+static void anx7625_typec_two_ports_update(struct anx7625_data *ctx)
+{
+	if (ctx->typec_ports[0].dp_connected && ctx->typec_ports[1].dp_connected)
+		/* Both ports available, do nothing to retain the current one. */
+		return;
+	else if (ctx->typec_ports[0].dp_connected)
+		anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_NORMAL);
+	else if (ctx->typec_ports[1].dp_connected)
+		anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_REVERSE);
+}
+
 static int anx7625_typec_mux_set(struct typec_mux_dev *mux,
 				 struct typec_mux_state *state)
 {
+	struct anx7625_port_data *data = typec_mux_get_drvdata(mux);
+	struct anx7625_data *ctx = data->ctx;
+	struct device *dev = &ctx->client->dev;
+
+	bool old_dp_connected = (ctx->typec_ports[0].dp_connected ||
+				 ctx->typec_ports[1].dp_connected);
+	bool new_dp_connected;
+
+	if (ctx->num_typec_switches == 1)
+		return 0;
+
+	dev_dbg(dev, "mux_set dp_connected: c0=%d, c1=%d\n",
+		ctx->typec_ports[0].dp_connected, ctx->typec_ports[1].dp_connected);
+
+	data->dp_connected = (state->alt && state->alt->svid == USB_TYPEC_DP_SID &&
+			      state->alt->mode == USB_TYPEC_DP_MODE);
+
+	new_dp_connected = (ctx->typec_ports[0].dp_connected ||
+			    ctx->typec_ports[1].dp_connected);
+
+	/* dp on, power on first */
+	if (!old_dp_connected && new_dp_connected)
+		pm_runtime_get_sync(dev);
+
+	anx7625_typec_two_ports_update(ctx);
+
+	/* dp off, power off last */
+	if (old_dp_connected && !new_dp_connected)
+		pm_runtime_put_sync(dev);
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h b/drivers/gpu/drm/bridge/analogix/anx7625.h
index 76cfc64f7574..7d6c6fdf9a3a 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.h
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.h
@@ -55,6 +55,18 @@
 #define HPD_STATUS_CHANGE 0x80
 #define HPD_STATUS 0x80
 
+#define TCPC_SWITCH_0 0xB4
+#define SW_SEL1_DPTX0_RX2 BIT(0)
+#define SW_SEL1_DPTX0_RX1 BIT(1)
+#define SW_SEL1_SSRX_RX2 BIT(4)
+#define SW_SEL1_SSRX_RX1 BIT(5)
+
+#define TCPC_SWITCH_1 0xB5
+#define SW_SEL2_DPTX1_TX2 BIT(0)
+#define SW_SEL2_DPTX1_TX1 BIT(1)
+#define SW_SEL2_SSTX_TX2 BIT(4)
+#define SW_SEL2_SSTX_TX1 BIT(5)
+
 /******** END of I2C Address 0x58 ********/
 
 /***************************************************************/
@@ -444,6 +456,7 @@ struct anx7625_i2c_client {
 };
 
 struct anx7625_port_data {
+	bool dp_connected;
 	struct typec_mux_dev *typec_mux;
 	struct anx7625_data *ctx;
 };
-- 
2.36.1.476.g0c4daa206d-goog

[PATCH v2 7/7] drm/bridge: anx7625: Add typec_mux_set callback function

[Prashant Malani]

From: Pin-Yen Lin <treapking@chromium.org>

Add the callback function when the driver receives state
changes of the Type-C port. The callback function configures the
crosspoint switch of the anx7625 bridge chip, which can change the
output pins of the signals according to the port state.

Signed-off-by: Pin-Yen Lin <treapking@chromium.org>
Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

Changes since v2:
- No changes.

 drivers/gpu/drm/bridge/analogix/anx7625.c | 58 +++++++++++++++++++++++
 drivers/gpu/drm/bridge/analogix/anx7625.h | 13 +++++
 2 files changed, 71 insertions(+)

diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
index d41a21103bd3..2c308d12fab2 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.c
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
@@ -15,6 +15,7 @@
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <linux/types.h>
+#include <linux/usb/typec_dp.h>
 #include <linux/usb/typec_mux.h>
 #include <linux/workqueue.h>
 
@@ -2582,9 +2583,66 @@ static void anx7625_runtime_disable(void *data)
 	pm_runtime_disable(data);
 }
 
+static void anx7625_set_crosspoint_switch(struct anx7625_data *ctx,
+					  enum typec_orientation orientation)
+{
+	if (orientation == TYPEC_ORIENTATION_NORMAL) {
+		anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0,
+				  SW_SEL1_SSRX_RX1 | SW_SEL1_DPTX0_RX2);
+		anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1,
+				  SW_SEL2_SSTX_TX1 | SW_SEL2_DPTX1_TX2);
+	} else if (orientation == TYPEC_ORIENTATION_REVERSE) {
+		anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0,
+				  SW_SEL1_SSRX_RX2 | SW_SEL1_DPTX0_RX1);
+		anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1,
+				  SW_SEL2_SSTX_TX2 | SW_SEL2_DPTX1_TX1);
+	}
+}
+
+static void anx7625_typec_two_ports_update(struct anx7625_data *ctx)
+{
+	if (ctx->typec_ports[0].dp_connected && ctx->typec_ports[1].dp_connected)
+		/* Both ports available, do nothing to retain the current one. */
+		return;
+	else if (ctx->typec_ports[0].dp_connected)
+		anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_NORMAL);
+	else if (ctx->typec_ports[1].dp_connected)
+		anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_REVERSE);
+}
+
 static int anx7625_typec_mux_set(struct typec_mux_dev *mux,
 				 struct typec_mux_state *state)
 {
+	struct anx7625_port_data *data = typec_mux_get_drvdata(mux);
+	struct anx7625_data *ctx = data->ctx;
+	struct device *dev = &ctx->client->dev;
+
+	bool old_dp_connected = (ctx->typec_ports[0].dp_connected ||
+				 ctx->typec_ports[1].dp_connected);
+	bool new_dp_connected;
+
+	if (ctx->num_typec_switches == 1)
+		return 0;
+
+	dev_dbg(dev, "mux_set dp_connected: c0=%d, c1=%d\n",
+		ctx->typec_ports[0].dp_connected, ctx->typec_ports[1].dp_connected);
+
+	data->dp_connected = (state->alt && state->alt->svid == USB_TYPEC_DP_SID &&
+			      state->alt->mode == USB_TYPEC_DP_MODE);
+
+	new_dp_connected = (ctx->typec_ports[0].dp_connected ||
+			    ctx->typec_ports[1].dp_connected);
+
+	/* dp on, power on first */
+	if (!old_dp_connected && new_dp_connected)
+		pm_runtime_get_sync(dev);
+
+	anx7625_typec_two_ports_update(ctx);
+
+	/* dp off, power off last */
+	if (old_dp_connected && !new_dp_connected)
+		pm_runtime_put_sync(dev);
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h b/drivers/gpu/drm/bridge/analogix/anx7625.h
index 76cfc64f7574..7d6c6fdf9a3a 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.h
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.h
@@ -55,6 +55,18 @@
 #define HPD_STATUS_CHANGE 0x80
 #define HPD_STATUS 0x80
 
+#define TCPC_SWITCH_0 0xB4
+#define SW_SEL1_DPTX0_RX2 BIT(0)
+#define SW_SEL1_DPTX0_RX1 BIT(1)
+#define SW_SEL1_SSRX_RX2 BIT(4)
+#define SW_SEL1_SSRX_RX1 BIT(5)
+
+#define TCPC_SWITCH_1 0xB5
+#define SW_SEL2_DPTX1_TX2 BIT(0)
+#define SW_SEL2_DPTX1_TX1 BIT(1)
+#define SW_SEL2_SSTX_TX2 BIT(4)
+#define SW_SEL2_SSTX_TX1 BIT(5)
+
 /******** END of I2C Address 0x58 ********/
 
 /***************************************************************/
@@ -444,6 +456,7 @@ struct anx7625_i2c_client {
 };
 
 struct anx7625_port_data {
+	bool dp_connected;
 	struct typec_mux_dev *typec_mux;
 	struct anx7625_data *ctx;
 };
-- 
2.36.1.476.g0c4daa206d-goog

Re: [PATCH v2 3/7] dt-bindings: usb: Add Type-C switch binding

[Nícolas F. R. A. Prado]

Hi Prashant,

thanks for the patch. Please see sugestions below.

On Thu, Jun 09, 2022 at 06:09:42PM +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).
> 
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
> 
> 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@chromium.org>
> +
> +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.

Since you have a single port, you can omit the ports node.

> +
> +    required:
> +      - port@0
> +
> +required:
> +  - compatible
> +  - ports
> +
> +anyOf:
> +  - required:
> +      - mode-switch
> +  - required:
> +      - orientation-switch
> +
> +additionalProperties: true
> +
> +examples:
> +  - |
> +    drm-bridge {
> +        usb-switch {
> +            compatible = "typec-switch";
> +            mode-switch;
> +            orientation-switch;
> +            ports {
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +
> +                port@0 {
> +                    reg = <0>;
> +                    anx_ep: endpoint {
> +                        remote-endpoint = <&typec_controller>;
> +                    };
> +                };
> +            };

So instead of ports you would have just 

               port {
                   anx_ep: endpoint {
                       remote-endpoint = <&typec_controller>;
                   };
               };

And the same simplification on patch 4, for the anx7625 dt-binding.

Thanks,
Nícolas

> +        };
> +    };
> -- 
> 2.36.1.476.g0c4daa206d-goog
> 

Re: [PATCH v2 3/7] dt-bindings: usb: Add Type-C switch binding

[Nícolas F. R. A. Prado]

Hi Prashant,

thanks for the patch. Please see sugestions below.

On Thu, Jun 09, 2022 at 06:09:42PM +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).
> 
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
> 
> 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@chromium.org>
> +
> +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.

Since you have a single port, you can omit the ports node.

> +
> +    required:
> +      - port@0
> +
> +required:
> +  - compatible
> +  - ports
> +
> +anyOf:
> +  - required:
> +      - mode-switch
> +  - required:
> +      - orientation-switch
> +
> +additionalProperties: true
> +
> +examples:
> +  - |
> +    drm-bridge {
> +        usb-switch {
> +            compatible = "typec-switch";
> +            mode-switch;
> +            orientation-switch;
> +            ports {
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +
> +                port@0 {
> +                    reg = <0>;
> +                    anx_ep: endpoint {
> +                        remote-endpoint = <&typec_controller>;
> +                    };
> +                };
> +            };

So instead of ports you would have just 

               port {
                   anx_ep: endpoint {
                       remote-endpoint = <&typec_controller>;
                   };
               };

And the same simplification on patch 4, for the anx7625 dt-binding.

Thanks,
Nícolas

> +        };
> +    };
> -- 
> 2.36.1.476.g0c4daa206d-goog
> 

Re: [PATCH v2 3/7] dt-bindings: usb: Add Type-C switch binding

[Prashant Malani]

Hi Nícolas,

On Mon, Jun 13, 2022 at 1:38 PM Nícolas F. R. A. Prado
<nfraprado@collabora.com> wrote:
>
> Hi Prashant,
>
> thanks for the patch. Please see sugestions below.
>
> On Thu, Jun 09, 2022 at 06:09:42PM +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).
> >
> > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > ---
> >
> > 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@chromium.org>
> > +
> > +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.
>
> Since you have a single port, you can omit the ports node.

I've kept "ports", since it should be possible to add additional ports
which connect the switch to some other entity (and kept separate from
port@0), if the individual driver desires this.
port@0 should only contain the end-point from the 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";
> > +            mode-switch;
> > +            orientation-switch;
> > +            ports {
> > +                #address-cells = <1>;
> > +                #size-cells = <0>;
> > +
> > +                port@0 {
> > +                    reg = <0>;
> > +                    anx_ep: endpoint {
> > +                        remote-endpoint = <&typec_controller>;
> > +                    };
> > +                };
> > +            };
>
> So instead of ports you would have just
>
>                port {
>                    anx_ep: endpoint {
>                        remote-endpoint = <&typec_controller>;
>                    };
>                };
>
> And the same simplification on patch 4, for the anx7625 dt-binding.
>
> Thanks,
> Nícolas
>
> > +        };
> > +    };
> > --
> > 2.36.1.476.g0c4daa206d-goog
> >

Re: [PATCH v2 3/7] dt-bindings: usb: Add Type-C switch binding

[Prashant Malani]

Hi Nícolas,

On Mon, Jun 13, 2022 at 1:38 PM Nícolas F. R. A. Prado
<nfraprado@collabora.com> wrote:
>
> Hi Prashant,
>
> thanks for the patch. Please see sugestions below.
>
> On Thu, Jun 09, 2022 at 06:09:42PM +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).
> >
> > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > ---
> >
> > 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@chromium.org>
> > +
> > +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.
>
> Since you have a single port, you can omit the ports node.

I've kept "ports", since it should be possible to add additional ports
which connect the switch to some other entity (and kept separate from
port@0), if the individual driver desires this.
port@0 should only contain the end-point from the 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";
> > +            mode-switch;
> > +            orientation-switch;
> > +            ports {
> > +                #address-cells = <1>;
> > +                #size-cells = <0>;
> > +
> > +                port@0 {
> > +                    reg = <0>;
> > +                    anx_ep: endpoint {
> > +                        remote-endpoint = <&typec_controller>;
> > +                    };
> > +                };
> > +            };
>
> So instead of ports you would have just
>
>                port {
>                    anx_ep: endpoint {
>                        remote-endpoint = <&typec_controller>;
>                    };
>                };
>
> And the same simplification on patch 4, for the anx7625 dt-binding.
>
> Thanks,
> Nícolas
>
> > +        };
> > +    };
> > --
> > 2.36.1.476.g0c4daa206d-goog
> >

Re: [PATCH v2 5/7] drm/bridge: anx7625: Register number of Type C switches

[Nícolas F. R. A. Prado]

Hi Prashant,

On Thu, Jun 09, 2022 at 06:09:44PM +0000, Prashant Malani wrote:
> Parse the "switches" node, if available, and count and store the number
> of Type-C switches within it. Since we currently don't do anything with
> this info, no functional changes are expected from this change.
> 
> This patch sets a foundation for the actual registering of Type-C
> switches with the Type-C connector class framework.
> 
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
> 
> Changes since v1:
> - No changes.
> 
>  drivers/gpu/drm/bridge/analogix/anx7625.c | 20 ++++++++++++++++++++
>  drivers/gpu/drm/bridge/analogix/anx7625.h |  1 +
>  2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> index 53a5da6c49dd..07ed44c6b839 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> @@ -2581,6 +2581,22 @@ static void anx7625_runtime_disable(void *data)
>  	pm_runtime_disable(data);
>  }
>  
> +static int anx7625_register_typec_switches(struct device *device, struct anx7625_data *ctx)
> +{
> +	struct device_node *of = NULL;
> +	int ret = 0;
> +
> +	of = of_get_child_by_name(device->of_node, "switches");
> +	if (!of)
> +		return -ENODEV;
> +
> +	ctx->num_typec_switches = of_get_child_count(of);
> +	if (ctx->num_typec_switches <= 0)
> +		return -ENODEV;

Since the hardware only allows at most 2 switches (based on the dt-bindings),
you should have a define for that limit and check that it isn't exceeded here.

Thanks,
Nícolas

> +
> +	return ret;
> +}
> +
>  static int anx7625_i2c_probe(struct i2c_client *client,
>  			     const struct i2c_device_id *id)
>  {
> @@ -2686,6 +2702,10 @@ static int anx7625_i2c_probe(struct i2c_client *client,
>  	if (platform->pdata.intp_irq)
>  		queue_work(platform->workqueue, &platform->work);
>  
> +	ret = anx7625_register_typec_switches(dev, platform);
> +	if (ret)
> +		dev_info(dev, "Didn't register Type C switches, err: %d\n", ret);
> +
>  	platform->bridge.funcs = &anx7625_bridge_funcs;
>  	platform->bridge.of_node = client->dev.of_node;
>  	if (!anx7625_of_panel_on_aux_bus(&client->dev))
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h b/drivers/gpu/drm/bridge/analogix/anx7625.h
> index e257a84db962..d5cbca708842 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.h
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.h
> @@ -473,6 +473,7 @@ struct anx7625_data {
>  	struct drm_connector *connector;
>  	struct mipi_dsi_device *dsi;
>  	struct drm_dp_aux aux;
> +	int num_typec_switches;
>  };
>  
>  #endif  /* __ANX7625_H__ */
> -- 
> 2.36.1.476.g0c4daa206d-goog
> 

Re: [PATCH v2 5/7] drm/bridge: anx7625: Register number of Type C switches

[Nícolas F. R. A. Prado]

Hi Prashant,

On Thu, Jun 09, 2022 at 06:09:44PM +0000, Prashant Malani wrote:
> Parse the "switches" node, if available, and count and store the number
> of Type-C switches within it. Since we currently don't do anything with
> this info, no functional changes are expected from this change.
> 
> This patch sets a foundation for the actual registering of Type-C
> switches with the Type-C connector class framework.
> 
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
> 
> Changes since v1:
> - No changes.
> 
>  drivers/gpu/drm/bridge/analogix/anx7625.c | 20 ++++++++++++++++++++
>  drivers/gpu/drm/bridge/analogix/anx7625.h |  1 +
>  2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> index 53a5da6c49dd..07ed44c6b839 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> @@ -2581,6 +2581,22 @@ static void anx7625_runtime_disable(void *data)
>  	pm_runtime_disable(data);
>  }
>  
> +static int anx7625_register_typec_switches(struct device *device, struct anx7625_data *ctx)
> +{
> +	struct device_node *of = NULL;
> +	int ret = 0;
> +
> +	of = of_get_child_by_name(device->of_node, "switches");
> +	if (!of)
> +		return -ENODEV;
> +
> +	ctx->num_typec_switches = of_get_child_count(of);
> +	if (ctx->num_typec_switches <= 0)
> +		return -ENODEV;

Since the hardware only allows at most 2 switches (based on the dt-bindings),
you should have a define for that limit and check that it isn't exceeded here.

Thanks,
Nícolas

> +
> +	return ret;
> +}
> +
>  static int anx7625_i2c_probe(struct i2c_client *client,
>  			     const struct i2c_device_id *id)
>  {
> @@ -2686,6 +2702,10 @@ static int anx7625_i2c_probe(struct i2c_client *client,
>  	if (platform->pdata.intp_irq)
>  		queue_work(platform->workqueue, &platform->work);
>  
> +	ret = anx7625_register_typec_switches(dev, platform);
> +	if (ret)
> +		dev_info(dev, "Didn't register Type C switches, err: %d\n", ret);
> +
>  	platform->bridge.funcs = &anx7625_bridge_funcs;
>  	platform->bridge.of_node = client->dev.of_node;
>  	if (!anx7625_of_panel_on_aux_bus(&client->dev))
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h b/drivers/gpu/drm/bridge/analogix/anx7625.h
> index e257a84db962..d5cbca708842 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.h
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.h
> @@ -473,6 +473,7 @@ struct anx7625_data {
>  	struct drm_connector *connector;
>  	struct mipi_dsi_device *dsi;
>  	struct drm_dp_aux aux;
> +	int num_typec_switches;
>  };
>  
>  #endif  /* __ANX7625_H__ */
> -- 
> 2.36.1.476.g0c4daa206d-goog
> 

Re: [PATCH v2 5/7] drm/bridge: anx7625: Register number of Type C switches

[Prashant Malani]

Hi Nícolas,

On Mon, Jun 13, 2022 at 1:45 PM Nícolas F. R. A. Prado
<nfraprado@collabora.com> wrote:
>
> Hi Prashant,
>
> On Thu, Jun 09, 2022 at 06:09:44PM +0000, Prashant Malani wrote:
> > Parse the "switches" node, if available, and count and store the number
> > of Type-C switches within it. Since we currently don't do anything with
> > this info, no functional changes are expected from this change.
> >
> > This patch sets a foundation for the actual registering of Type-C
> > switches with the Type-C connector class framework.
> >
> > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > ---
> >
> > Changes since v1:
> > - No changes.
> >
> >  drivers/gpu/drm/bridge/analogix/anx7625.c | 20 ++++++++++++++++++++
> >  drivers/gpu/drm/bridge/analogix/anx7625.h |  1 +
> >  2 files changed, 21 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > index 53a5da6c49dd..07ed44c6b839 100644
> > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > @@ -2581,6 +2581,22 @@ static void anx7625_runtime_disable(void *data)
> >       pm_runtime_disable(data);
> >  }
> >
> > +static int anx7625_register_typec_switches(struct device *device, struct anx7625_data *ctx)
> > +{
> > +     struct device_node *of = NULL;
> > +     int ret = 0;
> > +
> > +     of = of_get_child_by_name(device->of_node, "switches");
> > +     if (!of)
> > +             return -ENODEV;
> > +
> > +     ctx->num_typec_switches = of_get_child_count(of);
> > +     if (ctx->num_typec_switches <= 0)
> > +             return -ENODEV;
>
> Since the hardware only allows at most 2 switches (based on the dt-bindings),
> you should have a define for that limit and check that it isn't exceeded here.

This is already enforced by the DT binding (see the
"patternProperties" in patch 4/7, suggested by Krzysztof in the
previous version)

>
> Thanks,
> Nícolas
>
> > +
> > +     return ret;
> > +}
> > +
> >  static int anx7625_i2c_probe(struct i2c_client *client,
> >                            const struct i2c_device_id *id)
> >  {
> > @@ -2686,6 +2702,10 @@ static int anx7625_i2c_probe(struct i2c_client *client,
> >       if (platform->pdata.intp_irq)
> >               queue_work(platform->workqueue, &platform->work);
> >
> > +     ret = anx7625_register_typec_switches(dev, platform);
> > +     if (ret)
> > +             dev_info(dev, "Didn't register Type C switches, err: %d\n", ret);
> > +
> >       platform->bridge.funcs = &anx7625_bridge_funcs;
> >       platform->bridge.of_node = client->dev.of_node;
> >       if (!anx7625_of_panel_on_aux_bus(&client->dev))
> > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h b/drivers/gpu/drm/bridge/analogix/anx7625.h
> > index e257a84db962..d5cbca708842 100644
> > --- a/drivers/gpu/drm/bridge/analogix/anx7625.h
> > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.h
> > @@ -473,6 +473,7 @@ struct anx7625_data {
> >       struct drm_connector *connector;
> >       struct mipi_dsi_device *dsi;
> >       struct drm_dp_aux aux;
> > +     int num_typec_switches;
> >  };
> >
> >  #endif  /* __ANX7625_H__ */
> > --
> > 2.36.1.476.g0c4daa206d-goog
> >

Re: [PATCH v2 5/7] drm/bridge: anx7625: Register number of Type C switches

[Prashant Malani]

Hi Nícolas,

On Mon, Jun 13, 2022 at 1:45 PM Nícolas F. R. A. Prado
<nfraprado@collabora.com> wrote:
>
> Hi Prashant,
>
> On Thu, Jun 09, 2022 at 06:09:44PM +0000, Prashant Malani wrote:
> > Parse the "switches" node, if available, and count and store the number
> > of Type-C switches within it. Since we currently don't do anything with
> > this info, no functional changes are expected from this change.
> >
> > This patch sets a foundation for the actual registering of Type-C
> > switches with the Type-C connector class framework.
> >
> > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > ---
> >
> > Changes since v1:
> > - No changes.
> >
> >  drivers/gpu/drm/bridge/analogix/anx7625.c | 20 ++++++++++++++++++++
> >  drivers/gpu/drm/bridge/analogix/anx7625.h |  1 +
> >  2 files changed, 21 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > index 53a5da6c49dd..07ed44c6b839 100644
> > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > @@ -2581,6 +2581,22 @@ static void anx7625_runtime_disable(void *data)
> >       pm_runtime_disable(data);
> >  }
> >
> > +static int anx7625_register_typec_switches(struct device *device, struct anx7625_data *ctx)
> > +{
> > +     struct device_node *of = NULL;
> > +     int ret = 0;
> > +
> > +     of = of_get_child_by_name(device->of_node, "switches");
> > +     if (!of)
> > +             return -ENODEV;
> > +
> > +     ctx->num_typec_switches = of_get_child_count(of);
> > +     if (ctx->num_typec_switches <= 0)
> > +             return -ENODEV;
>
> Since the hardware only allows at most 2 switches (based on the dt-bindings),
> you should have a define for that limit and check that it isn't exceeded here.

This is already enforced by the DT binding (see the
"patternProperties" in patch 4/7, suggested by Krzysztof in the
previous version)

>
> Thanks,
> Nícolas
>
> > +
> > +     return ret;
> > +}
> > +
> >  static int anx7625_i2c_probe(struct i2c_client *client,
> >                            const struct i2c_device_id *id)
> >  {
> > @@ -2686,6 +2702,10 @@ static int anx7625_i2c_probe(struct i2c_client *client,
> >       if (platform->pdata.intp_irq)
> >               queue_work(platform->workqueue, &platform->work);
> >
> > +     ret = anx7625_register_typec_switches(dev, platform);
> > +     if (ret)
> > +             dev_info(dev, "Didn't register Type C switches, err: %d\n", ret);
> > +
> >       platform->bridge.funcs = &anx7625_bridge_funcs;
> >       platform->bridge.of_node = client->dev.of_node;
> >       if (!anx7625_of_panel_on_aux_bus(&client->dev))
> > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h b/drivers/gpu/drm/bridge/analogix/anx7625.h
> > index e257a84db962..d5cbca708842 100644
> > --- a/drivers/gpu/drm/bridge/analogix/anx7625.h
> > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.h
> > @@ -473,6 +473,7 @@ struct anx7625_data {
> >       struct drm_connector *connector;
> >       struct mipi_dsi_device *dsi;
> >       struct drm_dp_aux aux;
> > +     int num_typec_switches;
> >  };
> >
> >  #endif  /* __ANX7625_H__ */
> > --
> > 2.36.1.476.g0c4daa206d-goog
> >

Re: [PATCH v2 7/7] drm/bridge: anx7625: Add typec_mux_set callback function

[Nícolas F. R. A. Prado]

Hi Prashant,

On Thu, Jun 09, 2022 at 06:09:46PM +0000, Prashant Malani wrote:
> From: Pin-Yen Lin <treapking@chromium.org>
> 
> Add the callback function when the driver receives state
> changes of the Type-C port. The callback function configures the
> crosspoint switch of the anx7625 bridge chip, which can change the
> output pins of the signals according to the port state.
> 
> Signed-off-by: Pin-Yen Lin <treapking@chromium.org>
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
> 
> Changes since v2:
> - No changes.
> 
>  drivers/gpu/drm/bridge/analogix/anx7625.c | 58 +++++++++++++++++++++++
>  drivers/gpu/drm/bridge/analogix/anx7625.h | 13 +++++
>  2 files changed, 71 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> index d41a21103bd3..2c308d12fab2 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> @@ -15,6 +15,7 @@
>  #include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
>  #include <linux/types.h>
> +#include <linux/usb/typec_dp.h>
>  #include <linux/usb/typec_mux.h>
>  #include <linux/workqueue.h>
>  
> @@ -2582,9 +2583,66 @@ static void anx7625_runtime_disable(void *data)
>  	pm_runtime_disable(data);
>  }
>  
> +static void anx7625_set_crosspoint_switch(struct anx7625_data *ctx,
> +					  enum typec_orientation orientation)
> +{
> +	if (orientation == TYPEC_ORIENTATION_NORMAL) {
> +		anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0,
> +				  SW_SEL1_SSRX_RX1 | SW_SEL1_DPTX0_RX2);
> +		anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1,
> +				  SW_SEL2_SSTX_TX1 | SW_SEL2_DPTX1_TX2);
> +	} else if (orientation == TYPEC_ORIENTATION_REVERSE) {
> +		anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0,
> +				  SW_SEL1_SSRX_RX2 | SW_SEL1_DPTX0_RX1);
> +		anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1,
> +				  SW_SEL2_SSTX_TX2 | SW_SEL2_DPTX1_TX1);
> +	}
> +}
> +
> +static void anx7625_typec_two_ports_update(struct anx7625_data *ctx)
> +{
> +	if (ctx->typec_ports[0].dp_connected && ctx->typec_ports[1].dp_connected)
> +		/* Both ports available, do nothing to retain the current one. */
> +		return;
> +	else if (ctx->typec_ports[0].dp_connected)
> +		anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_NORMAL);
> +	else if (ctx->typec_ports[1].dp_connected)
> +		anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_REVERSE);
> +}
> +
>  static int anx7625_typec_mux_set(struct typec_mux_dev *mux,
>  				 struct typec_mux_state *state)
>  {
> +	struct anx7625_port_data *data = typec_mux_get_drvdata(mux);
> +	struct anx7625_data *ctx = data->ctx;
> +	struct device *dev = &ctx->client->dev;
> +
> +	bool old_dp_connected = (ctx->typec_ports[0].dp_connected ||
> +				 ctx->typec_ports[1].dp_connected);

Here you're assuming you have 2 switches. Given that this on its own doesn't do
anything, just move it after the 

	if (ctx->num_typec_switches == 1)
		return 0;

check.

Thanks,
Nícolas

> +	bool new_dp_connected;
> +
> +	if (ctx->num_typec_switches == 1)
> +		return 0;
> +
> +	dev_dbg(dev, "mux_set dp_connected: c0=%d, c1=%d\n",
> +		ctx->typec_ports[0].dp_connected, ctx->typec_ports[1].dp_connected);
> +
> +	data->dp_connected = (state->alt && state->alt->svid == USB_TYPEC_DP_SID &&
> +			      state->alt->mode == USB_TYPEC_DP_MODE);
> +
> +	new_dp_connected = (ctx->typec_ports[0].dp_connected ||
> +			    ctx->typec_ports[1].dp_connected);
> +
> +	/* dp on, power on first */
> +	if (!old_dp_connected && new_dp_connected)
> +		pm_runtime_get_sync(dev);
> +
> +	anx7625_typec_two_ports_update(ctx);
> +
> +	/* dp off, power off last */
> +	if (old_dp_connected && !new_dp_connected)
> +		pm_runtime_put_sync(dev);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h b/drivers/gpu/drm/bridge/analogix/anx7625.h
> index 76cfc64f7574..7d6c6fdf9a3a 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.h
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.h
> @@ -55,6 +55,18 @@
>  #define HPD_STATUS_CHANGE 0x80
>  #define HPD_STATUS 0x80
>  
> +#define TCPC_SWITCH_0 0xB4
> +#define SW_SEL1_DPTX0_RX2 BIT(0)
> +#define SW_SEL1_DPTX0_RX1 BIT(1)
> +#define SW_SEL1_SSRX_RX2 BIT(4)
> +#define SW_SEL1_SSRX_RX1 BIT(5)
> +
> +#define TCPC_SWITCH_1 0xB5
> +#define SW_SEL2_DPTX1_TX2 BIT(0)
> +#define SW_SEL2_DPTX1_TX1 BIT(1)
> +#define SW_SEL2_SSTX_TX2 BIT(4)
> +#define SW_SEL2_SSTX_TX1 BIT(5)
> +
>  /******** END of I2C Address 0x58 ********/
>  
>  /***************************************************************/
> @@ -444,6 +456,7 @@ struct anx7625_i2c_client {
>  };
>  
>  struct anx7625_port_data {
> +	bool dp_connected;
>  	struct typec_mux_dev *typec_mux;
>  	struct anx7625_data *ctx;
>  };
> -- 
> 2.36.1.476.g0c4daa206d-goog
> 

Re: [PATCH v2 7/7] drm/bridge: anx7625: Add typec_mux_set callback function

[Nícolas F. R. A. Prado]

Hi Prashant,

On Thu, Jun 09, 2022 at 06:09:46PM +0000, Prashant Malani wrote:
> From: Pin-Yen Lin <treapking@chromium.org>
> 
> Add the callback function when the driver receives state
> changes of the Type-C port. The callback function configures the
> crosspoint switch of the anx7625 bridge chip, which can change the
> output pins of the signals according to the port state.
> 
> Signed-off-by: Pin-Yen Lin <treapking@chromium.org>
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
> 
> Changes since v2:
> - No changes.
> 
>  drivers/gpu/drm/bridge/analogix/anx7625.c | 58 +++++++++++++++++++++++
>  drivers/gpu/drm/bridge/analogix/anx7625.h | 13 +++++
>  2 files changed, 71 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> index d41a21103bd3..2c308d12fab2 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> @@ -15,6 +15,7 @@
>  #include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
>  #include <linux/types.h>
> +#include <linux/usb/typec_dp.h>
>  #include <linux/usb/typec_mux.h>
>  #include <linux/workqueue.h>
>  
> @@ -2582,9 +2583,66 @@ static void anx7625_runtime_disable(void *data)
>  	pm_runtime_disable(data);
>  }
>  
> +static void anx7625_set_crosspoint_switch(struct anx7625_data *ctx,
> +					  enum typec_orientation orientation)
> +{
> +	if (orientation == TYPEC_ORIENTATION_NORMAL) {
> +		anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0,
> +				  SW_SEL1_SSRX_RX1 | SW_SEL1_DPTX0_RX2);
> +		anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1,
> +				  SW_SEL2_SSTX_TX1 | SW_SEL2_DPTX1_TX2);
> +	} else if (orientation == TYPEC_ORIENTATION_REVERSE) {
> +		anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0,
> +				  SW_SEL1_SSRX_RX2 | SW_SEL1_DPTX0_RX1);
> +		anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1,
> +				  SW_SEL2_SSTX_TX2 | SW_SEL2_DPTX1_TX1);
> +	}
> +}
> +
> +static void anx7625_typec_two_ports_update(struct anx7625_data *ctx)
> +{
> +	if (ctx->typec_ports[0].dp_connected && ctx->typec_ports[1].dp_connected)
> +		/* Both ports available, do nothing to retain the current one. */
> +		return;
> +	else if (ctx->typec_ports[0].dp_connected)
> +		anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_NORMAL);
> +	else if (ctx->typec_ports[1].dp_connected)
> +		anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_REVERSE);
> +}
> +
>  static int anx7625_typec_mux_set(struct typec_mux_dev *mux,
>  				 struct typec_mux_state *state)
>  {
> +	struct anx7625_port_data *data = typec_mux_get_drvdata(mux);
> +	struct anx7625_data *ctx = data->ctx;
> +	struct device *dev = &ctx->client->dev;
> +
> +	bool old_dp_connected = (ctx->typec_ports[0].dp_connected ||
> +				 ctx->typec_ports[1].dp_connected);

Here you're assuming you have 2 switches. Given that this on its own doesn't do
anything, just move it after the 

	if (ctx->num_typec_switches == 1)
		return 0;

check.

Thanks,
Nícolas

> +	bool new_dp_connected;
> +
> +	if (ctx->num_typec_switches == 1)
> +		return 0;
> +
> +	dev_dbg(dev, "mux_set dp_connected: c0=%d, c1=%d\n",
> +		ctx->typec_ports[0].dp_connected, ctx->typec_ports[1].dp_connected);
> +
> +	data->dp_connected = (state->alt && state->alt->svid == USB_TYPEC_DP_SID &&
> +			      state->alt->mode == USB_TYPEC_DP_MODE);
> +
> +	new_dp_connected = (ctx->typec_ports[0].dp_connected ||
> +			    ctx->typec_ports[1].dp_connected);
> +
> +	/* dp on, power on first */
> +	if (!old_dp_connected && new_dp_connected)
> +		pm_runtime_get_sync(dev);
> +
> +	anx7625_typec_two_ports_update(ctx);
> +
> +	/* dp off, power off last */
> +	if (old_dp_connected && !new_dp_connected)
> +		pm_runtime_put_sync(dev);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h b/drivers/gpu/drm/bridge/analogix/anx7625.h
> index 76cfc64f7574..7d6c6fdf9a3a 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.h
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.h
> @@ -55,6 +55,18 @@
>  #define HPD_STATUS_CHANGE 0x80
>  #define HPD_STATUS 0x80
>  
> +#define TCPC_SWITCH_0 0xB4
> +#define SW_SEL1_DPTX0_RX2 BIT(0)
> +#define SW_SEL1_DPTX0_RX1 BIT(1)
> +#define SW_SEL1_SSRX_RX2 BIT(4)
> +#define SW_SEL1_SSRX_RX1 BIT(5)
> +
> +#define TCPC_SWITCH_1 0xB5
> +#define SW_SEL2_DPTX1_TX2 BIT(0)
> +#define SW_SEL2_DPTX1_TX1 BIT(1)
> +#define SW_SEL2_SSTX_TX2 BIT(4)
> +#define SW_SEL2_SSTX_TX1 BIT(5)
> +
>  /******** END of I2C Address 0x58 ********/
>  
>  /***************************************************************/
> @@ -444,6 +456,7 @@ struct anx7625_i2c_client {
>  };
>  
>  struct anx7625_port_data {
> +	bool dp_connected;
>  	struct typec_mux_dev *typec_mux;
>  	struct anx7625_data *ctx;
>  };
> -- 
> 2.36.1.476.g0c4daa206d-goog
> 

Re: [PATCH v2 0/7] usb: typec: Introduce typec-switch binding

[Nícolas F. R. A. Prado]

On Thu, Jun 09, 2022 at 06:09:39PM +0000, Prashant Malani wrote:
> This series introduces a binding for Type-C data lane switches. These
> control the routing and operating modes of USB Type-C data lanes based
> on the PD messaging from the Type-C port driver regarding connected
> peripherals.
> 
> The first patch introduces a change to the Type-C mux class mode-switch
> matching code, while the second adds a config guard to a Type-C header.
> The next couple of patches introduce the new "typec-switch" binding as
> well as one user of it (the ANX7625 drm bridge).
> 
> The remaining patches add functionality to the anx7625 driver to
> register the mode-switches, as well as program its crosspoint
> switch depending on which Type-C port has a DisplayPort (DP) peripheral
> connected to it.
> 
> v1: https://lore.kernel.org/linux-usb/20220607190131.1647511-1-pmalani@chromium.org/
> 
> Changes since v1:
> - Fixed function signature error in "else case" of typec_mux.h
> - typec-switch.yaml: Fixed indentation, compatible, and node names in examples.
> - anx7625.yaml:
>     + Introduced patternProperties for "switch" children (suggested by Krzysztof Kozlowski).
>     + Added unevaluatedProperties descriptor (suggested by Krzysztof Kozlowski).
>     + Added "address-cells" and "size-cells" properties to "switches".
> 
> Pin-Yen Lin (1):
>   drm/bridge: anx7625: Add typec_mux_set callback function
> 
> Prashant Malani (6):
>   usb: typec: mux: Allow muxes to specify mode-switch
>   usb: typec: mux: Add CONFIG guards for functions
>   dt-bindings: usb: Add Type-C switch binding
>   dt-bindings: drm/bridge: anx7625: Add mode-switch support
>   drm/bridge: anx7625: Register number of Type C switches
>   drm/bridge: anx7625: Register Type-C mode switches

For the whole series,

Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

Tested that external display works on both ports of mt8192-asurada-spherion.

Thanks,
Nícolas

> 
>  .../display/bridge/analogix,anx7625.yaml      |  63 ++++++++
>  .../devicetree/bindings/usb/typec-switch.yaml |  74 +++++++++
>  drivers/gpu/drm/bridge/analogix/anx7625.c     | 151 ++++++++++++++++++
>  drivers/gpu/drm/bridge/analogix/anx7625.h     |  20 +++
>  drivers/usb/typec/mux.c                       |   8 +-
>  include/linux/usb/typec_mux.h                 |  38 +++++
>  6 files changed, 352 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/usb/typec-switch.yaml
> 
> -- 
> 2.36.1.476.g0c4daa206d-goog
> 

Re: [PATCH v2 0/7] usb: typec: Introduce typec-switch binding

[Nícolas F. R. A. Prado]

On Thu, Jun 09, 2022 at 06:09:39PM +0000, Prashant Malani wrote:
> This series introduces a binding for Type-C data lane switches. These
> control the routing and operating modes of USB Type-C data lanes based
> on the PD messaging from the Type-C port driver regarding connected
> peripherals.
> 
> The first patch introduces a change to the Type-C mux class mode-switch
> matching code, while the second adds a config guard to a Type-C header.
> The next couple of patches introduce the new "typec-switch" binding as
> well as one user of it (the ANX7625 drm bridge).
> 
> The remaining patches add functionality to the anx7625 driver to
> register the mode-switches, as well as program its crosspoint
> switch depending on which Type-C port has a DisplayPort (DP) peripheral
> connected to it.
> 
> v1: https://lore.kernel.org/linux-usb/20220607190131.1647511-1-pmalani@chromium.org/
> 
> Changes since v1:
> - Fixed function signature error in "else case" of typec_mux.h
> - typec-switch.yaml: Fixed indentation, compatible, and node names in examples.
> - anx7625.yaml:
>     + Introduced patternProperties for "switch" children (suggested by Krzysztof Kozlowski).
>     + Added unevaluatedProperties descriptor (suggested by Krzysztof Kozlowski).
>     + Added "address-cells" and "size-cells" properties to "switches".
> 
> Pin-Yen Lin (1):
>   drm/bridge: anx7625: Add typec_mux_set callback function
> 
> Prashant Malani (6):
>   usb: typec: mux: Allow muxes to specify mode-switch
>   usb: typec: mux: Add CONFIG guards for functions
>   dt-bindings: usb: Add Type-C switch binding
>   dt-bindings: drm/bridge: anx7625: Add mode-switch support
>   drm/bridge: anx7625: Register number of Type C switches
>   drm/bridge: anx7625: Register Type-C mode switches

For the whole series,

Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

Tested that external display works on both ports of mt8192-asurada-spherion.

Thanks,
Nícolas

> 
>  .../display/bridge/analogix,anx7625.yaml      |  63 ++++++++
>  .../devicetree/bindings/usb/typec-switch.yaml |  74 +++++++++
>  drivers/gpu/drm/bridge/analogix/anx7625.c     | 151 ++++++++++++++++++
>  drivers/gpu/drm/bridge/analogix/anx7625.h     |  20 +++
>  drivers/usb/typec/mux.c                       |   8 +-
>  include/linux/usb/typec_mux.h                 |  38 +++++
>  6 files changed, 352 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/usb/typec-switch.yaml
> 
> -- 
> 2.36.1.476.g0c4daa206d-goog
> 

Re: [PATCH v2 7/7] drm/bridge: anx7625: Add typec_mux_set callback function

[Prashant Malani]

Hi Nícolas,

On Mon, Jun 13, 2022 at 1:51 PM Nícolas F. R. A. Prado
<nfraprado@collabora.com> wrote:
>
> Hi Prashant,
>
> On Thu, Jun 09, 2022 at 06:09:46PM +0000, Prashant Malani wrote:
> > From: Pin-Yen Lin <treapking@chromium.org>
> >
> > Add the callback function when the driver receives state
> > changes of the Type-C port. The callback function configures the
> > crosspoint switch of the anx7625 bridge chip, which can change the
> > output pins of the signals according to the port state.
> >
> > Signed-off-by: Pin-Yen Lin <treapking@chromium.org>
> > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > ---
> >
> > Changes since v2:
> > - No changes.
> >
> >  drivers/gpu/drm/bridge/analogix/anx7625.c | 58 +++++++++++++++++++++++
> >  drivers/gpu/drm/bridge/analogix/anx7625.h | 13 +++++
> >  2 files changed, 71 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > index d41a21103bd3..2c308d12fab2 100644
> > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > @@ -15,6 +15,7 @@
> >  #include <linux/regulator/consumer.h>
> >  #include <linux/slab.h>
> >  #include <linux/types.h>
> > +#include <linux/usb/typec_dp.h>
> >  #include <linux/usb/typec_mux.h>
> >  #include <linux/workqueue.h>
> >
> > @@ -2582,9 +2583,66 @@ static void anx7625_runtime_disable(void *data)
> >       pm_runtime_disable(data);
> >  }
> >
> > +static void anx7625_set_crosspoint_switch(struct anx7625_data *ctx,
> > +                                       enum typec_orientation orientation)
> > +{
> > +     if (orientation == TYPEC_ORIENTATION_NORMAL) {
> > +             anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0,
> > +                               SW_SEL1_SSRX_RX1 | SW_SEL1_DPTX0_RX2);
> > +             anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1,
> > +                               SW_SEL2_SSTX_TX1 | SW_SEL2_DPTX1_TX2);
> > +     } else if (orientation == TYPEC_ORIENTATION_REVERSE) {
> > +             anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0,
> > +                               SW_SEL1_SSRX_RX2 | SW_SEL1_DPTX0_RX1);
> > +             anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1,
> > +                               SW_SEL2_SSTX_TX2 | SW_SEL2_DPTX1_TX1);
> > +     }
> > +}
> > +
> > +static void anx7625_typec_two_ports_update(struct anx7625_data *ctx)
> > +{
> > +     if (ctx->typec_ports[0].dp_connected && ctx->typec_ports[1].dp_connected)
> > +             /* Both ports available, do nothing to retain the current one. */
> > +             return;
> > +     else if (ctx->typec_ports[0].dp_connected)
> > +             anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_NORMAL);
> > +     else if (ctx->typec_ports[1].dp_connected)
> > +             anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_REVERSE);
> > +}
> > +
> >  static int anx7625_typec_mux_set(struct typec_mux_dev *mux,
> >                                struct typec_mux_state *state)
> >  {
> > +     struct anx7625_port_data *data = typec_mux_get_drvdata(mux);
> > +     struct anx7625_data *ctx = data->ctx;
> > +     struct device *dev = &ctx->client->dev;
> > +
> > +     bool old_dp_connected = (ctx->typec_ports[0].dp_connected ||
> > +                              ctx->typec_ports[1].dp_connected);
>
> Here you're assuming you have 2 switches. Given that this on its own doesn't do
> anything, just move it after the
>
>         if (ctx->num_typec_switches == 1)
>                 return 0;
>
> check.

Sure. I will fix this up in v3. Thanks!

>
> Thanks,
> Nícolas
>
> > +     bool new_dp_connected;
> > +
> > +     if (ctx->num_typec_switches == 1)
> > +             return 0;
> > +
> > +     dev_dbg(dev, "mux_set dp_connected: c0=%d, c1=%d\n",
> > +             ctx->typec_ports[0].dp_connected, ctx->typec_ports[1].dp_connected);
> > +
> > +     data->dp_connected = (state->alt && state->alt->svid == USB_TYPEC_DP_SID &&
> > +                           state->alt->mode == USB_TYPEC_DP_MODE);
> > +
> > +     new_dp_connected = (ctx->typec_ports[0].dp_connected ||
> > +                         ctx->typec_ports[1].dp_connected);
> > +
> > +     /* dp on, power on first */
> > +     if (!old_dp_connected && new_dp_connected)
> > +             pm_runtime_get_sync(dev);
> > +
> > +     anx7625_typec_two_ports_update(ctx);
> > +
> > +     /* dp off, power off last */
> > +     if (old_dp_connected && !new_dp_connected)
> > +             pm_runtime_put_sync(dev);
> > +
> >       return 0;
> >  }
> >
> > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h b/drivers/gpu/drm/bridge/analogix/anx7625.h
> > index 76cfc64f7574..7d6c6fdf9a3a 100644
> > --- a/drivers/gpu/drm/bridge/analogix/anx7625.h
> > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.h
> > @@ -55,6 +55,18 @@
> >  #define HPD_STATUS_CHANGE 0x80
> >  #define HPD_STATUS 0x80
> >
> > +#define TCPC_SWITCH_0 0xB4
> > +#define SW_SEL1_DPTX0_RX2 BIT(0)
> > +#define SW_SEL1_DPTX0_RX1 BIT(1)
> > +#define SW_SEL1_SSRX_RX2 BIT(4)
> > +#define SW_SEL1_SSRX_RX1 BIT(5)
> > +
> > +#define TCPC_SWITCH_1 0xB5
> > +#define SW_SEL2_DPTX1_TX2 BIT(0)
> > +#define SW_SEL2_DPTX1_TX1 BIT(1)
> > +#define SW_SEL2_SSTX_TX2 BIT(4)
> > +#define SW_SEL2_SSTX_TX1 BIT(5)
> > +
> >  /******** END of I2C Address 0x58 ********/
> >
> >  /***************************************************************/
> > @@ -444,6 +456,7 @@ struct anx7625_i2c_client {
> >  };
> >
> >  struct anx7625_port_data {
> > +     bool dp_connected;
> >       struct typec_mux_dev *typec_mux;
> >       struct anx7625_data *ctx;
> >  };
> > --
> > 2.36.1.476.g0c4daa206d-goog
> >

Re: [PATCH v2 7/7] drm/bridge: anx7625: Add typec_mux_set callback function

[Prashant Malani]

Hi Nícolas,

On Mon, Jun 13, 2022 at 1:51 PM Nícolas F. R. A. Prado
<nfraprado@collabora.com> wrote:
>
> Hi Prashant,
>
> On Thu, Jun 09, 2022 at 06:09:46PM +0000, Prashant Malani wrote:
> > From: Pin-Yen Lin <treapking@chromium.org>
> >
> > Add the callback function when the driver receives state
> > changes of the Type-C port. The callback function configures the
> > crosspoint switch of the anx7625 bridge chip, which can change the
> > output pins of the signals according to the port state.
> >
> > Signed-off-by: Pin-Yen Lin <treapking@chromium.org>
> > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > ---
> >
> > Changes since v2:
> > - No changes.
> >
> >  drivers/gpu/drm/bridge/analogix/anx7625.c | 58 +++++++++++++++++++++++
> >  drivers/gpu/drm/bridge/analogix/anx7625.h | 13 +++++
> >  2 files changed, 71 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > index d41a21103bd3..2c308d12fab2 100644
> > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > @@ -15,6 +15,7 @@
> >  #include <linux/regulator/consumer.h>
> >  #include <linux/slab.h>
> >  #include <linux/types.h>
> > +#include <linux/usb/typec_dp.h>
> >  #include <linux/usb/typec_mux.h>
> >  #include <linux/workqueue.h>
> >
> > @@ -2582,9 +2583,66 @@ static void anx7625_runtime_disable(void *data)
> >       pm_runtime_disable(data);
> >  }
> >
> > +static void anx7625_set_crosspoint_switch(struct anx7625_data *ctx,
> > +                                       enum typec_orientation orientation)
> > +{
> > +     if (orientation == TYPEC_ORIENTATION_NORMAL) {
> > +             anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0,
> > +                               SW_SEL1_SSRX_RX1 | SW_SEL1_DPTX0_RX2);
> > +             anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1,
> > +                               SW_SEL2_SSTX_TX1 | SW_SEL2_DPTX1_TX2);
> > +     } else if (orientation == TYPEC_ORIENTATION_REVERSE) {
> > +             anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0,
> > +                               SW_SEL1_SSRX_RX2 | SW_SEL1_DPTX0_RX1);
> > +             anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1,
> > +                               SW_SEL2_SSTX_TX2 | SW_SEL2_DPTX1_TX1);
> > +     }
> > +}
> > +
> > +static void anx7625_typec_two_ports_update(struct anx7625_data *ctx)
> > +{
> > +     if (ctx->typec_ports[0].dp_connected && ctx->typec_ports[1].dp_connected)
> > +             /* Both ports available, do nothing to retain the current one. */
> > +             return;
> > +     else if (ctx->typec_ports[0].dp_connected)
> > +             anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_NORMAL);
> > +     else if (ctx->typec_ports[1].dp_connected)
> > +             anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_REVERSE);
> > +}
> > +
> >  static int anx7625_typec_mux_set(struct typec_mux_dev *mux,
> >                                struct typec_mux_state *state)
> >  {
> > +     struct anx7625_port_data *data = typec_mux_get_drvdata(mux);
> > +     struct anx7625_data *ctx = data->ctx;
> > +     struct device *dev = &ctx->client->dev;
> > +
> > +     bool old_dp_connected = (ctx->typec_ports[0].dp_connected ||
> > +                              ctx->typec_ports[1].dp_connected);
>
> Here you're assuming you have 2 switches. Given that this on its own doesn't do
> anything, just move it after the
>
>         if (ctx->num_typec_switches == 1)
>                 return 0;
>
> check.

Sure. I will fix this up in v3. Thanks!

>
> Thanks,
> Nícolas
>
> > +     bool new_dp_connected;
> > +
> > +     if (ctx->num_typec_switches == 1)
> > +             return 0;
> > +
> > +     dev_dbg(dev, "mux_set dp_connected: c0=%d, c1=%d\n",
> > +             ctx->typec_ports[0].dp_connected, ctx->typec_ports[1].dp_connected);
> > +
> > +     data->dp_connected = (state->alt && state->alt->svid == USB_TYPEC_DP_SID &&
> > +                           state->alt->mode == USB_TYPEC_DP_MODE);
> > +
> > +     new_dp_connected = (ctx->typec_ports[0].dp_connected ||
> > +                         ctx->typec_ports[1].dp_connected);
> > +
> > +     /* dp on, power on first */
> > +     if (!old_dp_connected && new_dp_connected)
> > +             pm_runtime_get_sync(dev);
> > +
> > +     anx7625_typec_two_ports_update(ctx);
> > +
> > +     /* dp off, power off last */
> > +     if (old_dp_connected && !new_dp_connected)
> > +             pm_runtime_put_sync(dev);
> > +
> >       return 0;
> >  }
> >
> > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h b/drivers/gpu/drm/bridge/analogix/anx7625.h
> > index 76cfc64f7574..7d6c6fdf9a3a 100644
> > --- a/drivers/gpu/drm/bridge/analogix/anx7625.h
> > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.h
> > @@ -55,6 +55,18 @@
> >  #define HPD_STATUS_CHANGE 0x80
> >  #define HPD_STATUS 0x80
> >
> > +#define TCPC_SWITCH_0 0xB4
> > +#define SW_SEL1_DPTX0_RX2 BIT(0)
> > +#define SW_SEL1_DPTX0_RX1 BIT(1)
> > +#define SW_SEL1_SSRX_RX2 BIT(4)
> > +#define SW_SEL1_SSRX_RX1 BIT(5)
> > +
> > +#define TCPC_SWITCH_1 0xB5
> > +#define SW_SEL2_DPTX1_TX2 BIT(0)
> > +#define SW_SEL2_DPTX1_TX1 BIT(1)
> > +#define SW_SEL2_SSTX_TX2 BIT(4)
> > +#define SW_SEL2_SSTX_TX1 BIT(5)
> > +
> >  /******** END of I2C Address 0x58 ********/
> >
> >  /***************************************************************/
> > @@ -444,6 +456,7 @@ struct anx7625_i2c_client {
> >  };
> >
> >  struct anx7625_port_data {
> > +     bool dp_connected;
> >       struct typec_mux_dev *typec_mux;
> >       struct anx7625_data *ctx;
> >  };
> > --
> > 2.36.1.476.g0c4daa206d-goog
> >

Re: [PATCH v2 1/7] usb: typec: mux: Allow muxes to specify mode-switch

[Heikki Krogerus]

On Thu, Jun 09, 2022 at 06:09:40PM +0000, Prashant Malani wrote:
> Loosen the typec_mux_match() requirements so that searches where an
> alt mode is not specified, but the target mux device lists the
> "mode-switch" property, return a success.
> 
> This is helpful in Type C port drivers which would like to get a pointer
> to the mux switch associated with a Type C port, but don't want to
> specify a particular alt mode.
> 
> Signed-off-by: Prashant Malani <pmalani@chromium.org>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
> 
> Changes since v1:
> - No changes.
> 
>  drivers/usb/typec/mux.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c
> index fd55c2c516a5..464330776cd6 100644
> --- a/drivers/usb/typec/mux.c
> +++ b/drivers/usb/typec/mux.c
> @@ -281,9 +281,13 @@ static void *typec_mux_match(struct fwnode_handle *fwnode, const char *id,
>  	if (match)
>  		goto find_mux;
>  
> -	/* Accessory Mode muxes */
>  	if (!desc) {
> -		match = fwnode_property_present(fwnode, "accessory");
> +		/*
> +		 * Accessory Mode muxes & muxes which explicitly specify
> +		 * the required identifier can avoid SVID matching.
> +		 */
> +		match = fwnode_property_present(fwnode, "accessory") ||
> +			fwnode_property_present(fwnode, id);
>  		if (match)
>  			goto find_mux;
>  		return NULL;
> -- 
> 2.36.1.476.g0c4daa206d-goog

-- 
heikki

Re: [PATCH v2 1/7] usb: typec: mux: Allow muxes to specify mode-switch

[Heikki Krogerus]

On Thu, Jun 09, 2022 at 06:09:40PM +0000, Prashant Malani wrote:
> Loosen the typec_mux_match() requirements so that searches where an
> alt mode is not specified, but the target mux device lists the
> "mode-switch" property, return a success.
> 
> This is helpful in Type C port drivers which would like to get a pointer
> to the mux switch associated with a Type C port, but don't want to
> specify a particular alt mode.
> 
> Signed-off-by: Prashant Malani <pmalani@chromium.org>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
> 
> Changes since v1:
> - No changes.
> 
>  drivers/usb/typec/mux.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c
> index fd55c2c516a5..464330776cd6 100644
> --- a/drivers/usb/typec/mux.c
> +++ b/drivers/usb/typec/mux.c
> @@ -281,9 +281,13 @@ static void *typec_mux_match(struct fwnode_handle *fwnode, const char *id,
>  	if (match)
>  		goto find_mux;
>  
> -	/* Accessory Mode muxes */
>  	if (!desc) {
> -		match = fwnode_property_present(fwnode, "accessory");
> +		/*
> +		 * Accessory Mode muxes & muxes which explicitly specify
> +		 * the required identifier can avoid SVID matching.
> +		 */
> +		match = fwnode_property_present(fwnode, "accessory") ||
> +			fwnode_property_present(fwnode, id);
>  		if (match)
>  			goto find_mux;
>  		return NULL;
> -- 
> 2.36.1.476.g0c4daa206d-goog

-- 
heikki

Re: [PATCH v2 2/7] usb: typec: mux: Add CONFIG guards for functions

[Heikki Krogerus]

Hi,

On Thu, Jun 09, 2022 at 06:09:41PM +0000, Prashant Malani wrote:
> There are some drivers that can use the Type C mux API, but don't have
> to. Introduce CONFIG guards for the mux functions so that drivers can
> include the header file and not run into compilation errors on systems
> which don't have CONFIG_TYPEC enabled. When CONFIG_TYPEC is not enabled,
> the Type C mux functions will be stub versions of the original calls.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
> 
> Changes since v1:
> - Added static inline to stub functions.
> - Updated function signature of stub functions from "struct typec_mux"
>   to "struct typec_mux_dev" in accordance with updates from commit
>   713fd49b430c ("usb: typec: mux: Introduce indirection")
> 
>  include/linux/usb/typec_mux.h | 38 +++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/include/linux/usb/typec_mux.h b/include/linux/usb/typec_mux.h
> index ee57781dcf28..9eda6146fd26 100644
> --- a/include/linux/usb/typec_mux.h
> +++ b/include/linux/usb/typec_mux.h
> @@ -58,6 +58,8 @@ struct typec_mux_desc {
>  	void *drvdata;
>  };
>  
> +#if IS_ENABLED(CONFIG_TYPEC) || IS_MODULE(CONFIG_TYPEC)
> +
>  struct typec_mux *fwnode_typec_mux_get(struct fwnode_handle *fwnode,
>  				       const struct typec_altmode_desc *desc);
>  void typec_mux_put(struct typec_mux *mux);
> @@ -76,4 +78,40 @@ void typec_mux_unregister(struct typec_mux_dev *mux);
>  void typec_mux_set_drvdata(struct typec_mux_dev *mux, void *data);
>  void *typec_mux_get_drvdata(struct typec_mux_dev *mux);
>  
> +#else
> +
> +static inline struct typec_mux *fwnode_typec_mux_get(struct fwnode_handle *fwnode,
> +				       const struct typec_altmode_desc *desc)
> +{
> +	return ERR_PTR(-EOPNOTSUPP);
> +}

The mux is optional resource for the drivers - fwnode_typec_mux_get()
returns NULL if there is no mux for the caller - so it's better to
just return NULL from this stub.

> +static inline void typec_mux_put(struct typec_mux *mux) {}
> +
> +static inline int typec_mux_set(struct typec_mux *mux, struct typec_mux_state *state)
> +{
> +	return -EOPNOTSUPP;
> +}

Return 0 in this case. That way this stub matches the function
behaviour:

        ...
        if (IS_ERR_OR_NULL(mux))
                return 0;
        ...

> +static inline struct typec_mux *
> +typec_mux_get(struct device *dev, const struct typec_altmode_desc *desc)
> +{
> +	return ERR_PTR(-EOPNOTSUPP);
> +}

You don't need this one. Just leave the original outside of the ifdef.
It's already an inline wrapper function.

> +static inline struct typec_mux_dev *
> +typec_mux_register(struct device *parent, const struct typec_mux_desc *desc)
> +{
> +	return ERR_PTR(-EOPNOTSUPP);
> +}
> +static inline void typec_mux_unregister(struct typec_mux_dev *mux) {}
> +
> +static inline void typec_mux_set_drvdata(struct typec_mux_dev *mux, void *data) {}
> +static inline void *typec_mux_get_drvdata(struct typec_mux_dev *mux)
> +{
> +	return ERR_PTR(-EOPNOTSUPP);
> +}
> +
> +#endif /* CONFIG_TYPEC */
> +
>  #endif /* __USB_TYPEC_MUX */

thanks,

-- 
heikki

Re: [PATCH v2 2/7] usb: typec: mux: Add CONFIG guards for functions

[Heikki Krogerus]

Hi,

On Thu, Jun 09, 2022 at 06:09:41PM +0000, Prashant Malani wrote:
> There are some drivers that can use the Type C mux API, but don't have
> to. Introduce CONFIG guards for the mux functions so that drivers can
> include the header file and not run into compilation errors on systems
> which don't have CONFIG_TYPEC enabled. When CONFIG_TYPEC is not enabled,
> the Type C mux functions will be stub versions of the original calls.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
> 
> Changes since v1:
> - Added static inline to stub functions.
> - Updated function signature of stub functions from "struct typec_mux"
>   to "struct typec_mux_dev" in accordance with updates from commit
>   713fd49b430c ("usb: typec: mux: Introduce indirection")
> 
>  include/linux/usb/typec_mux.h | 38 +++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/include/linux/usb/typec_mux.h b/include/linux/usb/typec_mux.h
> index ee57781dcf28..9eda6146fd26 100644
> --- a/include/linux/usb/typec_mux.h
> +++ b/include/linux/usb/typec_mux.h
> @@ -58,6 +58,8 @@ struct typec_mux_desc {
>  	void *drvdata;
>  };
>  
> +#if IS_ENABLED(CONFIG_TYPEC) || IS_MODULE(CONFIG_TYPEC)
> +
>  struct typec_mux *fwnode_typec_mux_get(struct fwnode_handle *fwnode,
>  				       const struct typec_altmode_desc *desc);
>  void typec_mux_put(struct typec_mux *mux);
> @@ -76,4 +78,40 @@ void typec_mux_unregister(struct typec_mux_dev *mux);
>  void typec_mux_set_drvdata(struct typec_mux_dev *mux, void *data);
>  void *typec_mux_get_drvdata(struct typec_mux_dev *mux);
>  
> +#else
> +
> +static inline struct typec_mux *fwnode_typec_mux_get(struct fwnode_handle *fwnode,
> +				       const struct typec_altmode_desc *desc)
> +{
> +	return ERR_PTR(-EOPNOTSUPP);
> +}

The mux is optional resource for the drivers - fwnode_typec_mux_get()
returns NULL if there is no mux for the caller - so it's better to
just return NULL from this stub.

> +static inline void typec_mux_put(struct typec_mux *mux) {}
> +
> +static inline int typec_mux_set(struct typec_mux *mux, struct typec_mux_state *state)
> +{
> +	return -EOPNOTSUPP;
> +}

Return 0 in this case. That way this stub matches the function
behaviour:

        ...
        if (IS_ERR_OR_NULL(mux))
                return 0;
        ...

> +static inline struct typec_mux *
> +typec_mux_get(struct device *dev, const struct typec_altmode_desc *desc)
> +{
> +	return ERR_PTR(-EOPNOTSUPP);
> +}

You don't need this one. Just leave the original outside of the ifdef.
It's already an inline wrapper function.

> +static inline struct typec_mux_dev *
> +typec_mux_register(struct device *parent, const struct typec_mux_desc *desc)
> +{
> +	return ERR_PTR(-EOPNOTSUPP);
> +}
> +static inline void typec_mux_unregister(struct typec_mux_dev *mux) {}
> +
> +static inline void typec_mux_set_drvdata(struct typec_mux_dev *mux, void *data) {}
> +static inline void *typec_mux_get_drvdata(struct typec_mux_dev *mux)
> +{
> +	return ERR_PTR(-EOPNOTSUPP);
> +}
> +
> +#endif /* CONFIG_TYPEC */
> +
>  #endif /* __USB_TYPEC_MUX */

thanks,

-- 
heikki

Re: [PATCH v2 7/7] drm/bridge: anx7625: Add typec_mux_set callback function

[AngeloGioacchino Del Regno]

Il 09/06/22 20:09, Prashant Malani ha scritto:
> From: Pin-Yen Lin <treapking@chromium.org>
> 
> Add the callback function when the driver receives state
> changes of the Type-C port. The callback function configures the
> crosspoint switch of the anx7625 bridge chip, which can change the
> output pins of the signals according to the port state.
> 
> Signed-off-by: Pin-Yen Lin <treapking@chromium.org>
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
> 
> Changes since v2:
> - No changes.
> 
>   drivers/gpu/drm/bridge/analogix/anx7625.c | 58 +++++++++++++++++++++++
>   drivers/gpu/drm/bridge/analogix/anx7625.h | 13 +++++
>   2 files changed, 71 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> index d41a21103bd3..2c308d12fab2 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> @@ -15,6 +15,7 @@
>   #include <linux/regulator/consumer.h>
>   #include <linux/slab.h>
>   #include <linux/types.h>
> +#include <linux/usb/typec_dp.h>
>   #include <linux/usb/typec_mux.h>
>   #include <linux/workqueue.h>
>   
> @@ -2582,9 +2583,66 @@ static void anx7625_runtime_disable(void *data)
>   	pm_runtime_disable(data);
>   }
>   
> +static void anx7625_set_crosspoint_switch(struct anx7625_data *ctx,
> +					  enum typec_orientation orientation)
> +{
> +	if (orientation == TYPEC_ORIENTATION_NORMAL) {
> +		anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0,
> +				  SW_SEL1_SSRX_RX1 | SW_SEL1_DPTX0_RX2);
> +		anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1,
> +				  SW_SEL2_SSTX_TX1 | SW_SEL2_DPTX1_TX2);
> +	} else if (orientation == TYPEC_ORIENTATION_REVERSE) {
> +		anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0,
> +				  SW_SEL1_SSRX_RX2 | SW_SEL1_DPTX0_RX1);
> +		anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1,
> +				  SW_SEL2_SSTX_TX2 | SW_SEL2_DPTX1_TX1);
> +	}
> +}
> +
> +static void anx7625_typec_two_ports_update(struct anx7625_data *ctx)
> +{
> +	if (ctx->typec_ports[0].dp_connected && ctx->typec_ports[1].dp_connected)
> +		/* Both ports available, do nothing to retain the current one. */
> +		return;
> +	else if (ctx->typec_ports[0].dp_connected)
> +		anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_NORMAL);
> +	else if (ctx->typec_ports[1].dp_connected)
> +		anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_REVERSE);
> +}
> +
>   static int anx7625_typec_mux_set(struct typec_mux_dev *mux,
>   				 struct typec_mux_state *state)
>   {
> +	struct anx7625_port_data *data = typec_mux_get_drvdata(mux);
> +	struct anx7625_data *ctx = data->ctx;
> +	struct device *dev = &ctx->client->dev;
> +
> +	bool old_dp_connected = (ctx->typec_ports[0].dp_connected ||
> +				 ctx->typec_ports[1].dp_connected);

So the old connection state is "either port0 or port1 are currently connected"...

> +	bool new_dp_connected;
> +
> +	if (ctx->num_typec_switches == 1)
> +		return 0;
> +
> +	dev_dbg(dev, "mux_set dp_connected: c0=%d, c1=%d\n",
> +		ctx->typec_ports[0].dp_connected, ctx->typec_ports[1].dp_connected);
> +
> +	data->dp_connected = (state->alt && state->alt->svid == USB_TYPEC_DP_SID &&
> +			      state->alt->mode == USB_TYPEC_DP_MODE);
> + > +	new_dp_connected = (ctx->typec_ports[0].dp_connected ||
> +			    ctx->typec_ports[1].dp_connected);

...and the new connection state is the same as the old one, because I don't see
anything that could ever modify it in this function's flow, until reaching this
assignment.

> +
> +	/* dp on, power on first */
> +	if (!old_dp_connected && new_dp_connected)
> +		pm_runtime_get_sync(dev);

...so that will never happen...

> +
> +	anx7625_typec_two_ports_update(ctx);
> +
> +	/* dp off, power off last */
> +	if (old_dp_connected && !new_dp_connected)
> +		pm_runtime_put_sync(dev);

...and same here.

Regards,
Angelo

Re: [PATCH v2 7/7] drm/bridge: anx7625: Add typec_mux_set callback function

[AngeloGioacchino Del Regno]

Il 09/06/22 20:09, Prashant Malani ha scritto:
> From: Pin-Yen Lin <treapking@chromium.org>
> 
> Add the callback function when the driver receives state
> changes of the Type-C port. The callback function configures the
> crosspoint switch of the anx7625 bridge chip, which can change the
> output pins of the signals according to the port state.
> 
> Signed-off-by: Pin-Yen Lin <treapking@chromium.org>
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
> 
> Changes since v2:
> - No changes.
> 
>   drivers/gpu/drm/bridge/analogix/anx7625.c | 58 +++++++++++++++++++++++
>   drivers/gpu/drm/bridge/analogix/anx7625.h | 13 +++++
>   2 files changed, 71 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> index d41a21103bd3..2c308d12fab2 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> @@ -15,6 +15,7 @@
>   #include <linux/regulator/consumer.h>
>   #include <linux/slab.h>
>   #include <linux/types.h>
> +#include <linux/usb/typec_dp.h>
>   #include <linux/usb/typec_mux.h>
>   #include <linux/workqueue.h>
>   
> @@ -2582,9 +2583,66 @@ static void anx7625_runtime_disable(void *data)
>   	pm_runtime_disable(data);
>   }
>   
> +static void anx7625_set_crosspoint_switch(struct anx7625_data *ctx,
> +					  enum typec_orientation orientation)
> +{
> +	if (orientation == TYPEC_ORIENTATION_NORMAL) {
> +		anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0,
> +				  SW_SEL1_SSRX_RX1 | SW_SEL1_DPTX0_RX2);
> +		anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1,
> +				  SW_SEL2_SSTX_TX1 | SW_SEL2_DPTX1_TX2);
> +	} else if (orientation == TYPEC_ORIENTATION_REVERSE) {
> +		anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0,
> +				  SW_SEL1_SSRX_RX2 | SW_SEL1_DPTX0_RX1);
> +		anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1,
> +				  SW_SEL2_SSTX_TX2 | SW_SEL2_DPTX1_TX1);
> +	}
> +}
> +
> +static void anx7625_typec_two_ports_update(struct anx7625_data *ctx)
> +{
> +	if (ctx->typec_ports[0].dp_connected && ctx->typec_ports[1].dp_connected)
> +		/* Both ports available, do nothing to retain the current one. */
> +		return;
> +	else if (ctx->typec_ports[0].dp_connected)
> +		anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_NORMAL);
> +	else if (ctx->typec_ports[1].dp_connected)
> +		anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_REVERSE);
> +}
> +
>   static int anx7625_typec_mux_set(struct typec_mux_dev *mux,
>   				 struct typec_mux_state *state)
>   {
> +	struct anx7625_port_data *data = typec_mux_get_drvdata(mux);
> +	struct anx7625_data *ctx = data->ctx;
> +	struct device *dev = &ctx->client->dev;
> +
> +	bool old_dp_connected = (ctx->typec_ports[0].dp_connected ||
> +				 ctx->typec_ports[1].dp_connected);

So the old connection state is "either port0 or port1 are currently connected"...

> +	bool new_dp_connected;
> +
> +	if (ctx->num_typec_switches == 1)
> +		return 0;
> +
> +	dev_dbg(dev, "mux_set dp_connected: c0=%d, c1=%d\n",
> +		ctx->typec_ports[0].dp_connected, ctx->typec_ports[1].dp_connected);
> +
> +	data->dp_connected = (state->alt && state->alt->svid == USB_TYPEC_DP_SID &&
> +			      state->alt->mode == USB_TYPEC_DP_MODE);
> + > +	new_dp_connected = (ctx->typec_ports[0].dp_connected ||
> +			    ctx->typec_ports[1].dp_connected);

...and the new connection state is the same as the old one, because I don't see
anything that could ever modify it in this function's flow, until reaching this
assignment.

> +
> +	/* dp on, power on first */
> +	if (!old_dp_connected && new_dp_connected)
> +		pm_runtime_get_sync(dev);

...so that will never happen...

> +
> +	anx7625_typec_two_ports_update(ctx);
> +
> +	/* dp off, power off last */
> +	if (old_dp_connected && !new_dp_connected)
> +		pm_runtime_put_sync(dev);

...and same here.

Regards,
Angelo

Re: [PATCH v2 6/7] drm/bridge: anx7625: Register Type-C mode switches

[AngeloGioacchino Del Regno]

Il 09/06/22 20:09, Prashant Malani ha scritto:
> When the DT node has "switches" available, register a Type-C mode-switch
> for each listed "switch". This allows the driver to receive state
> information about what operating mode a Type-C port and its connected
> peripherals are in, as well as status information (like VDOs) related to
> that state.
> 
> The callback function is currently a stub, but subsequent patches will
> implement the required functionality.
> 
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
> 
> Changes since v2:
> - No changes.
> 
>   drivers/gpu/drm/bridge/analogix/anx7625.c | 73 +++++++++++++++++++++++
>   drivers/gpu/drm/bridge/analogix/anx7625.h |  6 ++
>   2 files changed, 79 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> index 07ed44c6b839..d41a21103bd3 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> @@ -15,6 +15,7 @@
>   #include <linux/regulator/consumer.h>
>   #include <linux/slab.h>
>   #include <linux/types.h>
> +#include <linux/usb/typec_mux.h>
>   #include <linux/workqueue.h>
>   
>   #include <linux/of_gpio.h>
> @@ -2581,9 +2582,59 @@ static void anx7625_runtime_disable(void *data)
>   	pm_runtime_disable(data);
>   }
>   
> +static int anx7625_typec_mux_set(struct typec_mux_dev *mux,
> +				 struct typec_mux_state *state)
> +{
> +	return 0;
> +}
> +
> +static int anx7625_register_mode_switch(struct device *dev, struct device_node *node,
> +					struct anx7625_data *ctx)
> +{
> +	struct anx7625_port_data *port_data;
> +	struct typec_mux_desc mux_desc = {};
> +	char name[32];
> +	u32 port_num;
> +	int ret;
> +
> +	ret = of_property_read_u32(node, "reg", &port_num);
> +	if (ret)
> +		return ret;
> +
> +	if (port_num >= ctx->num_typec_switches) {
> +		dev_err(dev, "Invalid port number specified: %d\n", port_num);
> +		return -EINVAL;
> +	}
> +
> +	port_data = &ctx->typec_ports[port_num];
> +	port_data->ctx = ctx;
> +	mux_desc.fwnode = &node->fwnode;
> +	mux_desc.drvdata = port_data;
> +	snprintf(name, sizeof(name), "%s-%u", node->name, port_num);
> +	mux_desc.name = name;
> +	mux_desc.set = anx7625_typec_mux_set;
> +
> +	port_data->typec_mux = typec_mux_register(dev, &mux_desc);
> +	if (IS_ERR(port_data->typec_mux)) {
> +		ret = PTR_ERR(port_data->typec_mux);
> +		dev_err(dev, "Mode switch register for port %d failed: %d", port_num, ret);
> +	}

Please return 0 at the end of this function.

	if (IS_ERR(....)) {
		......code......
		return ret;
	}

	return 0;
}

> +
> +	return ret;
> +}
> +
> +static void anx7625_unregister_typec_switches(struct anx7625_data *ctx)
> +{
> +	int i;
> +
> +	for (i = 0; i < ctx->num_typec_switches; i++)
> +		typec_mux_unregister(ctx->typec_ports[i].typec_mux);
> +}
> +
>   static int anx7625_register_typec_switches(struct device *device, struct anx7625_data *ctx)
>   {
>   	struct device_node *of = NULL;
> +	struct device_node *sw;
>   	int ret = 0;
>   
>   	of = of_get_child_by_name(device->of_node, "switches");
> @@ -2594,6 +2645,26 @@ static int anx7625_register_typec_switches(struct device *device, struct anx7625
>   	if (ctx->num_typec_switches <= 0)
>   		return -ENODEV;
>   
> +	ctx->typec_ports = devm_kzalloc(device,
> +					ctx->num_typec_switches * sizeof(struct anx7625_port_data),
> +					GFP_KERNEL);
> +	if (!ctx->typec_ports)
> +		return -ENOMEM;
> +
> +	/* Register switches for each connector. */
> +	for_each_available_child_of_node(of, sw) {
> +		if (!of_property_read_bool(sw, "mode-switch"))
> +			continue;
> +		ret = anx7625_register_mode_switch(device, sw, ctx);
> +		if (ret) {
> +			dev_err(device, "Failed to register mode switch: %d\n", ret);
> +			break;
> +		}
> +	}
> +
> +	if (ret)
> +		anx7625_unregister_typec_switches(ctx);
> +
>   	return ret;
>   }
>   
> @@ -2759,6 +2830,8 @@ static int anx7625_i2c_remove(struct i2c_client *client)
>   
>   	drm_bridge_remove(&platform->bridge);
>   
> +	anx7625_unregister_typec_switches(platform);
> +
>   	if (platform->pdata.intp_irq)
>   		destroy_workqueue(platform->workqueue);
>   
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h b/drivers/gpu/drm/bridge/analogix/anx7625.h
> index d5cbca708842..76cfc64f7574 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.h
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.h
> @@ -443,6 +443,11 @@ struct anx7625_i2c_client {
>   	struct i2c_client *tcpc_client;
>   };
>   
> +struct anx7625_port_data {
> +	struct typec_mux_dev *typec_mux;
> +	struct anx7625_data *ctx;
> +};
> +
>   struct anx7625_data {
>   	struct anx7625_platform_data pdata;
>   	struct platform_device *audio_pdev;
> @@ -474,6 +479,7 @@ struct anx7625_data {
>   	struct mipi_dsi_device *dsi;
>   	struct drm_dp_aux aux;
>   	int num_typec_switches;
> +	struct anx7625_port_data *typec_ports;
>   };
>   
>   #endif  /* __ANX7625_H__ */

Re: [PATCH v2 6/7] drm/bridge: anx7625: Register Type-C mode switches

[AngeloGioacchino Del Regno]

Il 09/06/22 20:09, Prashant Malani ha scritto:
> When the DT node has "switches" available, register a Type-C mode-switch
> for each listed "switch". This allows the driver to receive state
> information about what operating mode a Type-C port and its connected
> peripherals are in, as well as status information (like VDOs) related to
> that state.
> 
> The callback function is currently a stub, but subsequent patches will
> implement the required functionality.
> 
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
> 
> Changes since v2:
> - No changes.
> 
>   drivers/gpu/drm/bridge/analogix/anx7625.c | 73 +++++++++++++++++++++++
>   drivers/gpu/drm/bridge/analogix/anx7625.h |  6 ++
>   2 files changed, 79 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> index 07ed44c6b839..d41a21103bd3 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> @@ -15,6 +15,7 @@
>   #include <linux/regulator/consumer.h>
>   #include <linux/slab.h>
>   #include <linux/types.h>
> +#include <linux/usb/typec_mux.h>
>   #include <linux/workqueue.h>
>   
>   #include <linux/of_gpio.h>
> @@ -2581,9 +2582,59 @@ static void anx7625_runtime_disable(void *data)
>   	pm_runtime_disable(data);
>   }
>   
> +static int anx7625_typec_mux_set(struct typec_mux_dev *mux,
> +				 struct typec_mux_state *state)
> +{
> +	return 0;
> +}
> +
> +static int anx7625_register_mode_switch(struct device *dev, struct device_node *node,
> +					struct anx7625_data *ctx)
> +{
> +	struct anx7625_port_data *port_data;
> +	struct typec_mux_desc mux_desc = {};
> +	char name[32];
> +	u32 port_num;
> +	int ret;
> +
> +	ret = of_property_read_u32(node, "reg", &port_num);
> +	if (ret)
> +		return ret;
> +
> +	if (port_num >= ctx->num_typec_switches) {
> +		dev_err(dev, "Invalid port number specified: %d\n", port_num);
> +		return -EINVAL;
> +	}
> +
> +	port_data = &ctx->typec_ports[port_num];
> +	port_data->ctx = ctx;
> +	mux_desc.fwnode = &node->fwnode;
> +	mux_desc.drvdata = port_data;
> +	snprintf(name, sizeof(name), "%s-%u", node->name, port_num);
> +	mux_desc.name = name;
> +	mux_desc.set = anx7625_typec_mux_set;
> +
> +	port_data->typec_mux = typec_mux_register(dev, &mux_desc);
> +	if (IS_ERR(port_data->typec_mux)) {
> +		ret = PTR_ERR(port_data->typec_mux);
> +		dev_err(dev, "Mode switch register for port %d failed: %d", port_num, ret);
> +	}

Please return 0 at the end of this function.

	if (IS_ERR(....)) {
		......code......
		return ret;
	}

	return 0;
}

> +
> +	return ret;
> +}
> +
> +static void anx7625_unregister_typec_switches(struct anx7625_data *ctx)
> +{
> +	int i;
> +
> +	for (i = 0; i < ctx->num_typec_switches; i++)
> +		typec_mux_unregister(ctx->typec_ports[i].typec_mux);
> +}
> +
>   static int anx7625_register_typec_switches(struct device *device, struct anx7625_data *ctx)
>   {
>   	struct device_node *of = NULL;
> +	struct device_node *sw;
>   	int ret = 0;
>   
>   	of = of_get_child_by_name(device->of_node, "switches");
> @@ -2594,6 +2645,26 @@ static int anx7625_register_typec_switches(struct device *device, struct anx7625
>   	if (ctx->num_typec_switches <= 0)
>   		return -ENODEV;
>   
> +	ctx->typec_ports = devm_kzalloc(device,
> +					ctx->num_typec_switches * sizeof(struct anx7625_port_data),
> +					GFP_KERNEL);
> +	if (!ctx->typec_ports)
> +		return -ENOMEM;
> +
> +	/* Register switches for each connector. */
> +	for_each_available_child_of_node(of, sw) {
> +		if (!of_property_read_bool(sw, "mode-switch"))
> +			continue;
> +		ret = anx7625_register_mode_switch(device, sw, ctx);
> +		if (ret) {
> +			dev_err(device, "Failed to register mode switch: %d\n", ret);
> +			break;
> +		}
> +	}
> +
> +	if (ret)
> +		anx7625_unregister_typec_switches(ctx);
> +
>   	return ret;
>   }
>   
> @@ -2759,6 +2830,8 @@ static int anx7625_i2c_remove(struct i2c_client *client)
>   
>   	drm_bridge_remove(&platform->bridge);
>   
> +	anx7625_unregister_typec_switches(platform);
> +
>   	if (platform->pdata.intp_irq)
>   		destroy_workqueue(platform->workqueue);
>   
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h b/drivers/gpu/drm/bridge/analogix/anx7625.h
> index d5cbca708842..76cfc64f7574 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.h
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.h
> @@ -443,6 +443,11 @@ struct anx7625_i2c_client {
>   	struct i2c_client *tcpc_client;
>   };
>   
> +struct anx7625_port_data {
> +	struct typec_mux_dev *typec_mux;
> +	struct anx7625_data *ctx;
> +};
> +
>   struct anx7625_data {
>   	struct anx7625_platform_data pdata;
>   	struct platform_device *audio_pdev;
> @@ -474,6 +479,7 @@ struct anx7625_data {
>   	struct mipi_dsi_device *dsi;
>   	struct drm_dp_aux aux;
>   	int num_typec_switches;
> +	struct anx7625_port_data *typec_ports;
>   };
>   
>   #endif  /* __ANX7625_H__ */

Re: [PATCH v2 5/7] drm/bridge: anx7625: Register number of Type C switches

[AngeloGioacchino Del Regno]

Il 09/06/22 20:09, Prashant Malani ha scritto:
> Parse the "switches" node, if available, and count and store the number
> of Type-C switches within it. Since we currently don't do anything with
> this info, no functional changes are expected from this change.
> 
> This patch sets a foundation for the actual registering of Type-C
> switches with the Type-C connector class framework.
> 
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
> 
> Changes since v1:
> - No changes.
> 
>   drivers/gpu/drm/bridge/analogix/anx7625.c | 20 ++++++++++++++++++++
>   drivers/gpu/drm/bridge/analogix/anx7625.h |  1 +
>   2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> index 53a5da6c49dd..07ed44c6b839 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> @@ -2581,6 +2581,22 @@ static void anx7625_runtime_disable(void *data)
>   	pm_runtime_disable(data);
>   }
>   
> +static int anx7625_register_typec_switches(struct device *device, struct anx7625_data *ctx)
> +{
> +	struct device_node *of = NULL;
> +	int ret = 0;
> +
> +	of = of_get_child_by_name(device->of_node, "switches");
> +	if (!of)
> +		return -ENODEV;
> +
> +	ctx->num_typec_switches = of_get_child_count(of);
> +	if (ctx->num_typec_switches <= 0)
> +		return -ENODEV;
> +
> +	return ret;

You aren't using the `ret` variable for anything other than returning zero:
remove it and simply return 0 here.

> +}
> +
>   static int anx7625_i2c_probe(struct i2c_client *client,
>   			     const struct i2c_device_id *id)
>   {
> @@ -2686,6 +2702,10 @@ static int anx7625_i2c_probe(struct i2c_client *client,
>   	if (platform->pdata.intp_irq)
>   		queue_work(platform->workqueue, &platform->work);
>   
> +	ret = anx7625_register_typec_switches(dev, platform);
> +	if (ret)
> +		dev_info(dev, "Didn't register Type C switches, err: %d\n", ret);

Type-C switches are optional for this driver and this will print a sort of error
on boards that are *not* declaring any switches on purpose (because perhaps they
don't have any, or for any other reason).

Even though this is a dev_info and not a dev_err, it's still printing an alarming
(and useless, in the aforementioned case) message.

Please fix this.

Regards,
Angelo

Re: [PATCH v2 5/7] drm/bridge: anx7625: Register number of Type C switches

[AngeloGioacchino Del Regno]

Il 09/06/22 20:09, Prashant Malani ha scritto:
> Parse the "switches" node, if available, and count and store the number
> of Type-C switches within it. Since we currently don't do anything with
> this info, no functional changes are expected from this change.
> 
> This patch sets a foundation for the actual registering of Type-C
> switches with the Type-C connector class framework.
> 
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
> 
> Changes since v1:
> - No changes.
> 
>   drivers/gpu/drm/bridge/analogix/anx7625.c | 20 ++++++++++++++++++++
>   drivers/gpu/drm/bridge/analogix/anx7625.h |  1 +
>   2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> index 53a5da6c49dd..07ed44c6b839 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> @@ -2581,6 +2581,22 @@ static void anx7625_runtime_disable(void *data)
>   	pm_runtime_disable(data);
>   }
>   
> +static int anx7625_register_typec_switches(struct device *device, struct anx7625_data *ctx)
> +{
> +	struct device_node *of = NULL;
> +	int ret = 0;
> +
> +	of = of_get_child_by_name(device->of_node, "switches");
> +	if (!of)
> +		return -ENODEV;
> +
> +	ctx->num_typec_switches = of_get_child_count(of);
> +	if (ctx->num_typec_switches <= 0)
> +		return -ENODEV;
> +
> +	return ret;

You aren't using the `ret` variable for anything other than returning zero:
remove it and simply return 0 here.

> +}
> +
>   static int anx7625_i2c_probe(struct i2c_client *client,
>   			     const struct i2c_device_id *id)
>   {
> @@ -2686,6 +2702,10 @@ static int anx7625_i2c_probe(struct i2c_client *client,
>   	if (platform->pdata.intp_irq)
>   		queue_work(platform->workqueue, &platform->work);
>   
> +	ret = anx7625_register_typec_switches(dev, platform);
> +	if (ret)
> +		dev_info(dev, "Didn't register Type C switches, err: %d\n", ret);

Type-C switches are optional for this driver and this will print a sort of error
on boards that are *not* declaring any switches on purpose (because perhaps they
don't have any, or for any other reason).

Even though this is a dev_info and not a dev_err, it's still printing an alarming
(and useless, in the aforementioned case) message.

Please fix this.

Regards,
Angelo

Re: [PATCH v2 2/7] usb: typec: mux: Add CONFIG guards for functions

[AngeloGioacchino Del Regno]

Il 09/06/22 20:09, Prashant Malani ha scritto:
> There are some drivers that can use the Type C mux API, but don't have
> to. Introduce CONFIG guards for the mux functions so that drivers can
> include the header file and not run into compilation errors on systems
> which don't have CONFIG_TYPEC enabled. When CONFIG_TYPEC is not enabled,
> the Type C mux functions will be stub versions of the original calls.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
> 
> Changes since v1:
> - Added static inline to stub functions.
> - Updated function signature of stub functions from "struct typec_mux"
>    to "struct typec_mux_dev" in accordance with updates from commit
>    713fd49b430c ("usb: typec: mux: Introduce indirection")
> 
>   include/linux/usb/typec_mux.h | 38 +++++++++++++++++++++++++++++++++++
>   1 file changed, 38 insertions(+)
> 
> diff --git a/include/linux/usb/typec_mux.h b/include/linux/usb/typec_mux.h
> index ee57781dcf28..9eda6146fd26 100644
> --- a/include/linux/usb/typec_mux.h
> +++ b/include/linux/usb/typec_mux.h
> @@ -58,6 +58,8 @@ struct typec_mux_desc {
>   	void *drvdata;
>   };
>   
> +#if IS_ENABLED(CONFIG_TYPEC) || IS_MODULE(CONFIG_TYPEC)

IS_ENABLED(x) evaluates to 1 when (x == 'y' || x == 'm')
IS_MODULE(x) evaluates to 1 when              (x == 'm')

....this means that a IS_ENABLED(CONFIG_TYPEC) check is enough, and
the latter is redundant.

Regards,
Angelo

Re: [PATCH v2 2/7] usb: typec: mux: Add CONFIG guards for functions

[AngeloGioacchino Del Regno]

Il 09/06/22 20:09, Prashant Malani ha scritto:
> There are some drivers that can use the Type C mux API, but don't have
> to. Introduce CONFIG guards for the mux functions so that drivers can
> include the header file and not run into compilation errors on systems
> which don't have CONFIG_TYPEC enabled. When CONFIG_TYPEC is not enabled,
> the Type C mux functions will be stub versions of the original calls.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
> 
> Changes since v1:
> - Added static inline to stub functions.
> - Updated function signature of stub functions from "struct typec_mux"
>    to "struct typec_mux_dev" in accordance with updates from commit
>    713fd49b430c ("usb: typec: mux: Introduce indirection")
> 
>   include/linux/usb/typec_mux.h | 38 +++++++++++++++++++++++++++++++++++
>   1 file changed, 38 insertions(+)
> 
> diff --git a/include/linux/usb/typec_mux.h b/include/linux/usb/typec_mux.h
> index ee57781dcf28..9eda6146fd26 100644
> --- a/include/linux/usb/typec_mux.h
> +++ b/include/linux/usb/typec_mux.h
> @@ -58,6 +58,8 @@ struct typec_mux_desc {
>   	void *drvdata;
>   };
>   
> +#if IS_ENABLED(CONFIG_TYPEC) || IS_MODULE(CONFIG_TYPEC)

IS_ENABLED(x) evaluates to 1 when (x == 'y' || x == 'm')
IS_MODULE(x) evaluates to 1 when              (x == 'm')

....this means that a IS_ENABLED(CONFIG_TYPEC) check is enough, and
the latter is redundant.

Regards,
Angelo

Re: [PATCH v2 1/7] usb: typec: mux: Allow muxes to specify mode-switch

[AngeloGioacchino Del Regno]

Il 09/06/22 20:09, Prashant Malani ha scritto:
> Loosen the typec_mux_match() requirements so that searches where an
> alt mode is not specified, but the target mux device lists the
> "mode-switch" property, return a success.
> 
> This is helpful in Type C port drivers which would like to get a pointer
> to the mux switch associated with a Type C port, but don't want to
> specify a particular alt mode.
> 
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH v2 1/7] usb: typec: mux: Allow muxes to specify mode-switch

[AngeloGioacchino Del Regno]

Il 09/06/22 20:09, Prashant Malani ha scritto:
> Loosen the typec_mux_match() requirements so that searches where an
> alt mode is not specified, but the target mux device lists the
> "mode-switch" property, return a success.
> 
> This is helpful in Type C port drivers which would like to get a pointer
> to the mux switch associated with a Type C port, but don't want to
> specify a particular alt mode.
> 
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH v2 7/7] drm/bridge: anx7625: Add typec_mux_set callback function

[Pin-yen Lin]

Hi AngeloGioacchino,


On Tue, Jun 14, 2022 at 4:15 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 09/06/22 20:09, Prashant Malani ha scritto:
> > From: Pin-Yen Lin <treapking@chromium.org>
> >
> > Add the callback function when the driver receives state
> > changes of the Type-C port. The callback function configures the
> > crosspoint switch of the anx7625 bridge chip, which can change the
> > output pins of the signals according to the port state.
> >
> > Signed-off-by: Pin-Yen Lin <treapking@chromium.org>
> > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > ---
> >
> > Changes since v2:
> > - No changes.
> >
> >   drivers/gpu/drm/bridge/analogix/anx7625.c | 58 +++++++++++++++++++++++
> >   drivers/gpu/drm/bridge/analogix/anx7625.h | 13 +++++
> >   2 files changed, 71 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > index d41a21103bd3..2c308d12fab2 100644
> > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > @@ -15,6 +15,7 @@
> >   #include <linux/regulator/consumer.h>
> >   #include <linux/slab.h>
> >   #include <linux/types.h>
> > +#include <linux/usb/typec_dp.h>
> >   #include <linux/usb/typec_mux.h>
> >   #include <linux/workqueue.h>
> >
> > @@ -2582,9 +2583,66 @@ static void anx7625_runtime_disable(void *data)
> >       pm_runtime_disable(data);
> >   }
> >
> > +static void anx7625_set_crosspoint_switch(struct anx7625_data *ctx,
> > +                                       enum typec_orientation orientation)
> > +{
> > +     if (orientation == TYPEC_ORIENTATION_NORMAL) {
> > +             anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0,
> > +                               SW_SEL1_SSRX_RX1 | SW_SEL1_DPTX0_RX2);
> > +             anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1,
> > +                               SW_SEL2_SSTX_TX1 | SW_SEL2_DPTX1_TX2);
> > +     } else if (orientation == TYPEC_ORIENTATION_REVERSE) {
> > +             anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0,
> > +                               SW_SEL1_SSRX_RX2 | SW_SEL1_DPTX0_RX1);
> > +             anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1,
> > +                               SW_SEL2_SSTX_TX2 | SW_SEL2_DPTX1_TX1);
> > +     }
> > +}
> > +
> > +static void anx7625_typec_two_ports_update(struct anx7625_data *ctx)
> > +{
> > +     if (ctx->typec_ports[0].dp_connected && ctx->typec_ports[1].dp_connected)
> > +             /* Both ports available, do nothing to retain the current one. */
> > +             return;
> > +     else if (ctx->typec_ports[0].dp_connected)
> > +             anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_NORMAL);
> > +     else if (ctx->typec_ports[1].dp_connected)
> > +             anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_REVERSE);
> > +}
> > +
> >   static int anx7625_typec_mux_set(struct typec_mux_dev *mux,
> >                                struct typec_mux_state *state)
> >   {
> > +     struct anx7625_port_data *data = typec_mux_get_drvdata(mux);
> > +     struct anx7625_data *ctx = data->ctx;
> > +     struct device *dev = &ctx->client->dev;
> > +
> > +     bool old_dp_connected = (ctx->typec_ports[0].dp_connected ||
> > +                              ctx->typec_ports[1].dp_connected);
>
> So the old connection state is "either port0 or port1 are currently connected"...
>
> > +     bool new_dp_connected;
> > +
> > +     if (ctx->num_typec_switches == 1)
> > +             return 0;
> > +
> > +     dev_dbg(dev, "mux_set dp_connected: c0=%d, c1=%d\n",
> > +             ctx->typec_ports[0].dp_connected, ctx->typec_ports[1].dp_connected);
> > +
> > +     data->dp_connected = (state->alt && state->alt->svid == USB_TYPEC_DP_SID &&
> > +                           state->alt->mode == USB_TYPEC_DP_MODE);
> > + > + new_dp_connected = (ctx->typec_ports[0].dp_connected ||
> > +                         ctx->typec_ports[1].dp_connected);
>
> ...and the new connection state is the same as the old one, because I don't see
> anything that could ever modify it in this function's flow, until reaching this
> assignment.

The typec mux driver data (`struct anx7625_port_data *data =
typec_mux_get_drvdata(mux)`) is set to one of the
`ctx->typec_ports[*]` in `anx7625_register_mode_switch` (see patch 6
of this series).

So, the `data->dp_connected = ...` assignment may change the new
connection state.

Best regards,
Pin-yen

>
> > +
> > +     /* dp on, power on first */
> > +     if (!old_dp_connected && new_dp_connected)
> > +             pm_runtime_get_sync(dev);
>
> ...so that will never happen...
>
> > +
> > +     anx7625_typec_two_ports_update(ctx);
> > +
> > +     /* dp off, power off last */
> > +     if (old_dp_connected && !new_dp_connected)
> > +             pm_runtime_put_sync(dev);
>
> ...and same here.
>
> Regards,
> Angelo

Re: [PATCH v2 7/7] drm/bridge: anx7625: Add typec_mux_set callback function

[Pin-yen Lin]

Hi AngeloGioacchino,


On Tue, Jun 14, 2022 at 4:15 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 09/06/22 20:09, Prashant Malani ha scritto:
> > From: Pin-Yen Lin <treapking@chromium.org>
> >
> > Add the callback function when the driver receives state
> > changes of the Type-C port. The callback function configures the
> > crosspoint switch of the anx7625 bridge chip, which can change the
> > output pins of the signals according to the port state.
> >
> > Signed-off-by: Pin-Yen Lin <treapking@chromium.org>
> > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > ---
> >
> > Changes since v2:
> > - No changes.
> >
> >   drivers/gpu/drm/bridge/analogix/anx7625.c | 58 +++++++++++++++++++++++
> >   drivers/gpu/drm/bridge/analogix/anx7625.h | 13 +++++
> >   2 files changed, 71 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > index d41a21103bd3..2c308d12fab2 100644
> > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > @@ -15,6 +15,7 @@
> >   #include <linux/regulator/consumer.h>
> >   #include <linux/slab.h>
> >   #include <linux/types.h>
> > +#include <linux/usb/typec_dp.h>
> >   #include <linux/usb/typec_mux.h>
> >   #include <linux/workqueue.h>
> >
> > @@ -2582,9 +2583,66 @@ static void anx7625_runtime_disable(void *data)
> >       pm_runtime_disable(data);
> >   }
> >
> > +static void anx7625_set_crosspoint_switch(struct anx7625_data *ctx,
> > +                                       enum typec_orientation orientation)
> > +{
> > +     if (orientation == TYPEC_ORIENTATION_NORMAL) {
> > +             anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0,
> > +                               SW_SEL1_SSRX_RX1 | SW_SEL1_DPTX0_RX2);
> > +             anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1,
> > +                               SW_SEL2_SSTX_TX1 | SW_SEL2_DPTX1_TX2);
> > +     } else if (orientation == TYPEC_ORIENTATION_REVERSE) {
> > +             anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0,
> > +                               SW_SEL1_SSRX_RX2 | SW_SEL1_DPTX0_RX1);
> > +             anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1,
> > +                               SW_SEL2_SSTX_TX2 | SW_SEL2_DPTX1_TX1);
> > +     }
> > +}
> > +
> > +static void anx7625_typec_two_ports_update(struct anx7625_data *ctx)
> > +{
> > +     if (ctx->typec_ports[0].dp_connected && ctx->typec_ports[1].dp_connected)
> > +             /* Both ports available, do nothing to retain the current one. */
> > +             return;
> > +     else if (ctx->typec_ports[0].dp_connected)
> > +             anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_NORMAL);
> > +     else if (ctx->typec_ports[1].dp_connected)
> > +             anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_REVERSE);
> > +}
> > +
> >   static int anx7625_typec_mux_set(struct typec_mux_dev *mux,
> >                                struct typec_mux_state *state)
> >   {
> > +     struct anx7625_port_data *data = typec_mux_get_drvdata(mux);
> > +     struct anx7625_data *ctx = data->ctx;
> > +     struct device *dev = &ctx->client->dev;
> > +
> > +     bool old_dp_connected = (ctx->typec_ports[0].dp_connected ||
> > +                              ctx->typec_ports[1].dp_connected);
>
> So the old connection state is "either port0 or port1 are currently connected"...
>
> > +     bool new_dp_connected;
> > +
> > +     if (ctx->num_typec_switches == 1)
> > +             return 0;
> > +
> > +     dev_dbg(dev, "mux_set dp_connected: c0=%d, c1=%d\n",
> > +             ctx->typec_ports[0].dp_connected, ctx->typec_ports[1].dp_connected);
> > +
> > +     data->dp_connected = (state->alt && state->alt->svid == USB_TYPEC_DP_SID &&
> > +                           state->alt->mode == USB_TYPEC_DP_MODE);
> > + > + new_dp_connected = (ctx->typec_ports[0].dp_connected ||
> > +                         ctx->typec_ports[1].dp_connected);
>
> ...and the new connection state is the same as the old one, because I don't see
> anything that could ever modify it in this function's flow, until reaching this
> assignment.

The typec mux driver data (`struct anx7625_port_data *data =
typec_mux_get_drvdata(mux)`) is set to one of the
`ctx->typec_ports[*]` in `anx7625_register_mode_switch` (see patch 6
of this series).

So, the `data->dp_connected = ...` assignment may change the new
connection state.

Best regards,
Pin-yen

>
> > +
> > +     /* dp on, power on first */
> > +     if (!old_dp_connected && new_dp_connected)
> > +             pm_runtime_get_sync(dev);
>
> ...so that will never happen...
>
> > +
> > +     anx7625_typec_two_ports_update(ctx);
> > +
> > +     /* dp off, power off last */
> > +     if (old_dp_connected && !new_dp_connected)
> > +             pm_runtime_put_sync(dev);
>
> ...and same here.
>
> Regards,
> Angelo

Re: [PATCH v2 6/7] drm/bridge: anx7625: Register Type-C mode switches

[Prashant Malani]

On Tue, Jun 14, 2022 at 1:18 AM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 09/06/22 20:09, Prashant Malani ha scritto:
> > When the DT node has "switches" available, register a Type-C mode-switch
> > for each listed "switch". This allows the driver to receive state
> > information about what operating mode a Type-C port and its connected
> > peripherals are in, as well as status information (like VDOs) related to
> > that state.
> >
> > The callback function is currently a stub, but subsequent patches will
> > implement the required functionality.
> >
> > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > ---
> >
> > Changes since v2:
> > - No changes.
> >
> >   drivers/gpu/drm/bridge/analogix/anx7625.c | 73 +++++++++++++++++++++++
> >   drivers/gpu/drm/bridge/analogix/anx7625.h |  6 ++
> >   2 files changed, 79 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > index 07ed44c6b839..d41a21103bd3 100644
> > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > @@ -15,6 +15,7 @@
> >   #include <linux/regulator/consumer.h>
> >   #include <linux/slab.h>
> >   #include <linux/types.h>
> > +#include <linux/usb/typec_mux.h>
> >   #include <linux/workqueue.h>
> >
> >   #include <linux/of_gpio.h>
> > @@ -2581,9 +2582,59 @@ static void anx7625_runtime_disable(void *data)
> >       pm_runtime_disable(data);
> >   }
> >
> > +static int anx7625_typec_mux_set(struct typec_mux_dev *mux,
> > +                              struct typec_mux_state *state)
> > +{
> > +     return 0;
> > +}
> > +
> > +static int anx7625_register_mode_switch(struct device *dev, struct device_node *node,
> > +                                     struct anx7625_data *ctx)
> > +{
> > +     struct anx7625_port_data *port_data;
> > +     struct typec_mux_desc mux_desc = {};
> > +     char name[32];
> > +     u32 port_num;
> > +     int ret;
> > +
> > +     ret = of_property_read_u32(node, "reg", &port_num);
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (port_num >= ctx->num_typec_switches) {
> > +             dev_err(dev, "Invalid port number specified: %d\n", port_num);
> > +             return -EINVAL;
> > +     }
> > +
> > +     port_data = &ctx->typec_ports[port_num];
> > +     port_data->ctx = ctx;
> > +     mux_desc.fwnode = &node->fwnode;
> > +     mux_desc.drvdata = port_data;
> > +     snprintf(name, sizeof(name), "%s-%u", node->name, port_num);
> > +     mux_desc.name = name;
> > +     mux_desc.set = anx7625_typec_mux_set;
> > +
> > +     port_data->typec_mux = typec_mux_register(dev, &mux_desc);
> > +     if (IS_ERR(port_data->typec_mux)) {
> > +             ret = PTR_ERR(port_data->typec_mux);
> > +             dev_err(dev, "Mode switch register for port %d failed: %d", port_num, ret);
> > +     }
>
> Please return 0 at the end of this function.
>
>         if (IS_ERR(....)) {
>                 ......code......
>                 return ret;
>         }
>
>         return 0;
> }

May I ask why? We're not missing any return paths. I would rather we
keep it as is (which has the valid return value).

>
> > +
> > +     return ret;
> > +}
> > +
> > +static void anx7625_unregister_typec_switches(struct anx7625_data *ctx)
> > +{
> > +     int i;
> > +
> > +     for (i = 0; i < ctx->num_typec_switches; i++)
> > +             typec_mux_unregister(ctx->typec_ports[i].typec_mux);
> > +}
> > +
> >   static int anx7625_register_typec_switches(struct device *device, struct anx7625_data *ctx)
> >   {
> >       struct device_node *of = NULL;
> > +     struct device_node *sw;
> >       int ret = 0;
> >
> >       of = of_get_child_by_name(device->of_node, "switches");
> > @@ -2594,6 +2645,26 @@ static int anx7625_register_typec_switches(struct device *device, struct anx7625
> >       if (ctx->num_typec_switches <= 0)
> >               return -ENODEV;
> >
> > +     ctx->typec_ports = devm_kzalloc(device,
> > +                                     ctx->num_typec_switches * sizeof(struct anx7625_port_data),
> > +                                     GFP_KERNEL);
> > +     if (!ctx->typec_ports)
> > +             return -ENOMEM;
> > +
> > +     /* Register switches for each connector. */
> > +     for_each_available_child_of_node(of, sw) {
> > +             if (!of_property_read_bool(sw, "mode-switch"))
> > +                     continue;
> > +             ret = anx7625_register_mode_switch(device, sw, ctx);
> > +             if (ret) {
> > +                     dev_err(device, "Failed to register mode switch: %d\n", ret);
> > +                     break;
> > +             }
> > +     }
> > +
> > +     if (ret)
> > +             anx7625_unregister_typec_switches(ctx);
> > +
> >       return ret;
> >   }
> >
> > @@ -2759,6 +2830,8 @@ static int anx7625_i2c_remove(struct i2c_client *client)
> >
> >       drm_bridge_remove(&platform->bridge);
> >
> > +     anx7625_unregister_typec_switches(platform);
> > +
> >       if (platform->pdata.intp_irq)
> >               destroy_workqueue(platform->workqueue);
> >
> > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h b/drivers/gpu/drm/bridge/analogix/anx7625.h
> > index d5cbca708842..76cfc64f7574 100644
> > --- a/drivers/gpu/drm/bridge/analogix/anx7625.h
> > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.h
> > @@ -443,6 +443,11 @@ struct anx7625_i2c_client {
> >       struct i2c_client *tcpc_client;
> >   };
> >
> > +struct anx7625_port_data {
> > +     struct typec_mux_dev *typec_mux;
> > +     struct anx7625_data *ctx;
> > +};
> > +
> >   struct anx7625_data {
> >       struct anx7625_platform_data pdata;
> >       struct platform_device *audio_pdev;
> > @@ -474,6 +479,7 @@ struct anx7625_data {
> >       struct mipi_dsi_device *dsi;
> >       struct drm_dp_aux aux;
> >       int num_typec_switches;
> > +     struct anx7625_port_data *typec_ports;
> >   };
> >
> >   #endif  /* __ANX7625_H__ */
>

Re: [PATCH v2 6/7] drm/bridge: anx7625: Register Type-C mode switches

[Prashant Malani]

On Tue, Jun 14, 2022 at 1:18 AM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 09/06/22 20:09, Prashant Malani ha scritto:
> > When the DT node has "switches" available, register a Type-C mode-switch
> > for each listed "switch". This allows the driver to receive state
> > information about what operating mode a Type-C port and its connected
> > peripherals are in, as well as status information (like VDOs) related to
> > that state.
> >
> > The callback function is currently a stub, but subsequent patches will
> > implement the required functionality.
> >
> > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > ---
> >
> > Changes since v2:
> > - No changes.
> >
> >   drivers/gpu/drm/bridge/analogix/anx7625.c | 73 +++++++++++++++++++++++
> >   drivers/gpu/drm/bridge/analogix/anx7625.h |  6 ++
> >   2 files changed, 79 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > index 07ed44c6b839..d41a21103bd3 100644
> > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > @@ -15,6 +15,7 @@
> >   #include <linux/regulator/consumer.h>
> >   #include <linux/slab.h>
> >   #include <linux/types.h>
> > +#include <linux/usb/typec_mux.h>
> >   #include <linux/workqueue.h>
> >
> >   #include <linux/of_gpio.h>
> > @@ -2581,9 +2582,59 @@ static void anx7625_runtime_disable(void *data)
> >       pm_runtime_disable(data);
> >   }
> >
> > +static int anx7625_typec_mux_set(struct typec_mux_dev *mux,
> > +                              struct typec_mux_state *state)
> > +{
> > +     return 0;
> > +}
> > +
> > +static int anx7625_register_mode_switch(struct device *dev, struct device_node *node,
> > +                                     struct anx7625_data *ctx)
> > +{
> > +     struct anx7625_port_data *port_data;
> > +     struct typec_mux_desc mux_desc = {};
> > +     char name[32];
> > +     u32 port_num;
> > +     int ret;
> > +
> > +     ret = of_property_read_u32(node, "reg", &port_num);
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (port_num >= ctx->num_typec_switches) {
> > +             dev_err(dev, "Invalid port number specified: %d\n", port_num);
> > +             return -EINVAL;
> > +     }
> > +
> > +     port_data = &ctx->typec_ports[port_num];
> > +     port_data->ctx = ctx;
> > +     mux_desc.fwnode = &node->fwnode;
> > +     mux_desc.drvdata = port_data;
> > +     snprintf(name, sizeof(name), "%s-%u", node->name, port_num);
> > +     mux_desc.name = name;
> > +     mux_desc.set = anx7625_typec_mux_set;
> > +
> > +     port_data->typec_mux = typec_mux_register(dev, &mux_desc);
> > +     if (IS_ERR(port_data->typec_mux)) {
> > +             ret = PTR_ERR(port_data->typec_mux);
> > +             dev_err(dev, "Mode switch register for port %d failed: %d", port_num, ret);
> > +     }
>
> Please return 0 at the end of this function.
>
>         if (IS_ERR(....)) {
>                 ......code......
>                 return ret;
>         }
>
>         return 0;
> }

May I ask why? We're not missing any return paths. I would rather we
keep it as is (which has the valid return value).

>
> > +
> > +     return ret;
> > +}
> > +
> > +static void anx7625_unregister_typec_switches(struct anx7625_data *ctx)
> > +{
> > +     int i;
> > +
> > +     for (i = 0; i < ctx->num_typec_switches; i++)
> > +             typec_mux_unregister(ctx->typec_ports[i].typec_mux);
> > +}
> > +
> >   static int anx7625_register_typec_switches(struct device *device, struct anx7625_data *ctx)
> >   {
> >       struct device_node *of = NULL;
> > +     struct device_node *sw;
> >       int ret = 0;
> >
> >       of = of_get_child_by_name(device->of_node, "switches");
> > @@ -2594,6 +2645,26 @@ static int anx7625_register_typec_switches(struct device *device, struct anx7625
> >       if (ctx->num_typec_switches <= 0)
> >               return -ENODEV;
> >
> > +     ctx->typec_ports = devm_kzalloc(device,
> > +                                     ctx->num_typec_switches * sizeof(struct anx7625_port_data),
> > +                                     GFP_KERNEL);
> > +     if (!ctx->typec_ports)
> > +             return -ENOMEM;
> > +
> > +     /* Register switches for each connector. */
> > +     for_each_available_child_of_node(of, sw) {
> > +             if (!of_property_read_bool(sw, "mode-switch"))
> > +                     continue;
> > +             ret = anx7625_register_mode_switch(device, sw, ctx);
> > +             if (ret) {
> > +                     dev_err(device, "Failed to register mode switch: %d\n", ret);
> > +                     break;
> > +             }
> > +     }
> > +
> > +     if (ret)
> > +             anx7625_unregister_typec_switches(ctx);
> > +
> >       return ret;
> >   }
> >
> > @@ -2759,6 +2830,8 @@ static int anx7625_i2c_remove(struct i2c_client *client)
> >
> >       drm_bridge_remove(&platform->bridge);
> >
> > +     anx7625_unregister_typec_switches(platform);
> > +
> >       if (platform->pdata.intp_irq)
> >               destroy_workqueue(platform->workqueue);
> >
> > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h b/drivers/gpu/drm/bridge/analogix/anx7625.h
> > index d5cbca708842..76cfc64f7574 100644
> > --- a/drivers/gpu/drm/bridge/analogix/anx7625.h
> > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.h
> > @@ -443,6 +443,11 @@ struct anx7625_i2c_client {
> >       struct i2c_client *tcpc_client;
> >   };
> >
> > +struct anx7625_port_data {
> > +     struct typec_mux_dev *typec_mux;
> > +     struct anx7625_data *ctx;
> > +};
> > +
> >   struct anx7625_data {
> >       struct anx7625_platform_data pdata;
> >       struct platform_device *audio_pdev;
> > @@ -474,6 +479,7 @@ struct anx7625_data {
> >       struct mipi_dsi_device *dsi;
> >       struct drm_dp_aux aux;
> >       int num_typec_switches;
> > +     struct anx7625_port_data *typec_ports;
> >   };
> >
> >   #endif  /* __ANX7625_H__ */
>

Re: [PATCH v2 7/7] drm/bridge: anx7625: Add typec_mux_set callback function

[Prashant Malani]

On Tue, Jun 14, 2022 at 2:08 AM Pin-yen Lin <treapking@chromium.org> wrote:
>
> Hi AngeloGioacchino,
>
>
> On Tue, Jun 14, 2022 at 4:15 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
> >
> > Il 09/06/22 20:09, Prashant Malani ha scritto:
> > > From: Pin-Yen Lin <treapking@chromium.org>
> > >
> > > Add the callback function when the driver receives state
> > > changes of the Type-C port. The callback function configures the
> > > crosspoint switch of the anx7625 bridge chip, which can change the
> > > output pins of the signals according to the port state.
> > >
> > > Signed-off-by: Pin-Yen Lin <treapking@chromium.org>
> > > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > > ---
> > >
> > > Changes since v2:
> > > - No changes.
> > >
> > >   drivers/gpu/drm/bridge/analogix/anx7625.c | 58 +++++++++++++++++++++++
> > >   drivers/gpu/drm/bridge/analogix/anx7625.h | 13 +++++
> > >   2 files changed, 71 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > > index d41a21103bd3..2c308d12fab2 100644
> > > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > > @@ -15,6 +15,7 @@
> > >   #include <linux/regulator/consumer.h>
> > >   #include <linux/slab.h>
> > >   #include <linux/types.h>
> > > +#include <linux/usb/typec_dp.h>
> > >   #include <linux/usb/typec_mux.h>
> > >   #include <linux/workqueue.h>
> > >
> > > @@ -2582,9 +2583,66 @@ static void anx7625_runtime_disable(void *data)
> > >       pm_runtime_disable(data);
> > >   }
> > >
> > > +static void anx7625_set_crosspoint_switch(struct anx7625_data *ctx,
> > > +                                       enum typec_orientation orientation)
> > > +{
> > > +     if (orientation == TYPEC_ORIENTATION_NORMAL) {
> > > +             anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0,
> > > +                               SW_SEL1_SSRX_RX1 | SW_SEL1_DPTX0_RX2);
> > > +             anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1,
> > > +                               SW_SEL2_SSTX_TX1 | SW_SEL2_DPTX1_TX2);
> > > +     } else if (orientation == TYPEC_ORIENTATION_REVERSE) {
> > > +             anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0,
> > > +                               SW_SEL1_SSRX_RX2 | SW_SEL1_DPTX0_RX1);
> > > +             anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1,
> > > +                               SW_SEL2_SSTX_TX2 | SW_SEL2_DPTX1_TX1);
> > > +     }
> > > +}
> > > +
> > > +static void anx7625_typec_two_ports_update(struct anx7625_data *ctx)
> > > +{
> > > +     if (ctx->typec_ports[0].dp_connected && ctx->typec_ports[1].dp_connected)
> > > +             /* Both ports available, do nothing to retain the current one. */
> > > +             return;
> > > +     else if (ctx->typec_ports[0].dp_connected)
> > > +             anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_NORMAL);
> > > +     else if (ctx->typec_ports[1].dp_connected)
> > > +             anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_REVERSE);
> > > +}
> > > +
> > >   static int anx7625_typec_mux_set(struct typec_mux_dev *mux,
> > >                                struct typec_mux_state *state)
> > >   {
> > > +     struct anx7625_port_data *data = typec_mux_get_drvdata(mux);
> > > +     struct anx7625_data *ctx = data->ctx;
> > > +     struct device *dev = &ctx->client->dev;
> > > +
> > > +     bool old_dp_connected = (ctx->typec_ports[0].dp_connected ||
> > > +                              ctx->typec_ports[1].dp_connected);
> >
> > So the old connection state is "either port0 or port1 are currently connected"...
> >
> > > +     bool new_dp_connected;
> > > +
> > > +     if (ctx->num_typec_switches == 1)
> > > +             return 0;
> > > +
> > > +     dev_dbg(dev, "mux_set dp_connected: c0=%d, c1=%d\n",
> > > +             ctx->typec_ports[0].dp_connected, ctx->typec_ports[1].dp_connected);
> > > +
> > > +     data->dp_connected = (state->alt && state->alt->svid == USB_TYPEC_DP_SID &&
> > > +                           state->alt->mode == USB_TYPEC_DP_MODE);
> > > + > + new_dp_connected = (ctx->typec_ports[0].dp_connected ||
> > > +                         ctx->typec_ports[1].dp_connected);
> >
> > ...and the new connection state is the same as the old one, because I don't see
> > anything that could ever modify it in this function's flow, until reaching this
> > assignment.
>
> The typec mux driver data (`struct anx7625_port_data *data =
> typec_mux_get_drvdata(mux)`) is set to one of the
> `ctx->typec_ports[*]` in `anx7625_register_mode_switch` (see patch 6
> of this series).
>
> So, the `data->dp_connected = ...` assignment may change the new
> connection state.

Angelo, I think your interpretation of this logic is not accurate..
|old_dp_connected| represents *whether* port1 or port0 has a DP
partner connected, not that *either* of them has it.

So, this logic looks OK to me.


>
> Best regards,
> Pin-yen
>
> >
> > > +
> > > +     /* dp on, power on first */
> > > +     if (!old_dp_connected && new_dp_connected)
> > > +             pm_runtime_get_sync(dev);
> >
> > ...so that will never happen...
> >
> > > +
> > > +     anx7625_typec_two_ports_update(ctx);
> > > +
> > > +     /* dp off, power off last */
> > > +     if (old_dp_connected && !new_dp_connected)
> > > +             pm_runtime_put_sync(dev);
> >
> > ...and same here.
> >
> > Regards,
> > Angelo

Re: [PATCH v2 7/7] drm/bridge: anx7625: Add typec_mux_set callback function

[Prashant Malani]

On Tue, Jun 14, 2022 at 2:08 AM Pin-yen Lin <treapking@chromium.org> wrote:
>
> Hi AngeloGioacchino,
>
>
> On Tue, Jun 14, 2022 at 4:15 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
> >
> > Il 09/06/22 20:09, Prashant Malani ha scritto:
> > > From: Pin-Yen Lin <treapking@chromium.org>
> > >
> > > Add the callback function when the driver receives state
> > > changes of the Type-C port. The callback function configures the
> > > crosspoint switch of the anx7625 bridge chip, which can change the
> > > output pins of the signals according to the port state.
> > >
> > > Signed-off-by: Pin-Yen Lin <treapking@chromium.org>
> > > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > > ---
> > >
> > > Changes since v2:
> > > - No changes.
> > >
> > >   drivers/gpu/drm/bridge/analogix/anx7625.c | 58 +++++++++++++++++++++++
> > >   drivers/gpu/drm/bridge/analogix/anx7625.h | 13 +++++
> > >   2 files changed, 71 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > > index d41a21103bd3..2c308d12fab2 100644
> > > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > > @@ -15,6 +15,7 @@
> > >   #include <linux/regulator/consumer.h>
> > >   #include <linux/slab.h>
> > >   #include <linux/types.h>
> > > +#include <linux/usb/typec_dp.h>
> > >   #include <linux/usb/typec_mux.h>
> > >   #include <linux/workqueue.h>
> > >
> > > @@ -2582,9 +2583,66 @@ static void anx7625_runtime_disable(void *data)
> > >       pm_runtime_disable(data);
> > >   }
> > >
> > > +static void anx7625_set_crosspoint_switch(struct anx7625_data *ctx,
> > > +                                       enum typec_orientation orientation)
> > > +{
> > > +     if (orientation == TYPEC_ORIENTATION_NORMAL) {
> > > +             anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0,
> > > +                               SW_SEL1_SSRX_RX1 | SW_SEL1_DPTX0_RX2);
> > > +             anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1,
> > > +                               SW_SEL2_SSTX_TX1 | SW_SEL2_DPTX1_TX2);
> > > +     } else if (orientation == TYPEC_ORIENTATION_REVERSE) {
> > > +             anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0,
> > > +                               SW_SEL1_SSRX_RX2 | SW_SEL1_DPTX0_RX1);
> > > +             anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1,
> > > +                               SW_SEL2_SSTX_TX2 | SW_SEL2_DPTX1_TX1);
> > > +     }
> > > +}
> > > +
> > > +static void anx7625_typec_two_ports_update(struct anx7625_data *ctx)
> > > +{
> > > +     if (ctx->typec_ports[0].dp_connected && ctx->typec_ports[1].dp_connected)
> > > +             /* Both ports available, do nothing to retain the current one. */
> > > +             return;
> > > +     else if (ctx->typec_ports[0].dp_connected)
> > > +             anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_NORMAL);
> > > +     else if (ctx->typec_ports[1].dp_connected)
> > > +             anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_REVERSE);
> > > +}
> > > +
> > >   static int anx7625_typec_mux_set(struct typec_mux_dev *mux,
> > >                                struct typec_mux_state *state)
> > >   {
> > > +     struct anx7625_port_data *data = typec_mux_get_drvdata(mux);
> > > +     struct anx7625_data *ctx = data->ctx;
> > > +     struct device *dev = &ctx->client->dev;
> > > +
> > > +     bool old_dp_connected = (ctx->typec_ports[0].dp_connected ||
> > > +                              ctx->typec_ports[1].dp_connected);
> >
> > So the old connection state is "either port0 or port1 are currently connected"...
> >
> > > +     bool new_dp_connected;
> > > +
> > > +     if (ctx->num_typec_switches == 1)
> > > +             return 0;
> > > +
> > > +     dev_dbg(dev, "mux_set dp_connected: c0=%d, c1=%d\n",
> > > +             ctx->typec_ports[0].dp_connected, ctx->typec_ports[1].dp_connected);
> > > +
> > > +     data->dp_connected = (state->alt && state->alt->svid == USB_TYPEC_DP_SID &&
> > > +                           state->alt->mode == USB_TYPEC_DP_MODE);
> > > + > + new_dp_connected = (ctx->typec_ports[0].dp_connected ||
> > > +                         ctx->typec_ports[1].dp_connected);
> >
> > ...and the new connection state is the same as the old one, because I don't see
> > anything that could ever modify it in this function's flow, until reaching this
> > assignment.
>
> The typec mux driver data (`struct anx7625_port_data *data =
> typec_mux_get_drvdata(mux)`) is set to one of the
> `ctx->typec_ports[*]` in `anx7625_register_mode_switch` (see patch 6
> of this series).
>
> So, the `data->dp_connected = ...` assignment may change the new
> connection state.

Angelo, I think your interpretation of this logic is not accurate..
|old_dp_connected| represents *whether* port1 or port0 has a DP
partner connected, not that *either* of them has it.

So, this logic looks OK to me.


>
> Best regards,
> Pin-yen
>
> >
> > > +
> > > +     /* dp on, power on first */
> > > +     if (!old_dp_connected && new_dp_connected)
> > > +             pm_runtime_get_sync(dev);
> >
> > ...so that will never happen...
> >
> > > +
> > > +     anx7625_typec_two_ports_update(ctx);
> > > +
> > > +     /* dp off, power off last */
> > > +     if (old_dp_connected && !new_dp_connected)
> > > +             pm_runtime_put_sync(dev);
> >
> > ...and same here.
> >
> > Regards,
> > Angelo

Re: [PATCH v2 5/7] drm/bridge: anx7625: Register number of Type C switches

[Prashant Malani]

On Tue, Jun 14, 2022 at 1:22 AM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 09/06/22 20:09, Prashant Malani ha scritto:
> > Parse the "switches" node, if available, and count and store the number
> > of Type-C switches within it. Since we currently don't do anything with
> > this info, no functional changes are expected from this change.
> >
> > This patch sets a foundation for the actual registering of Type-C
> > switches with the Type-C connector class framework.
> >
> > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > ---
> >
> > Changes since v1:
> > - No changes.
> >
> >   drivers/gpu/drm/bridge/analogix/anx7625.c | 20 ++++++++++++++++++++
> >   drivers/gpu/drm/bridge/analogix/anx7625.h |  1 +
> >   2 files changed, 21 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > index 53a5da6c49dd..07ed44c6b839 100644
> > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > @@ -2581,6 +2581,22 @@ static void anx7625_runtime_disable(void *data)
> >       pm_runtime_disable(data);
> >   }
> >
> > +static int anx7625_register_typec_switches(struct device *device, struct anx7625_data *ctx)
> > +{
> > +     struct device_node *of = NULL;
> > +     int ret = 0;
> > +
> > +     of = of_get_child_by_name(device->of_node, "switches");
> > +     if (!of)
> > +             return -ENODEV;
> > +
> > +     ctx->num_typec_switches = of_get_child_count(of);
> > +     if (ctx->num_typec_switches <= 0)
> > +             return -ENODEV;
> > +
> > +     return ret;
>
> You aren't using the `ret` variable for anything other than returning zero:
> remove it and simply return 0 here.
The very next patch does use it, but sure I'll remove it from here and
introduce it in v6.
>
> > +}
> > +
> >   static int anx7625_i2c_probe(struct i2c_client *client,
> >                            const struct i2c_device_id *id)
> >   {
> > @@ -2686,6 +2702,10 @@ static int anx7625_i2c_probe(struct i2c_client *client,
> >       if (platform->pdata.intp_irq)
> >               queue_work(platform->workqueue, &platform->work);
> >
> > +     ret = anx7625_register_typec_switches(dev, platform);
> > +     if (ret)
> > +             dev_info(dev, "Didn't register Type C switches, err: %d\n", ret);
>
> Type-C switches are optional for this driver and this will print a sort of error
> on boards that are *not* declaring any switches on purpose (because perhaps they
> don't have any, or for any other reason).
>
> Even though this is a dev_info and not a dev_err, it's still printing an alarming
> (and useless, in the aforementioned case) message.
I'll go ahead and convert this to dev_warn, but only trigger if there
is an error other than ENODEV.

>
> Please fix this.
>
> Regards,
> Angelo
>

Re: [PATCH v2 5/7] drm/bridge: anx7625: Register number of Type C switches

[Prashant Malani]

On Tue, Jun 14, 2022 at 1:22 AM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 09/06/22 20:09, Prashant Malani ha scritto:
> > Parse the "switches" node, if available, and count and store the number
> > of Type-C switches within it. Since we currently don't do anything with
> > this info, no functional changes are expected from this change.
> >
> > This patch sets a foundation for the actual registering of Type-C
> > switches with the Type-C connector class framework.
> >
> > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > ---
> >
> > Changes since v1:
> > - No changes.
> >
> >   drivers/gpu/drm/bridge/analogix/anx7625.c | 20 ++++++++++++++++++++
> >   drivers/gpu/drm/bridge/analogix/anx7625.h |  1 +
> >   2 files changed, 21 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > index 53a5da6c49dd..07ed44c6b839 100644
> > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > @@ -2581,6 +2581,22 @@ static void anx7625_runtime_disable(void *data)
> >       pm_runtime_disable(data);
> >   }
> >
> > +static int anx7625_register_typec_switches(struct device *device, struct anx7625_data *ctx)
> > +{
> > +     struct device_node *of = NULL;
> > +     int ret = 0;
> > +
> > +     of = of_get_child_by_name(device->of_node, "switches");
> > +     if (!of)
> > +             return -ENODEV;
> > +
> > +     ctx->num_typec_switches = of_get_child_count(of);
> > +     if (ctx->num_typec_switches <= 0)
> > +             return -ENODEV;
> > +
> > +     return ret;
>
> You aren't using the `ret` variable for anything other than returning zero:
> remove it and simply return 0 here.
The very next patch does use it, but sure I'll remove it from here and
introduce it in v6.
>
> > +}
> > +
> >   static int anx7625_i2c_probe(struct i2c_client *client,
> >                            const struct i2c_device_id *id)
> >   {
> > @@ -2686,6 +2702,10 @@ static int anx7625_i2c_probe(struct i2c_client *client,
> >       if (platform->pdata.intp_irq)
> >               queue_work(platform->workqueue, &platform->work);
> >
> > +     ret = anx7625_register_typec_switches(dev, platform);
> > +     if (ret)
> > +             dev_info(dev, "Didn't register Type C switches, err: %d\n", ret);
>
> Type-C switches are optional for this driver and this will print a sort of error
> on boards that are *not* declaring any switches on purpose (because perhaps they
> don't have any, or for any other reason).
>
> Even though this is a dev_info and not a dev_err, it's still printing an alarming
> (and useless, in the aforementioned case) message.
I'll go ahead and convert this to dev_warn, but only trigger if there
is an error other than ENODEV.

>
> Please fix this.
>
> Regards,
> Angelo
>

Re: [PATCH v2 6/7] drm/bridge: anx7625: Register Type-C mode switches

[AngeloGioacchino Del Regno]

Il 14/06/22 18:57, Prashant Malani ha scritto:
> On Tue, Jun 14, 2022 at 1:18 AM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Il 09/06/22 20:09, Prashant Malani ha scritto:
>>> When the DT node has "switches" available, register a Type-C mode-switch
>>> for each listed "switch". This allows the driver to receive state
>>> information about what operating mode a Type-C port and its connected
>>> peripherals are in, as well as status information (like VDOs) related to
>>> that state.
>>>
>>> The callback function is currently a stub, but subsequent patches will
>>> implement the required functionality.
>>>
>>> Signed-off-by: Prashant Malani <pmalani@chromium.org>
>>> ---
>>>
>>> Changes since v2:
>>> - No changes.
>>>
>>>    drivers/gpu/drm/bridge/analogix/anx7625.c | 73 +++++++++++++++++++++++
>>>    drivers/gpu/drm/bridge/analogix/anx7625.h |  6 ++
>>>    2 files changed, 79 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
>>> index 07ed44c6b839..d41a21103bd3 100644
>>> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
>>> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
>>> @@ -15,6 +15,7 @@
>>>    #include <linux/regulator/consumer.h>
>>>    #include <linux/slab.h>
>>>    #include <linux/types.h>
>>> +#include <linux/usb/typec_mux.h>
>>>    #include <linux/workqueue.h>
>>>
>>>    #include <linux/of_gpio.h>
>>> @@ -2581,9 +2582,59 @@ static void anx7625_runtime_disable(void *data)
>>>        pm_runtime_disable(data);
>>>    }
>>>
>>> +static int anx7625_typec_mux_set(struct typec_mux_dev *mux,
>>> +                              struct typec_mux_state *state)
>>> +{
>>> +     return 0;
>>> +}
>>> +
>>> +static int anx7625_register_mode_switch(struct device *dev, struct device_node *node,
>>> +                                     struct anx7625_data *ctx)
>>> +{
>>> +     struct anx7625_port_data *port_data;
>>> +     struct typec_mux_desc mux_desc = {};
>>> +     char name[32];
>>> +     u32 port_num;
>>> +     int ret;
>>> +
>>> +     ret = of_property_read_u32(node, "reg", &port_num);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     if (port_num >= ctx->num_typec_switches) {
>>> +             dev_err(dev, "Invalid port number specified: %d\n", port_num);
>>> +             return -EINVAL;
>>> +     }
>>> +
>>> +     port_data = &ctx->typec_ports[port_num];
>>> +     port_data->ctx = ctx;
>>> +     mux_desc.fwnode = &node->fwnode;
>>> +     mux_desc.drvdata = port_data;
>>> +     snprintf(name, sizeof(name), "%s-%u", node->name, port_num);
>>> +     mux_desc.name = name;
>>> +     mux_desc.set = anx7625_typec_mux_set;
>>> +
>>> +     port_data->typec_mux = typec_mux_register(dev, &mux_desc);
>>> +     if (IS_ERR(port_data->typec_mux)) {
>>> +             ret = PTR_ERR(port_data->typec_mux);
>>> +             dev_err(dev, "Mode switch register for port %d failed: %d", port_num, ret);
>>> +     }
>>
>> Please return 0 at the end of this function.
>>
>>          if (IS_ERR(....)) {
>>                  ......code......
>>                  return ret;
>>          }
>>
>>          return 0;
>> }
> 
> May I ask why? We're not missing any return paths. I would rather we
> keep it as is (which has the valid return value).
> 

I know that you're not missing any return paths.

That's only because the proposed one is a common pattern in the kernel
and it's only for consistency.

Regards,
Angelo

Re: [PATCH v2 6/7] drm/bridge: anx7625: Register Type-C mode switches

[AngeloGioacchino Del Regno]

Il 14/06/22 18:57, Prashant Malani ha scritto:
> On Tue, Jun 14, 2022 at 1:18 AM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Il 09/06/22 20:09, Prashant Malani ha scritto:
>>> When the DT node has "switches" available, register a Type-C mode-switch
>>> for each listed "switch". This allows the driver to receive state
>>> information about what operating mode a Type-C port and its connected
>>> peripherals are in, as well as status information (like VDOs) related to
>>> that state.
>>>
>>> The callback function is currently a stub, but subsequent patches will
>>> implement the required functionality.
>>>
>>> Signed-off-by: Prashant Malani <pmalani@chromium.org>
>>> ---
>>>
>>> Changes since v2:
>>> - No changes.
>>>
>>>    drivers/gpu/drm/bridge/analogix/anx7625.c | 73 +++++++++++++++++++++++
>>>    drivers/gpu/drm/bridge/analogix/anx7625.h |  6 ++
>>>    2 files changed, 79 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
>>> index 07ed44c6b839..d41a21103bd3 100644
>>> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
>>> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
>>> @@ -15,6 +15,7 @@
>>>    #include <linux/regulator/consumer.h>
>>>    #include <linux/slab.h>
>>>    #include <linux/types.h>
>>> +#include <linux/usb/typec_mux.h>
>>>    #include <linux/workqueue.h>
>>>
>>>    #include <linux/of_gpio.h>
>>> @@ -2581,9 +2582,59 @@ static void anx7625_runtime_disable(void *data)
>>>        pm_runtime_disable(data);
>>>    }
>>>
>>> +static int anx7625_typec_mux_set(struct typec_mux_dev *mux,
>>> +                              struct typec_mux_state *state)
>>> +{
>>> +     return 0;
>>> +}
>>> +
>>> +static int anx7625_register_mode_switch(struct device *dev, struct device_node *node,
>>> +                                     struct anx7625_data *ctx)
>>> +{
>>> +     struct anx7625_port_data *port_data;
>>> +     struct typec_mux_desc mux_desc = {};
>>> +     char name[32];
>>> +     u32 port_num;
>>> +     int ret;
>>> +
>>> +     ret = of_property_read_u32(node, "reg", &port_num);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     if (port_num >= ctx->num_typec_switches) {
>>> +             dev_err(dev, "Invalid port number specified: %d\n", port_num);
>>> +             return -EINVAL;
>>> +     }
>>> +
>>> +     port_data = &ctx->typec_ports[port_num];
>>> +     port_data->ctx = ctx;
>>> +     mux_desc.fwnode = &node->fwnode;
>>> +     mux_desc.drvdata = port_data;
>>> +     snprintf(name, sizeof(name), "%s-%u", node->name, port_num);
>>> +     mux_desc.name = name;
>>> +     mux_desc.set = anx7625_typec_mux_set;
>>> +
>>> +     port_data->typec_mux = typec_mux_register(dev, &mux_desc);
>>> +     if (IS_ERR(port_data->typec_mux)) {
>>> +             ret = PTR_ERR(port_data->typec_mux);
>>> +             dev_err(dev, "Mode switch register for port %d failed: %d", port_num, ret);
>>> +     }
>>
>> Please return 0 at the end of this function.
>>
>>          if (IS_ERR(....)) {
>>                  ......code......
>>                  return ret;
>>          }
>>
>>          return 0;
>> }
> 
> May I ask why? We're not missing any return paths. I would rather we
> keep it as is (which has the valid return value).
> 

I know that you're not missing any return paths.

That's only because the proposed one is a common pattern in the kernel
and it's only for consistency.

Regards,
Angelo

Re: [PATCH v2 7/7] drm/bridge: anx7625: Add typec_mux_set callback function

[AngeloGioacchino Del Regno]

Il 14/06/22 18:58, Prashant Malani ha scritto:
> On Tue, Jun 14, 2022 at 2:08 AM Pin-yen Lin <treapking@chromium.org> wrote:
>>
>> Hi AngeloGioacchino,
>>
>>
>> On Tue, Jun 14, 2022 at 4:15 PM AngeloGioacchino Del Regno
>> <angelogioacchino.delregno@collabora.com> wrote:
>>>
>>> Il 09/06/22 20:09, Prashant Malani ha scritto:
>>>> From: Pin-Yen Lin <treapking@chromium.org>
>>>>
>>>> Add the callback function when the driver receives state
>>>> changes of the Type-C port. The callback function configures the
>>>> crosspoint switch of the anx7625 bridge chip, which can change the
>>>> output pins of the signals according to the port state.
>>>>
>>>> Signed-off-by: Pin-Yen Lin <treapking@chromium.org>
>>>> Signed-off-by: Prashant Malani <pmalani@chromium.org>
>>>> ---
>>>>
>>>> Changes since v2:
>>>> - No changes.
>>>>
>>>>    drivers/gpu/drm/bridge/analogix/anx7625.c | 58 +++++++++++++++++++++++
>>>>    drivers/gpu/drm/bridge/analogix/anx7625.h | 13 +++++
>>>>    2 files changed, 71 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
>>>> index d41a21103bd3..2c308d12fab2 100644
>>>> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
>>>> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
>>>> @@ -15,6 +15,7 @@
>>>>    #include <linux/regulator/consumer.h>
>>>>    #include <linux/slab.h>
>>>>    #include <linux/types.h>
>>>> +#include <linux/usb/typec_dp.h>
>>>>    #include <linux/usb/typec_mux.h>
>>>>    #include <linux/workqueue.h>
>>>>
>>>> @@ -2582,9 +2583,66 @@ static void anx7625_runtime_disable(void *data)
>>>>        pm_runtime_disable(data);
>>>>    }
>>>>
>>>> +static void anx7625_set_crosspoint_switch(struct anx7625_data *ctx,
>>>> +                                       enum typec_orientation orientation)
>>>> +{
>>>> +     if (orientation == TYPEC_ORIENTATION_NORMAL) {
>>>> +             anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0,
>>>> +                               SW_SEL1_SSRX_RX1 | SW_SEL1_DPTX0_RX2);
>>>> +             anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1,
>>>> +                               SW_SEL2_SSTX_TX1 | SW_SEL2_DPTX1_TX2);
>>>> +     } else if (orientation == TYPEC_ORIENTATION_REVERSE) {
>>>> +             anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0,
>>>> +                               SW_SEL1_SSRX_RX2 | SW_SEL1_DPTX0_RX1);
>>>> +             anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1,
>>>> +                               SW_SEL2_SSTX_TX2 | SW_SEL2_DPTX1_TX1);
>>>> +     }
>>>> +}
>>>> +
>>>> +static void anx7625_typec_two_ports_update(struct anx7625_data *ctx)
>>>> +{
>>>> +     if (ctx->typec_ports[0].dp_connected && ctx->typec_ports[1].dp_connected)
>>>> +             /* Both ports available, do nothing to retain the current one. */
>>>> +             return;
>>>> +     else if (ctx->typec_ports[0].dp_connected)
>>>> +             anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_NORMAL);
>>>> +     else if (ctx->typec_ports[1].dp_connected)
>>>> +             anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_REVERSE);
>>>> +}
>>>> +
>>>>    static int anx7625_typec_mux_set(struct typec_mux_dev *mux,
>>>>                                 struct typec_mux_state *state)
>>>>    {
>>>> +     struct anx7625_port_data *data = typec_mux_get_drvdata(mux);
>>>> +     struct anx7625_data *ctx = data->ctx;
>>>> +     struct device *dev = &ctx->client->dev;
>>>> +
>>>> +     bool old_dp_connected = (ctx->typec_ports[0].dp_connected ||
>>>> +                              ctx->typec_ports[1].dp_connected);
>>>
>>> So the old connection state is "either port0 or port1 are currently connected"...
>>>
>>>> +     bool new_dp_connected;
>>>> +
>>>> +     if (ctx->num_typec_switches == 1)
>>>> +             return 0;
>>>> +
>>>> +     dev_dbg(dev, "mux_set dp_connected: c0=%d, c1=%d\n",
>>>> +             ctx->typec_ports[0].dp_connected, ctx->typec_ports[1].dp_connected);
>>>> +
>>>> +     data->dp_connected = (state->alt && state->alt->svid == USB_TYPEC_DP_SID &&
>>>> +                           state->alt->mode == USB_TYPEC_DP_MODE);
>>>> + > + new_dp_connected = (ctx->typec_ports[0].dp_connected ||
>>>> +                         ctx->typec_ports[1].dp_connected);
>>>
>>> ...and the new connection state is the same as the old one, because I don't see
>>> anything that could ever modify it in this function's flow, until reaching this
>>> assignment.
>>
>> The typec mux driver data (`struct anx7625_port_data *data =
>> typec_mux_get_drvdata(mux)`) is set to one of the
>> `ctx->typec_ports[*]` in `anx7625_register_mode_switch` (see patch 6
>> of this series).
>>
>> So, the `data->dp_connected = ...` assignment may change the new
>> connection state.
> 
> Angelo, I think your interpretation of this logic is not accurate..
> |old_dp_connected| represents *whether* port1 or port0 has a DP
> partner connected, not that *either* of them has it.
> 
> So, this logic looks OK to me.
> 

Hello Prashant,

You're completely right: I've finally seen where this is happening, so yes
we don't know, nor care at that moment, whether data->dp_connected is port0
or port1, we assign and check 'em both again, which is actually smart.

I'm sorry for the misunderstandment - and thank you for your reply.

Feel free to add my

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Regards,
Angelo

Re: [PATCH v2 7/7] drm/bridge: anx7625: Add typec_mux_set callback function

[AngeloGioacchino Del Regno]

Il 14/06/22 18:58, Prashant Malani ha scritto:
> On Tue, Jun 14, 2022 at 2:08 AM Pin-yen Lin <treapking@chromium.org> wrote:
>>
>> Hi AngeloGioacchino,
>>
>>
>> On Tue, Jun 14, 2022 at 4:15 PM AngeloGioacchino Del Regno
>> <angelogioacchino.delregno@collabora.com> wrote:
>>>
>>> Il 09/06/22 20:09, Prashant Malani ha scritto:
>>>> From: Pin-Yen Lin <treapking@chromium.org>
>>>>
>>>> Add the callback function when the driver receives state
>>>> changes of the Type-C port. The callback function configures the
>>>> crosspoint switch of the anx7625 bridge chip, which can change the
>>>> output pins of the signals according to the port state.
>>>>
>>>> Signed-off-by: Pin-Yen Lin <treapking@chromium.org>
>>>> Signed-off-by: Prashant Malani <pmalani@chromium.org>
>>>> ---
>>>>
>>>> Changes since v2:
>>>> - No changes.
>>>>
>>>>    drivers/gpu/drm/bridge/analogix/anx7625.c | 58 +++++++++++++++++++++++
>>>>    drivers/gpu/drm/bridge/analogix/anx7625.h | 13 +++++
>>>>    2 files changed, 71 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
>>>> index d41a21103bd3..2c308d12fab2 100644
>>>> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
>>>> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
>>>> @@ -15,6 +15,7 @@
>>>>    #include <linux/regulator/consumer.h>
>>>>    #include <linux/slab.h>
>>>>    #include <linux/types.h>
>>>> +#include <linux/usb/typec_dp.h>
>>>>    #include <linux/usb/typec_mux.h>
>>>>    #include <linux/workqueue.h>
>>>>
>>>> @@ -2582,9 +2583,66 @@ static void anx7625_runtime_disable(void *data)
>>>>        pm_runtime_disable(data);
>>>>    }
>>>>
>>>> +static void anx7625_set_crosspoint_switch(struct anx7625_data *ctx,
>>>> +                                       enum typec_orientation orientation)
>>>> +{
>>>> +     if (orientation == TYPEC_ORIENTATION_NORMAL) {
>>>> +             anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0,
>>>> +                               SW_SEL1_SSRX_RX1 | SW_SEL1_DPTX0_RX2);
>>>> +             anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1,
>>>> +                               SW_SEL2_SSTX_TX1 | SW_SEL2_DPTX1_TX2);
>>>> +     } else if (orientation == TYPEC_ORIENTATION_REVERSE) {
>>>> +             anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_0,
>>>> +                               SW_SEL1_SSRX_RX2 | SW_SEL1_DPTX0_RX1);
>>>> +             anx7625_reg_write(ctx, ctx->i2c.tcpc_client, TCPC_SWITCH_1,
>>>> +                               SW_SEL2_SSTX_TX2 | SW_SEL2_DPTX1_TX1);
>>>> +     }
>>>> +}
>>>> +
>>>> +static void anx7625_typec_two_ports_update(struct anx7625_data *ctx)
>>>> +{
>>>> +     if (ctx->typec_ports[0].dp_connected && ctx->typec_ports[1].dp_connected)
>>>> +             /* Both ports available, do nothing to retain the current one. */
>>>> +             return;
>>>> +     else if (ctx->typec_ports[0].dp_connected)
>>>> +             anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_NORMAL);
>>>> +     else if (ctx->typec_ports[1].dp_connected)
>>>> +             anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_REVERSE);
>>>> +}
>>>> +
>>>>    static int anx7625_typec_mux_set(struct typec_mux_dev *mux,
>>>>                                 struct typec_mux_state *state)
>>>>    {
>>>> +     struct anx7625_port_data *data = typec_mux_get_drvdata(mux);
>>>> +     struct anx7625_data *ctx = data->ctx;
>>>> +     struct device *dev = &ctx->client->dev;
>>>> +
>>>> +     bool old_dp_connected = (ctx->typec_ports[0].dp_connected ||
>>>> +                              ctx->typec_ports[1].dp_connected);
>>>
>>> So the old connection state is "either port0 or port1 are currently connected"...
>>>
>>>> +     bool new_dp_connected;
>>>> +
>>>> +     if (ctx->num_typec_switches == 1)
>>>> +             return 0;
>>>> +
>>>> +     dev_dbg(dev, "mux_set dp_connected: c0=%d, c1=%d\n",
>>>> +             ctx->typec_ports[0].dp_connected, ctx->typec_ports[1].dp_connected);
>>>> +
>>>> +     data->dp_connected = (state->alt && state->alt->svid == USB_TYPEC_DP_SID &&
>>>> +                           state->alt->mode == USB_TYPEC_DP_MODE);
>>>> + > + new_dp_connected = (ctx->typec_ports[0].dp_connected ||
>>>> +                         ctx->typec_ports[1].dp_connected);
>>>
>>> ...and the new connection state is the same as the old one, because I don't see
>>> anything that could ever modify it in this function's flow, until reaching this
>>> assignment.
>>
>> The typec mux driver data (`struct anx7625_port_data *data =
>> typec_mux_get_drvdata(mux)`) is set to one of the
>> `ctx->typec_ports[*]` in `anx7625_register_mode_switch` (see patch 6
>> of this series).
>>
>> So, the `data->dp_connected = ...` assignment may change the new
>> connection state.
> 
> Angelo, I think your interpretation of this logic is not accurate..
> |old_dp_connected| represents *whether* port1 or port0 has a DP
> partner connected, not that *either* of them has it.
> 
> So, this logic looks OK to me.
> 

Hello Prashant,

You're completely right: I've finally seen where this is happening, so yes
we don't know, nor care at that moment, whether data->dp_connected is port0
or port1, we assign and check 'em both again, which is actually smart.

I'm sorry for the misunderstandment - and thank you for your reply.

Feel free to add my

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Regards,
Angelo

Re: [PATCH v2 6/7] drm/bridge: anx7625: Register Type-C mode switches

[Prashant Malani]

On Wed, Jun 15, 2022 at 1:45 AM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 14/06/22 18:57, Prashant Malani ha scritto:
> > On Tue, Jun 14, 2022 at 1:18 AM AngeloGioacchino Del Regno
> > <angelogioacchino.delregno@collabora.com> wrote:
> >>
> >> Il 09/06/22 20:09, Prashant Malani ha scritto:
> >>> When the DT node has "switches" available, register a Type-C mode-switch
> >>> for each listed "switch". This allows the driver to receive state
> >>> information about what operating mode a Type-C port and its connected
> >>> peripherals are in, as well as status information (like VDOs) related to
> >>> that state.
> >>>
> >>> The callback function is currently a stub, but subsequent patches will
> >>> implement the required functionality.
> >>>
> >>> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> >>> ---
> >>>
> >>> Changes since v2:
> >>> - No changes.
> >>>
> >>>    drivers/gpu/drm/bridge/analogix/anx7625.c | 73 +++++++++++++++++++++++
> >>>    drivers/gpu/drm/bridge/analogix/anx7625.h |  6 ++
> >>>    2 files changed, 79 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> >>> index 07ed44c6b839..d41a21103bd3 100644
> >>> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> >>> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> >>> @@ -15,6 +15,7 @@
> >>>    #include <linux/regulator/consumer.h>
> >>>    #include <linux/slab.h>
> >>>    #include <linux/types.h>
> >>> +#include <linux/usb/typec_mux.h>
> >>>    #include <linux/workqueue.h>
> >>>
> >>>    #include <linux/of_gpio.h>
> >>> @@ -2581,9 +2582,59 @@ static void anx7625_runtime_disable(void *data)
> >>>        pm_runtime_disable(data);
> >>>    }
> >>>
> >>> +static int anx7625_typec_mux_set(struct typec_mux_dev *mux,
> >>> +                              struct typec_mux_state *state)
> >>> +{
> >>> +     return 0;
> >>> +}
> >>> +
> >>> +static int anx7625_register_mode_switch(struct device *dev, struct device_node *node,
> >>> +                                     struct anx7625_data *ctx)
> >>> +{
> >>> +     struct anx7625_port_data *port_data;
> >>> +     struct typec_mux_desc mux_desc = {};
> >>> +     char name[32];
> >>> +     u32 port_num;
> >>> +     int ret;
> >>> +
> >>> +     ret = of_property_read_u32(node, "reg", &port_num);
> >>> +     if (ret)
> >>> +             return ret;
> >>> +
> >>> +     if (port_num >= ctx->num_typec_switches) {
> >>> +             dev_err(dev, "Invalid port number specified: %d\n", port_num);
> >>> +             return -EINVAL;
> >>> +     }
> >>> +
> >>> +     port_data = &ctx->typec_ports[port_num];
> >>> +     port_data->ctx = ctx;
> >>> +     mux_desc.fwnode = &node->fwnode;
> >>> +     mux_desc.drvdata = port_data;
> >>> +     snprintf(name, sizeof(name), "%s-%u", node->name, port_num);
> >>> +     mux_desc.name = name;
> >>> +     mux_desc.set = anx7625_typec_mux_set;
> >>> +
> >>> +     port_data->typec_mux = typec_mux_register(dev, &mux_desc);
> >>> +     if (IS_ERR(port_data->typec_mux)) {
> >>> +             ret = PTR_ERR(port_data->typec_mux);
> >>> +             dev_err(dev, "Mode switch register for port %d failed: %d", port_num, ret);
> >>> +     }
> >>
> >> Please return 0 at the end of this function.
> >>
> >>          if (IS_ERR(....)) {
> >>                  ......code......
> >>                  return ret;
> >>          }
> >>
> >>          return 0;
> >> }
> >
> > May I ask why? We're not missing any return paths. I would rather we
> > keep it as is (which has the valid return value).
> >
>
> I know that you're not missing any return paths.
>
> That's only because the proposed one is a common pattern in the kernel
> and it's only for consistency.

Thanks for the additional details. Since this isn't addressing any
specific bug, and I
notice varied usages of "return ret" in this file itself [1][2], I'd
prefer keeping it as is.

[1]: https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/analogix/anx7625.c#L296
[2]: https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/analogix/anx7625.c#L436

>
> Regards,
> Angelo
>

Re: [PATCH v2 6/7] drm/bridge: anx7625: Register Type-C mode switches

[Prashant Malani]

On Wed, Jun 15, 2022 at 1:45 AM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 14/06/22 18:57, Prashant Malani ha scritto:
> > On Tue, Jun 14, 2022 at 1:18 AM AngeloGioacchino Del Regno
> > <angelogioacchino.delregno@collabora.com> wrote:
> >>
> >> Il 09/06/22 20:09, Prashant Malani ha scritto:
> >>> When the DT node has "switches" available, register a Type-C mode-switch
> >>> for each listed "switch". This allows the driver to receive state
> >>> information about what operating mode a Type-C port and its connected
> >>> peripherals are in, as well as status information (like VDOs) related to
> >>> that state.
> >>>
> >>> The callback function is currently a stub, but subsequent patches will
> >>> implement the required functionality.
> >>>
> >>> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> >>> ---
> >>>
> >>> Changes since v2:
> >>> - No changes.
> >>>
> >>>    drivers/gpu/drm/bridge/analogix/anx7625.c | 73 +++++++++++++++++++++++
> >>>    drivers/gpu/drm/bridge/analogix/anx7625.h |  6 ++
> >>>    2 files changed, 79 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> >>> index 07ed44c6b839..d41a21103bd3 100644
> >>> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> >>> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> >>> @@ -15,6 +15,7 @@
> >>>    #include <linux/regulator/consumer.h>
> >>>    #include <linux/slab.h>
> >>>    #include <linux/types.h>
> >>> +#include <linux/usb/typec_mux.h>
> >>>    #include <linux/workqueue.h>
> >>>
> >>>    #include <linux/of_gpio.h>
> >>> @@ -2581,9 +2582,59 @@ static void anx7625_runtime_disable(void *data)
> >>>        pm_runtime_disable(data);
> >>>    }
> >>>
> >>> +static int anx7625_typec_mux_set(struct typec_mux_dev *mux,
> >>> +                              struct typec_mux_state *state)
> >>> +{
> >>> +     return 0;
> >>> +}
> >>> +
> >>> +static int anx7625_register_mode_switch(struct device *dev, struct device_node *node,
> >>> +                                     struct anx7625_data *ctx)
> >>> +{
> >>> +     struct anx7625_port_data *port_data;
> >>> +     struct typec_mux_desc mux_desc = {};
> >>> +     char name[32];
> >>> +     u32 port_num;
> >>> +     int ret;
> >>> +
> >>> +     ret = of_property_read_u32(node, "reg", &port_num);
> >>> +     if (ret)
> >>> +             return ret;
> >>> +
> >>> +     if (port_num >= ctx->num_typec_switches) {
> >>> +             dev_err(dev, "Invalid port number specified: %d\n", port_num);
> >>> +             return -EINVAL;
> >>> +     }
> >>> +
> >>> +     port_data = &ctx->typec_ports[port_num];
> >>> +     port_data->ctx = ctx;
> >>> +     mux_desc.fwnode = &node->fwnode;
> >>> +     mux_desc.drvdata = port_data;
> >>> +     snprintf(name, sizeof(name), "%s-%u", node->name, port_num);
> >>> +     mux_desc.name = name;
> >>> +     mux_desc.set = anx7625_typec_mux_set;
> >>> +
> >>> +     port_data->typec_mux = typec_mux_register(dev, &mux_desc);
> >>> +     if (IS_ERR(port_data->typec_mux)) {
> >>> +             ret = PTR_ERR(port_data->typec_mux);
> >>> +             dev_err(dev, "Mode switch register for port %d failed: %d", port_num, ret);
> >>> +     }
> >>
> >> Please return 0 at the end of this function.
> >>
> >>          if (IS_ERR(....)) {
> >>                  ......code......
> >>                  return ret;
> >>          }
> >>
> >>          return 0;
> >> }
> >
> > May I ask why? We're not missing any return paths. I would rather we
> > keep it as is (which has the valid return value).
> >
>
> I know that you're not missing any return paths.
>
> That's only because the proposed one is a common pattern in the kernel
> and it's only for consistency.

Thanks for the additional details. Since this isn't addressing any
specific bug, and I
notice varied usages of "return ret" in this file itself [1][2], I'd
prefer keeping it as is.

[1]: https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/analogix/anx7625.c#L296
[2]: https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/analogix/anx7625.c#L436

>
> Regards,
> Angelo
>