Messages in this thread Patch in this message | | | Subject | Re: [PATCH v3 13/13] platform/x86: intel_cht_int33fe: Replacing the old connections with references | From | Hans de Goede <> | Date | Tue, 16 Apr 2019 23:35:36 +0200 |
| |
Hi,
On 12-04-19 15:41, Heikki Krogerus wrote: > Now that the software nodes support references, and the > device connection API support parsing fwnode references, > replacing the old connection descriptions with software node > references. Relying on device names when matching the > connection would not have been possible to link the USB > Type-C connector and the DisplayPort connector together, but > with real references it's not problem. > > The DisplayPort ACPI node is dag up, and the drivers own > software node for the DisplayPort is set as the secondary > node for it. The USB Type-C connector refers the software > node, but it is now tied to the ACPI node, and therefore any > device entry (struct drm_connector in practice) that the > node combo is assigned to. > > The USB role switch device does not have ACPI node, so we > have to wait for the device to appear. Then we can simply > assign our software node for the to the device. > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
So as promised I've been testing this series and this commit breaks type-c functionality on devices using this driver.
The problem is that typec_switch_get() and typec_mux_get() after this both return the same pointer, which is pointing to the switch, so typec_mux_get() is returning the wrong pointer.
This is not surprising since the references for both are both pointing to the fwnode attached to the piusb30532 devices:
args[0].fwnode = data->node[INT33FE_NODE_PI3USB30532];
So the class_find_device here:
static void *typec_switch_match(struct device_connection *con, int ep, void *data) { struct device *dev;
if (con->fwnode) { if (con->id && !fwnode_property_present(con->fwnode, con->id)) return NULL;
dev = class_find_device(&typec_mux_class, NULL, con->fwnode, fwnode_match); } else { dev = class_find_device(&typec_mux_class, NULL, con->endpoint[ep], name_match); }
return dev ? to_typec_switch(dev) : ERR_PTR(-EPROBE_DEFER); }
Simply returns the first typec_mux_class device registered.
I see 2 possible solutions to this problem:
1) Use separate typec_mux_class and typec_orientation_switch_class-es
2) Merge struct typec_switch and struct typec_mux into a single struct, so that all typec_mux_class devices have the same memory layout, add a subclass enum to this new merged struct and use that to identify which of the typec_mux_class devices with the same fwnode pointer we want.
Any other suggestions?
Regards,
Hans
> --- > drivers/platform/x86/intel_cht_int33fe.c | 93 +++++++++++++++++++----- > 1 file changed, 76 insertions(+), 17 deletions(-) > > diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c > index 7711667a3454..e6a1ea7f33af 100644 > --- a/drivers/platform/x86/intel_cht_int33fe.c > +++ b/drivers/platform/x86/intel_cht_int33fe.c > @@ -39,15 +39,22 @@ enum { > INT33FE_NODE_MAX, > }; > > +enum { > + INT33FE_REF_ORIENTATION, > + INT33FE_REF_MODE_MUX, > + INT33FE_REF_DISPLAYPORT, > + INT33FE_REF_ROLE_SWITCH, > + INT33FE_REF_MAX, > +}; > + > struct cht_int33fe_data { > struct i2c_client *max17047; > struct i2c_client *fusb302; > struct i2c_client *pi3usb30532; > - /* Contain a list-head must be per device */ > - struct device_connection connections[4]; > > struct fwnode_handle *dp; > struct fwnode_handle *node[INT33FE_NODE_MAX]; > + struct software_node_reference *ref[INT33FE_REF_MAX]; > }; > > /* > @@ -280,6 +287,65 @@ static int cht_int33fe_add_nodes(struct cht_int33fe_data *data) > return ret; > } > > +static void cht_int33fe_remove_references(struct cht_int33fe_data *data) > +{ > + int i; > + > + for (i = 0; i < INT33FE_REF_MAX; i++) > + fwnode_remove_software_node_reference(data->ref[i]); > +} > + > +static int cht_int33fe_add_references(struct cht_int33fe_data *data) > +{ > + struct fwnode_reference_args args[2] = { }; > + struct software_node_reference *ref; > + struct fwnode_handle *con; > + > + con = data->node[INT33FE_NODE_USB_CONNECTOR]; > + > + /* USB Type-C muxes */ > + args[0].fwnode = data->node[INT33FE_NODE_PI3USB30532]; > + args[1].fwnode = NULL; > + > + ref = fwnode_create_software_node_reference(con, "orientation-switch", > + args); > + if (IS_ERR(ref)) > + return PTR_ERR(ref); > + data->ref[INT33FE_REF_ORIENTATION] = ref; > + > + ref = fwnode_create_software_node_reference(con, "mode-switch", args); > + if (IS_ERR(ref)) { > + cht_int33fe_remove_references(data); > + return PTR_ERR(ref); > + } > + data->ref[INT33FE_REF_MODE_MUX] = ref; > + > + /* USB role switch */ > + args[0].fwnode = data->node[INT33FE_NODE_ROLE_SWITCH]; > + args[1].fwnode = NULL; > + > + ref = fwnode_create_software_node_reference(con, "usb-role-switch", > + args); > + if (IS_ERR(ref)) { > + cht_int33fe_remove_references(data); > + return PTR_ERR(ref); > + } > + data->ref[INT33FE_REF_ROLE_SWITCH] = ref; > + > + /* DisplayPort */ > + args[0].fwnode = data->node[INT33FE_NODE_DISPLAYPORT]; > + args[1].fwnode = NULL; > + > + ref = fwnode_create_software_node_reference(con, "displayport", args); > + if (IS_ERR(ref)) { > + cht_int33fe_remove_references(data); > + return PTR_ERR(ref); > + } > + data->ref[INT33FE_REF_DISPLAYPORT] = ref; > + > + return 0; > +} > + > static int cht_int33fe_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -348,22 +414,14 @@ static int cht_int33fe_probe(struct platform_device *pdev) > if (ret) > return ret; > > - /* Work around BIOS bug, see comment on cht_int33fe_check_for_max17047 */ > - ret = cht_int33fe_register_max17047(dev, data); > + ret = cht_int33fe_add_references(data); > if (ret) > goto out_remove_nodes; > > - data->connections[0].endpoint[0] = "port0"; > - data->connections[0].endpoint[1] = "i2c-pi3usb30532-switch"; > - data->connections[0].id = "orientation-switch"; > - data->connections[1].endpoint[0] = "port0"; > - data->connections[1].endpoint[1] = "i2c-pi3usb30532-mux"; > - data->connections[1].id = "mode-switch"; > - data->connections[2].endpoint[0] = "i2c-fusb302"; > - data->connections[2].endpoint[1] = "intel_xhci_usb_sw-role-switch"; > - data->connections[2].id = "usb-role-switch"; > - > - device_connections_add(data->connections); > + /* Work around BIOS bug, see comment on cht_int33fe_check_for_max17047 */ > + ret = cht_int33fe_register_max17047(dev, data); > + if (ret) > + goto out_remove_references; > > memset(&board_info, 0, sizeof(board_info)); > strlcpy(board_info.type, "typec_fusb302", I2C_NAME_SIZE); > @@ -398,7 +456,8 @@ static int cht_int33fe_probe(struct platform_device *pdev) > out_unregister_max17047: > i2c_unregister_device(data->max17047); > > - device_connections_remove(data->connections); > +out_remove_references: > + cht_int33fe_remove_references(data); > > out_remove_nodes: > cht_int33fe_remove_nodes(data); > @@ -414,7 +473,7 @@ static int cht_int33fe_remove(struct platform_device *pdev) > i2c_unregister_device(data->fusb302); > i2c_unregister_device(data->max17047); > > - device_connections_remove(data->connections); > + cht_int33fe_remove_references(data); > cht_int33fe_remove_nodes(data); > > return 0; >
| |