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 [off-list ref] wrote:

Hi Andi,

Thanks for the review.

On Wed, Feb 8, 2023 at 5:25 AM Andi Shyti [off-list ref] wrote:

quoted

Hi Pin-yen,

[...]

quoted

+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...

quoted

+     }
+
+     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/ (local)

Pin-yen

quoted

quoted

+}
+
+/**
+ * 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

quoted

+
+     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

quoted

quoted

+             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

quoted

+     return ret;
+}
+EXPORT_SYMBOL(drm_dp_register_typec_switches);

[...]