| Message ID | 20230204133040.1236799-4-treapking@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Register Type-C mode-switch in DP bridge endpoints | expand |
On Sat, Feb 04, 2023 at 09:30:34PM +0800, Pin-yen Lin wrote: > Add helpers to register and unregister Type-C "switches" for bridges > capable of switching their output between two downstream devices. > > The helper registers USB Type-C mode switches when the "mode-switch" > and the "reg" properties are available in Device Tree. ... > + fwnode_for_each_child_node(port, sw) { > + if (fwnode_property_present(sw, "mode-switch")) This idiom is being used at least twice (in this code, haven't checked the rest of the patches, though), perhaps #define fwnode_for_each_typec_mode_switch(port, sw) \ fwnode_for_each_child_node(port, sw) \ if (!fwnode_property_present(sw, "mode-switch")) {} else ?
On Mon, 06 Feb 2023, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Sat, Feb 04, 2023 at 09:30:34PM +0800, Pin-yen Lin wrote: >> Add helpers to register and unregister Type-C "switches" for bridges >> capable of switching their output between two downstream devices. >> >> The helper registers USB Type-C mode switches when the "mode-switch" >> and the "reg" properties are available in Device Tree. > > ... > >> + fwnode_for_each_child_node(port, sw) { >> + if (fwnode_property_present(sw, "mode-switch")) > > This idiom is being used at least twice (in this code, haven't checked the rest > of the patches, though), perhaps > > #define fwnode_for_each_typec_mode_switch(port, sw) \ > fwnode_for_each_child_node(port, sw) \ > if (!fwnode_property_present(sw, "mode-switch")) {} else > > ? See for_each_if() in drm_util.h. BR, Jani.
Hi Pin-yen, [...] > +static int drm_dp_register_mode_switch(struct device *dev, > + struct fwnode_handle *fwnode, > + struct drm_dp_typec_switch_desc *switch_desc, > + void *data, typec_mux_set_fn_t mux_set) > +{ > + struct drm_dp_typec_port_data *port_data; > + struct typec_mux_desc mux_desc = {}; > + char name[32]; > + u32 port_num; > + int ret; > + > + ret = fwnode_property_read_u32(fwnode, "reg", &port_num); > + if (ret) { > + dev_err(dev, "Failed to read reg property: %d\n", ret); > + return ret; > + } > + > + port_data = &switch_desc->typec_ports[port_num]; > + port_data->data = data; > + port_data->port_num = port_num; > + port_data->fwnode = fwnode; > + mux_desc.fwnode = fwnode; > + mux_desc.drvdata = port_data; > + snprintf(name, sizeof(name), "%pfwP-%u", fwnode, port_num); > + mux_desc.name = name; > + mux_desc.set = mux_set; > + > + port_data->typec_mux = typec_mux_register(dev, &mux_desc); > + if (IS_ERR(port_data->typec_mux)) { > + ret = PTR_ERR(port_data->typec_mux); > + dev_err(dev, "Mode switch register for port %d failed: %d\n", > + port_num, ret); > + > + return ret; you don't need this return here... > + } > + > + return 0; Just "return ret;" here. > +} > + > +/** > + * drm_dp_register_typec_switches() - register Type-C switches > + * @dev: Device that registers Type-C switches > + * @port: Device node for the switch > + * @switch_desc: A Type-C switch descriptor > + * @data: Private data for the switches > + * @mux_set: Callback function for typec_mux_set > + * > + * This function registers USB Type-C switches for DP bridges that can switch > + * the output signal between their output pins. > + * > + * Currently only mode switches are implemented, and the function assumes the > + * given @port device node has endpoints with "mode-switch" property. > + * The port number is determined by the "reg" property of the endpoint. > + */ > +int drm_dp_register_typec_switches(struct device *dev, struct fwnode_handle *port, > + struct drm_dp_typec_switch_desc *switch_desc, > + void *data, typec_mux_set_fn_t mux_set) > +{ > + struct fwnode_handle *sw; > + int ret; > + > + fwnode_for_each_child_node(port, sw) { > + if (fwnode_property_present(sw, "mode-switch")) > + switch_desc->num_typec_switches++; > + } no need for brackets here > + > + if (!switch_desc->num_typec_switches) { > + dev_dbg(dev, "No Type-C switches node found\n"); dev_warn()? > + return 0; > + } > + > + switch_desc->typec_ports = devm_kcalloc( > + dev, switch_desc->num_typec_switches, > + sizeof(struct drm_dp_typec_port_data), GFP_KERNEL); > + > + if (!switch_desc->typec_ports) > + return -ENOMEM; > + > + /* Register switches for each connector. */ > + fwnode_for_each_child_node(port, sw) { > + if (!fwnode_property_present(sw, "mode-switch")) > + continue; > + ret = drm_dp_register_mode_switch(dev, sw, switch_desc, data, mux_set); > + if (ret) > + goto err_unregister_typec_switches; > + } > + > + return 0; > + > +err_unregister_typec_switches: > + fwnode_handle_put(sw); > + drm_dp_unregister_typec_switches(switch_desc); > + dev_err(dev, "Failed to register mode switch: %d\n", ret); there is a bit of dmesg spamming. Please choose where you want to print the error, either in this function or in drm_dp_register_mode_switch(). Andi > + return ret; > +} > +EXPORT_SYMBOL(drm_dp_register_typec_switches); [...]
Hi Andi, Thanks for the review. On Wed, Feb 8, 2023 at 5:25 AM Andi Shyti <andi.shyti@linux.intel.com> wrote: > Hi Pin-yen, > > [...] > > > +static int drm_dp_register_mode_switch(struct device *dev, > > + struct fwnode_handle *fwnode, > > + struct drm_dp_typec_switch_desc > *switch_desc, > > + void *data, typec_mux_set_fn_t > mux_set) > > +{ > > + struct drm_dp_typec_port_data *port_data; > > + struct typec_mux_desc mux_desc = {}; > > + char name[32]; > > + u32 port_num; > > + int ret; > > + > > + ret = fwnode_property_read_u32(fwnode, "reg", &port_num); > > + if (ret) { > > + dev_err(dev, "Failed to read reg property: %d\n", ret); > > + return ret; > > + } > > + > > + port_data = &switch_desc->typec_ports[port_num]; > > + port_data->data = data; > > + port_data->port_num = port_num; > > + port_data->fwnode = fwnode; > > + mux_desc.fwnode = fwnode; > > + mux_desc.drvdata = port_data; > > + snprintf(name, sizeof(name), "%pfwP-%u", fwnode, port_num); > > + mux_desc.name = name; > > + mux_desc.set = mux_set; > > + > > + port_data->typec_mux = typec_mux_register(dev, &mux_desc); > > + if (IS_ERR(port_data->typec_mux)) { > > + ret = PTR_ERR(port_data->typec_mux); > > + dev_err(dev, "Mode switch register for port %d failed: > %d\n", > > + port_num, ret); > > + > > + return ret; > > you don't need this return here... > > > + } > > + > > + return 0; > > Just "return ret;" here. > > +} > > + > > +/** > > + * drm_dp_register_typec_switches() - register Type-C switches > > + * @dev: Device that registers Type-C switches > > + * @port: Device node for the switch > > + * @switch_desc: A Type-C switch descriptor > > + * @data: Private data for the switches > > + * @mux_set: Callback function for typec_mux_set > > + * > > + * This function registers USB Type-C switches for DP bridges that can > switch > > + * the output signal between their output pins. > > + * > > + * Currently only mode switches are implemented, and the function > assumes the > > + * given @port device node has endpoints with "mode-switch" property. > > + * The port number is determined by the "reg" property of the endpoint. > > + */ > > +int drm_dp_register_typec_switches(struct device *dev, struct > fwnode_handle *port, > > + struct drm_dp_typec_switch_desc > *switch_desc, > > + void *data, typec_mux_set_fn_t mux_set) > > +{ > > + struct fwnode_handle *sw; > > + int ret; > > + > > + fwnode_for_each_child_node(port, sw) { > > + if (fwnode_property_present(sw, "mode-switch")) > > + switch_desc->num_typec_switches++; > > + } > > no need for brackets here > > > + > > + if (!switch_desc->num_typec_switches) { > > + dev_dbg(dev, "No Type-C switches node found\n"); > > dev_warn()? > I used dev_dbg here because the users might call this without checking if there are mode switch endpoints present, and this is the case for the current users (it6505 and anx7625). If we use dev_warn here, there will be warnings every time even on use cases without Type-C switches. Thanks and regards, Pin-yen > > > + return 0; > > + } > > + > > + switch_desc->typec_ports = devm_kcalloc( > > + dev, switch_desc->num_typec_switches, > > + sizeof(struct drm_dp_typec_port_data), GFP_KERNEL); > > + > > + if (!switch_desc->typec_ports) > > + return -ENOMEM; > > + > > + /* Register switches for each connector. */ > > + fwnode_for_each_child_node(port, sw) { > > + if (!fwnode_property_present(sw, "mode-switch")) > > + continue; > > + ret = drm_dp_register_mode_switch(dev, sw, switch_desc, > data, mux_set); > > + if (ret) > > + goto err_unregister_typec_switches; > > + } > > + > > + return 0; > > + > > +err_unregister_typec_switches: > > + fwnode_handle_put(sw); > > + drm_dp_unregister_typec_switches(switch_desc); > > + dev_err(dev, "Failed to register mode switch: %d\n", ret); > > there is a bit of dmesg spamming. Please choose where you want to > print the error, either in this function or in > drm_dp_register_mode_switch(). > > Andi > > > + return ret; > > +} > > +EXPORT_SYMBOL(drm_dp_register_typec_switches); > > [...] >
I think I accidentally used HTML mode for the previous email. Sorry about that. On Mon, Feb 20, 2023 at 4:41 PM Pin-yen Lin <treapking@chromium.org> wrote: > > Hi Andi, > > Thanks for the review. > > On Wed, Feb 8, 2023 at 5:25 AM Andi Shyti <andi.shyti@linux.intel.com> wrote: >> >> Hi Pin-yen, >> >> [...] >> >> > +static int drm_dp_register_mode_switch(struct device *dev, >> > + struct fwnode_handle *fwnode, >> > + struct drm_dp_typec_switch_desc *switch_desc, >> > + void *data, typec_mux_set_fn_t mux_set) >> > +{ >> > + struct drm_dp_typec_port_data *port_data; >> > + struct typec_mux_desc mux_desc = {}; >> > + char name[32]; >> > + u32 port_num; >> > + int ret; >> > + >> > + ret = fwnode_property_read_u32(fwnode, "reg", &port_num); >> > + if (ret) { >> > + dev_err(dev, "Failed to read reg property: %d\n", ret); >> > + return ret; >> > + } >> > + >> > + port_data = &switch_desc->typec_ports[port_num]; >> > + port_data->data = data; >> > + port_data->port_num = port_num; >> > + port_data->fwnode = fwnode; >> > + mux_desc.fwnode = fwnode; >> > + mux_desc.drvdata = port_data; >> > + snprintf(name, sizeof(name), "%pfwP-%u", fwnode, port_num); >> > + mux_desc.name = name; >> > + mux_desc.set = mux_set; >> > + >> > + port_data->typec_mux = typec_mux_register(dev, &mux_desc); >> > + if (IS_ERR(port_data->typec_mux)) { >> > + ret = PTR_ERR(port_data->typec_mux); >> > + dev_err(dev, "Mode switch register for port %d failed: %d\n", >> > + port_num, ret); >> > + >> > + return ret; >> >> you don't need this return here... >> >> > + } >> > + >> > + return 0; >> >> Just "return ret;" here. This was actually suggested by Angelo in [1]. I personally don't have a strong opinion on either approach. [1]https://lore.kernel.org/all/023519eb-0adb-3b08-71b9-afb92a6cceaf@collabora.com/ Pin-yen >> >> >> > +} >> > + >> > +/** >> > + * drm_dp_register_typec_switches() - register Type-C switches >> > + * @dev: Device that registers Type-C switches >> > + * @port: Device node for the switch >> > + * @switch_desc: A Type-C switch descriptor >> > + * @data: Private data for the switches >> > + * @mux_set: Callback function for typec_mux_set >> > + * >> > + * This function registers USB Type-C switches for DP bridges that can switch >> > + * the output signal between their output pins. >> > + * >> > + * Currently only mode switches are implemented, and the function assumes the >> > + * given @port device node has endpoints with "mode-switch" property. >> > + * The port number is determined by the "reg" property of the endpoint. >> > + */ >> > +int drm_dp_register_typec_switches(struct device *dev, struct fwnode_handle *port, >> > + struct drm_dp_typec_switch_desc *switch_desc, >> > + void *data, typec_mux_set_fn_t mux_set) >> > +{ >> > + struct fwnode_handle *sw; >> > + int ret; >> > + >> > + fwnode_for_each_child_node(port, sw) { >> > + if (fwnode_property_present(sw, "mode-switch")) >> > + switch_desc->num_typec_switches++; >> > + } >> >> no need for brackets here >> >> > + >> > + if (!switch_desc->num_typec_switches) { >> > + dev_dbg(dev, "No Type-C switches node found\n"); >> >> dev_warn()? > > > I used dev_dbg here because the users might call this without checking if there are mode switch endpoints present, and this is the case for the current users (it6505 and anx7625). If we use dev_warn here, there will be warnings every time even on use cases without Type-C switches. > > Thanks and regards, > Pin-yen >> >> >> > + return 0; >> > + } >> > + >> > + switch_desc->typec_ports = devm_kcalloc( >> > + dev, switch_desc->num_typec_switches, >> > + sizeof(struct drm_dp_typec_port_data), GFP_KERNEL); >> > + >> > + if (!switch_desc->typec_ports) >> > + return -ENOMEM; >> > + >> > + /* Register switches for each connector. */ >> > + fwnode_for_each_child_node(port, sw) { >> > + if (!fwnode_property_present(sw, "mode-switch")) >> > + continue; >> > + ret = drm_dp_register_mode_switch(dev, sw, switch_desc, data, mux_set); >> > + if (ret) >> > + goto err_unregister_typec_switches; >> > + } >> > + >> > + return 0; >> > + >> > +err_unregister_typec_switches: >> > + fwnode_handle_put(sw); >> > + drm_dp_unregister_typec_switches(switch_desc); >> > + dev_err(dev, "Failed to register mode switch: %d\n", ret); >> >> there is a bit of dmesg spamming. Please choose where you want to >> print the error, either in this function or in >> drm_dp_register_mode_switch(). >> >> Andi >> >> > + return ret; >> > +} >> > +EXPORT_SYMBOL(drm_dp_register_typec_switches); >> >> [...]
diff --git a/drivers/gpu/drm/display/Makefile b/drivers/gpu/drm/display/Makefile index 17ac4a1006a8..ef80b9fde615 100644 --- a/drivers/gpu/drm/display/Makefile +++ b/drivers/gpu/drm/display/Makefile @@ -14,5 +14,6 @@ drm_display_helper-$(CONFIG_DRM_DISPLAY_HDMI_HELPER) += \ drm_scdc_helper.o drm_display_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o drm_display_helper-$(CONFIG_DRM_DP_CEC) += drm_dp_cec.o +drm_display_helper-$(CONFIG_TYPEC) += drm_dp_typec_helper.o obj-$(CONFIG_DRM_DISPLAY_HELPER) += drm_display_helper.o diff --git a/drivers/gpu/drm/display/drm_dp_typec_helper.c b/drivers/gpu/drm/display/drm_dp_typec_helper.c new file mode 100644 index 000000000000..b11a268da57f --- /dev/null +++ b/drivers/gpu/drm/display/drm_dp_typec_helper.c @@ -0,0 +1,114 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <linux/usb/typec_mux.h> +#include <drm/display/drm_dp_helper.h> + +static int drm_dp_register_mode_switch(struct device *dev, + struct fwnode_handle *fwnode, + struct drm_dp_typec_switch_desc *switch_desc, + void *data, typec_mux_set_fn_t mux_set) +{ + struct drm_dp_typec_port_data *port_data; + struct typec_mux_desc mux_desc = {}; + char name[32]; + u32 port_num; + int ret; + + ret = fwnode_property_read_u32(fwnode, "reg", &port_num); + if (ret) { + dev_err(dev, "Failed to read reg property: %d\n", ret); + return ret; + } + + port_data = &switch_desc->typec_ports[port_num]; + port_data->data = data; + port_data->port_num = port_num; + port_data->fwnode = fwnode; + mux_desc.fwnode = fwnode; + mux_desc.drvdata = port_data; + snprintf(name, sizeof(name), "%pfwP-%u", fwnode, port_num); + mux_desc.name = name; + mux_desc.set = mux_set; + + port_data->typec_mux = typec_mux_register(dev, &mux_desc); + if (IS_ERR(port_data->typec_mux)) { + ret = PTR_ERR(port_data->typec_mux); + dev_err(dev, "Mode switch register for port %d failed: %d\n", + port_num, ret); + + return ret; + } + + return 0; +} + +/** + * drm_dp_register_typec_switches() - register Type-C switches + * @dev: Device that registers Type-C switches + * @port: Device node for the switch + * @switch_desc: A Type-C switch descriptor + * @data: Private data for the switches + * @mux_set: Callback function for typec_mux_set + * + * This function registers USB Type-C switches for DP bridges that can switch + * the output signal between their output pins. + * + * Currently only mode switches are implemented, and the function assumes the + * given @port device node has endpoints with "mode-switch" property. + * The port number is determined by the "reg" property of the endpoint. + */ +int drm_dp_register_typec_switches(struct device *dev, struct fwnode_handle *port, + struct drm_dp_typec_switch_desc *switch_desc, + void *data, typec_mux_set_fn_t mux_set) +{ + struct fwnode_handle *sw; + int ret; + + fwnode_for_each_child_node(port, sw) { + if (fwnode_property_present(sw, "mode-switch")) + switch_desc->num_typec_switches++; + } + + if (!switch_desc->num_typec_switches) { + dev_dbg(dev, "No Type-C switches node found\n"); + return 0; + } + + switch_desc->typec_ports = devm_kcalloc( + dev, switch_desc->num_typec_switches, + sizeof(struct drm_dp_typec_port_data), GFP_KERNEL); + + if (!switch_desc->typec_ports) + return -ENOMEM; + + /* Register switches for each connector. */ + fwnode_for_each_child_node(port, sw) { + if (!fwnode_property_present(sw, "mode-switch")) + continue; + ret = drm_dp_register_mode_switch(dev, sw, switch_desc, data, mux_set); + if (ret) + goto err_unregister_typec_switches; + } + + return 0; + +err_unregister_typec_switches: + fwnode_handle_put(sw); + drm_dp_unregister_typec_switches(switch_desc); + dev_err(dev, "Failed to register mode switch: %d\n", ret); + return ret; +} +EXPORT_SYMBOL(drm_dp_register_typec_switches); + +/** + * drm_dp_unregister_typec_switches() - unregister Type-C switches + * @switch_desc: A Type-C switch descriptor + */ +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc) +{ + int i; + + for (i = 0; i < switch_desc->num_typec_switches; i++) + typec_mux_unregister(switch_desc->typec_ports[i].typec_mux); +} +EXPORT_SYMBOL(drm_dp_unregister_typec_switches); diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h index ab55453f2d2c..d9213739de72 100644 --- a/include/drm/display/drm_dp_helper.h +++ b/include/drm/display/drm_dp_helper.h @@ -25,6 +25,7 @@ #include <linux/delay.h> #include <linux/i2c.h> +#include <linux/usb/typec_mux.h> #include <drm/display/drm_dp.h> #include <drm/drm_connector.h> @@ -763,4 +764,34 @@ bool drm_dp_downstream_rgb_to_ycbcr_conversion(const u8 dpcd[DP_RECEIVER_CAP_SIZ const u8 port_cap[4], u8 color_spc); int drm_dp_pcon_convert_rgb_to_ycbcr(struct drm_dp_aux *aux, u8 color_spc); +struct drm_dp_typec_port_data { + struct typec_mux_dev *typec_mux; + int port_num; + struct fwnode_handle *fwnode; + void *data; +}; + +struct drm_dp_typec_switch_desc { + int num_typec_switches; + struct drm_dp_typec_port_data *typec_ports; +}; + +#ifdef CONFIG_TYPEC +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc); +int drm_dp_register_typec_switches(struct device *dev, struct fwnode_handle *port, + struct drm_dp_typec_switch_desc *switch_desc, + void *data, typec_mux_set_fn_t mux_set); +#else +static inline void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc) +{ +} +static inline int drm_dp_register_typec_switches( + struct device *dev, struct fwnode_handle *port, + struct drm_dp_typec_switch_desc *switch_desc, void *data, + typec_mux_set_fn_t mux_set) +{ + return -EOPNOTSUPP; +} +#endif + #endif /* _DRM_DP_HELPER_H_ */
Add helpers to register and unregister Type-C "switches" for bridges capable of switching their output between two downstream devices. The helper registers USB Type-C mode switches when the "mode-switch" and the "reg" properties are available in Device Tree. Signed-off-by: Pin-yen Lin <treapking@chromium.org> --- Changes in v11: - Use fwnode helpers instead of DT - Moved the helpers to a new file - Use "reg" instead of "data-lanes" to determine the port number - Dropped collected tags due to new changes Changes in v10: - Collected Reviewed-by and Tested-by tags - Replaced "void *" with "typec_mux_set_fn_t" for mux_set callbacks - Print out the node name when errors on parsing DT - Use dev_dbg instead of dev_warn when no Type-C switch nodes available - Made the return path of drm_dp_register_mode_switch clearer Changes in v8: - Fixed the build issue when CONFIG_TYPEC=m - Fixed some style issues Changes in v7: - Extracted the common codes to a helper function - New in v7 drivers/gpu/drm/display/Makefile | 1 + drivers/gpu/drm/display/drm_dp_typec_helper.c | 114 ++++++++++++++++++ include/drm/display/drm_dp_helper.h | 31 +++++ 3 files changed, 146 insertions(+) create mode 100644 drivers/gpu/drm/display/drm_dp_typec_helper.c