<div dir="ltr"><div dir="ltr">Hi Andi,<div> </div><div>Thanks for the review.</div></div> <div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Feb 8, 2023 at 5:25 AM Andi Shyti <<a href="mailto:andi.shyti@linux.intel.com">andi.shyti@linux.intel.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">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); > + <a href="http://mux_desc.name" rel="noreferrer" target="_blank">mux_desc.name</a> = 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. </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > +} > + > +/** > + * 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()? </blockquote><div> </div><div>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.</div><div> </div><div>Thanks and regards,</div><div>Pin-yen</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > + 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); [...] </blockquote></div></div>