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.
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.
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 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 | 99 ++++++++++++-
.../bindings/display/bridge/ite,it6505.yaml | 93 ++++++++++--
drivers/base/property.c | 15 ++
drivers/gpu/drm/bridge/Kconfig | 21 +--
drivers/gpu/drm/bridge/analogix/Kconfig | 1 +
drivers/gpu/drm/bridge/analogix/anx7625.c | 105 +++++++++++++-
drivers/gpu/drm/bridge/analogix/anx7625.h | 13 ++
drivers/gpu/drm/bridge/ite-it6505.c | 119 +++++++++++++++-
drivers/gpu/drm/display/drm_dp_helper.c | 134 ++++++++++++++++++
drivers/platform/chrome/cros_ec_typec.c | 10 ++
include/drm/display/drm_dp_helper.h | 17 +++
11 files changed, 598 insertions(+), 29 deletions(-)
--
2.39.0.314.g84b9a713c41-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 v10:
- Collected Reviewed-by and Tested-by tags
Changes in v6:
- New in v6
drivers/base/property.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/drivers/base/property.c b/drivers/base/property.c
index 2a5a37fcd998..48877af4e444 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -1223,6 +1223,21 @@ 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++;
+ }
+ }
+
node = fwnode_graph_get_remote_port_parent(ep);
if (!fwnode_device_is_available(node)) {
fwnode_handle_put(node);
--
2.39.0.314.g84b9a713c41-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 "data-lanes" properties are available in Device Tree.
Signed-off-by: Pin-yen Lin <[email protected]>
Tested-by: Chen-Yu Tsai <[email protected]>
Reviewed-by: Chen-Yu Tsai <[email protected]>
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
---
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/drm_dp_helper.c | 134 ++++++++++++++++++++++++
include/drm/display/drm_dp_helper.h | 17 +++
2 files changed, 151 insertions(+)
diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
index 16565a0a5da6..a2ec40a621cb 100644
--- a/drivers/gpu/drm/display/drm_dp_helper.c
+++ b/drivers/gpu/drm/display/drm_dp_helper.c
@@ -30,11 +30,13 @@
#include <linux/sched.h>
#include <linux/seq_file.h>
#include <linux/string_helpers.h>
+#include <linux/usb/typec_mux.h>
#include <linux/dynamic_debug.h>
#include <drm/display/drm_dp_helper.h>
#include <drm/display/drm_dp_mst_helper.h>
#include <drm/drm_edid.h>
+#include <drm/drm_of.h>
#include <drm/drm_print.h>
#include <drm/drm_vblank.h>
#include <drm/drm_panel.h>
@@ -3891,3 +3893,135 @@ int drm_panel_dp_aux_backlight(struct drm_panel *panel, struct drm_dp_aux *aux)
EXPORT_SYMBOL(drm_panel_dp_aux_backlight);
#endif
+
+#if IS_REACHABLE(CONFIG_TYPEC)
+static int drm_dp_register_mode_switch(struct device *dev, struct device_node *node,
+ 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 dp_lanes[2];
+ int ret, num_lanes, port_num = -1;
+
+ num_lanes = drm_of_get_data_lanes_count(node, 0, 2);
+ if (num_lanes <= 0) {
+ dev_err(dev, "Error on getting data lanes count from %s: %d\n",
+ node->name, num_lanes);
+ return num_lanes;
+ }
+
+ ret = of_property_read_u32_array(node, "data-lanes", dp_lanes, num_lanes);
+ if (ret) {
+ dev_err(dev, "Failed to read the data-lanes variable from %s: %d\n",
+ node->name, ret);
+ return ret;
+ }
+
+ port_num = dp_lanes[0] / 2;
+
+ port_data = &switch_desc->typec_ports[port_num];
+ port_data->data = data;
+ 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 = 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.
+ * Register the endpoint as port 0 if the "data-lanes" property falls in 0/1,
+ * and register it as port 1 if "data-lanes" falls in 2/3.
+ */
+int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
+ struct drm_dp_typec_switch_desc *switch_desc,
+ void *data, typec_mux_set_fn_t mux_set)
+{
+ struct device_node *sw;
+ int ret;
+
+ for_each_child_of_node(port, sw) {
+ if (of_property_read_bool(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. */
+ for_each_child_of_node(port, sw) {
+ if (!of_property_read_bool(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:
+ of_node_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);
+#else
+void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc)
+{
+}
+EXPORT_SYMBOL(drm_dp_register_typec_switches);
+int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
+ struct drm_dp_typec_switch_desc *switch_desc,
+ void *data, typec_mux_set_fn_t mux_set)
+{
+ return 0;
+}
+EXPORT_SYMBOL(drm_dp_unregister_typec_switches);
+#endif
diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
index ab55453f2d2c..5a3824f13b4e 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,20 @@ 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;
+ void *data;
+ bool dp_connected;
+};
+
+struct drm_dp_typec_switch_desc {
+ int num_typec_switches;
+ struct drm_dp_typec_port_data *typec_ports;
+};
+
+void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc);
+int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
+ struct drm_dp_typec_switch_desc *switch_desc,
+ void *data, typec_mux_set_fn_t mux_set);
+
#endif /* _DRM_DP_HELPER_H_ */
--
2.39.0.314.g84b9a713c41-goog
On 12/01/2023 06:20, Pin-yen Lin 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.
Please cc everybody on all patches. Having received just a single patch
made me spend time on having to look them up on lore.
>
> 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.
>
> 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 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 | 99 ++++++++++++-
> .../bindings/display/bridge/ite,it6505.yaml | 93 ++++++++++--
> drivers/base/property.c | 15 ++
> drivers/gpu/drm/bridge/Kconfig | 21 +--
> drivers/gpu/drm/bridge/analogix/Kconfig | 1 +
> drivers/gpu/drm/bridge/analogix/anx7625.c | 105 +++++++++++++-
> drivers/gpu/drm/bridge/analogix/anx7625.h | 13 ++
> drivers/gpu/drm/bridge/ite-it6505.c | 119 +++++++++++++++-
> drivers/gpu/drm/display/drm_dp_helper.c | 134 ++++++++++++++++++
> drivers/platform/chrome/cros_ec_typec.c | 10 ++
> include/drm/display/drm_dp_helper.h | 17 +++
> 11 files changed, 598 insertions(+), 29 deletions(-)
>
--
With best wishes
Dmitry
On 12/01/2023 06:20, 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 "data-lanes" properties are available in Device Tree.
>
> Signed-off-by: Pin-yen Lin <[email protected]>
> Tested-by: Chen-Yu Tsai <[email protected]>
> Reviewed-by: Chen-Yu Tsai <[email protected]>
> Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
>
> ---
>
> 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/drm_dp_helper.c | 134 ++++++++++++++++++++++++
> include/drm/display/drm_dp_helper.h | 17 +++
> 2 files changed, 151 insertions(+)
>
> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> index 16565a0a5da6..a2ec40a621cb 100644
> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> @@ -30,11 +30,13 @@
> #include <linux/sched.h>
> #include <linux/seq_file.h>
> #include <linux/string_helpers.h>
> +#include <linux/usb/typec_mux.h>
> #include <linux/dynamic_debug.h>
>
> #include <drm/display/drm_dp_helper.h>
> #include <drm/display/drm_dp_mst_helper.h>
> #include <drm/drm_edid.h>
> +#include <drm/drm_of.h>
> #include <drm/drm_print.h>
> #include <drm/drm_vblank.h>
> #include <drm/drm_panel.h>
> @@ -3891,3 +3893,135 @@ int drm_panel_dp_aux_backlight(struct drm_panel *panel, struct drm_dp_aux *aux)
> EXPORT_SYMBOL(drm_panel_dp_aux_backlight);
>
> #endif
> +
> +#if IS_REACHABLE(CONFIG_TYPEC)
> +static int drm_dp_register_mode_switch(struct device *dev, struct device_node *node,
> + 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 dp_lanes[2];
> + int ret, num_lanes, port_num = -1;
> +
> + num_lanes = drm_of_get_data_lanes_count(node, 0, 2);
2 looks incorrect. IIRC DP altmode can support up to 4 lanes.
> + if (num_lanes <= 0) {
> + dev_err(dev, "Error on getting data lanes count from %s: %d\n",
> + node->name, num_lanes);
> + return num_lanes;
> + }
> +
> + ret = of_property_read_u32_array(node, "data-lanes", dp_lanes, num_lanes);
> + if (ret) {
> + dev_err(dev, "Failed to read the data-lanes variable from %s: %d\n",
> + node->name, ret);
> + return ret;
> + }
> +
> + port_num = dp_lanes[0] / 2;
> +
> + port_data = &switch_desc->typec_ports[port_num];
> + port_data->data = data;
> + 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 = 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.
> + * Register the endpoint as port 0 if the "data-lanes" property falls in 0/1,
> + * and register it as port 1 if "data-lanes" falls in 2/3.
> + */
> +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
> + struct drm_dp_typec_switch_desc *switch_desc,
> + void *data, typec_mux_set_fn_t mux_set)
> +{
> + struct device_node *sw;
> + int ret;
> +
> + for_each_child_of_node(port, sw) {
> + if (of_property_read_bool(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. */
> + for_each_child_of_node(port, sw) {
> + if (!of_property_read_bool(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:
> + of_node_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);
> +#else
> +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc)
> +{
> +}
> +EXPORT_SYMBOL(drm_dp_register_typec_switches);
> +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
> + struct drm_dp_typec_switch_desc *switch_desc,
> + void *data, typec_mux_set_fn_t mux_set)
> +{
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_dp_unregister_typec_switches);
> +#endif
> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> index ab55453f2d2c..5a3824f13b4e 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,20 @@ 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;
> + void *data;
> + bool dp_connected;
> +};
> +
> +struct drm_dp_typec_switch_desc {
> + int num_typec_switches;
> + struct drm_dp_typec_port_data *typec_ports;
> +};
> +
> +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc);
> +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
> + struct drm_dp_typec_switch_desc *switch_desc,
> + void *data, typec_mux_set_fn_t mux_set);
> +
> #endif /* _DRM_DP_HELPER_H_ */
--
With best wishes
Dmitry
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]>
---
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.0.314.g84b9a713c41-goog
Hi Dmitry,
On Thu, Jan 12, 2023 at 12:34 PM Dmitry Baryshkov
<[email protected]> wrote:
>
> On 12/01/2023 06:20, Pin-yen Lin 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.
>
> Please cc everybody on all patches. Having received just a single patch
> made me spend time on having to look them up on lore.
Sorry about that. I'll prevent that in the future series.
>
> >
> > 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.
> >
> > 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 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 | 99 ++++++++++++-
> > .../bindings/display/bridge/ite,it6505.yaml | 93 ++++++++++--
> > drivers/base/property.c | 15 ++
> > drivers/gpu/drm/bridge/Kconfig | 21 +--
> > drivers/gpu/drm/bridge/analogix/Kconfig | 1 +
> > drivers/gpu/drm/bridge/analogix/anx7625.c | 105 +++++++++++++-
> > drivers/gpu/drm/bridge/analogix/anx7625.h | 13 ++
> > drivers/gpu/drm/bridge/ite-it6505.c | 119 +++++++++++++++-
> > drivers/gpu/drm/display/drm_dp_helper.c | 134 ++++++++++++++++++
> > drivers/platform/chrome/cros_ec_typec.c | 10 ++
> > include/drm/display/drm_dp_helper.h | 17 +++
> > 11 files changed, 598 insertions(+), 29 deletions(-)
> >
>
> --
> With best wishes
> Dmitry
>
Thanks and regards,
Pin-yen
Hi Dmitry,
Thanks for the review.
On Thu, Jan 12, 2023 at 12:40 PM Dmitry Baryshkov
<[email protected]> wrote:
>
> On 12/01/2023 06:20, 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 "data-lanes" properties are available in Device Tree.
> >
> > Signed-off-by: Pin-yen Lin <[email protected]>
> > Tested-by: Chen-Yu Tsai <[email protected]>
> > Reviewed-by: Chen-Yu Tsai <[email protected]>
> > Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
> >
> > ---
> >
> > 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/drm_dp_helper.c | 134 ++++++++++++++++++++++++
> > include/drm/display/drm_dp_helper.h | 17 +++
> > 2 files changed, 151 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> > index 16565a0a5da6..a2ec40a621cb 100644
> > --- a/drivers/gpu/drm/display/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> > @@ -30,11 +30,13 @@
> > #include <linux/sched.h>
> > #include <linux/seq_file.h>
> > #include <linux/string_helpers.h>
> > +#include <linux/usb/typec_mux.h>
> > #include <linux/dynamic_debug.h>
> >
> > #include <drm/display/drm_dp_helper.h>
> > #include <drm/display/drm_dp_mst_helper.h>
> > #include <drm/drm_edid.h>
> > +#include <drm/drm_of.h>
> > #include <drm/drm_print.h>
> > #include <drm/drm_vblank.h>
> > #include <drm/drm_panel.h>
> > @@ -3891,3 +3893,135 @@ int drm_panel_dp_aux_backlight(struct drm_panel *panel, struct drm_dp_aux *aux)
> > EXPORT_SYMBOL(drm_panel_dp_aux_backlight);
> >
> > #endif
> > +
> > +#if IS_REACHABLE(CONFIG_TYPEC)
> > +static int drm_dp_register_mode_switch(struct device *dev, struct device_node *node,
> > + 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 dp_lanes[2];
> > + int ret, num_lanes, port_num = -1;
> > +
> > + num_lanes = drm_of_get_data_lanes_count(node, 0, 2);
>
> 2 looks incorrect. IIRC DP altmode can support up to 4 lanes.
This function is implemented for 4-lane DP bridges to switch its
outputs between 2 downstreams. So, I assume that there will only be at
most 2 lanes for each downstream. I don't think a 4-lane downstream
makes sense for mode switches unless we want to support bridges with
more than 4 lanes.
>
> > + if (num_lanes <= 0) {
> > + dev_err(dev, "Error on getting data lanes count from %s: %d\n",
> > + node->name, num_lanes);
> > + return num_lanes;
> > + }
> > +
> > + ret = of_property_read_u32_array(node, "data-lanes", dp_lanes, num_lanes);
> > + if (ret) {
> > + dev_err(dev, "Failed to read the data-lanes variable from %s: %d\n",
> > + node->name, ret);
> > + return ret;
> > + }
> > +
> > + port_num = dp_lanes[0] / 2;
> > +
> > + port_data = &switch_desc->typec_ports[port_num];
> > + port_data->data = data;
> > + 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 = 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.
> > + * Register the endpoint as port 0 if the "data-lanes" property falls in 0/1,
> > + * and register it as port 1 if "data-lanes" falls in 2/3.
> > + */
> > +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
> > + struct drm_dp_typec_switch_desc *switch_desc,
> > + void *data, typec_mux_set_fn_t mux_set)
> > +{
> > + struct device_node *sw;
> > + int ret;
> > +
> > + for_each_child_of_node(port, sw) {
> > + if (of_property_read_bool(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. */
> > + for_each_child_of_node(port, sw) {
> > + if (!of_property_read_bool(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:
> > + of_node_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);
> > +#else
> > +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc)
> > +{
> > +}
> > +EXPORT_SYMBOL(drm_dp_register_typec_switches);
> > +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
> > + struct drm_dp_typec_switch_desc *switch_desc,
> > + void *data, typec_mux_set_fn_t mux_set)
> > +{
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(drm_dp_unregister_typec_switches);
> > +#endif
> > diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> > index ab55453f2d2c..5a3824f13b4e 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,20 @@ 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;
> > + void *data;
> > + bool dp_connected;
> > +};
> > +
> > +struct drm_dp_typec_switch_desc {
> > + int num_typec_switches;
> > + struct drm_dp_typec_port_data *typec_ports;
> > +};
> > +
> > +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc);
> > +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
> > + struct drm_dp_typec_switch_desc *switch_desc,
> > + void *data, typec_mux_set_fn_t mux_set);
> > +
> > #endif /* _DRM_DP_HELPER_H_ */
>
> --
> With best wishes
> Dmitry
>
Best regards,
Pin-yen
On Thu, Jan 12, 2023 at 1:24 PM Dmitry Baryshkov
<[email protected]> wrote:
>
> On 12/01/2023 07:19, Pin-yen Lin wrote:
> > Hi Dmitry,
> >
> > Thanks for the review.
> >
> > On Thu, Jan 12, 2023 at 12:40 PM Dmitry Baryshkov
> > <[email protected]> wrote:
> >>
> >> On 12/01/2023 06:20, 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 "data-lanes" properties are available in Device Tree.
> >>>
> >>> Signed-off-by: Pin-yen Lin <[email protected]>
> >>> Tested-by: Chen-Yu Tsai <[email protected]>
> >>> Reviewed-by: Chen-Yu Tsai <[email protected]>
> >>> Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
> >>>
> >>> ---
> >>>
> >>> 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/drm_dp_helper.c | 134 ++++++++++++++++++++++++
> >>> include/drm/display/drm_dp_helper.h | 17 +++
> >>> 2 files changed, 151 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> >>> index 16565a0a5da6..a2ec40a621cb 100644
> >>> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> >>> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> >>> @@ -30,11 +30,13 @@
> >>> #include <linux/sched.h>
> >>> #include <linux/seq_file.h>
> >>> #include <linux/string_helpers.h>
> >>> +#include <linux/usb/typec_mux.h>
> >>> #include <linux/dynamic_debug.h>
> >>>
> >>> #include <drm/display/drm_dp_helper.h>
> >>> #include <drm/display/drm_dp_mst_helper.h>
> >>> #include <drm/drm_edid.h>
> >>> +#include <drm/drm_of.h>
> >>> #include <drm/drm_print.h>
> >>> #include <drm/drm_vblank.h>
> >>> #include <drm/drm_panel.h>
> >>> @@ -3891,3 +3893,135 @@ int drm_panel_dp_aux_backlight(struct drm_panel *panel, struct drm_dp_aux *aux)
> >>> EXPORT_SYMBOL(drm_panel_dp_aux_backlight);
> >>>
> >>> #endif
> >>> +
> >>> +#if IS_REACHABLE(CONFIG_TYPEC)
> >>> +static int drm_dp_register_mode_switch(struct device *dev, struct device_node *node,
> >>> + 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 dp_lanes[2];
> >>> + int ret, num_lanes, port_num = -1;
> >>> +
> >>> + num_lanes = drm_of_get_data_lanes_count(node, 0, 2);
> >>
> >> 2 looks incorrect. IIRC DP altmode can support up to 4 lanes.
> >
> > This function is implemented for 4-lane DP bridges to switch its
> > outputs between 2 downstreams. So, I assume that there will only be at
> > most 2 lanes for each downstream. I don't think a 4-lane downstream
> > makes sense for mode switches unless we want to support bridges with
> > more than 4 lanes.
>
> Yes. However by using 4 here you'd make the helper generic and cover
> both your case and the generic case. We don't need this for the msm case
> (since the mux is handled by the PHY). But if not for the PHY, I'd have
> used such helper (with max_lanes = 4).
>
I wonder if simply using 4 here really makes it more generic here.
This function assumes the mapping between "data-lanes" and the port
number (e.g., 0/1 --> port 0) and hard-coded the way to parse the
property.
Is it better to use "reg" instead of "data-lanes" to determine the
port number? The drivers can still read the DT node to get the
"data-lanes" property if they want to do some fancy stuffs around
that.
> >>
> >>> + if (num_lanes <= 0) {
> >>> + dev_err(dev, "Error on getting data lanes count from %s: %d\n",
> >>> + node->name, num_lanes);
> >>> + return num_lanes;
> >>> + }
> >>> +
> >>> + ret = of_property_read_u32_array(node, "data-lanes", dp_lanes, num_lanes);
> >>> + if (ret) {
> >>> + dev_err(dev, "Failed to read the data-lanes variable from %s: %d\n",
> >>> + node->name, ret);
> >>> + return ret;
> >>> + }
> >>> +
> >>> + port_num = dp_lanes[0] / 2;
> >>> +
> >>> + port_data = &switch_desc->typec_ports[port_num];
> >>> + port_data->data = data;
> >>> + 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 = 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.
> >>> + * Register the endpoint as port 0 if the "data-lanes" property falls in 0/1,
> >>> + * and register it as port 1 if "data-lanes" falls in 2/3.
> >>> + */
> >>> +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
> >>> + struct drm_dp_typec_switch_desc *switch_desc,
> >>> + void *data, typec_mux_set_fn_t mux_set)
> >>> +{
> >>> + struct device_node *sw;
> >>> + int ret;
> >>> +
> >>> + for_each_child_of_node(port, sw) {
> >>> + if (of_property_read_bool(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. */
> >>> + for_each_child_of_node(port, sw) {
> >>> + if (!of_property_read_bool(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:
> >>> + of_node_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);
> >>> +#else
> >>> +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc)
> >>> +{
> >>> +}
> >>> +EXPORT_SYMBOL(drm_dp_register_typec_switches);
> >>> +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
> >>> + struct drm_dp_typec_switch_desc *switch_desc,
> >>> + void *data, typec_mux_set_fn_t mux_set)
> >>> +{
> >>> + return 0;
> >>> +}
> >>> +EXPORT_SYMBOL(drm_dp_unregister_typec_switches);
> >>> +#endif
> >>> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> >>> index ab55453f2d2c..5a3824f13b4e 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,20 @@ 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;
> >>> + void *data;
> >>> + bool dp_connected;
> >>> +};
> >>> +
> >>> +struct drm_dp_typec_switch_desc {
> >>> + int num_typec_switches;
> >>> + struct drm_dp_typec_port_data *typec_ports;
> >>> +};
> >>> +
> >>> +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc);
> >>> +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
> >>> + struct drm_dp_typec_switch_desc *switch_desc,
> >>> + void *data, typec_mux_set_fn_t mux_set);
> >>> +
> >>> #endif /* _DRM_DP_HELPER_H_ */
> >>
> >> --
> >> With best wishes
> >> Dmitry
> >>
> >
> > Best regards,
> > Pin-yen
>
> --
> With best wishes
> Dmitry
>
Best regards,
Pin-yen
On 12/01/2023 07:48, Pin-yen Lin wrote:
> On Thu, Jan 12, 2023 at 1:24 PM Dmitry Baryshkov
> <[email protected]> wrote:
>>
>> On 12/01/2023 07:19, Pin-yen Lin wrote:
>>> Hi Dmitry,
>>>
>>> Thanks for the review.
>>>
>>> On Thu, Jan 12, 2023 at 12:40 PM Dmitry Baryshkov
>>> <[email protected]> wrote:
>>>>
>>>> On 12/01/2023 06:20, 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 "data-lanes" properties are available in Device Tree.
>>>>>
>>>>> Signed-off-by: Pin-yen Lin <[email protected]>
>>>>> Tested-by: Chen-Yu Tsai <[email protected]>
>>>>> Reviewed-by: Chen-Yu Tsai <[email protected]>
>>>>> Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
>>>>>
>>>>> ---
>>>>>
>>>>> 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/drm_dp_helper.c | 134 ++++++++++++++++++++++++
>>>>> include/drm/display/drm_dp_helper.h | 17 +++
>>>>> 2 files changed, 151 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
>>>>> index 16565a0a5da6..a2ec40a621cb 100644
>>>>> --- a/drivers/gpu/drm/display/drm_dp_helper.c
>>>>> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
>>>>> @@ -30,11 +30,13 @@
>>>>> #include <linux/sched.h>
>>>>> #include <linux/seq_file.h>
>>>>> #include <linux/string_helpers.h>
>>>>> +#include <linux/usb/typec_mux.h>
>>>>> #include <linux/dynamic_debug.h>
>>>>>
>>>>> #include <drm/display/drm_dp_helper.h>
>>>>> #include <drm/display/drm_dp_mst_helper.h>
>>>>> #include <drm/drm_edid.h>
>>>>> +#include <drm/drm_of.h>
>>>>> #include <drm/drm_print.h>
>>>>> #include <drm/drm_vblank.h>
>>>>> #include <drm/drm_panel.h>
>>>>> @@ -3891,3 +3893,135 @@ int drm_panel_dp_aux_backlight(struct drm_panel *panel, struct drm_dp_aux *aux)
>>>>> EXPORT_SYMBOL(drm_panel_dp_aux_backlight);
>>>>>
>>>>> #endif
>>>>> +
>>>>> +#if IS_REACHABLE(CONFIG_TYPEC)
>>>>> +static int drm_dp_register_mode_switch(struct device *dev, struct device_node *node,
>>>>> + 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 dp_lanes[2];
>>>>> + int ret, num_lanes, port_num = -1;
>>>>> +
>>>>> + num_lanes = drm_of_get_data_lanes_count(node, 0, 2);
>>>>
>>>> 2 looks incorrect. IIRC DP altmode can support up to 4 lanes.
>>>
>>> This function is implemented for 4-lane DP bridges to switch its
>>> outputs between 2 downstreams. So, I assume that there will only be at
>>> most 2 lanes for each downstream. I don't think a 4-lane downstream
>>> makes sense for mode switches unless we want to support bridges with
>>> more than 4 lanes.
>>
>> Yes. However by using 4 here you'd make the helper generic and cover
>> both your case and the generic case. We don't need this for the msm case
>> (since the mux is handled by the PHY). But if not for the PHY, I'd have
>> used such helper (with max_lanes = 4).
>>
> I wonder if simply using 4 here really makes it more generic here.
> This function assumes the mapping between "data-lanes" and the port
> number (e.g., 0/1 --> port 0) and hard-coded the way to parse the
> property.
>
> Is it better to use "reg" instead of "data-lanes" to determine the
> port number? The drivers can still read the DT node to get the
> "data-lanes" property if they want to do some fancy stuffs around
> that.
Yes, I admit, this sounds more logical.
>>>>
>>>>> + if (num_lanes <= 0) {
>>>>> + dev_err(dev, "Error on getting data lanes count from %s: %d\n",
>>>>> + node->name, num_lanes);
>>>>> + return num_lanes;
>>>>> + }
>>>>> +
>>>>> + ret = of_property_read_u32_array(node, "data-lanes", dp_lanes, num_lanes);
>>>>> + if (ret) {
>>>>> + dev_err(dev, "Failed to read the data-lanes variable from %s: %d\n",
>>>>> + node->name, ret);
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + port_num = dp_lanes[0] / 2;
>>>>> +
>>>>> + port_data = &switch_desc->typec_ports[port_num];
>>>>> + port_data->data = data;
>>>>> + 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 = 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.
>>>>> + * Register the endpoint as port 0 if the "data-lanes" property falls in 0/1,
>>>>> + * and register it as port 1 if "data-lanes" falls in 2/3.
>>>>> + */
>>>>> +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
>>>>> + struct drm_dp_typec_switch_desc *switch_desc,
>>>>> + void *data, typec_mux_set_fn_t mux_set)
>>>>> +{
>>>>> + struct device_node *sw;
>>>>> + int ret;
>>>>> +
>>>>> + for_each_child_of_node(port, sw) {
>>>>> + if (of_property_read_bool(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. */
>>>>> + for_each_child_of_node(port, sw) {
>>>>> + if (!of_property_read_bool(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:
>>>>> + of_node_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);
>>>>> +#else
>>>>> +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc)
>>>>> +{
>>>>> +}
>>>>> +EXPORT_SYMBOL(drm_dp_register_typec_switches);
>>>>> +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
>>>>> + struct drm_dp_typec_switch_desc *switch_desc,
>>>>> + void *data, typec_mux_set_fn_t mux_set)
>>>>> +{
>>>>> + return 0;
>>>>> +}
>>>>> +EXPORT_SYMBOL(drm_dp_unregister_typec_switches);
>>>>> +#endif
>>>>> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
>>>>> index ab55453f2d2c..5a3824f13b4e 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,20 @@ 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;
>>>>> + void *data;
>>>>> + bool dp_connected;
>>>>> +};
>>>>> +
>>>>> +struct drm_dp_typec_switch_desc {
>>>>> + int num_typec_switches;
>>>>> + struct drm_dp_typec_port_data *typec_ports;
>>>>> +};
>>>>> +
>>>>> +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc);
>>>>> +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
>>>>> + struct drm_dp_typec_switch_desc *switch_desc,
>>>>> + void *data, typec_mux_set_fn_t mux_set);
>>>>> +
>>>>> #endif /* _DRM_DP_HELPER_H_ */
>>>>
>>>> --
>>>> With best wishes
>>>> Dmitry
>>>>
>>>
>>> Best regards,
>>> Pin-yen
>>
>> --
>> With best wishes
>> Dmitry
>>
> Best regards,
> Pin-yen
--
With best wishes
Dmitry
On 12/01/2023 07:19, Pin-yen Lin wrote:
> Hi Dmitry,
>
> Thanks for the review.
>
> On Thu, Jan 12, 2023 at 12:40 PM Dmitry Baryshkov
> <[email protected]> wrote:
>>
>> On 12/01/2023 06:20, 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 "data-lanes" properties are available in Device Tree.
>>>
>>> Signed-off-by: Pin-yen Lin <[email protected]>
>>> Tested-by: Chen-Yu Tsai <[email protected]>
>>> Reviewed-by: Chen-Yu Tsai <[email protected]>
>>> Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
>>>
>>> ---
>>>
>>> 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/drm_dp_helper.c | 134 ++++++++++++++++++++++++
>>> include/drm/display/drm_dp_helper.h | 17 +++
>>> 2 files changed, 151 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
>>> index 16565a0a5da6..a2ec40a621cb 100644
>>> --- a/drivers/gpu/drm/display/drm_dp_helper.c
>>> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
>>> @@ -30,11 +30,13 @@
>>> #include <linux/sched.h>
>>> #include <linux/seq_file.h>
>>> #include <linux/string_helpers.h>
>>> +#include <linux/usb/typec_mux.h>
>>> #include <linux/dynamic_debug.h>
>>>
>>> #include <drm/display/drm_dp_helper.h>
>>> #include <drm/display/drm_dp_mst_helper.h>
>>> #include <drm/drm_edid.h>
>>> +#include <drm/drm_of.h>
>>> #include <drm/drm_print.h>
>>> #include <drm/drm_vblank.h>
>>> #include <drm/drm_panel.h>
>>> @@ -3891,3 +3893,135 @@ int drm_panel_dp_aux_backlight(struct drm_panel *panel, struct drm_dp_aux *aux)
>>> EXPORT_SYMBOL(drm_panel_dp_aux_backlight);
>>>
>>> #endif
>>> +
>>> +#if IS_REACHABLE(CONFIG_TYPEC)
>>> +static int drm_dp_register_mode_switch(struct device *dev, struct device_node *node,
>>> + 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 dp_lanes[2];
>>> + int ret, num_lanes, port_num = -1;
>>> +
>>> + num_lanes = drm_of_get_data_lanes_count(node, 0, 2);
>>
>> 2 looks incorrect. IIRC DP altmode can support up to 4 lanes.
>
> This function is implemented for 4-lane DP bridges to switch its
> outputs between 2 downstreams. So, I assume that there will only be at
> most 2 lanes for each downstream. I don't think a 4-lane downstream
> makes sense for mode switches unless we want to support bridges with
> more than 4 lanes.
Yes. However by using 4 here you'd make the helper generic and cover
both your case and the generic case. We don't need this for the msm case
(since the mux is handled by the PHY). But if not for the PHY, I'd have
used such helper (with max_lanes = 4).
>>
>>> + if (num_lanes <= 0) {
>>> + dev_err(dev, "Error on getting data lanes count from %s: %d\n",
>>> + node->name, num_lanes);
>>> + return num_lanes;
>>> + }
>>> +
>>> + ret = of_property_read_u32_array(node, "data-lanes", dp_lanes, num_lanes);
>>> + if (ret) {
>>> + dev_err(dev, "Failed to read the data-lanes variable from %s: %d\n",
>>> + node->name, ret);
>>> + return ret;
>>> + }
>>> +
>>> + port_num = dp_lanes[0] / 2;
>>> +
>>> + port_data = &switch_desc->typec_ports[port_num];
>>> + port_data->data = data;
>>> + 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 = 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.
>>> + * Register the endpoint as port 0 if the "data-lanes" property falls in 0/1,
>>> + * and register it as port 1 if "data-lanes" falls in 2/3.
>>> + */
>>> +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
>>> + struct drm_dp_typec_switch_desc *switch_desc,
>>> + void *data, typec_mux_set_fn_t mux_set)
>>> +{
>>> + struct device_node *sw;
>>> + int ret;
>>> +
>>> + for_each_child_of_node(port, sw) {
>>> + if (of_property_read_bool(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. */
>>> + for_each_child_of_node(port, sw) {
>>> + if (!of_property_read_bool(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:
>>> + of_node_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);
>>> +#else
>>> +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc)
>>> +{
>>> +}
>>> +EXPORT_SYMBOL(drm_dp_register_typec_switches);
>>> +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
>>> + struct drm_dp_typec_switch_desc *switch_desc,
>>> + void *data, typec_mux_set_fn_t mux_set)
>>> +{
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL(drm_dp_unregister_typec_switches);
>>> +#endif
>>> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
>>> index ab55453f2d2c..5a3824f13b4e 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,20 @@ 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;
>>> + void *data;
>>> + bool dp_connected;
>>> +};
>>> +
>>> +struct drm_dp_typec_switch_desc {
>>> + int num_typec_switches;
>>> + struct drm_dp_typec_port_data *typec_ports;
>>> +};
>>> +
>>> +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc);
>>> +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
>>> + struct drm_dp_typec_switch_desc *switch_desc,
>>> + void *data, typec_mux_set_fn_t mux_set);
>>> +
>>> #endif /* _DRM_DP_HELPER_H_ */
>>
>> --
>> With best wishes
>> Dmitry
>>
>
> Best regards,
> Pin-yen
--
With best wishes
Dmitry
On Thu, Jan 12, 2023 at 1:50 PM Dmitry Baryshkov
<[email protected]> wrote:
>
> On 12/01/2023 07:48, Pin-yen Lin wrote:
> > On Thu, Jan 12, 2023 at 1:24 PM Dmitry Baryshkov
> > <[email protected]> wrote:
> >>
> >> On 12/01/2023 07:19, Pin-yen Lin wrote:
> >>> Hi Dmitry,
> >>>
> >>> Thanks for the review.
> >>>
> >>> On Thu, Jan 12, 2023 at 12:40 PM Dmitry Baryshkov
> >>> <[email protected]> wrote:
> >>>>
> >>>> On 12/01/2023 06:20, 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 "data-lanes" properties are available in Device Tree.
> >>>>>
> >>>>> Signed-off-by: Pin-yen Lin <[email protected]>
> >>>>> Tested-by: Chen-Yu Tsai <[email protected]>
> >>>>> Reviewed-by: Chen-Yu Tsai <[email protected]>
> >>>>> Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
> >>>>>
> >>>>> ---
> >>>>>
> >>>>> 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/drm_dp_helper.c | 134 ++++++++++++++++++++++++
> >>>>> include/drm/display/drm_dp_helper.h | 17 +++
> >>>>> 2 files changed, 151 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> >>>>> index 16565a0a5da6..a2ec40a621cb 100644
> >>>>> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> >>>>> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> >>>>> @@ -30,11 +30,13 @@
> >>>>> #include <linux/sched.h>
> >>>>> #include <linux/seq_file.h>
> >>>>> #include <linux/string_helpers.h>
> >>>>> +#include <linux/usb/typec_mux.h>
> >>>>> #include <linux/dynamic_debug.h>
> >>>>>
> >>>>> #include <drm/display/drm_dp_helper.h>
> >>>>> #include <drm/display/drm_dp_mst_helper.h>
> >>>>> #include <drm/drm_edid.h>
> >>>>> +#include <drm/drm_of.h>
> >>>>> #include <drm/drm_print.h>
> >>>>> #include <drm/drm_vblank.h>
> >>>>> #include <drm/drm_panel.h>
> >>>>> @@ -3891,3 +3893,135 @@ int drm_panel_dp_aux_backlight(struct drm_panel *panel, struct drm_dp_aux *aux)
> >>>>> EXPORT_SYMBOL(drm_panel_dp_aux_backlight);
> >>>>>
> >>>>> #endif
> >>>>> +
> >>>>> +#if IS_REACHABLE(CONFIG_TYPEC)
> >>>>> +static int drm_dp_register_mode_switch(struct device *dev, struct device_node *node,
> >>>>> + 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 dp_lanes[2];
> >>>>> + int ret, num_lanes, port_num = -1;
> >>>>> +
> >>>>> + num_lanes = drm_of_get_data_lanes_count(node, 0, 2);
> >>>>
> >>>> 2 looks incorrect. IIRC DP altmode can support up to 4 lanes.
> >>>
> >>> This function is implemented for 4-lane DP bridges to switch its
> >>> outputs between 2 downstreams. So, I assume that there will only be at
> >>> most 2 lanes for each downstream. I don't think a 4-lane downstream
> >>> makes sense for mode switches unless we want to support bridges with
> >>> more than 4 lanes.
> >>
> >> Yes. However by using 4 here you'd make the helper generic and cover
> >> both your case and the generic case. We don't need this for the msm case
> >> (since the mux is handled by the PHY). But if not for the PHY, I'd have
> >> used such helper (with max_lanes = 4).
> >>
> > I wonder if simply using 4 here really makes it more generic here.
> > This function assumes the mapping between "data-lanes" and the port
> > number (e.g., 0/1 --> port 0) and hard-coded the way to parse the
> > property.
> >
> > Is it better to use "reg" instead of "data-lanes" to determine the
> > port number? The drivers can still read the DT node to get the
> > "data-lanes" property if they want to do some fancy stuffs around
> > that.
>
> Yes, I admit, this sounds more logical.
>
Thanks for the reply. I'll do that in v11.
> >>>>
> >>>>> + if (num_lanes <= 0) {
> >>>>> + dev_err(dev, "Error on getting data lanes count from %s: %d\n",
> >>>>> + node->name, num_lanes);
> >>>>> + return num_lanes;
> >>>>> + }
> >>>>> +
> >>>>> + ret = of_property_read_u32_array(node, "data-lanes", dp_lanes, num_lanes);
> >>>>> + if (ret) {
> >>>>> + dev_err(dev, "Failed to read the data-lanes variable from %s: %d\n",
> >>>>> + node->name, ret);
> >>>>> + return ret;
> >>>>> + }
> >>>>> +
> >>>>> + port_num = dp_lanes[0] / 2;
> >>>>> +
> >>>>> + port_data = &switch_desc->typec_ports[port_num];
> >>>>> + port_data->data = data;
> >>>>> + 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 = 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.
> >>>>> + * Register the endpoint as port 0 if the "data-lanes" property falls in 0/1,
> >>>>> + * and register it as port 1 if "data-lanes" falls in 2/3.
> >>>>> + */
> >>>>> +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
> >>>>> + struct drm_dp_typec_switch_desc *switch_desc,
> >>>>> + void *data, typec_mux_set_fn_t mux_set)
> >>>>> +{
> >>>>> + struct device_node *sw;
> >>>>> + int ret;
> >>>>> +
> >>>>> + for_each_child_of_node(port, sw) {
> >>>>> + if (of_property_read_bool(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. */
> >>>>> + for_each_child_of_node(port, sw) {
> >>>>> + if (!of_property_read_bool(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:
> >>>>> + of_node_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);
> >>>>> +#else
> >>>>> +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc)
> >>>>> +{
> >>>>> +}
> >>>>> +EXPORT_SYMBOL(drm_dp_register_typec_switches);
> >>>>> +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
> >>>>> + struct drm_dp_typec_switch_desc *switch_desc,
> >>>>> + void *data, typec_mux_set_fn_t mux_set)
> >>>>> +{
> >>>>> + return 0;
> >>>>> +}
> >>>>> +EXPORT_SYMBOL(drm_dp_unregister_typec_switches);
> >>>>> +#endif
> >>>>> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> >>>>> index ab55453f2d2c..5a3824f13b4e 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,20 @@ 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;
> >>>>> + void *data;
> >>>>> + bool dp_connected;
> >>>>> +};
> >>>>> +
> >>>>> +struct drm_dp_typec_switch_desc {
> >>>>> + int num_typec_switches;
> >>>>> + struct drm_dp_typec_port_data *typec_ports;
> >>>>> +};
> >>>>> +
> >>>>> +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc);
> >>>>> +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
> >>>>> + struct drm_dp_typec_switch_desc *switch_desc,
> >>>>> + void *data, typec_mux_set_fn_t mux_set);
> >>>>> +
> >>>>> #endif /* _DRM_DP_HELPER_H_ */
> >>>>
> >>>> --
> >>>> With best wishes
> >>>> Dmitry
> >>>>
> >>>
> >>> Best regards,
> >>> Pin-yen
> >>
> >> --
> >> With best wishes
> >> Dmitry
> >>
> > Best regards,
> > Pin-yen
>
> --
> With best wishes
> Dmitry
>
Best regards,
Pin-yen
On Thu, 12 Jan 2023, Pin-yen Lin <[email protected]> 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 "data-lanes" properties are available in Device Tree.
>
> Signed-off-by: Pin-yen Lin <[email protected]>
> Tested-by: Chen-Yu Tsai <[email protected]>
> Reviewed-by: Chen-Yu Tsai <[email protected]>
> Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
>
> ---
>
> 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/drm_dp_helper.c | 134 ++++++++++++++++++++++++
> include/drm/display/drm_dp_helper.h | 17 +++
> 2 files changed, 151 insertions(+)
>
> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> index 16565a0a5da6..a2ec40a621cb 100644
> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> @@ -30,11 +30,13 @@
> #include <linux/sched.h>
> #include <linux/seq_file.h>
> #include <linux/string_helpers.h>
> +#include <linux/usb/typec_mux.h>
> #include <linux/dynamic_debug.h>
>
> #include <drm/display/drm_dp_helper.h>
> #include <drm/display/drm_dp_mst_helper.h>
> #include <drm/drm_edid.h>
> +#include <drm/drm_of.h>
> #include <drm/drm_print.h>
> #include <drm/drm_vblank.h>
> #include <drm/drm_panel.h>
> @@ -3891,3 +3893,135 @@ int drm_panel_dp_aux_backlight(struct drm_panel *panel, struct drm_dp_aux *aux)
> EXPORT_SYMBOL(drm_panel_dp_aux_backlight);
>
> #endif
> +
> +#if IS_REACHABLE(CONFIG_TYPEC)
I think IS_REACHABLE() is a workaround for not getting the Kconfig
dependencies right. It allows configurations that silently just don't
work, instead of warning about it at config time. It fixes a build
issue, but trades it for an end user configuration issue that you don't
get any feedback about, and is hard to figure out. It's for people who
deal with build issues, but don't need to deal with user issues.
BR,
Jani.
> +static int drm_dp_register_mode_switch(struct device *dev, struct device_node *node,
> + 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 dp_lanes[2];
> + int ret, num_lanes, port_num = -1;
> +
> + num_lanes = drm_of_get_data_lanes_count(node, 0, 2);
> + if (num_lanes <= 0) {
> + dev_err(dev, "Error on getting data lanes count from %s: %d\n",
> + node->name, num_lanes);
> + return num_lanes;
> + }
> +
> + ret = of_property_read_u32_array(node, "data-lanes", dp_lanes, num_lanes);
> + if (ret) {
> + dev_err(dev, "Failed to read the data-lanes variable from %s: %d\n",
> + node->name, ret);
> + return ret;
> + }
> +
> + port_num = dp_lanes[0] / 2;
> +
> + port_data = &switch_desc->typec_ports[port_num];
> + port_data->data = data;
> + 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 = 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.
> + * Register the endpoint as port 0 if the "data-lanes" property falls in 0/1,
> + * and register it as port 1 if "data-lanes" falls in 2/3.
> + */
> +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
> + struct drm_dp_typec_switch_desc *switch_desc,
> + void *data, typec_mux_set_fn_t mux_set)
> +{
> + struct device_node *sw;
> + int ret;
> +
> + for_each_child_of_node(port, sw) {
> + if (of_property_read_bool(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. */
> + for_each_child_of_node(port, sw) {
> + if (!of_property_read_bool(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:
> + of_node_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);
> +#else
> +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc)
> +{
> +}
> +EXPORT_SYMBOL(drm_dp_register_typec_switches);
> +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
> + struct drm_dp_typec_switch_desc *switch_desc,
> + void *data, typec_mux_set_fn_t mux_set)
> +{
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_dp_unregister_typec_switches);
> +#endif
> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> index ab55453f2d2c..5a3824f13b4e 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,20 @@ 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;
> + void *data;
> + bool dp_connected;
> +};
> +
> +struct drm_dp_typec_switch_desc {
> + int num_typec_switches;
> + struct drm_dp_typec_port_data *typec_ports;
> +};
> +
> +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc);
> +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
> + struct drm_dp_typec_switch_desc *switch_desc,
> + void *data, typec_mux_set_fn_t mux_set);
> +
> #endif /* _DRM_DP_HELPER_H_ */
--
Jani Nikula, Intel Open Source Graphics Center
On Thu, Jan 12, 2023 at 06:34:52AM +0200, Dmitry Baryshkov wrote:
> On 12/01/2023 06:20, Pin-yen Lin 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.
>
> Please cc everybody on all patches. Having received just a single patch made
> me spend time on having to look them up on lore.
Sorry but this seems a bit outdated requirement nowadays.
Why? Because we have `b4` in each famous Linux distro and
nice lore.kernel.org. It's really easy for the experienced
maintainer to catch up the whole thread.
Putting _all_ people on all patches may be an overkill.
What people should really get is the cover letter (assuming
that it explains well the structure of the series). The rest
depends.
--
With Best Regards,
Andy Shevchenko
On Thu, Jan 12, 2023 at 12:20:56PM +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]>
Acked-by: Heikki Krogerus <[email protected]>
> ---
>
> Changes in v10:
> - Collected Reviewed-by and Tested-by tags
>
> Changes in v6:
> - New in v6
>
> drivers/base/property.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 2a5a37fcd998..48877af4e444 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -1223,6 +1223,21 @@ 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++;
> + }
> + }
> +
> node = fwnode_graph_get_remote_port_parent(ep);
> if (!fwnode_device_is_available(node)) {
> fwnode_handle_put(node);
> --
> 2.39.0.314.g84b9a713c41-goog
--
heikki
Hi Pin-yen,
On Thu, Jan 12, 2023 at 12:20:56PM +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]>
>
> ---
>
> Changes in v10:
> - Collected Reviewed-by and Tested-by tags
>
> Changes in v6:
> - New in v6
>
> drivers/base/property.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 2a5a37fcd998..48877af4e444 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -1223,6 +1223,21 @@ 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++;
> + }
> + }
Aren't you missing fwnode_handle-put(node) here??
> +
> node = fwnode_graph_get_remote_port_parent(ep);
> if (!fwnode_device_is_available(node)) {
> fwnode_handle_put(node);
--
Kind regards,
Sakari Ailus
On Thu, Jan 12, 2023 at 12:20:57PM +0800, Pin-yen Lin wrote:
> 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]>
FWIW:
Acked-by: Heikki Krogerus <[email protected]>
> ---
>
> 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.0.314.g84b9a713c41-goog
--
heikki
HI Sakari,
On Thu, Jan 12, 2023 at 5:32 AM Sakari Ailus
<[email protected]> wrote:
>
> Hi Pin-yen,
>
> On Thu, Jan 12, 2023 at 12:20:56PM +0800, Pin-yen Lin wrote:
> > From: Prashant Malani <[email protected]>
> > + /*
> > + * 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++;
> > + }
> > + }
>
> Aren't you missing fwnode_handle-put(node) here??
It shouldn't be necessary. We aren't break-ing/continue-ing here,
and fwnode_handle_put(node) is called latter in the loop [1][2]
BR,
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/property.c#n1256
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/property.c#n1261
On Wed, Jan 11, 2023 at 10:21 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.
>
> 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.
>
> 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 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
4 versions in a week! Please slow down your pace. When you send a new
version, you move to the end of my review queue.
IIRC, these 2 chips are a bit different in what the mode switch or
muxing looks like. One had a built-in mux and the other doesn't? Do I
have to go research this again? No, you need to explain all this in
this series.
Rob
Hi Rob,
On Fri, Jan 13, 2023 at 6:44 AM Rob Herring <[email protected]> wrote:
>
> On Wed, Jan 11, 2023 at 10:21 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.
> >
> > 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.
> >
> > 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 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
>
> 4 versions in a week! Please slow down your pace. When you send a new
> version, you move to the end of my review queue.
I see. I'll keep this in mind in the future series.
>
> IIRC, these 2 chips are a bit different in what the mode switch or
> muxing looks like. One had a built-in mux and the other doesn't? Do I
> have to go research this again? No, you need to explain all this in
> this series.
Yes, anx7625 has a built-in mux while it6505 doesn't, but it's for
another use case.
IIUC the built-in mux in anx7625 is designed for automatically
switching between two orientations of a single Type-C connector, and
in that case we might need to register an orientation switch. But we
don't have hardware for this use case.
The use case this series aimed is having two downstreams for the
bridges, and registering two mode switches to switch between them. In
this use case, the built-in mux of anx7625 is not used and the
behavior of the switches is the same as it6505.
Explanations and TODOs have been added in the anx7625 driver change. I
can also mention this in the cover letter in the future series and
please let me know if anything is not clear for you.
>
> Rob
Thanks and regards,
Pin-yen
Hi Jani,
Thanks for the review.
On Thu, Jan 12, 2023 at 4:37 PM Jani Nikula <[email protected]> wrote:
>
> On Thu, 12 Jan 2023, Pin-yen Lin <[email protected]> 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 "data-lanes" properties are available in Device Tree.
> >
> > Signed-off-by: Pin-yen Lin <[email protected]>
> > Tested-by: Chen-Yu Tsai <[email protected]>
> > Reviewed-by: Chen-Yu Tsai <[email protected]>
> > Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
> >
> > ---
> >
> > 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/drm_dp_helper.c | 134 ++++++++++++++++++++++++
> > include/drm/display/drm_dp_helper.h | 17 +++
> > 2 files changed, 151 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> > index 16565a0a5da6..a2ec40a621cb 100644
> > --- a/drivers/gpu/drm/display/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> > @@ -30,11 +30,13 @@
> > #include <linux/sched.h>
> > #include <linux/seq_file.h>
> > #include <linux/string_helpers.h>
> > +#include <linux/usb/typec_mux.h>
> > #include <linux/dynamic_debug.h>
> >
> > #include <drm/display/drm_dp_helper.h>
> > #include <drm/display/drm_dp_mst_helper.h>
> > #include <drm/drm_edid.h>
> > +#include <drm/drm_of.h>
> > #include <drm/drm_print.h>
> > #include <drm/drm_vblank.h>
> > #include <drm/drm_panel.h>
> > @@ -3891,3 +3893,135 @@ int drm_panel_dp_aux_backlight(struct drm_panel *panel, struct drm_dp_aux *aux)
> > EXPORT_SYMBOL(drm_panel_dp_aux_backlight);
> >
> > #endif
> > +
> > +#if IS_REACHABLE(CONFIG_TYPEC)
>
> I think IS_REACHABLE() is a workaround for not getting the Kconfig
> dependencies right. It allows configurations that silently just don't
> work, instead of warning about it at config time. It fixes a build
> issue, but trades it for an end user configuration issue that you don't
> get any feedback about, and is hard to figure out. It's for people who
> deal with build issues, but don't need to deal with user issues.
>
> BR,
> Jani.
>
I've added "depends on TYPEC || TYPEC=n" on the Kconfigs of its users
(i.e., anx7625 and it6505). I didn't do this on DRM_DISPLAY_DP_HELPER
because that stops all users of DRM_DISPLAY_DP_HELPER, which is a
quite generic helper file, from building TYPEC as a module.
Or, do you have any other suggestions on this? Move these functions to
a separate file?
>
>
> > +static int drm_dp_register_mode_switch(struct device *dev, struct device_node *node,
> > + 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 dp_lanes[2];
> > + int ret, num_lanes, port_num = -1;
> > +
> > + num_lanes = drm_of_get_data_lanes_count(node, 0, 2);
> > + if (num_lanes <= 0) {
> > + dev_err(dev, "Error on getting data lanes count from %s: %d\n",
> > + node->name, num_lanes);
> > + return num_lanes;
> > + }
> > +
> > + ret = of_property_read_u32_array(node, "data-lanes", dp_lanes, num_lanes);
> > + if (ret) {
> > + dev_err(dev, "Failed to read the data-lanes variable from %s: %d\n",
> > + node->name, ret);
> > + return ret;
> > + }
> > +
> > + port_num = dp_lanes[0] / 2;
> > +
> > + port_data = &switch_desc->typec_ports[port_num];
> > + port_data->data = data;
> > + 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 = 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.
> > + * Register the endpoint as port 0 if the "data-lanes" property falls in 0/1,
> > + * and register it as port 1 if "data-lanes" falls in 2/3.
> > + */
> > +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
> > + struct drm_dp_typec_switch_desc *switch_desc,
> > + void *data, typec_mux_set_fn_t mux_set)
> > +{
> > + struct device_node *sw;
> > + int ret;
> > +
> > + for_each_child_of_node(port, sw) {
> > + if (of_property_read_bool(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. */
> > + for_each_child_of_node(port, sw) {
> > + if (!of_property_read_bool(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:
> > + of_node_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);
> > +#else
> > +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc)
> > +{
> > +}
> > +EXPORT_SYMBOL(drm_dp_register_typec_switches);
> > +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
> > + struct drm_dp_typec_switch_desc *switch_desc,
> > + void *data, typec_mux_set_fn_t mux_set)
> > +{
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(drm_dp_unregister_typec_switches);
> > +#endif
> > diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> > index ab55453f2d2c..5a3824f13b4e 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,20 @@ 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;
> > + void *data;
> > + bool dp_connected;
> > +};
> > +
> > +struct drm_dp_typec_switch_desc {
> > + int num_typec_switches;
> > + struct drm_dp_typec_port_data *typec_ports;
> > +};
> > +
> > +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc);
> > +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
> > + struct drm_dp_typec_switch_desc *switch_desc,
> > + void *data, typec_mux_set_fn_t mux_set);
> > +
> > #endif /* _DRM_DP_HELPER_H_ */
>
> --
> Jani Nikula, Intel Open Source Graphics Center
Best regards,
Pin-yen
Hi,
On Thu, Jan 12, 2023 at 12:20:58PM +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 "data-lanes" properties are available in Device Tree.
Let's not make this kind of helpers DT only, please. See below ...
> Signed-off-by: Pin-yen Lin <[email protected]>
> Tested-by: Chen-Yu Tsai <[email protected]>
> Reviewed-by: Chen-Yu Tsai <[email protected]>
> Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
>
> ---
>
> 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/drm_dp_helper.c | 134 ++++++++++++++++++++++++
> include/drm/display/drm_dp_helper.h | 17 +++
> 2 files changed, 151 insertions(+)
>
> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> index 16565a0a5da6..a2ec40a621cb 100644
> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> @@ -30,11 +30,13 @@
> #include <linux/sched.h>
> #include <linux/seq_file.h>
> #include <linux/string_helpers.h>
> +#include <linux/usb/typec_mux.h>
> #include <linux/dynamic_debug.h>
>
> #include <drm/display/drm_dp_helper.h>
> #include <drm/display/drm_dp_mst_helper.h>
> #include <drm/drm_edid.h>
> +#include <drm/drm_of.h>
> #include <drm/drm_print.h>
> #include <drm/drm_vblank.h>
> #include <drm/drm_panel.h>
> @@ -3891,3 +3893,135 @@ int drm_panel_dp_aux_backlight(struct drm_panel *panel, struct drm_dp_aux *aux)
> EXPORT_SYMBOL(drm_panel_dp_aux_backlight);
>
> #endif
> +
> +#if IS_REACHABLE(CONFIG_TYPEC)
I think Jani already pointed out that that is wrong. Just move these
into a separate file and enable them silently in the Makefile when
TYPEC is enabled - so no separate Kconfig option.
> +static int drm_dp_register_mode_switch(struct device *dev, struct device_node *node,
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 dp_lanes[2];
> + int ret, num_lanes, port_num = -1;
> +
> + num_lanes = drm_of_get_data_lanes_count(node, 0, 2);
> + if (num_lanes <= 0) {
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 %s: %d\n",
> + node->name, num_lanes);
> + return num_lanes;
> + }
> +
> + ret = of_property_read_u32_array(node, "data-lanes", dp_lanes, num_lanes);
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 from %s: %d\n",
> + node->name, ret);
fwnode_get_name(fwnode), ret);
> + return ret;
> + }
> +
> + port_num = dp_lanes[0] / 2;
> +
> + port_data = &switch_desc->typec_ports[port_num];
> + port_data->data = data;
> + mux_desc.fwnode = &node->fwnode;
mux_desc.fwnode = fwnode;
> + mux_desc.drvdata = port_data;
> + snprintf(name, sizeof(name), "%s-%u", node->name, port_num);
snprintf(name, sizeof(name), "%s-%u", fwnode_get_name(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.
> + * Register the endpoint as port 0 if the "data-lanes" property falls in 0/1,
> + * and register it as port 1 if "data-lanes" falls in 2/3.
> + */
> +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
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 device_node *sw;
struct fwnode_handle *sw;
> + int ret;
> +
> + for_each_child_of_node(port, sw) {
> + if (of_property_read_bool(sw, "mode-switch"))
> + switch_desc->num_typec_switches++;
> + }
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. */
> + for_each_child_of_node(port, sw) {
> + if (!of_property_read_bool(sw, "mode-switch"))
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:
> + of_node_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);
> +#else
> +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc)
> +{
> +}
> +EXPORT_SYMBOL(drm_dp_register_typec_switches);
> +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
> + struct drm_dp_typec_switch_desc *switch_desc,
> + void *data, typec_mux_set_fn_t mux_set)
> +{
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_dp_unregister_typec_switches);
> +#endif
> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> index ab55453f2d2c..5a3824f13b4e 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,20 @@ 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;
> + void *data;
> + bool dp_connected;
> +};
> +
> +struct drm_dp_typec_switch_desc {
> + int num_typec_switches;
> + struct drm_dp_typec_port_data *typec_ports;
> +};
> +
> +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc);
> +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
> + struct drm_dp_typec_switch_desc *switch_desc,
> + void *data, typec_mux_set_fn_t mux_set);
> +
> #endif /* _DRM_DP_HELPER_H_ */
The function stubs go here if they are needed.
thanks,
--
heikki
On Fri, Jan 13, 2023 at 11:23:44AM +0200, Heikki Krogerus wrote:
> On Thu, Jan 12, 2023 at 12:20:58PM +0800, Pin-yen Lin wrote:
...
> > + dev_err(dev, "Failed to read the data-lanes variable from %s: %d\n",
> > + node->name, ret);
>
> fwnode_get_name(fwnode), ret);
Or even %pfwP ?
...
> > + snprintf(name, sizeof(name), "%s-%u", node->name, port_num);
>
> snprintf(name, sizeof(name), "%s-%u", fwnode_get_name(fwnode), port_num);
Ditto.
--
With Best Regards,
Andy Shevchenko
On Thu, Jan 12, 2023 at 02:31:45PM -0800, Prashant Malani wrote:
> On Thu, Jan 12, 2023 at 5:32 AM Sakari Ailus
> <[email protected]> wrote:
> > On Thu, Jan 12, 2023 at 12:20:56PM +0800, Pin-yen Lin wrote:
> > > From: Prashant Malani <[email protected]>
...
> > > + /*
> > > + * 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++;
> > > + }
> > > + }
> >
> > Aren't you missing fwnode_handle-put(node) here??
>
> It shouldn't be necessary. We aren't break-ing/continue-ing here,
> and fwnode_handle_put(node) is called latter in the loop [1][2]
>
> BR,
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/property.c#n1256
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/property.c#n1261
I'm really puzzled what do you mean by all this.
Sakari is right, btw.
--
With Best Regards,
Andy Shevchenko
Hi Prashant,
On Thu, Jan 12, 2023 at 02:31:45PM -0800, Prashant Malani wrote:
> HI Sakari,
>
> On Thu, Jan 12, 2023 at 5:32 AM Sakari Ailus
> <[email protected]> wrote:
> >
> > Hi Pin-yen,
> >
> > On Thu, Jan 12, 2023 at 12:20:56PM +0800, Pin-yen Lin wrote:
> > > From: Prashant Malani <[email protected]>
> > > + /*
> > > + * 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++;
> > > + }
> > > + }
> >
> > Aren't you missing fwnode_handle-put(node) here??
>
> It shouldn't be necessary. We aren't break-ing/continue-ing here,
> and fwnode_handle_put(node) is called latter in the loop [1][2]
It is, but node is overwritten just below this chunk --- before
fwnode_handle_put() is called on it.
>
> BR,
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/property.c#n1256
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/property.c#n1261
--
Regards,
Sakari Ailus
On Mon, Jan 16, 2023 at 5:07 AM Sakari Ailus
<[email protected]> wrote:
>
> Hi Prashant,
>
> On Thu, Jan 12, 2023 at 02:31:45PM -0800, Prashant Malani wrote:
> > HI Sakari,
> >
> > On Thu, Jan 12, 2023 at 5:32 AM Sakari Ailus
> > <[email protected]> wrote:
> > >
> > > Hi Pin-yen,
> > >
> > > On Thu, Jan 12, 2023 at 12:20:56PM +0800, Pin-yen Lin wrote:
> > > > From: Prashant Malani <[email protected]>
> > > > + /*
> > > > + * 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++;
> > > > + }
> > > > + }
> > >
> > > Aren't you missing fwnode_handle-put(node) here??
> >
> > It shouldn't be necessary. We aren't break-ing/continue-ing here,
> > and fwnode_handle_put(node) is called latter in the loop [1][2]
>
> It is, but node is overwritten just below this chunk --- before
> fwnode_handle_put() is called on it.
Ack. Thanks for pointing that out. My bad!
Pin-yen, please make this update when you send out a v11.
BR,
-Prashant