This series introduces bindings for anx7625/it6505 to register Type-C
mode-switch in their output endpoints, and use data-lanes property to
describe the pin connections.
This series is not directly related to the built-in mux in anx7625,
which automatically switches between the two orientations of a single
Type-C connector. This series adds support of registering mode switches
for two downstream devices, while we use orientation switches for two
orientations of the Type-C connector.
The first two patch modifies fwnode_graph_devcon_matches and
cros_typec_init_ports to enable the registration of the switches.
Patch 4~6 introduce the bindings for anx7625 and the corresponding driver
modifications.
Patch 7~9 add similar bindings and driver changes for it6505.
v10: https://lore.kernel.org/all/[email protected]/
v9: https://lore.kernel.org/all/[email protected]/
v8: https://lore.kernel.org/all/[email protected]/
v7: https://lore.kernel.org/all/[email protected]/
v6: https://lore.kernel.org/all/[email protected]/
v5: https://lore.kernel.org/linux-usb/[email protected]/
Changes in v11:
- Added missing fwnode_handle_put in drivers/base/property.c
- Collected Acked-by tag
- Use fwnode helpers instead of DT
- Moved the helpers to a new file
- Use "reg" instead of "data-lanes" to determine the port number
- Updated the description of the endpoints in the bindings
- Referenced video-interfaces.yaml instead for the endpoints binding
- Removed duplicated definitions from inherited schema
- Moved the "data-lanes" parsing logics to bridge drivers
- Removed Kconfig dependencies for the bridge drivers
- Updated the usage of the private bridge driver data
- Added a clarification on the anx7625 built-in mux in the cover letter
Changes in v10:
- Collected Reviewed-by and Tested-by tags
- Replaced "void *" with "typec_mux_set_fn_t" for mux_set callbacks
- Print out the node name when errors on parsing DT
- Use dev_dbg instead of dev_warn when no Type-C switch nodes available
- Made the return path of drm_dp_register_mode_switch clearer
- Added a TODO for implementing orientation switch for anx7625
- Updated the commit message for the absence of orientation switch
- Fixed typo in the commit message
Changes in v9:
- Collected Reviewed-by tag
- Fixed subject prefix again
- Changed the naming of the example node for it6505
Changes in v8:
- Fixed the build issue when CONFIG_TYPEC=m
- Fixed some style issues
- Fixed the subject prefixes for the bindings patch
- Fixed the bindings for data-lanes properties
Changes in v7:
- Fix the long comment lines
- Extracted the common codes to a helper function
- Fixed style issues in anx7625 driver
- Removed DT property validation in anx7625 driver.
- Fixed style issues in it6505 driver
- Removed the redundant sleep in it6505 driver
- Removed DT property validation in it6505 driver
- Rebased to drm-misc-next
- Fixed indentations in bindings patches
- Added a new patch to fix indentations in Kconfig
Changes in v6:
- Changed it6505_typec_mux_set callback function to accommodate with
the latest drm-misc patches
- Changed the driver implementation to accommodate with the new binding
- Dropped typec-switch binding and use endpoints and data-lanes properties
to describe the pin connections
- Added new patches (patch 1,2,4) to fix probing issues
- Changed the bindings of it6505/anx7625 and modified the drivers
accordingly
- Merged it6505/anx7625 driver changes into a single patch
Pin-yen Lin (7):
drm/display: Add Type-C switch helpers
dt-bindings: display: bridge: anx7625: Add mode-switch support
drm/bridge: anx7625: Check for Type-C during panel registration
drm/bridge: anx7625: Register Type C mode switches
dt-bindings: display: bridge: it6505: Add mode-switch support
drm/bridge: it6505: Fix Kconfig indentation
drm/bridge: it6505: Register Type C mode switches
Prashant Malani (2):
device property: Add remote endpoint to devcon matcher
platform/chrome: cros_ec_typec: Purge blocking switch devlinks
.../display/bridge/analogix,anx7625.yaml | 94 ++++++++-
.../bindings/display/bridge/ite,it6505.yaml | 101 ++++++++--
drivers/base/property.c | 16 ++
drivers/gpu/drm/bridge/Kconfig | 20 +-
drivers/gpu/drm/bridge/analogix/anx7625.c | 162 +++++++++++++++-
drivers/gpu/drm/bridge/analogix/anx7625.h | 20 ++
drivers/gpu/drm/bridge/ite-it6505.c | 179 +++++++++++++++++-
drivers/gpu/drm/display/Makefile | 1 +
drivers/gpu/drm/display/drm_dp_typec_helper.c | 114 +++++++++++
drivers/platform/chrome/cros_ec_typec.c | 10 +
include/drm/display/drm_dp_helper.h | 31 +++
11 files changed, 717 insertions(+), 31 deletions(-)
create mode 100644 drivers/gpu/drm/display/drm_dp_typec_helper.c
--
2.39.1.519.gcb327c4b5f-goog
From: Prashant Malani <[email protected]>
When searching the device graph for device matches, check the
remote-endpoint itself for a match.
Some drivers register devices for individual endpoints. This allows
the matcher code to evaluate those for a match too, instead
of only looking at the remote parent devices. This is required when a
device supports two mode switches in its endpoints, so we can't simply
register the mode switch with the parent node.
Signed-off-by: Prashant Malani <[email protected]>
Signed-off-by: Pin-yen Lin <[email protected]>
Reviewed-by: Chen-Yu Tsai <[email protected]>
Tested-by: Chen-Yu Tsai <[email protected]>
---
Changes in v11:
- Added missing fwnode_handle_put in drivers/base/property.c
Changes in v10:
- Collected Reviewed-by and Tested-by tags
Changes in v6:
- New in v6
drivers/base/property.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/base/property.c b/drivers/base/property.c
index 2a5a37fcd998..e6f915b72eb7 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -1223,6 +1223,22 @@ static unsigned int fwnode_graph_devcon_matches(struct fwnode_handle *fwnode,
break;
}
+ /*
+ * Some drivers may register devices for endpoints. Check
+ * the remote-endpoints for matches in addition to the remote
+ * port parent.
+ */
+ node = fwnode_graph_get_remote_endpoint(ep);
+ if (fwnode_device_is_available(node)) {
+ ret = match(node, con_id, data);
+ if (ret) {
+ if (matches)
+ matches[count] = ret;
+ count++;
+ }
+ }
+ fwnode_handle_put(node);
+
node = fwnode_graph_get_remote_port_parent(ep);
if (!fwnode_device_is_available(node)) {
fwnode_handle_put(node);
--
2.39.1.519.gcb327c4b5f-goog
From: Prashant Malani <[email protected]>
When using OF graph, the fw_devlink code will create links between the
individual port driver (cros-ec-typec here) and the parent device for
a Type-C switch (like mode-switch). Since the mode-switch will in turn
have the usb-c-connector (i.e the child of the port driver) as a
supplier, fw_devlink will not be able to resolve the cyclic dependency
correctly.
As a result, the mode-switch driver probe() never runs, so mode-switches
are never registered. Because of that, the port driver probe constantly
fails with -EPROBE_DEFER, because the Type-C connector class requires all
switch devices to be registered prior to port registration.
To break this deadlock and allow the mode-switch registration to occur,
purge all the usb-c-connector nodes' absent suppliers. This eliminates
the connector as a supplier for a switch and allows it to be probed.
Signed-off-by: Prashant Malani <[email protected]>
Signed-off-by: Pin-yen Lin <[email protected]>
Reviewed-by: Chen-Yu Tsai <[email protected]>
Tested-by: Chen-Yu Tsai <[email protected]>
Acked-by: Heikki Krogerus <[email protected]>
---
Changes in v11:
- Collected Acked-by tag
Changes in v10:
- Collected Reviewed-by and Tested-by tags
Changes in v7:
- Fix the long comment lines
Changes in v6:
- New in v6
drivers/platform/chrome/cros_ec_typec.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index 2a7ff14dc37e..302474a647cc 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -382,6 +382,16 @@ static int cros_typec_init_ports(struct cros_typec_data *typec)
return -EINVAL;
}
+ /*
+ * OF graph may have set up some device links with switches,
+ * since connectors have their own compatible. Purge these
+ * to avoid a deadlock in switch probe (the switch mistakenly
+ * assumes the connector is a supplier).
+ */
+ if (dev_of_node(dev))
+ device_for_each_child_node(dev, fwnode)
+ fw_devlink_purge_absent_suppliers(fwnode);
+
/* DT uses "reg" to specify port number. */
port_prop = dev->of_node ? "reg" : "port-number";
device_for_each_child_node(dev, fwnode) {
--
2.39.1.519.gcb327c4b5f-goog
Add helpers to register and unregister Type-C "switches" for bridges
capable of switching their output between two downstream devices.
The helper registers USB Type-C mode switches when the "mode-switch"
and the "reg" properties are available in Device Tree.
Signed-off-by: Pin-yen Lin <[email protected]>
---
Changes in v11:
- Use fwnode helpers instead of DT
- Moved the helpers to a new file
- Use "reg" instead of "data-lanes" to determine the port number
- Dropped collected tags due to new changes
Changes in v10:
- Collected Reviewed-by and Tested-by tags
- Replaced "void *" with "typec_mux_set_fn_t" for mux_set callbacks
- Print out the node name when errors on parsing DT
- Use dev_dbg instead of dev_warn when no Type-C switch nodes available
- Made the return path of drm_dp_register_mode_switch clearer
Changes in v8:
- Fixed the build issue when CONFIG_TYPEC=m
- Fixed some style issues
Changes in v7:
- Extracted the common codes to a helper function
- New in v7
drivers/gpu/drm/display/Makefile | 1 +
drivers/gpu/drm/display/drm_dp_typec_helper.c | 114 ++++++++++++++++++
include/drm/display/drm_dp_helper.h | 31 +++++
3 files changed, 146 insertions(+)
create mode 100644 drivers/gpu/drm/display/drm_dp_typec_helper.c
diff --git a/drivers/gpu/drm/display/Makefile b/drivers/gpu/drm/display/Makefile
index 17ac4a1006a8..ef80b9fde615 100644
--- a/drivers/gpu/drm/display/Makefile
+++ b/drivers/gpu/drm/display/Makefile
@@ -14,5 +14,6 @@ drm_display_helper-$(CONFIG_DRM_DISPLAY_HDMI_HELPER) += \
drm_scdc_helper.o
drm_display_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
drm_display_helper-$(CONFIG_DRM_DP_CEC) += drm_dp_cec.o
+drm_display_helper-$(CONFIG_TYPEC) += drm_dp_typec_helper.o
obj-$(CONFIG_DRM_DISPLAY_HELPER) += drm_display_helper.o
diff --git a/drivers/gpu/drm/display/drm_dp_typec_helper.c b/drivers/gpu/drm/display/drm_dp_typec_helper.c
new file mode 100644
index 000000000000..b11a268da57f
--- /dev/null
+++ b/drivers/gpu/drm/display/drm_dp_typec_helper.c
@@ -0,0 +1,114 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/usb/typec_mux.h>
+#include <drm/display/drm_dp_helper.h>
+
+static int drm_dp_register_mode_switch(struct device *dev,
+ struct fwnode_handle *fwnode,
+ struct drm_dp_typec_switch_desc *switch_desc,
+ void *data, typec_mux_set_fn_t mux_set)
+{
+ struct drm_dp_typec_port_data *port_data;
+ struct typec_mux_desc mux_desc = {};
+ char name[32];
+ u32 port_num;
+ int ret;
+
+ ret = fwnode_property_read_u32(fwnode, "reg", &port_num);
+ if (ret) {
+ dev_err(dev, "Failed to read reg property: %d\n", ret);
+ return ret;
+ }
+
+ port_data = &switch_desc->typec_ports[port_num];
+ port_data->data = data;
+ port_data->port_num = port_num;
+ port_data->fwnode = fwnode;
+ mux_desc.fwnode = fwnode;
+ mux_desc.drvdata = port_data;
+ snprintf(name, sizeof(name), "%pfwP-%u", fwnode, port_num);
+ mux_desc.name = name;
+ mux_desc.set = 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\n",
+ port_num, ret);
+
+ return ret;
+ }
+
+ return 0;
+}
+
+/**
+ * drm_dp_register_typec_switches() - register Type-C switches
+ * @dev: Device that registers Type-C switches
+ * @port: Device node for the switch
+ * @switch_desc: A Type-C switch descriptor
+ * @data: Private data for the switches
+ * @mux_set: Callback function for typec_mux_set
+ *
+ * This function registers USB Type-C switches for DP bridges that can switch
+ * the output signal between their output pins.
+ *
+ * Currently only mode switches are implemented, and the function assumes the
+ * given @port device node has endpoints with "mode-switch" property.
+ * The port number is determined by the "reg" property of the endpoint.
+ */
+int drm_dp_register_typec_switches(struct device *dev, struct fwnode_handle *port,
+ struct drm_dp_typec_switch_desc *switch_desc,
+ void *data, typec_mux_set_fn_t mux_set)
+{
+ struct fwnode_handle *sw;
+ int ret;
+
+ fwnode_for_each_child_node(port, sw) {
+ if (fwnode_property_present(sw, "mode-switch"))
+ switch_desc->num_typec_switches++;
+ }
+
+ if (!switch_desc->num_typec_switches) {
+ dev_dbg(dev, "No Type-C switches node found\n");
+ return 0;
+ }
+
+ switch_desc->typec_ports = devm_kcalloc(
+ dev, switch_desc->num_typec_switches,
+ sizeof(struct drm_dp_typec_port_data), GFP_KERNEL);
+
+ if (!switch_desc->typec_ports)
+ return -ENOMEM;
+
+ /* Register switches for each connector. */
+ fwnode_for_each_child_node(port, sw) {
+ if (!fwnode_property_present(sw, "mode-switch"))
+ continue;
+ ret = drm_dp_register_mode_switch(dev, sw, switch_desc, data, mux_set);
+ if (ret)
+ goto err_unregister_typec_switches;
+ }
+
+ return 0;
+
+err_unregister_typec_switches:
+ fwnode_handle_put(sw);
+ drm_dp_unregister_typec_switches(switch_desc);
+ dev_err(dev, "Failed to register mode switch: %d\n", ret);
+ return ret;
+}
+EXPORT_SYMBOL(drm_dp_register_typec_switches);
+
+/**
+ * drm_dp_unregister_typec_switches() - unregister Type-C switches
+ * @switch_desc: A Type-C switch descriptor
+ */
+void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc)
+{
+ int i;
+
+ for (i = 0; i < switch_desc->num_typec_switches; i++)
+ typec_mux_unregister(switch_desc->typec_ports[i].typec_mux);
+}
+EXPORT_SYMBOL(drm_dp_unregister_typec_switches);
diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
index ab55453f2d2c..d9213739de72 100644
--- a/include/drm/display/drm_dp_helper.h
+++ b/include/drm/display/drm_dp_helper.h
@@ -25,6 +25,7 @@
#include <linux/delay.h>
#include <linux/i2c.h>
+#include <linux/usb/typec_mux.h>
#include <drm/display/drm_dp.h>
#include <drm/drm_connector.h>
@@ -763,4 +764,34 @@ bool drm_dp_downstream_rgb_to_ycbcr_conversion(const u8 dpcd[DP_RECEIVER_CAP_SIZ
const u8 port_cap[4], u8 color_spc);
int drm_dp_pcon_convert_rgb_to_ycbcr(struct drm_dp_aux *aux, u8 color_spc);
+struct drm_dp_typec_port_data {
+ struct typec_mux_dev *typec_mux;
+ int port_num;
+ struct fwnode_handle *fwnode;
+ void *data;
+};
+
+struct drm_dp_typec_switch_desc {
+ int num_typec_switches;
+ struct drm_dp_typec_port_data *typec_ports;
+};
+
+#ifdef CONFIG_TYPEC
+void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc);
+int drm_dp_register_typec_switches(struct device *dev, struct fwnode_handle *port,
+ struct drm_dp_typec_switch_desc *switch_desc,
+ void *data, typec_mux_set_fn_t mux_set);
+#else
+static inline void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc)
+{
+}
+static inline int drm_dp_register_typec_switches(
+ struct device *dev, struct fwnode_handle *port,
+ struct drm_dp_typec_switch_desc *switch_desc, void *data,
+ typec_mux_set_fn_t mux_set)
+{
+ return -EOPNOTSUPP;
+}
+#endif
+
#endif /* _DRM_DP_HELPER_H_ */
--
2.39.1.519.gcb327c4b5f-goog
Analogix 7625 can be used in systems to switch the DP traffic between
two downstreams, which can be USB Type-C DisplayPort alternate mode
lane or regular DisplayPort output ports.
Update the binding to accommodate this usage by introducing a
data-lanes and a mode-switch property on endpoints.
Also include the link to the product brief in the bindings.
Signed-off-by: Pin-yen Lin <[email protected]>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Reviewed-by: Chen-Yu Tsai <[email protected]>
Tested-by: Chen-Yu Tsai <[email protected]>
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
---
Changes in v11:
- Updated the description of the endpoints
- Referenced video-interfaces.yaml instead for the endpoints
Changes in v10:
- Collected Reviewed-by and Tested-by tags
Changes in v9:
- Collected Reviewed-by tag
Changes in v8:
- Updated anx7625 bindings for data-lane property
- Fixed the subject prefix
Changes in v7:
- Fixed issues reported by dt_binding_check
- Updated the schema and the example dts for data-lanes.
- Changed to generic naming for the example dts node.
Changes in v6:
- Remove switches node and use endpoints and data-lanes property to
describe the connections.
.../display/bridge/analogix,anx7625.yaml | 94 ++++++++++++++++++-
1 file changed, 91 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
index 4590186c4a0b..f287e8e0602f 100644
--- a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
@@ -12,7 +12,8 @@ maintainers:
description: |
The ANX7625 is an ultra-low power 4K Mobile HD Transmitter
- designed for portable devices.
+ designed for portable devices. Product brief is available at
+ https://www.analogix.com/en/system/files/AA-002291-PB-6-ANX7625_ProductBrief.pdf
properties:
compatible:
@@ -112,9 +113,46 @@ properties:
data-lanes: true
port@1:
- $ref: /schemas/graph.yaml#/properties/port
+ $ref: /schemas/graph.yaml#/$defs/port-base
description:
- Video port for panel or connector.
+ Video port for panel or connector. Each endpoint connects to a video
+ output downstream, and the "data-lanes" property is used to describe
+ the pin connections. 0, 1, 2, 3 in "data-lanes" maps to SSRX1, SSTX1,
+ SSRX2, SSTX2, respectively.
+
+ patternProperties:
+ "^endpoint@[01]$":
+ $ref: /schemas/media/video-interfaces.yaml#
+ properties:
+ reg: true
+
+ remote-endpoint: true
+
+ data-lanes:
+ oneOf:
+ - items:
+ - enum: [0, 1, 2, 3]
+
+ - items:
+ - const: 0
+ - const: 1
+
+ - items:
+ - const: 2
+ - const: 3
+
+ - items:
+ - const: 0
+ - const: 1
+ - const: 2
+ - const: 3
+
+ mode-switch:
+ type: boolean
+ description: Register this node as a Type-C mode switch or not.
+
+ required:
+ - remote-endpoint
required:
- port@0
@@ -186,3 +224,53 @@ examples:
};
};
};
+ - |
+ i2c3 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ encoder@58 {
+ compatible = "analogix,anx7625";
+ reg = <0x58>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&anx7625_dp_pins>;
+ enable-gpios = <&pio 176 GPIO_ACTIVE_HIGH>;
+ reset-gpios = <&pio 177 GPIO_ACTIVE_HIGH>;
+ vdd10-supply = <&pp1100_dpbrdg>;
+ vdd18-supply = <&pp1800_dpbrdg_dx>;
+ vdd33-supply = <&pp3300_dpbrdg_dx>;
+ analogix,audio-enable;
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+ anx7625_dp_in: endpoint {
+ bus-type = <7>;
+ remote-endpoint = <&dpi_out>;
+ };
+ };
+
+ port@1 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ reg = <1>;
+ anx_typec0: endpoint@0 {
+ reg = <0>;
+ mode-switch;
+ data-lanes = <0 1>;
+ remote-endpoint = <&typec_port0>;
+ };
+ anx_typec1: endpoint@1 {
+ reg = <1>;
+ mode-switch;
+ data-lanes = <2 3>;
+ remote-endpoint = <&typec_port1>;
+ };
+ };
+ };
+ };
+ };
--
2.39.1.519.gcb327c4b5f-goog
The output port endpoints can be connected to USB-C connectors.
Running drm_of_find_panel_or_bridge() with such endpoints leads to
a continuous return value of -EPROBE_DEFER, even though there is
no panel present.
To avoid this, check for the existence of a "mode-switch" property in
the port endpoint, and skip panel registration completely if so.
Signed-off-by: Pin-yen Lin <[email protected]>
Reviewed-by: Chen-Yu Tsai <[email protected]>
Tested-by: Chen-Yu Tsai <[email protected]>
---
(no changes since v10)
Changes in v10:
- Collected Reviewed-by and Tested-by tags
Changes in v6:
- New in v6
drivers/gpu/drm/bridge/analogix/anx7625.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
index b375887e655d..1cf242130b91 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.c
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
@@ -1649,7 +1649,7 @@ static int anx7625_get_swing_setting(struct device *dev,
static int anx7625_parse_dt(struct device *dev,
struct anx7625_platform_data *pdata)
{
- struct device_node *np = dev->of_node, *ep0;
+ struct device_node *np = dev->of_node, *ep0, *sw;
int bus_type, mipi_lanes;
anx7625_get_swing_setting(dev, pdata);
@@ -1688,6 +1688,17 @@ static int anx7625_parse_dt(struct device *dev,
if (of_property_read_bool(np, "analogix,audio-enable"))
pdata->audio_en = 1;
+ /*
+ * Don't bother finding a panel if a Type-C `mode-switch` property is
+ * present in one of the endpoints.
+ */
+ for_each_endpoint_of_node(np, sw) {
+ if (of_property_read_bool(sw, "mode-switch")) {
+ of_node_put(sw);
+ return 0;
+ }
+ }
+
pdata->panel_bridge = devm_drm_of_get_bridge(dev, np, 1, 0);
if (IS_ERR(pdata->panel_bridge)) {
if (PTR_ERR(pdata->panel_bridge) == -ENODEV) {
--
2.39.1.519.gcb327c4b5f-goog
Register USB Type-C mode switches when the "mode-switch" property and
relevant ports are available in Device Tree. Configure the crosspoint
switch based on the entered alternate mode for a specific Type-C
connector.
Crosspoint switch can also be used for switching the output signal for
different orientations of a single USB Type-C connector, but the
orientation switch is not implemented yet. A TODO is added for this.
Signed-off-by: Pin-yen Lin <[email protected]>
---
Changes in v11:
- Added back "data-lanes" parsing logics
- Removed Kconfig dependency
- Updated the usage of the private data
- Dropped Tested-by tag because of the new changes
Changes in v10:
- Added a TODO for implementing orientation switch for anx7625
- Updated the commit message for the absence of orientation switch
- Fixed typo in the commit message
- Collected Tested-by tag
Changes in v7:
- Fixed style issues in anx7625 driver
- Removed DT property validation in anx7625 driver.
- Extracted common codes to another commit.
Changes in v6:
- Squashed to a single patch
drivers/gpu/drm/bridge/analogix/anx7625.c | 149 ++++++++++++++++++++++
drivers/gpu/drm/bridge/analogix/anx7625.h | 20 +++
2 files changed, 169 insertions(+)
diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
index 1cf242130b91..7bcffc2987a7 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.c
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
@@ -15,6 +15,8 @@
#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>
#include <linux/of_gpio.h>
@@ -2572,6 +2574,146 @@ 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)
+{
+ int i;
+ /* Check if both ports available and do nothing to retain the current one */
+ if (ctx->port_data[0].dp_connected && ctx->port_data[1].dp_connected)
+ return;
+
+ for (i = 0; i < 2; i++) {
+ if (ctx->port_data[i].dp_connected)
+ anx7625_set_crosspoint_switch(
+ ctx, ctx->port_data[i].orientation);
+ }
+}
+
+static int anx7625_typec_mux_set(struct typec_mux_dev *mux,
+ struct typec_mux_state *state)
+{
+ struct drm_dp_typec_port_data *port = typec_mux_get_drvdata(mux);
+ struct anx7625_data *ctx = (struct anx7625_data *) port->data;
+ struct device *dev = &ctx->client->dev;
+ struct drm_dp_typec_switch_desc switch_desc = ctx->switch_desc;
+ bool new_dp_connected, old_dp_connected;
+
+ if (switch_desc.num_typec_switches == 1)
+ return 0;
+
+ wait_for_completion(&ctx->mux_register);
+
+ old_dp_connected = ctx->port_data[0].dp_connected ||
+ ctx->port_data[1].dp_connected;
+
+ ctx->port_data[port->port_num].dp_connected =
+ state->alt && state->alt->svid == USB_TYPEC_DP_SID &&
+ state->alt->mode == USB_TYPEC_DP_MODE;
+
+ dev_dbg(dev, "mux_set dp_connected: c0=%d, c1=%d\n",
+ ctx->port_data[0].dp_connected, ctx->port_data[1].dp_connected);
+
+ new_dp_connected = ctx->port_data[0].dp_connected ||
+ ctx->port_data[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;
+}
+
+static void anx7625_unregister_typec_switches(struct anx7625_data *ctx)
+{
+ drm_dp_unregister_typec_switches(&ctx->switch_desc);
+}
+
+static int anx7625_register_typec_switches(struct device *dev, struct anx7625_data *ctx)
+{
+ struct device_node *port_node = of_graph_get_port_by_id(dev->of_node, 1);
+ struct drm_dp_typec_switch_desc *switch_desc = &ctx->switch_desc;
+ int ret;
+ u32 dp_lanes[4];
+ unsigned int i, num_lanes;
+
+ /*
+ * Currently, only mode switch is implemented.
+ * TODO: Implement Type-C orientation switch for anx7625.
+ */
+ ret = drm_dp_register_typec_switches(dev, &port_node->fwnode,
+ &ctx->switch_desc, ctx,
+ anx7625_typec_mux_set);
+ if (ret)
+ return ret;
+
+ ctx->port_data = devm_kcalloc(
+ dev, switch_desc->num_typec_switches,
+ sizeof(struct anx7625_typec_port_data), GFP_KERNEL);
+
+ if (!ctx->port_data) {
+ ret = -ENOMEM;
+ goto unregister_mux;
+ }
+
+ for (i = 0; i < switch_desc->num_typec_switches; i++) {
+ struct drm_dp_typec_port_data *port = &switch_desc->typec_ports[i];
+ struct fwnode_handle *fwnode = port->fwnode;
+
+ num_lanes = fwnode_property_read_u32_array(fwnode, "data-lanes",
+ NULL, 0);
+
+ if (num_lanes <= 0 || num_lanes > 2) {
+ dev_err(dev,
+ "Error on getting data lanes count from %pfwP: %d\n",
+ fwnode, num_lanes);
+ ret = num_lanes;
+ goto unregister_mux;
+ }
+
+ ret = fwnode_property_read_u32_array(fwnode, "data-lanes",
+ dp_lanes, num_lanes);
+ if (ret) {
+ dev_err(dev,
+ "Failed to read the data-lanes variable: %d\n",
+ ret);
+ goto unregister_mux;
+ }
+
+ ctx->port_data[i].orientation = (dp_lanes[0] / 2 == 0) ?
+ TYPEC_ORIENTATION_NORMAL : TYPEC_ORIENTATION_REVERSE;
+ ctx->port_data[i].dp_connected = false;
+ }
+ complete_all(&ctx->mux_register);
+
+ return 0;
+
+unregister_mux:
+ complete_all(&ctx->mux_register);
+ anx7625_unregister_typec_switches(ctx);
+ return ret;
+}
+
static int anx7625_i2c_probe(struct i2c_client *client)
{
struct anx7625_data *platform;
@@ -2609,6 +2751,7 @@ static int anx7625_i2c_probe(struct i2c_client *client)
mutex_init(&platform->lock);
mutex_init(&platform->hdcp_wq_lock);
+ init_completion(&platform->mux_register);
INIT_DELAYED_WORK(&platform->hdcp_work, hdcp_check_work_func);
platform->hdcp_workqueue = create_workqueue("hdcp workqueue");
@@ -2679,6 +2822,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 && ret != -ENODEV)
+ dev_warn(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))
@@ -2730,6 +2877,8 @@ static void 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 14f33d6be289..0efbe06c693e 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 ********/
/***************************************************************/
@@ -449,6 +461,11 @@ struct anx7625_i2c_client {
struct i2c_client *tcpc_client;
};
+struct anx7625_typec_port_data {
+ bool dp_connected;
+ enum typec_orientation orientation;
+};
+
struct anx7625_data {
struct anx7625_platform_data pdata;
struct platform_device *audio_pdev;
@@ -479,6 +496,9 @@ struct anx7625_data {
struct drm_connector *connector;
struct mipi_dsi_device *dsi;
struct drm_dp_aux aux;
+ struct completion mux_register;
+ struct drm_dp_typec_switch_desc switch_desc;
+ struct anx7625_typec_port_data *port_data;
};
#endif /* __ANX7625_H__ */
--
2.39.1.519.gcb327c4b5f-goog
ITE IT6505 can be used in systems to switch the DP traffic between
two downstreams, which can be USB Type-C DisplayPort alternate mode
lane or regular DisplayPort output ports.
Update the binding to accommodate this usage by introducing a
data-lanes and a mode-switch property on endpoints.
Signed-off-by: Pin-yen Lin <[email protected]>
---
Changes in v11:
- Updated the description of the endpoints in the bindings
- Referenced video-interfaces.yaml instead for the endpoints binding
- Removed duplicated definitions from inherited schema
Changes in v9:
- Fixed subject prefix again
- Changed the naming of the example node for it6505
Changes in v8:
- Updated bindings for data-lanes property
- Fixed subject prefix
Changes in v7:
- Fixed issues reported by dt_binding_check.
- Updated the schema and the example dts for data-lanes.
- Changed to generic naming for the example dts node.
Changes in v6:
- Remove switches node and use endpoints and data-lanes property to
describe the connections.
.../bindings/display/bridge/ite,it6505.yaml | 101 +++++++++++++++---
1 file changed, 88 insertions(+), 13 deletions(-)
diff --git a/Documentation/devicetree/bindings/display/bridge/ite,it6505.yaml b/Documentation/devicetree/bindings/display/bridge/ite,it6505.yaml
index b16a9d9127dd..8ae9c5cba22c 100644
--- a/Documentation/devicetree/bindings/display/bridge/ite,it6505.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/ite,it6505.yaml
@@ -75,22 +75,49 @@ properties:
port@1:
$ref: /schemas/graph.yaml#/$defs/port-base
unevaluatedProperties: false
- description: Video port for DP output
+ description:
+ Video port for DP output. Each endpoint connects to a video output
+ downstream, and the "data-lanes" property is used to describe the pin
+ connections. 0, 1, 2, 3 in "data-lanes" maps to TX0, TX1, TX2, TX3,
+ respectively.
- properties:
- endpoint:
- $ref: /schemas/graph.yaml#/$defs/endpoint-base
+
+ patternProperties:
+ "^endpoint@[01]$":
+ $ref: /schemas/media/video-interfaces.yaml#
unevaluatedProperties: false
properties:
+ reg: true
+
+ remote-endpoint: true
+
data-lanes:
- minItems: 1
- uniqueItems: true
- items:
- - enum: [ 0, 1 ]
- - const: 1
- - const: 2
- - const: 3
+ oneOf:
+ - items:
+ - enum: [0, 1, 2, 3]
+
+ - items:
+ - const: 0
+ - const: 1
+
+ - items:
+ - const: 2
+ - const: 3
+
+ - items:
+ - const: 0
+ - const: 1
+ - const: 2
+ - const: 3
+
+ mode-switch:
+ type: boolean
+ description: Register this node as a Type-C mode switch or not.
+
+ required:
+ - reg
+ - remote-endpoint
required:
- port@0
@@ -102,7 +129,6 @@ required:
- pwr18-supply
- interrupts
- reset-gpios
- - extcon
- ports
additionalProperties: false
@@ -139,8 +165,11 @@ examples:
};
port@1 {
+ #address-cells = <1>;
+ #size-cells = <0>;
reg = <1>;
- it6505_out: endpoint {
+ it6505_out: endpoint@0 {
+ reg = <0>;
remote-endpoint = <&dp_in>;
data-lanes = <0 1>;
};
@@ -148,3 +177,49 @@ examples:
};
};
};
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ dp-bridge@5c {
+ compatible = "ite,it6505";
+ interrupts = <8 IRQ_TYPE_LEVEL_LOW 8 0>;
+ reg = <0x5c>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&it6505_pins>;
+ ovdd-supply = <&mt6366_vsim2_reg>;
+ pwr18-supply = <&pp1800_dpbrdg_dx>;
+ reset-gpios = <&pio 177 0>;
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ port@0 {
+ reg = <0>;
+ it6505_dpi_in: endpoint {
+ remote-endpoint = <&dpi_out>;
+ };
+ };
+ port@1 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <1>;
+ ite_typec0: endpoint@0 {
+ reg = <0>;
+ mode-switch;
+ data-lanes = <0 1>;
+ remote-endpoint = <&typec_port0>;
+ };
+ ite_typec1: endpoint@1 {
+ reg = <1>;
+ mode-switch;
+ data-lanes = <2 3>;
+ remote-endpoint = <&typec_port1>;
+ };
+ };
+ };
+ };
+ };
--
2.39.1.519.gcb327c4b5f-goog
Replace the spaces with tab characters in the Kconfig file.
Signed-off-by: Pin-yen Lin <[email protected]>
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
---
(no changes since v10)
Changes in v10:
- Collected Reviewed-by tag
Changes in v7:
- New in v7
drivers/gpu/drm/bridge/Kconfig | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 8b2226f72b24..46c35d34bd4f 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -74,19 +74,19 @@ config DRM_FSL_LDB
Support for i.MX8MP DPI-to-LVDS on-SoC encoder.
config DRM_ITE_IT6505
- tristate "ITE IT6505 DisplayPort bridge"
- depends on OF
+ tristate "ITE IT6505 DisplayPort bridge"
+ depends on OF
select DRM_DISPLAY_DP_HELPER
select DRM_DISPLAY_HDCP_HELPER
select DRM_DISPLAY_HELPER
- select DRM_DP_AUX_BUS
- select DRM_KMS_HELPER
- select DRM_DP_HELPER
- select EXTCON
- select CRYPTO
- select CRYPTO_HASH
- help
- ITE IT6505 DisplayPort bridge chip driver.
+ select DRM_DP_AUX_BUS
+ select DRM_KMS_HELPER
+ select DRM_DP_HELPER
+ select EXTCON
+ select CRYPTO
+ select CRYPTO_HASH
+ help
+ ITE IT6505 DisplayPort bridge chip driver.
config DRM_LONTIUM_LT8912B
tristate "Lontium LT8912B DSI/HDMI bridge"
--
2.39.1.519.gcb327c4b5f-goog
Register USB Type-C mode switches when the "mode-switch" property and
relevant port are available in Device Tree. Configure the "lane_swap"
state based on the entered alternate mode for a specific Type-C
connector, which ends up updating the lane swap registers of the it6505
chip.
Signed-off-by: Pin-yen Lin <[email protected]>
---
Changes in v11:
- Added back "data-lanes" parsing logics
- Removed Kconfig dependency
- Updated the usage of the private data
Changes in v7:
- Fixed style issues in it6505 driver
- Removed the redundant sleep in it6505 driver
- Removed DT property validation in it6505 driver
- Rebased to drm-misc-next
- Extracted common codes to another commit
Changes in v6:
- Changed it6505_typec_mux_set callback function to accommodate with
the latest drm-misc patches
- Changed the driver implementation to accommodate with the new binding
- Squashed to a single patch
drivers/gpu/drm/bridge/ite-it6505.c | 179 +++++++++++++++++++++++++++-
1 file changed, 175 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index 9cda2df21b88..902f27875177 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -17,6 +17,8 @@
#include <linux/regmap.h>
#include <linux/regulator/consumer.h>
#include <linux/types.h>
+#include <linux/usb/typec_dp.h>
+#include <linux/usb/typec_mux.h>
#include <linux/wait.h>
#include <crypto/hash.h>
@@ -28,6 +30,7 @@
#include <drm/drm_crtc.h>
#include <drm/drm_crtc_helper.h>
#include <drm/drm_edid.h>
+#include <drm/drm_of.h>
#include <drm/drm_print.h>
#include <drm/drm_probe_helper.h>
@@ -402,6 +405,11 @@ struct debugfs_entries {
const struct file_operations *fops;
};
+struct it6505_typec_port_data {
+ bool dp_connected;
+ bool lane_swap;
+};
+
struct it6505 {
struct drm_dp_aux aux;
struct drm_bridge bridge;
@@ -455,6 +463,9 @@ struct it6505 {
struct delayed_work delayed_audio;
struct it6505_audio_data audio;
struct dentry *debugfs;
+ struct completion mux_register;
+ struct drm_dp_typec_switch_desc switch_desc;
+ struct it6505_typec_port_data *port_data;
/* it6505 driver hold option */
bool enable_drv_hold;
@@ -3346,12 +3357,157 @@ static void it6505_shutdown(struct i2c_client *client)
it6505_lane_off(it6505);
}
+static void it6505_typec_ports_update(struct it6505 *it6505)
+{
+ int i;
+
+ /* Check if both ports available and do nothing to retain the current one */
+ if (it6505->port_data[0].dp_connected && it6505->port_data[1].dp_connected)
+ return;
+
+ for (i = 0; i < 2; i++) {
+ if (it6505->port_data[i].dp_connected)
+ it6505->lane_swap = it6505->port_data[i].lane_swap;
+ }
+}
+
+static int it6505_typec_mux_set(struct typec_mux_dev *mux,
+ struct typec_mux_state *state)
+{
+ struct drm_dp_typec_port_data *port = typec_mux_get_drvdata(mux);
+ struct it6505 *it6505 = (struct it6505 *) port->data;
+ struct device *dev = &it6505->client->dev;
+ struct drm_dp_typec_switch_desc switch_desc = it6505->switch_desc;
+ bool old_dp_connected, new_dp_connected;
+
+ if (switch_desc.num_typec_switches == 1)
+ return 0;
+
+ mutex_lock(&it6505->extcon_lock);
+ wait_for_completion(&it6505->mux_register);
+
+ old_dp_connected = it6505->port_data[0].dp_connected ||
+ it6505->port_data[1].dp_connected;
+
+ it6505->port_data[port->port_num].dp_connected =
+ state->alt && state->alt->svid == USB_TYPEC_DP_SID &&
+ state->alt->mode == USB_TYPEC_DP_MODE;
+
+ dev_dbg(dev, "mux_set dp_connected: c0=%d, c1=%d\n",
+ it6505->port_data[0].dp_connected, it6505->port_data[1].dp_connected);
+
+ new_dp_connected = it6505->port_data[0].dp_connected ||
+ it6505->port_data[1].dp_connected;
+
+ if (it6505->enable_drv_hold) {
+ dev_dbg(dev, "enable driver hold\n");
+ goto unlock;
+ }
+
+ it6505_typec_ports_update(it6505);
+
+ if (!old_dp_connected && new_dp_connected) {
+ int ret = pm_runtime_get_sync(dev);
+
+ /*
+ * pm_runtime_force_suspend() disables runtime PM when the
+ * system enters suspend state. But on system resume, mux_set
+ * can be triggered before pm_runtime_force_resume() re-enables
+ * runtime PM. This makes the bridge stay powered off if the
+ * downstream display is connected when the system is suspended.
+ * Handling the error here to make sure the bridge is powered
+ * on, and leave the PM runtime usage count incremented so
+ * the future runtime PM calls is balanced.
+ */
+ if (ret < 0)
+ it6505_poweron(it6505);
+
+ complete_all(&it6505->extcon_completion);
+ }
+
+ if (old_dp_connected && !new_dp_connected) {
+ reinit_completion(&it6505->extcon_completion);
+ pm_runtime_put_sync(dev);
+ if (it6505->bridge.dev)
+ drm_helper_hpd_irq_event(it6505->bridge.dev);
+ memset(it6505->dpcd, 0, sizeof(it6505->dpcd));
+ }
+
+unlock:
+ mutex_unlock(&it6505->extcon_lock);
+ return 0;
+}
+
+static void it6505_unregister_typec_switches(struct it6505 *it6505)
+{
+ drm_dp_unregister_typec_switches(&it6505->switch_desc);
+}
+
+static int it6505_register_typec_switches(struct device *dev, struct it6505 *it6505)
+{
+ struct device_node *port_node = of_graph_get_port_by_id(dev->of_node, 1);
+ struct drm_dp_typec_switch_desc *switch_desc = &it6505->switch_desc;
+ int ret;
+ u32 dp_lanes[4];
+ unsigned int i, num_lanes;
+
+ ret = drm_dp_register_typec_switches(dev, &port_node->fwnode,
+ &it6505->switch_desc, it6505,
+ it6505_typec_mux_set);
+ if (ret)
+ return ret;
+
+ it6505->port_data = devm_kcalloc(
+ dev, switch_desc->num_typec_switches,
+ sizeof(struct it6505_typec_port_data), GFP_KERNEL);
+
+ if (!it6505->port_data) {
+ ret = -ENOMEM;
+ goto unregister_mux;
+ }
+
+ for (i = 0; i < switch_desc->num_typec_switches; i++) {
+ struct drm_dp_typec_port_data *port = &switch_desc->typec_ports[i];
+ struct fwnode_handle *fwnode = port->fwnode;
+
+ num_lanes = fwnode_property_read_u32_array(fwnode, "data-lanes",
+ NULL, 0);
+
+ if (num_lanes <= 0 || num_lanes > 2) {
+ dev_err(dev,
+ "Error on getting data lanes count from %pfwP: %d\n",
+ fwnode, num_lanes);
+ ret = num_lanes;
+ goto unregister_mux;
+ }
+
+ ret = fwnode_property_read_u32_array(fwnode, "data-lanes",
+ dp_lanes, num_lanes);
+ if (ret) {
+ dev_err(dev,
+ "Failed to read the data-lanes variable: %d\n",
+ ret);
+ goto unregister_mux;
+ }
+
+ it6505->port_data[i].lane_swap = (dp_lanes[0] / 2 == 1);
+ }
+ complete_all(&it6505->mux_register);
+
+ return 0;
+
+unregister_mux:
+ complete_all(&it6505->mux_register);
+ it6505_unregister_typec_switches(it6505);
+ return ret;
+}
+
static int it6505_i2c_probe(struct i2c_client *client)
{
struct it6505 *it6505;
struct device *dev = &client->dev;
struct extcon_dev *extcon;
- int err, intp_irq;
+ int err, intp_irq, ret;
it6505 = devm_kzalloc(&client->dev, sizeof(*it6505), GFP_KERNEL);
if (!it6505)
@@ -3371,11 +3527,25 @@ static int it6505_i2c_probe(struct i2c_client *client)
if (PTR_ERR(extcon) == -EPROBE_DEFER)
return -EPROBE_DEFER;
if (IS_ERR(extcon)) {
- dev_err(dev, "can not get extcon device!");
- return PTR_ERR(extcon);
+ if (PTR_ERR(extcon) != -ENODEV)
+ dev_warn(dev, "Cannot get extcon device: %ld\n",
+ PTR_ERR(extcon));
+ it6505->extcon = NULL;
+ } else {
+ it6505->extcon = extcon;
}
- it6505->extcon = extcon;
+ init_completion(&it6505->mux_register);
+ ret = it6505_register_typec_switches(dev, it6505);
+ if (ret) {
+ if (ret != -ENODEV)
+ dev_warn(dev, "Didn't register Type-C switches, err: %d\n",
+ ret);
+ if (!it6505->extcon) {
+ dev_err(dev, "Both extcon and typec-switch are not registered.\n");
+ return -EINVAL;
+ }
+ }
it6505->regmap = devm_regmap_init_i2c(client, &it6505_regmap_config);
if (IS_ERR(it6505->regmap)) {
@@ -3447,6 +3617,7 @@ static void it6505_i2c_remove(struct i2c_client *client)
it6505_debugfs_remove(it6505);
it6505_poweroff(it6505);
it6505_remove_edid(it6505);
+ it6505_unregister_typec_switches(it6505);
}
static const struct i2c_device_id it6505_id[] = {
--
2.39.1.519.gcb327c4b5f-goog
Hi Pin-yen,
On Sat, Feb 04, 2023 at 09:30:32PM +0800, Pin-yen Lin wrote:
> From: Prashant Malani <[email protected]>
>
> When searching the device graph for device matches, check the
> remote-endpoint itself for a match.
>
> Some drivers register devices for individual endpoints. This allows
> the matcher code to evaluate those for a match too, instead
> of only looking at the remote parent devices. This is required when a
> device supports two mode switches in its endpoints, so we can't simply
> register the mode switch with the parent node.
>
> Signed-off-by: Prashant Malani <[email protected]>
> Signed-off-by: Pin-yen Lin <[email protected]>
> Reviewed-by: Chen-Yu Tsai <[email protected]>
> Tested-by: Chen-Yu Tsai <[email protected]>
Thanks for the update.
I intended to give my Reviewed-by: but there's something still needs to be
addressed. See below.
>
> ---
>
> Changes in v11:
> - Added missing fwnode_handle_put in drivers/base/property.c
>
> Changes in v10:
> - Collected Reviewed-by and Tested-by tags
>
> Changes in v6:
> - New in v6
>
> drivers/base/property.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 2a5a37fcd998..e6f915b72eb7 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -1223,6 +1223,22 @@ static unsigned int fwnode_graph_devcon_matches(struct fwnode_handle *fwnode,
> break;
> }
>
> + /*
> + * Some drivers may register devices for endpoints. Check
> + * the remote-endpoints for matches in addition to the remote
> + * port parent.
> + */
> + node = fwnode_graph_get_remote_endpoint(ep);
Here fwnode_graph_get_remote_endpoint() returns an endpoint...
> + if (fwnode_device_is_available(node)) {
and you're calling fwnode_device_is_available() on the endpoint node, which
always returns true.
Shouldn't you call this on the device node instead? What about match()
below?
> + ret = match(node, con_id, data);
> + if (ret) {
> + if (matches)
> + matches[count] = ret;
> + count++;
> + }
> + }
> + fwnode_handle_put(node);
> +
> node = fwnode_graph_get_remote_port_parent(ep);
> if (!fwnode_device_is_available(node)) {
> fwnode_handle_put(node);
--
Kind regards,
Sakari Ailus
On Sat, Feb 4, 2023 at 9:30 PM Pin-yen Lin <[email protected]> wrote:
>
>
> This series introduces bindings for anx7625/it6505 to register Type-C
> mode-switch in their output endpoints, and use data-lanes property to
> describe the pin connections.
>
> This series is not directly related to the built-in mux in anx7625,
> which automatically switches between the two orientations of a single
> Type-C connector. This series adds support of registering mode switches
> for two downstream devices, while we use orientation switches for two
> orientations of the Type-C connector.
>
> The first two patch modifies fwnode_graph_devcon_matches and
> cros_typec_init_ports to enable the registration of the switches.
>
> Patch 4~6 introduce the bindings for anx7625 and the corresponding driver
> modifications.
>
> Patch 7~9 add similar bindings and driver changes for it6505.
>
> v10: https://lore.kernel.org/all/[email protected]/
> v9: https://lore.kernel.org/all/[email protected]/
> v8: https://lore.kernel.org/all/[email protected]/
> v7: https://lore.kernel.org/all/[email protected]/
> v6: https://lore.kernel.org/all/[email protected]/
> v5: https://lore.kernel.org/linux-usb/[email protected]/
>
> Changes in v11:
> - Added missing fwnode_handle_put in drivers/base/property.c
> - Collected Acked-by tag
> - Use fwnode helpers instead of DT
> - Moved the helpers to a new file
> - Use "reg" instead of "data-lanes" to determine the port number
> - Updated the description of the endpoints in the bindings
> - Referenced video-interfaces.yaml instead for the endpoints binding
> - Removed duplicated definitions from inherited schema
> - Moved the "data-lanes" parsing logics to bridge drivers
> - Removed Kconfig dependencies for the bridge drivers
> - Updated the usage of the private bridge driver data
> - Added a clarification on the anx7625 built-in mux in the cover letter
>
> Changes in v10:
> - Collected Reviewed-by and Tested-by tags
> - Replaced "void *" with "typec_mux_set_fn_t" for mux_set callbacks
> - Print out the node name when errors on parsing DT
> - Use dev_dbg instead of dev_warn when no Type-C switch nodes available
> - Made the return path of drm_dp_register_mode_switch clearer
> - Added a TODO for implementing orientation switch for anx7625
> - Updated the commit message for the absence of orientation switch
> - Fixed typo in the commit message
>
> Changes in v9:
> - Collected Reviewed-by tag
> - Fixed subject prefix again
> - Changed the naming of the example node for it6505
>
> Changes in v8:
> - Fixed the build issue when CONFIG_TYPEC=m
> - Fixed some style issues
> - Fixed the subject prefixes for the bindings patch
> - Fixed the bindings for data-lanes properties
>
> Changes in v7:
> - Fix the long comment lines
> - Extracted the common codes to a helper function
> - Fixed style issues in anx7625 driver
> - Removed DT property validation in anx7625 driver.
> - Fixed style issues in it6505 driver
> - Removed the redundant sleep in it6505 driver
> - Removed DT property validation in it6505 driver
> - Rebased to drm-misc-next
> - Fixed indentations in bindings patches
> - Added a new patch to fix indentations in Kconfig
>
> Changes in v6:
> - Changed it6505_typec_mux_set callback function to accommodate with
> the latest drm-misc patches
> - Changed the driver implementation to accommodate with the new binding
> - Dropped typec-switch binding and use endpoints and data-lanes properties
> to describe the pin connections
> - Added new patches (patch 1,2,4) to fix probing issues
> - Changed the bindings of it6505/anx7625 and modified the drivers
> accordingly
> - Merged it6505/anx7625 driver changes into a single patch
>
> Pin-yen Lin (7):
> drm/display: Add Type-C switch helpers
> dt-bindings: display: bridge: anx7625: Add mode-switch support
> drm/bridge: anx7625: Check for Type-C during panel registration
> drm/bridge: anx7625: Register Type C mode switches
> dt-bindings: display: bridge: it6505: Add mode-switch support
> drm/bridge: it6505: Fix Kconfig indentation
> drm/bridge: it6505: Register Type C mode switches
>
> Prashant Malani (2):
> device property: Add remote endpoint to devcon matcher
> platform/chrome: cros_ec_typec: Purge blocking switch devlinks
Whole series is
Tested-by: Chen-Yu Tsai <[email protected]>
on MT8192-based Hayato for anx7625 and not-yet-upstreamed MT8186 device
for it6505.
On Sat, Feb 4, 2023 at 9:31 PM Pin-yen Lin <[email protected]> wrote:
>
> Analogix 7625 can be used in systems to switch the DP traffic between
> two downstreams, which can be USB Type-C DisplayPort alternate mode
> lane or regular DisplayPort output ports.
>
> Update the binding to accommodate this usage by introducing a
> data-lanes and a mode-switch property on endpoints.
>
> Also include the link to the product brief in the bindings.
>
> Signed-off-by: Pin-yen Lin <[email protected]>
> Reviewed-by: Krzysztof Kozlowski <[email protected]>
> Reviewed-by: Chen-Yu Tsai <[email protected]>
> Tested-by: Chen-Yu Tsai <[email protected]>
> Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
>
> ---
>
> Changes in v11:
> - Updated the description of the endpoints
> - Referenced video-interfaces.yaml instead for the endpoints
>
> Changes in v10:
> - Collected Reviewed-by and Tested-by tags
>
> Changes in v9:
> - Collected Reviewed-by tag
>
> Changes in v8:
> - Updated anx7625 bindings for data-lane property
> - Fixed the subject prefix
>
> Changes in v7:
> - Fixed issues reported by dt_binding_check
> - Updated the schema and the example dts for data-lanes.
> - Changed to generic naming for the example dts node.
>
> Changes in v6:
> - Remove switches node and use endpoints and data-lanes property to
> describe the connections.
>
> .../display/bridge/analogix,anx7625.yaml | 94 ++++++++++++++++++-
> 1 file changed, 91 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
> index 4590186c4a0b..f287e8e0602f 100644
> --- a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
> @@ -12,7 +12,8 @@ maintainers:
>
> description: |
> The ANX7625 is an ultra-low power 4K Mobile HD Transmitter
> - designed for portable devices.
> + designed for portable devices. Product brief is available at
> + https://www.analogix.com/en/system/files/AA-002291-PB-6-ANX7625_ProductBrief.pdf
>
> properties:
> compatible:
> @@ -112,9 +113,46 @@ properties:
> data-lanes: true
>
> port@1:
> - $ref: /schemas/graph.yaml#/properties/port
> + $ref: /schemas/graph.yaml#/$defs/port-base
> description:
> - Video port for panel or connector.
> + Video port for panel or connector. Each endpoint connects to a video
> + output downstream, and the "data-lanes" property is used to describe
> + the pin connections. 0, 1, 2, 3 in "data-lanes" maps to SSRX1, SSTX1,
> + SSRX2, SSTX2, respectively.
> +
> + patternProperties:
> + "^endpoint@[01]$":
> + $ref: /schemas/media/video-interfaces.yaml#
> + properties:
> + reg: true
> +
> + remote-endpoint: true
> +
> + data-lanes:
> + oneOf:
> + - items:
> + - enum: [0, 1, 2, 3]
> +
> + - items:
> + - const: 0
> + - const: 1
> +
> + - items:
> + - const: 2
> + - const: 3
> +
> + - items:
> + - const: 0
> + - const: 1
> + - const: 2
> + - const: 3
Given that this bridge only supports up to two lanes, including all four
lanes doesn't make sense.
> +
> + mode-switch:
> + type: boolean
> + description: Register this node as a Type-C mode switch or not.
I would reword this as "Serves as Type-C mode switch if present"
ChenYu
On Sat, Feb 4, 2023 at 9:31 PM Pin-yen Lin <[email protected]> wrote:
>
> ITE IT6505 can be used in systems to switch the DP traffic between
> two downstreams, which can be USB Type-C DisplayPort alternate mode
> lane or regular DisplayPort output ports.
>
> Update the binding to accommodate this usage by introducing a
> data-lanes and a mode-switch property on endpoints.
>
> Signed-off-by: Pin-yen Lin <[email protected]>
>
> ---
>
> Changes in v11:
> - Updated the description of the endpoints in the bindings
> - Referenced video-interfaces.yaml instead for the endpoints binding
> - Removed duplicated definitions from inherited schema
>
> Changes in v9:
> - Fixed subject prefix again
> - Changed the naming of the example node for it6505
>
> Changes in v8:
> - Updated bindings for data-lanes property
> - Fixed subject prefix
>
> Changes in v7:
> - Fixed issues reported by dt_binding_check.
> - Updated the schema and the example dts for data-lanes.
> - Changed to generic naming for the example dts node.
>
> Changes in v6:
> - Remove switches node and use endpoints and data-lanes property to
> describe the connections.
>
> .../bindings/display/bridge/ite,it6505.yaml | 101 +++++++++++++++---
> 1 file changed, 88 insertions(+), 13 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/display/bridge/ite,it6505.yaml b/Documentation/devicetree/bindings/display/bridge/ite,it6505.yaml
> index b16a9d9127dd..8ae9c5cba22c 100644
> --- a/Documentation/devicetree/bindings/display/bridge/ite,it6505.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/ite,it6505.yaml
> @@ -75,22 +75,49 @@ properties:
> port@1:
> $ref: /schemas/graph.yaml#/$defs/port-base
> unevaluatedProperties: false
> - description: Video port for DP output
> + description:
> + Video port for DP output. Each endpoint connects to a video output
> + downstream, and the "data-lanes" property is used to describe the pin
> + connections. 0, 1, 2, 3 in "data-lanes" maps to TX0, TX1, TX2, TX3,
> + respectively.
>
> - properties:
> - endpoint:
> - $ref: /schemas/graph.yaml#/$defs/endpoint-base
> +
> + patternProperties:
> + "^endpoint@[01]$":
> + $ref: /schemas/media/video-interfaces.yaml#
> unevaluatedProperties: false
>
> properties:
> + reg: true
> +
> + remote-endpoint: true
> +
> data-lanes:
> - minItems: 1
> - uniqueItems: true
> - items:
> - - enum: [ 0, 1 ]
> - - const: 1
> - - const: 2
> - - const: 3
> + oneOf:
> + - items:
> + - enum: [0, 1, 2, 3]
Based on the datasheets we have and the downstream driver, I assume
the hardware implements "lane swap" as simply reversing the order
of the output pins.
In that case the hardware can't output 1 lane DP on any arbitrary lane,
but only lane 0 or 3.
> + - items:
> + - const: 0
> + - const: 1
> +
> + - items:
> + - const: 2
> + - const: 3
And maybe a bit pedantic, but have the order correct as:
- const: 3
- const: 2
> + - items:
> + - const: 0
> + - const: 1
> + - const: 2
> + - const: 3
> +
> + mode-switch:
> + type: boolean
> + description: Register this node as a Type-C mode switch or not.
Same as the anx7625 patch, I would reword this as "Serves as Type-C mode
switch if present".
ChenYu
> +
> + required:
> + - reg
> + - remote-endpoint
>
> required:
> - port@0
> @@ -102,7 +129,6 @@ required:
> - pwr18-supply
> - interrupts
> - reset-gpios
> - - extcon
> - ports
>
> additionalProperties: false
> @@ -139,8 +165,11 @@ examples:
> };
>
> port@1 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> reg = <1>;
> - it6505_out: endpoint {
> + it6505_out: endpoint@0 {
> + reg = <0>;
> remote-endpoint = <&dp_in>;
> data-lanes = <0 1>;
> };
> @@ -148,3 +177,49 @@ examples:
> };
> };
> };
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> +
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + dp-bridge@5c {
> + compatible = "ite,it6505";
> + interrupts = <8 IRQ_TYPE_LEVEL_LOW 8 0>;
> + reg = <0x5c>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&it6505_pins>;
> + ovdd-supply = <&mt6366_vsim2_reg>;
> + pwr18-supply = <&pp1800_dpbrdg_dx>;
> + reset-gpios = <&pio 177 0>;
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + port@0 {
> + reg = <0>;
> + it6505_dpi_in: endpoint {
> + remote-endpoint = <&dpi_out>;
> + };
> + };
> + port@1 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <1>;
> + ite_typec0: endpoint@0 {
> + reg = <0>;
> + mode-switch;
> + data-lanes = <0 1>;
> + remote-endpoint = <&typec_port0>;
> + };
> + ite_typec1: endpoint@1 {
> + reg = <1>;
> + mode-switch;
> + data-lanes = <2 3>;
> + remote-endpoint = <&typec_port1>;
> + };
> + };
> + };
> + };
> + };
> --
> 2.39.1.519.gcb327c4b5f-goog
>
On Sat, Feb 04, 2023 at 09:30:34PM +0800, Pin-yen Lin wrote:
> Add helpers to register and unregister Type-C "switches" for bridges
> capable of switching their output between two downstream devices.
>
> The helper registers USB Type-C mode switches when the "mode-switch"
> and the "reg" properties are available in Device Tree.
...
> + fwnode_for_each_child_node(port, sw) {
> + if (fwnode_property_present(sw, "mode-switch"))
This idiom is being used at least twice (in this code, haven't checked the rest
of the patches, though), perhaps
#define fwnode_for_each_typec_mode_switch(port, sw) \
fwnode_for_each_child_node(port, sw) \
if (!fwnode_property_present(sw, "mode-switch")) {} else
?
--
With Best Regards,
Andy Shevchenko
On Sat, Feb 04, 2023 at 09:30:36PM +0800, Pin-yen Lin wrote:
> The output port endpoints can be connected to USB-C connectors.
> Running drm_of_find_panel_or_bridge() with such endpoints leads to
> a continuous return value of -EPROBE_DEFER, even though there is
> no panel present.
>
> To avoid this, check for the existence of a "mode-switch" property in
> the port endpoint, and skip panel registration completely if so.
...
> + /*
> + * Don't bother finding a panel if a Type-C `mode-switch` property is
> + * present in one of the endpoints.
> + */
> + for_each_endpoint_of_node(np, sw) {
> + if (of_property_read_bool(sw, "mode-switch")) {
Might be that the same helper can be used here. Dunno if the endpoint traverse
should be used for the other driver as well.
> + of_node_put(sw);
> + return 0;
> + }
> + }
--
With Best Regards,
Andy Shevchenko
On Mon, 06 Feb 2023, Andy Shevchenko <[email protected]> wrote:
> On Sat, Feb 04, 2023 at 09:30:34PM +0800, Pin-yen Lin wrote:
>> Add helpers to register and unregister Type-C "switches" for bridges
>> capable of switching their output between two downstream devices.
>>
>> The helper registers USB Type-C mode switches when the "mode-switch"
>> and the "reg" properties are available in Device Tree.
>
> ...
>
>> + fwnode_for_each_child_node(port, sw) {
>> + if (fwnode_property_present(sw, "mode-switch"))
>
> This idiom is being used at least twice (in this code, haven't checked the rest
> of the patches, though), perhaps
>
> #define fwnode_for_each_typec_mode_switch(port, sw) \
> fwnode_for_each_child_node(port, sw) \
> if (!fwnode_property_present(sw, "mode-switch")) {} else
>
> ?
See for_each_if() in drm_util.h.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
On Sat, Feb 04, 2023 at 09:30:37PM +0800, Pin-yen Lin wrote:
> Register USB Type-C mode switches when the "mode-switch" property and
> relevant ports are available in Device Tree. Configure the crosspoint
> switch based on the entered alternate mode for a specific Type-C
> connector.
>
> Crosspoint switch can also be used for switching the output signal for
> different orientations of a single USB Type-C connector, but the
> orientation switch is not implemented yet. A TODO is added for this.
...
> + for (i = 0; i < 2; i++) {
> + if (ctx->port_data[i].dp_connected)
> + anx7625_set_crosspoint_switch(
> + ctx, ctx->port_data[i].orientation);
It's more than enough room to place ctx on the previous line.
> + }
...
> + struct anx7625_data *ctx = (struct anx7625_data *) port->data;
Redundant explicit casting.
...
> + struct device *dev = &ctx->client->dev;
Do you really need to keep client in that struct and not simply dev?
...
> + /* dp on, power on first */
DP ?
...
> + /* dp off, power off last */
Ditto.
...
> + num_lanes = fwnode_property_read_u32_array(fwnode, "data-lanes",
> + NULL, 0);
Read the kernel doc for this API and amend your code accordingly.
...
> + if (num_lanes <= 0 || num_lanes > 2) {
> + dev_err(dev,
> + "Error on getting data lanes count from %pfwP: %d\n",
> + fwnode, num_lanes);
> + ret = num_lanes;
ret == 0?! Carefully consider all cases.
> + goto unregister_mux;
> + }
--
With Best Regards,
Andy Shevchenko
On Sat, Feb 04, 2023 at 09:30:40PM +0800, Pin-yen Lin wrote:
> Register USB Type-C mode switches when the "mode-switch" property and
> relevant port are available in Device Tree. Configure the "lane_swap"
> state based on the entered alternate mode for a specific Type-C
> connector, which ends up updating the lane swap registers of the it6505
> chip.
Same / similar comments as per previous patch.
--
With Best Regards,
Andy Shevchenko
On Sat, Feb 04, 2023 at 09:30:38PM +0800, Pin-yen Lin wrote:
> ITE IT6505 can be used in systems to switch the DP traffic between
> two downstreams, which can be USB Type-C DisplayPort alternate mode
> lane or regular DisplayPort output ports.
>
> Update the binding to accommodate this usage by introducing a
> data-lanes and a mode-switch property on endpoints.
>
> Signed-off-by: Pin-yen Lin <[email protected]>
>
> ---
>
> Changes in v11:
> - Updated the description of the endpoints in the bindings
> - Referenced video-interfaces.yaml instead for the endpoints binding
> - Removed duplicated definitions from inherited schema
>
> Changes in v9:
> - Fixed subject prefix again
> - Changed the naming of the example node for it6505
>
> Changes in v8:
> - Updated bindings for data-lanes property
> - Fixed subject prefix
>
> Changes in v7:
> - Fixed issues reported by dt_binding_check.
> - Updated the schema and the example dts for data-lanes.
> - Changed to generic naming for the example dts node.
>
> Changes in v6:
> - Remove switches node and use endpoints and data-lanes property to
> describe the connections.
>
> .../bindings/display/bridge/ite,it6505.yaml | 101 +++++++++++++++---
> 1 file changed, 88 insertions(+), 13 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/display/bridge/ite,it6505.yaml b/Documentation/devicetree/bindings/display/bridge/ite,it6505.yaml
> index b16a9d9127dd..8ae9c5cba22c 100644
> --- a/Documentation/devicetree/bindings/display/bridge/ite,it6505.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/ite,it6505.yaml
> @@ -75,22 +75,49 @@ properties:
> port@1:
> $ref: /schemas/graph.yaml#/$defs/port-base
> unevaluatedProperties: false
> - description: Video port for DP output
> + description:
> + Video port for DP output. Each endpoint connects to a video output
> + downstream, and the "data-lanes" property is used to describe the pin
> + connections. 0, 1, 2, 3 in "data-lanes" maps to TX0, TX1, TX2, TX3,
> + respectively.
>
> - properties:
> - endpoint:
> - $ref: /schemas/graph.yaml#/$defs/endpoint-base
> +
> + patternProperties:
> + "^endpoint@[01]$":
> + $ref: /schemas/media/video-interfaces.yaml#
> unevaluatedProperties: false
>
> properties:
> + reg: true
> +
> + remote-endpoint: true
> +
> data-lanes:
> - minItems: 1
> - uniqueItems: true
> - items:
> - - enum: [ 0, 1 ]
> - - const: 1
> - - const: 2
> - - const: 3
> + oneOf:
> + - items:
> + - enum: [0, 1, 2, 3]
> +
> + - items:
> + - const: 0
> + - const: 1
> +
> + - items:
> + - const: 2
> + - const: 3
> +
> + - items:
> + - const: 0
> + - const: 1
> + - const: 2
> + - const: 3
> +
> + mode-switch:
> + type: boolean
> + description: Register this node as a Type-C mode switch or not.
Existing users put this property in the device's node, not the endpoint.
That seems more like a property of the device, than the DP link.
You are using fwnode_typec_mux_get(), right?
Rob
Hi Pin-yen,
[...]
> +static int drm_dp_register_mode_switch(struct device *dev,
> + struct fwnode_handle *fwnode,
> + struct drm_dp_typec_switch_desc *switch_desc,
> + void *data, typec_mux_set_fn_t mux_set)
> +{
> + struct drm_dp_typec_port_data *port_data;
> + struct typec_mux_desc mux_desc = {};
> + char name[32];
> + u32 port_num;
> + int ret;
> +
> + ret = fwnode_property_read_u32(fwnode, "reg", &port_num);
> + if (ret) {
> + dev_err(dev, "Failed to read reg property: %d\n", ret);
> + return ret;
> + }
> +
> + port_data = &switch_desc->typec_ports[port_num];
> + port_data->data = data;
> + port_data->port_num = port_num;
> + port_data->fwnode = fwnode;
> + mux_desc.fwnode = fwnode;
> + mux_desc.drvdata = port_data;
> + snprintf(name, sizeof(name), "%pfwP-%u", fwnode, port_num);
> + mux_desc.name = name;
> + mux_desc.set = 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\n",
> + port_num, ret);
> +
> + return ret;
you don't need this return here...
> + }
> +
> + return 0;
Just "return ret;" here.
> +}
> +
> +/**
> + * drm_dp_register_typec_switches() - register Type-C switches
> + * @dev: Device that registers Type-C switches
> + * @port: Device node for the switch
> + * @switch_desc: A Type-C switch descriptor
> + * @data: Private data for the switches
> + * @mux_set: Callback function for typec_mux_set
> + *
> + * This function registers USB Type-C switches for DP bridges that can switch
> + * the output signal between their output pins.
> + *
> + * Currently only mode switches are implemented, and the function assumes the
> + * given @port device node has endpoints with "mode-switch" property.
> + * The port number is determined by the "reg" property of the endpoint.
> + */
> +int drm_dp_register_typec_switches(struct device *dev, struct fwnode_handle *port,
> + struct drm_dp_typec_switch_desc *switch_desc,
> + void *data, typec_mux_set_fn_t mux_set)
> +{
> + struct fwnode_handle *sw;
> + int ret;
> +
> + fwnode_for_each_child_node(port, sw) {
> + if (fwnode_property_present(sw, "mode-switch"))
> + switch_desc->num_typec_switches++;
> + }
no need for brackets here
> +
> + if (!switch_desc->num_typec_switches) {
> + dev_dbg(dev, "No Type-C switches node found\n");
dev_warn()?
> + return 0;
> + }
> +
> + switch_desc->typec_ports = devm_kcalloc(
> + dev, switch_desc->num_typec_switches,
> + sizeof(struct drm_dp_typec_port_data), GFP_KERNEL);
> +
> + if (!switch_desc->typec_ports)
> + return -ENOMEM;
> +
> + /* Register switches for each connector. */
> + fwnode_for_each_child_node(port, sw) {
> + if (!fwnode_property_present(sw, "mode-switch"))
> + continue;
> + ret = drm_dp_register_mode_switch(dev, sw, switch_desc, data, mux_set);
> + if (ret)
> + goto err_unregister_typec_switches;
> + }
> +
> + return 0;
> +
> +err_unregister_typec_switches:
> + fwnode_handle_put(sw);
> + drm_dp_unregister_typec_switches(switch_desc);
> + dev_err(dev, "Failed to register mode switch: %d\n", ret);
there is a bit of dmesg spamming. Please choose where you want to
print the error, either in this function or in
drm_dp_register_mode_switch().
Andi
> + return ret;
> +}
> +EXPORT_SYMBOL(drm_dp_register_typec_switches);
[...]
Hi Rob,
Thanks for the review.
On Wed, Feb 8, 2023 at 4:52 AM Rob Herring <[email protected]> wrote:
>
> On Sat, Feb 04, 2023 at 09:30:38PM +0800, Pin-yen Lin wrote:
> > ITE IT6505 can be used in systems to switch the DP traffic between
> > two downstreams, which can be USB Type-C DisplayPort alternate mode
> > lane or regular DisplayPort output ports.
> >
> > Update the binding to accommodate this usage by introducing a
> > data-lanes and a mode-switch property on endpoints.
> >
> > Signed-off-by: Pin-yen Lin <[email protected]>
> >
> > ---
> >
> > Changes in v11:
> > - Updated the description of the endpoints in the bindings
> > - Referenced video-interfaces.yaml instead for the endpoints binding
> > - Removed duplicated definitions from inherited schema
> >
> > Changes in v9:
> > - Fixed subject prefix again
> > - Changed the naming of the example node for it6505
> >
> > Changes in v8:
> > - Updated bindings for data-lanes property
> > - Fixed subject prefix
> >
> > Changes in v7:
> > - Fixed issues reported by dt_binding_check.
> > - Updated the schema and the example dts for data-lanes.
> > - Changed to generic naming for the example dts node.
> >
> > Changes in v6:
> > - Remove switches node and use endpoints and data-lanes property to
> > describe the connections.
> >
> > .../bindings/display/bridge/ite,it6505.yaml | 101 +++++++++++++++---
> > 1 file changed, 88 insertions(+), 13 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/display/bridge/ite,it6505.yaml b/Documentation/devicetree/bindings/display/bridge/ite,it6505.yaml
> > index b16a9d9127dd..8ae9c5cba22c 100644
> > --- a/Documentation/devicetree/bindings/display/bridge/ite,it6505.yaml
> > +++ b/Documentation/devicetree/bindings/display/bridge/ite,it6505.yaml
> > @@ -75,22 +75,49 @@ properties:
> > port@1:
> > $ref: /schemas/graph.yaml#/$defs/port-base
> > unevaluatedProperties: false
> > - description: Video port for DP output
> > + description:
> > + Video port for DP output. Each endpoint connects to a video output
> > + downstream, and the "data-lanes" property is used to describe the pin
> > + connections. 0, 1, 2, 3 in "data-lanes" maps to TX0, TX1, TX2, TX3,
> > + respectively.
> >
> > - properties:
> > - endpoint:
> > - $ref: /schemas/graph.yaml#/$defs/endpoint-base
> > +
> > + patternProperties:
> > + "^endpoint@[01]$":
> > + $ref: /schemas/media/video-interfaces.yaml#
> > unevaluatedProperties: false
> >
> > properties:
> > + reg: true
> > +
> > + remote-endpoint: true
> > +
> > data-lanes:
> > - minItems: 1
> > - uniqueItems: true
> > - items:
> > - - enum: [ 0, 1 ]
> > - - const: 1
> > - - const: 2
> > - - const: 3
> > + oneOf:
> > + - items:
> > + - enum: [0, 1, 2, 3]
> > +
> > + - items:
> > + - const: 0
> > + - const: 1
> > +
> > + - items:
> > + - const: 2
> > + - const: 3
> > +
> > + - items:
> > + - const: 0
> > + - const: 1
> > + - const: 2
> > + - const: 3
> > +
> > + mode-switch:
> > + type: boolean
> > + description: Register this node as a Type-C mode switch or not.
>
> Existing users put this property in the device's node, not the endpoint.
> That seems more like a property of the device, than the DP link.
In our use case, we want to register two mode switches for the same
device. That's why we put the "mode-switch" property in the endpoints
instead of the device node.
>
> You are using fwnode_typec_mux_get(), right?
Yes. This is called by cros_ec_typec.c[1] in our use case.
[1]: https://elixir.bootlin.com/linux/latest/source/drivers/platform/chrome/cros_ec_typec.c#L148
Regards,
Pin-yen
>
> Rob
Hi Sakari,
Thanks for the review.
On Mon, Feb 6, 2023 at 5:11 AM Sakari Ailus
<[email protected]> wrote:
>
> Hi Pin-yen,
>
> On Sat, Feb 04, 2023 at 09:30:32PM +0800, Pin-yen Lin wrote:
> > From: Prashant Malani <[email protected]>
> >
> > When searching the device graph for device matches, check the
> > remote-endpoint itself for a match.
> >
> > Some drivers register devices for individual endpoints. This allows
> > the matcher code to evaluate those for a match too, instead
> > of only looking at the remote parent devices. This is required when a
> > device supports two mode switches in its endpoints, so we can't simply
> > register the mode switch with the parent node.
> >
> > Signed-off-by: Prashant Malani <[email protected]>
> > Signed-off-by: Pin-yen Lin <[email protected]>
> > Reviewed-by: Chen-Yu Tsai <[email protected]>
> > Tested-by: Chen-Yu Tsai <[email protected]>
>
> Thanks for the update.
>
> I intended to give my Reviewed-by: but there's something still needs to be
> addressed. See below.
>
> >
> > ---
> >
> > Changes in v11:
> > - Added missing fwnode_handle_put in drivers/base/property.c
> >
> > Changes in v10:
> > - Collected Reviewed-by and Tested-by tags
> >
> > Changes in v6:
> > - New in v6
> >
> > drivers/base/property.c | 16 ++++++++++++++++
> > 1 file changed, 16 insertions(+)
> >
> > diff --git a/drivers/base/property.c b/drivers/base/property.c
> > index 2a5a37fcd998..e6f915b72eb7 100644
> > --- a/drivers/base/property.c
> > +++ b/drivers/base/property.c
> > @@ -1223,6 +1223,22 @@ static unsigned int fwnode_graph_devcon_matches(struct fwnode_handle *fwnode,
> > break;
> > }
> >
> > + /*
> > + * Some drivers may register devices for endpoints. Check
> > + * the remote-endpoints for matches in addition to the remote
> > + * port parent.
> > + */
> > + node = fwnode_graph_get_remote_endpoint(ep);
>
> Here fwnode_graph_get_remote_endpoint() returns an endpoint...
>
> > + if (fwnode_device_is_available(node)) {
>
> and you're calling fwnode_device_is_available() on the endpoint node, which
> always returns true.
>
> Shouldn't you call this on the device node instead? What about match()
> below?
Yes we should have checked the availability on the device node itself
instead of the endpoint node. But regarding the match() call, we need
to call it with the endpoint node because that's where we put the
"mode-switch" properties and register the mode switches on. We can't
use the device node because we want to register two mode switches for
the same device node.
Regards,
Pin-yen
>
> > + ret = match(node, con_id, data);
> > + if (ret) {
> > + if (matches)
> > + matches[count] = ret;
> > + count++;
> > + }
> > + }
> > + fwnode_handle_put(node);
> > +
> > node = fwnode_graph_get_remote_port_parent(ep);
> > if (!fwnode_device_is_available(node)) {
> > fwnode_handle_put(node);
>
> --
> Kind regards,
>
> Sakari Ailus
Hi Pin-yen,
On Thu, Feb 09, 2023 at 12:28:33PM +0800, Pin-yen Lin wrote:
> Hi Sakari,
>
> Thanks for the review.
>
> On Mon, Feb 6, 2023 at 5:11 AM Sakari Ailus
> <[email protected]> wrote:
> >
> > Hi Pin-yen,
> >
> > On Sat, Feb 04, 2023 at 09:30:32PM +0800, Pin-yen Lin wrote:
> > > From: Prashant Malani <[email protected]>
> > >
> > > When searching the device graph for device matches, check the
> > > remote-endpoint itself for a match.
> > >
> > > Some drivers register devices for individual endpoints. This allows
> > > the matcher code to evaluate those for a match too, instead
> > > of only looking at the remote parent devices. This is required when a
> > > device supports two mode switches in its endpoints, so we can't simply
> > > register the mode switch with the parent node.
> > >
> > > Signed-off-by: Prashant Malani <[email protected]>
> > > Signed-off-by: Pin-yen Lin <[email protected]>
> > > Reviewed-by: Chen-Yu Tsai <[email protected]>
> > > Tested-by: Chen-Yu Tsai <[email protected]>
> >
> > Thanks for the update.
> >
> > I intended to give my Reviewed-by: but there's something still needs to be
> > addressed. See below.
> >
> > >
> > > ---
> > >
> > > Changes in v11:
> > > - Added missing fwnode_handle_put in drivers/base/property.c
> > >
> > > Changes in v10:
> > > - Collected Reviewed-by and Tested-by tags
> > >
> > > Changes in v6:
> > > - New in v6
> > >
> > > drivers/base/property.c | 16 ++++++++++++++++
> > > 1 file changed, 16 insertions(+)
> > >
> > > diff --git a/drivers/base/property.c b/drivers/base/property.c
> > > index 2a5a37fcd998..e6f915b72eb7 100644
> > > --- a/drivers/base/property.c
> > > +++ b/drivers/base/property.c
> > > @@ -1223,6 +1223,22 @@ static unsigned int fwnode_graph_devcon_matches(struct fwnode_handle *fwnode,
> > > break;
> > > }
> > >
> > > + /*
> > > + * Some drivers may register devices for endpoints. Check
> > > + * the remote-endpoints for matches in addition to the remote
> > > + * port parent.
> > > + */
> > > + node = fwnode_graph_get_remote_endpoint(ep);
> >
> > Here fwnode_graph_get_remote_endpoint() returns an endpoint...
> >
> > > + if (fwnode_device_is_available(node)) {
> >
> > and you're calling fwnode_device_is_available() on the endpoint node, which
> > always returns true.
> >
> > Shouldn't you call this on the device node instead? What about match()
> > below?
>
> Yes we should have checked the availability on the device node itself
> instead of the endpoint node. But regarding the match() call, we need
> to call it with the endpoint node because that's where we put the
> "mode-switch" properties and register the mode switches on. We can't
> use the device node because we want to register two mode switches for
> the same device node.
Ok.
I think it should be documented for both fwnode_connection_find_match() and
fwnode_connection_find_matches() may then be also called with the endpoint
node.
--
Regards,
Sakari Ailus
On Wed, Feb 8, 2023 at 10:00 PM Pin-yen Lin <[email protected]> wrote:
>
> Hi Rob,
>
> Thanks for the review.
>
> On Wed, Feb 8, 2023 at 4:52 AM Rob Herring <[email protected]> wrote:
> >
> > On Sat, Feb 04, 2023 at 09:30:38PM +0800, Pin-yen Lin wrote:
> > > ITE IT6505 can be used in systems to switch the DP traffic between
> > > two downstreams, which can be USB Type-C DisplayPort alternate mode
> > > lane or regular DisplayPort output ports.
> > >
> > > Update the binding to accommodate this usage by introducing a
> > > data-lanes and a mode-switch property on endpoints.
> > >
> > > Signed-off-by: Pin-yen Lin <[email protected]>
> > >
> > > ---
> > >
> > > Changes in v11:
> > > - Updated the description of the endpoints in the bindings
> > > - Referenced video-interfaces.yaml instead for the endpoints binding
> > > - Removed duplicated definitions from inherited schema
> > >
> > > Changes in v9:
> > > - Fixed subject prefix again
> > > - Changed the naming of the example node for it6505
> > >
> > > Changes in v8:
> > > - Updated bindings for data-lanes property
> > > - Fixed subject prefix
> > >
> > > Changes in v7:
> > > - Fixed issues reported by dt_binding_check.
> > > - Updated the schema and the example dts for data-lanes.
> > > - Changed to generic naming for the example dts node.
> > >
> > > Changes in v6:
> > > - Remove switches node and use endpoints and data-lanes property to
> > > describe the connections.
> > >
> > > .../bindings/display/bridge/ite,it6505.yaml | 101 +++++++++++++++---
> > > 1 file changed, 88 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/display/bridge/ite,it6505.yaml b/Documentation/devicetree/bindings/display/bridge/ite,it6505.yaml
> > > index b16a9d9127dd..8ae9c5cba22c 100644
> > > --- a/Documentation/devicetree/bindings/display/bridge/ite,it6505.yaml
> > > +++ b/Documentation/devicetree/bindings/display/bridge/ite,it6505.yaml
> > > @@ -75,22 +75,49 @@ properties:
> > > port@1:
> > > $ref: /schemas/graph.yaml#/$defs/port-base
> > > unevaluatedProperties: false
> > > - description: Video port for DP output
> > > + description:
> > > + Video port for DP output. Each endpoint connects to a video output
> > > + downstream, and the "data-lanes" property is used to describe the pin
> > > + connections. 0, 1, 2, 3 in "data-lanes" maps to TX0, TX1, TX2, TX3,
> > > + respectively.
> > >
> > > - properties:
> > > - endpoint:
> > > - $ref: /schemas/graph.yaml#/$defs/endpoint-base
> > > +
> > > + patternProperties:
> > > + "^endpoint@[01]$":
> > > + $ref: /schemas/media/video-interfaces.yaml#
> > > unevaluatedProperties: false
> > >
> > > properties:
> > > + reg: true
> > > +
> > > + remote-endpoint: true
> > > +
> > > data-lanes:
> > > - minItems: 1
> > > - uniqueItems: true
> > > - items:
> > > - - enum: [ 0, 1 ]
> > > - - const: 1
> > > - - const: 2
> > > - - const: 3
> > > + oneOf:
> > > + - items:
> > > + - enum: [0, 1, 2, 3]
> > > +
> > > + - items:
> > > + - const: 0
> > > + - const: 1
> > > +
> > > + - items:
> > > + - const: 2
> > > + - const: 3
> > > +
> > > + - items:
> > > + - const: 0
> > > + - const: 1
> > > + - const: 2
> > > + - const: 3
> > > +
> > > + mode-switch:
> > > + type: boolean
> > > + description: Register this node as a Type-C mode switch or not.
> >
> > Existing users put this property in the device's node, not the endpoint.
> > That seems more like a property of the device, than the DP link.
>
> In our use case, we want to register two mode switches for the same
> device. That's why we put the "mode-switch" property in the endpoints
> instead of the device node.
Then do that. Register a mode switch for each endpoint connected to a
USB-C connector. You can walk the graph to see what type of connector.
The only way I could see this as an issue is you have 2 USB-C
connectors and one is a mode switch and one is not. Not sure if such a
scenario is likely or possible. If it is, please educate me.
> > You are using fwnode_typec_mux_get(), right?
>
> Yes. This is called by cros_ec_typec.c[1] in our use case.
That code looks for 'mode-switch' in the device's node, not the
endpoint. So how does it work for you?
Rob
On Thu, Feb 9, 2023 at 9:58 PM Rob Herring <[email protected]> wrote:
>
> On Wed, Feb 8, 2023 at 10:00 PM Pin-yen Lin <[email protected]> wrote:
> >
> > Hi Rob,
> >
> > Thanks for the review.
> >
> > On Wed, Feb 8, 2023 at 4:52 AM Rob Herring <[email protected]> wrote:
> > >
> > > On Sat, Feb 04, 2023 at 09:30:38PM +0800, Pin-yen Lin wrote:
> > > > ITE IT6505 can be used in systems to switch the DP traffic between
> > > > two downstreams, which can be USB Type-C DisplayPort alternate mode
> > > > lane or regular DisplayPort output ports.
> > > >
> > > > Update the binding to accommodate this usage by introducing a
> > > > data-lanes and a mode-switch property on endpoints.
> > > >
> > > > Signed-off-by: Pin-yen Lin <[email protected]>
> > > >
> > > > ---
> > > >
> > > > Changes in v11:
> > > > - Updated the description of the endpoints in the bindings
> > > > - Referenced video-interfaces.yaml instead for the endpoints binding
> > > > - Removed duplicated definitions from inherited schema
> > > >
> > > > Changes in v9:
> > > > - Fixed subject prefix again
> > > > - Changed the naming of the example node for it6505
> > > >
> > > > Changes in v8:
> > > > - Updated bindings for data-lanes property
> > > > - Fixed subject prefix
> > > >
> > > > Changes in v7:
> > > > - Fixed issues reported by dt_binding_check.
> > > > - Updated the schema and the example dts for data-lanes.
> > > > - Changed to generic naming for the example dts node.
> > > >
> > > > Changes in v6:
> > > > - Remove switches node and use endpoints and data-lanes property to
> > > > describe the connections.
> > > >
> > > > .../bindings/display/bridge/ite,it6505.yaml | 101 +++++++++++++++---
> > > > 1 file changed, 88 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/display/bridge/ite,it6505.yaml b/Documentation/devicetree/bindings/display/bridge/ite,it6505.yaml
> > > > index b16a9d9127dd..8ae9c5cba22c 100644
> > > > --- a/Documentation/devicetree/bindings/display/bridge/ite,it6505.yaml
> > > > +++ b/Documentation/devicetree/bindings/display/bridge/ite,it6505.yaml
> > > > @@ -75,22 +75,49 @@ properties:
> > > > port@1:
> > > > $ref: /schemas/graph.yaml#/$defs/port-base
> > > > unevaluatedProperties: false
> > > > - description: Video port for DP output
> > > > + description:
> > > > + Video port for DP output. Each endpoint connects to a video output
> > > > + downstream, and the "data-lanes" property is used to describe the pin
> > > > + connections. 0, 1, 2, 3 in "data-lanes" maps to TX0, TX1, TX2, TX3,
> > > > + respectively.
> > > >
> > > > - properties:
> > > > - endpoint:
> > > > - $ref: /schemas/graph.yaml#/$defs/endpoint-base
> > > > +
> > > > + patternProperties:
> > > > + "^endpoint@[01]$":
> > > > + $ref: /schemas/media/video-interfaces.yaml#
> > > > unevaluatedProperties: false
> > > >
> > > > properties:
> > > > + reg: true
> > > > +
> > > > + remote-endpoint: true
> > > > +
> > > > data-lanes:
> > > > - minItems: 1
> > > > - uniqueItems: true
> > > > - items:
> > > > - - enum: [ 0, 1 ]
> > > > - - const: 1
> > > > - - const: 2
> > > > - - const: 3
> > > > + oneOf:
> > > > + - items:
> > > > + - enum: [0, 1, 2, 3]
> > > > +
> > > > + - items:
> > > > + - const: 0
> > > > + - const: 1
> > > > +
> > > > + - items:
> > > > + - const: 2
> > > > + - const: 3
> > > > +
> > > > + - items:
> > > > + - const: 0
> > > > + - const: 1
> > > > + - const: 2
> > > > + - const: 3
> > > > +
> > > > + mode-switch:
> > > > + type: boolean
> > > > + description: Register this node as a Type-C mode switch or not.
> > >
> > > Existing users put this property in the device's node, not the endpoint.
> > > That seems more like a property of the device, than the DP link.
> >
> > In our use case, we want to register two mode switches for the same
> > device. That's why we put the "mode-switch" property in the endpoints
> > instead of the device node.
>
> Then do that. Register a mode switch for each endpoint connected to a
> USB-C connector. You can walk the graph to see what type of connector.
>
> The only way I could see this as an issue is you have 2 USB-C
> connectors and one is a mode switch and one is not. Not sure if such a
> scenario is likely or possible. If it is, please educate me.
We can know which endpoints should be registered as a MUX by walking
through the graph, but the typec_mux_match[1] checks if the node
explicitly specifies a "mode-switch" property. So we still have to put
the property in the endpoints.
[1]: https://elixir.bootlin.com/linux/latest/source/drivers/usb/typec/mux.c#L265
>
> > > You are using fwnode_typec_mux_get(), right?
> >
> > Yes. This is called by cros_ec_typec.c[1] in our use case.
>
> That code looks for 'mode-switch' in the device's node, not the
> endpoint. So how does it work for you?
We modified the function in patch 1/9 of this series to make it also
look for endpoints.
>
> Rob
Regards,
Pin-yen
On Sat, Feb 04, 2023 at 09:30:37PM +0800, Pin-yen Lin wrote:
[..]
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
[..]
> +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);
This seems inverted compared to the binding. Binding says
0, 1, 2, 3 in "data-lanes" maps to SSRX1, SSTX1, SSRX2, SSTX2, respectively.
But in anx7625_register_typec_switches(), lanes 0-1 mean orientation normal,
then in this logic, you set RX2 and TX2 to carry the DP signals. So the driver
is mapping lanes 0-1 to SSRX2/SSTX2 and lanes 2-3 to SSRX1/SSTX1, the opposite
from the binding.
Thanks,
N?colas
> + } 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 int anx7625_register_typec_switches(struct device *dev, struct anx7625_data *ctx)
> +{
[..]
> + ctx->port_data[i].orientation = (dp_lanes[0] / 2 == 0) ?
> + TYPEC_ORIENTATION_NORMAL : TYPEC_ORIENTATION_REVERSE;
[..]
I think I accidentally used HTML mode for the previous email. Sorry about that.
On Mon, Feb 20, 2023 at 4:41 PM Pin-yen Lin <[email protected]> wrote:
>
> Hi Andi,
>
> Thanks for the review.
>
> On Wed, Feb 8, 2023 at 5:25 AM Andi Shyti <[email protected]> wrote:
>>
>> Hi Pin-yen,
>>
>> [...]
>>
>> > +static int drm_dp_register_mode_switch(struct device *dev,
>> > + struct fwnode_handle *fwnode,
>> > + struct drm_dp_typec_switch_desc *switch_desc,
>> > + void *data, typec_mux_set_fn_t mux_set)
>> > +{
>> > + struct drm_dp_typec_port_data *port_data;
>> > + struct typec_mux_desc mux_desc = {};
>> > + char name[32];
>> > + u32 port_num;
>> > + int ret;
>> > +
>> > + ret = fwnode_property_read_u32(fwnode, "reg", &port_num);
>> > + if (ret) {
>> > + dev_err(dev, "Failed to read reg property: %d\n", ret);
>> > + return ret;
>> > + }
>> > +
>> > + port_data = &switch_desc->typec_ports[port_num];
>> > + port_data->data = data;
>> > + port_data->port_num = port_num;
>> > + port_data->fwnode = fwnode;
>> > + mux_desc.fwnode = fwnode;
>> > + mux_desc.drvdata = port_data;
>> > + snprintf(name, sizeof(name), "%pfwP-%u", fwnode, port_num);
>> > + mux_desc.name = name;
>> > + mux_desc.set = 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\n",
>> > + port_num, ret);
>> > +
>> > + return ret;
>>
>> you don't need this return here...
>>
>> > + }
>> > +
>> > + return 0;
>>
>> Just "return ret;" here.
This was actually suggested by Angelo in [1]. I personally don't have
a strong opinion on either approach.
[1]https://lore.kernel.org/all/[email protected]/
Pin-yen
>>
>>
>> > +}
>> > +
>> > +/**
>> > + * drm_dp_register_typec_switches() - register Type-C switches
>> > + * @dev: Device that registers Type-C switches
>> > + * @port: Device node for the switch
>> > + * @switch_desc: A Type-C switch descriptor
>> > + * @data: Private data for the switches
>> > + * @mux_set: Callback function for typec_mux_set
>> > + *
>> > + * This function registers USB Type-C switches for DP bridges that can switch
>> > + * the output signal between their output pins.
>> > + *
>> > + * Currently only mode switches are implemented, and the function assumes the
>> > + * given @port device node has endpoints with "mode-switch" property.
>> > + * The port number is determined by the "reg" property of the endpoint.
>> > + */
>> > +int drm_dp_register_typec_switches(struct device *dev, struct fwnode_handle *port,
>> > + struct drm_dp_typec_switch_desc *switch_desc,
>> > + void *data, typec_mux_set_fn_t mux_set)
>> > +{
>> > + struct fwnode_handle *sw;
>> > + int ret;
>> > +
>> > + fwnode_for_each_child_node(port, sw) {
>> > + if (fwnode_property_present(sw, "mode-switch"))
>> > + switch_desc->num_typec_switches++;
>> > + }
>>
>> no need for brackets here
>>
>> > +
>> > + if (!switch_desc->num_typec_switches) {
>> > + dev_dbg(dev, "No Type-C switches node found\n");
>>
>> dev_warn()?
>
>
> I used dev_dbg here because the users might call this without checking if there are mode switch endpoints present, and this is the case for the current users (it6505 and anx7625). If we use dev_warn here, there will be warnings every time even on use cases without Type-C switches.
>
> Thanks and regards,
> Pin-yen
>>
>>
>> > + return 0;
>> > + }
>> > +
>> > + switch_desc->typec_ports = devm_kcalloc(
>> > + dev, switch_desc->num_typec_switches,
>> > + sizeof(struct drm_dp_typec_port_data), GFP_KERNEL);
>> > +
>> > + if (!switch_desc->typec_ports)
>> > + return -ENOMEM;
>> > +
>> > + /* Register switches for each connector. */
>> > + fwnode_for_each_child_node(port, sw) {
>> > + if (!fwnode_property_present(sw, "mode-switch"))
>> > + continue;
>> > + ret = drm_dp_register_mode_switch(dev, sw, switch_desc, data, mux_set);
>> > + if (ret)
>> > + goto err_unregister_typec_switches;
>> > + }
>> > +
>> > + return 0;
>> > +
>> > +err_unregister_typec_switches:
>> > + fwnode_handle_put(sw);
>> > + drm_dp_unregister_typec_switches(switch_desc);
>> > + dev_err(dev, "Failed to register mode switch: %d\n", ret);
>>
>> there is a bit of dmesg spamming. Please choose where you want to
>> print the error, either in this function or in
>> drm_dp_register_mode_switch().
>>
>> Andi
>>
>> > + return ret;
>> > +}
>> > +EXPORT_SYMBOL(drm_dp_register_typec_switches);
>>
>> [...]