| Message ID | 20230221095054.1868277-1-treapking@chromium.org |
|---|---|
| Headers | show |
| Series | Register Type-C mode-switch in DP bridge endpoints | expand |
On Tue, Feb 21, 2023 at 05:50:45PM +0800, Pin-yen Lin wrote: > From: Prashant Malani <pmalani@chromium.org> > > 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. ... > * @match: Function to check and convert the connection description > * > * Find a connection with unique identifier @con_id between @fwnode and another > - * device node. @match will be used to convert the connection description to > - * data the caller is expecting to be returned. > + * device node. For fwnode graph connections, the graph endpoints are also > + * checked. @match will be used to convert the connection description to data > + * the caller is expecting to be returned. > */ Please add a Return: section at the end of the kernel doc. Otherwise it complains that there is none. ... > * @matches_len: Length of @matches > * > * Find up to @matches_len connections with unique identifier @con_id between > - * @fwnode and other device nodes. @match will be used to convert the > - * connection description to data the caller is expecting to be returned > - * through the @matches array. > + * @fwnode and other device nodes. For fwnode graph connections, the graph > + * endpoints are also checked. @match will be used to convert the connection > + * description to data the caller is expecting to be returned through the > + * @matches array. > * If @matches is NULL @matches_len is ignored and the total number of resolved > * matches is returned. Ditto.
On Tue, Feb 21, 2023 at 05:50:49PM +0800, Pin-yen Lin wrote: > The output port endpoints can be connected to USB-C connectors. > Running drm_of_find_panel_or_bridge() with such endpoints leads to > a continuous return value of -EPROBE_DEFER, even though there is > no panel present. > > To avoid this, check for the existence of a "mode-switch" property in > the port endpoint, and skip panel registration completely if so. ... > + port_node = of_graph_get_port_by_id(np, 1); > + fwnode_for_each_typec_mode_switch(&port_node->fwnode, fwnode) { > + fwnode_handle_put(fwnode); > + return 0; > + } With the proposed count API: unsigned int count; ... port_node = ... count = typec_mode_switch_node_count(&port_node->fwnode); if (count == 0) return 0;
Hi Andy, Thanks for the review. On Tue, Feb 21, 2023 at 7:36 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Feb 21, 2023 at 05:50:51PM +0800, Pin-yen Lin wrote: > > Register USB Type-C mode switches when the "mode-switch" property and > > relevant ports are available in Device Tree. Configure the crosspoint > > switch based on the entered alternate mode for a specific Type-C > > connector. > > > > Crosspoint switch can also be used for switching the output signal for > > different orientations of a single USB Type-C connector, but the > > orientation switch is not implemented yet. A TODO is added for this. > > ... > > > +static void anx7625_typec_two_ports_update(struct anx7625_data *ctx) > > +{ > > + int i; > > unsigned? > > + Blank line. > > > + /* Check if both ports available and do nothing to retain the current one */ > > + if (ctx->port_data[0].dp_connected && ctx->port_data[1].dp_connected) > > + return; > > + > > + for (i = 0; i < 2; i++) { > > + if (ctx->port_data[i].dp_connected) > > + anx7625_set_crosspoint_switch(ctx, > > + ctx->port_data[i].orientation); > > + } > > +} > > ... > > > + ctx->port_data[port->port_num].dp_connected = > > + state->alt && state->alt->svid == USB_TYPEC_DP_SID && > > I would move the first parameter of && to the separate line for slightly better > readability. > > > + state->alt->mode == USB_TYPEC_DP_MODE; > > ... > > > + for (i = 0; i < switch_desc->num_typec_switches; i++) { > > + struct drm_dp_typec_port_data *port = &switch_desc->typec_ports[i]; > > + struct fwnode_handle *fwnode = port->fwnode; > > + > > + num_lanes = fwnode_property_count_u32(fwnode, "data-lanes"); > > > + > > Redundant blank line. > > > + if (num_lanes < 0) { > > + dev_err(dev, > > + "Error on getting data lanes count from %pfwP: %d\n", > > + fwnode, num_lanes); > > > + ret = num_lanes; > > Can be written differently: > > > + goto unregister_mux; > > + } > > ret = ... > if (ret < 0) { > ... > } > num_lanes = ret; > > > What if it's 0? The binding does not allow that, so I don't think we should check it here. I'll address other comments in the next version. Regards, Pin-yen > > > + 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: %d\n", > > + ret); > > + goto unregister_mux; > > + } > > + > > + ctx->port_data[i].orientation = (dp_lanes[0] / 2 == 0) ? > > + TYPEC_ORIENTATION_REVERSE : TYPEC_ORIENTATION_NORMAL; > > + ctx->port_data[i].dp_connected = false; > > + } > > + complete_all(&ctx->mux_register); > > + > > + return 0; > > + > > +unregister_mux: > > + complete_all(&ctx->mux_register); > > + anx7625_unregister_typec_switches(ctx); > > + return ret; > > +} > > -- > With Best Regards, > Andy Shevchenko > >
Hi Andy, Thanks for the review. On Tue, Feb 21, 2023 at 7:48 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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 <treapking@chromium.org> > > ... > > > + 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 <linux/delay.h> > > #include <linux/i2c.h> > > +#include <linux/usb/typec_mux.h> > > 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 <drm/display/drm_dp.h> > > #include <drm/drm_connector.h> > > ... > > > +#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 > >
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 <linux/delay.h> > > > #include <linux/i2c.h> > > > +#include <linux/usb/typec_mux.h> > > > > 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.