Re: [PATCH v12 03/10] drm/display: Add Type-C switch helpers
On Wed, Feb 22, 2023 at 04:53:41PM +0800, Pin-yen Lin wrote: > On Tue, Feb 21, 2023 at 7:48 PM Andy Shevchenko > > On Tue, Feb 21, 2023 at 05:50:47PM +0800, Pin-yen Lin wrote: ... > > > #include > > > #include > > > +#include > > > > I don't see users of this. > > But a few forward declarations are missing. > > I can put a `struct typec_mux_dev;` here, but there is also a usage of > typec_mux_set_fn_t. > > Should I add the typedef into this header file as well? No, inclusion is fine, it's me who didn't notice the type in use. -- With Best Regards, Andy Shevchenko
Re: [PATCH v12 03/10] drm/display: Add Type-C switch helpers
Hi Andy, Thanks for the review. On Tue, Feb 21, 2023 at 7:48 PM Andy Shevchenko wrote: > > On Tue, Feb 21, 2023 at 05:50:47PM +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. > > > > Signed-off-by: Pin-yen Lin > > ... > > > + fwnode_for_each_typec_mode_switch(port, sw) > > + switch_desc->num_typec_switches++; > > + > > + if (!switch_desc->num_typec_switches) { > > + dev_dbg(dev, "No Type-C switches node found\n"); > > + return 0; > > + } > > What about > > static inline unsigned int typec_mode_switch_node_count(... *port) > { > ... *sw; > unsigned int count = 0; > > for_each_typec_mode_switch_node(port, sw) > count++; > > return count; > } > > > And then it seems something like > > unsigned int count; > > count = typec_mode_switch_node_count(port); > if (!count) { > ... > } > > _switches = count; > > ... > > > + switch_desc->typec_ports = devm_kcalloc( > > + dev, switch_desc->num_typec_switches, > > Strange indentation. > > > + sizeof(struct drm_dp_typec_port_data), GFP_KERNEL); > > > + > > Redundant blank line. > > > + if (!switch_desc->typec_ports) > > + return -ENOMEM; > > ... > > > +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc > > *switch_desc) > > +{ > > + int i; > > unsigned? > > > + for (i = 0; i < switch_desc->num_typec_switches; i++) > > + typec_mux_unregister(switch_desc->typec_ports[i].typec_mux); > > +} > > ... > > > #include > > #include > > +#include > > I don't see users of this. > But a few forward declarations are missing. I can put a `struct typec_mux_dev;` here, but there is also a usage of typec_mux_set_fn_t. Should I add the typedef into this header file as well? Regards, Pin-yen > > > #include > > #include > > ... > > > +#define fwnode_for_each_typec_mode_switch(port, sw) \ > > + fwnode_for_each_child_node((port), (sw))\ > > + for_each_if(fwnode_property_present((sw), "mode-switch")) > > Please don't use fwnode namespace (see above), something like > > #define for_each_typec_mode_switch_node(port, sw) \ > ... > > -- > With Best Regards, > Andy Shevchenko > >
Re: [PATCH v12 03/10] drm/display: Add Type-C switch helpers
On Tue, Feb 21, 2023 at 05:50:47PM +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. > > Signed-off-by: Pin-yen Lin ... > + fwnode_for_each_typec_mode_switch(port, sw) > + switch_desc->num_typec_switches++; > + > + if (!switch_desc->num_typec_switches) { > + dev_dbg(dev, "No Type-C switches node found\n"); > + return 0; > + } What about static inline unsigned int typec_mode_switch_node_count(... *port) { ... *sw; unsigned int count = 0; for_each_typec_mode_switch_node(port, sw) count++; return count; } And then it seems something like unsigned int count; count = typec_mode_switch_node_count(port); if (!count) { ... } _switches = count; ... > + switch_desc->typec_ports = devm_kcalloc( > + dev, switch_desc->num_typec_switches, Strange indentation. > + sizeof(struct drm_dp_typec_port_data), GFP_KERNEL); > + Redundant blank line. > + if (!switch_desc->typec_ports) > + return -ENOMEM; ... > +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc > *switch_desc) > +{ > + int i; unsigned? > + for (i = 0; i < switch_desc->num_typec_switches; i++) > + typec_mux_unregister(switch_desc->typec_ports[i].typec_mux); > +} ... > #include > #include > +#include I don't see users of this. But a few forward declarations are missing. > #include > #include ... > +#define fwnode_for_each_typec_mode_switch(port, sw) \ > + fwnode_for_each_child_node((port), (sw))\ > + for_each_if(fwnode_property_present((sw), "mode-switch")) Please don't use fwnode namespace (see above), something like #define for_each_typec_mode_switch_node(port, sw) \ ... -- With Best Regards, Andy Shevchenko
[PATCH v12 03/10] drm/display: Add Type-C switch helpers
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 --- Changes in v12: - Add fwnode_for_each_typec_mode_switch macro - Remove a duplicated dmesg in the helper - Used IS_REACHABLE instead to guard the function signatures 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 | 108 ++ include/drm/display/drm_dp_helper.h | 35 ++ 3 files changed, 144 insertions(+) create mode 100644 drivers/gpu/drm/display/drm_dp_typec_helper.c diff --git a/drivers/gpu/drm/display/Makefile b/drivers/gpu/drm/display/Makefile index 17ac4a1006a8..ef80b9fde615 100644 --- a/drivers/gpu/drm/display/Makefile +++ b/drivers/gpu/drm/display/Makefile @@ -14,5 +14,6 @@ drm_display_helper-$(CONFIG_DRM_DISPLAY_HDMI_HELPER) += \ drm_scdc_helper.o drm_display_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o drm_display_helper-$(CONFIG_DRM_DP_CEC) += drm_dp_cec.o +drm_display_helper-$(CONFIG_TYPEC) += drm_dp_typec_helper.o obj-$(CONFIG_DRM_DISPLAY_HELPER) += drm_display_helper.o diff --git a/drivers/gpu/drm/display/drm_dp_typec_helper.c b/drivers/gpu/drm/display/drm_dp_typec_helper.c new file mode 100644 index ..161e4110f6b9 --- /dev/null +++ b/drivers/gpu/drm/display/drm_dp_typec_helper.c @@ -0,0 +1,108 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include +#include + +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", _num); + if (ret) { + dev_err(dev, "Failed to read reg property: %d\n", ret); + return ret; + } + + port_data = _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, _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_typec_mode_switch(port, sw) + 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),