| Message ID | 20190412134122.82903-14-heikki.krogerus@linux.intel.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | Software fwnode references | expand |
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; >
On Tue, Apr 16, 2019 at 11:35:36PM +0200, Hans de Goede wrote: > 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? I think the correct fix is that we supply separate nodes for both device entries. thanks,
Hi, On 17-04-19 08:39, Heikki Krogerus wrote: > On Tue, Apr 16, 2019 at 11:35:36PM +0200, Hans de Goede wrote: >> 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? > > I think the correct fix is that we supply separate nodes for both > device entries. That is not going to work since the (virtual) mux / orientation-switch devices are only registered once the driver binds to the piusb30532 i2c device, so when creating the nodes we only have the piusb30532 i2c device. I've been thinking some more about this and an easy fix is to have separate fwnode_match functions for typec_switch_match and typec_mux_match and have them check that the dev_name ends in "-mux" resp. "-switch" that requires only a very minimal change to "usb: typec: Registering real device entries for the muxes" and then everything should be fine. Note that another problem with this series which I noticed while testing is that the usb-role-switch is not being found at all anymore after this ("Replacing the old connections with references") patch. I still need start debugging that. Regards, Hans
On Wed, Apr 17, 2019 at 11:19:28AM +0200, Hans de Goede wrote: > Hi, > > On 17-04-19 08:39, Heikki Krogerus wrote: > > On Tue, Apr 16, 2019 at 11:35:36PM +0200, Hans de Goede wrote: > > > 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? > > > > I think the correct fix is that we supply separate nodes for both > > device entries. > > That is not going to work since the (virtual) mux / orientation-switch > devices are only registered once the driver binds to the piusb30532 i2c > device, so when creating the nodes we only have the piusb30532 i2c device. It's not a problem, that's why we have the software nodes. The nodes can be created before the device entires. The node for pi3usb30532 will just be the parent node for the new nodes we add for the mux and switch. > I've been thinking some more about this and an easy fix is to have separate > fwnode_match functions for typec_switch_match and typec_mux_match and have > them check that the dev_name ends in "-mux" resp. "-switch" that requires > only a very minimal change to "usb: typec: Registering real device entries for the muxes" > and then everything should be fine. I don't want to do anymore device name matching unless we have to, and here we don't have to. We can name the nodes for those virtual mux and switch, and then just do fwnode_find_named_child_node() in pi3usb30532.c for both of them. > Note that another problem with this series which I noticed while testing > is that the usb-role-switch is not being found at all anymore after > this ("Replacing the old connections with references") patch. I still need > start debugging that. OK, I'll wait for you results on that. thanks,
Hi, On 17-04-19 11:32, Heikki Krogerus wrote: > On Wed, Apr 17, 2019 at 11:19:28AM +0200, Hans de Goede wrote: >> Hi, >> >> On 17-04-19 08:39, Heikki Krogerus wrote: >>> On Tue, Apr 16, 2019 at 11:35:36PM +0200, Hans de Goede wrote: >>>> 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? >>> >>> I think the correct fix is that we supply separate nodes for both >>> device entries. >> >> That is not going to work since the (virtual) mux / orientation-switch >> devices are only registered once the driver binds to the piusb30532 i2c >> device, so when creating the nodes we only have the piusb30532 i2c device. > > It's not a problem, that's why we have the software nodes. The nodes > can be created before the device entires. The node for pi3usb30532 > will just be the parent node for the new nodes we add for the mux and > switch. > >> I've been thinking some more about this and an easy fix is to have separate >> fwnode_match functions for typec_switch_match and typec_mux_match and have >> them check that the dev_name ends in "-mux" resp. "-switch" that requires >> only a very minimal change to "usb: typec: Registering real device entries for the muxes" >> and then everything should be fine. > > I don't want to do anymore device name matching unless we have to, and > here we don't have to. We can name the nodes for those virtual mux and > switch, and then just do fwnode_find_named_child_node() in > pi3usb30532.c for both of them. Ok. >> Note that another problem with this series which I noticed while testing >> is that the usb-role-switch is not being found at all anymore after >> this ("Replacing the old connections with references") patch. I still need >> start debugging that. > > OK, I'll wait for you results on that. I understand that you want to wait with doing a v4 until this is resolved, but can you (privately) send me a v3.5 fixing the mux/switch issue so that I can test that (and get the problem out of the way for debugging the other stuff) ? I've also just replied to the following patch with a review remark: [PATCH v3 11/13] platform/x86: intel_cht_int33fe: Provide fwnode for the USB connector Since you need to respin the series anyways, it would be good if you can address that too. Regards, Hans
Hi, On 17-04-19 11:32, Heikki Krogerus wrote: > On Wed, Apr 17, 2019 at 11:19:28AM +0200, Hans de Goede wrote: <snip> >> That is not going to work since the (virtual) mux / orientation-switch >> devices are only registered once the driver binds to the piusb30532 i2c >> device, so when creating the nodes we only have the piusb30532 i2c device. > > It's not a problem, that's why we have the software nodes. The nodes > can be created before the device entires. The node for pi3usb30532 > will just be the parent node for the new nodes we add for the mux and > switch. > >> I've been thinking some more about this and an easy fix is to have separate >> fwnode_match functions for typec_switch_match and typec_mux_match and have >> them check that the dev_name ends in "-mux" resp. "-switch" that requires >> only a very minimal change to "usb: typec: Registering real device entries for the muxes" >> and then everything should be fine. > > I don't want to do anymore device name matching unless we have to, and > here we don't have to. We can name the nodes for those virtual mux and > switch, and then just do fwnode_find_named_child_node() in > pi3usb30532.c for both of them. Thinking more about this, I have a feeling that this makes things needlessly complicated, checking the dev_name *ends* in "-mux" resp. "-switch" should be 100% reliable since we call: dev_set_name(&sw->dev, "%s-switch", dev_name(parent)); dev_set_name(&mux->dev, "%s-mux", dev_name(parent)); When registering the switch / mux, so I believe doing name (suffix) comparison here is fine and much simpler. Anyways this is just my 2 cents on this, I'm happy with either solution, your choice. Regards, Hans
On Wed, Apr 17, 2019 at 12:15:18PM +0200, Hans de Goede wrote: > Hi, > > On 17-04-19 11:32, Heikki Krogerus wrote: > > On Wed, Apr 17, 2019 at 11:19:28AM +0200, Hans de Goede wrote: > > <snip> > > > > That is not going to work since the (virtual) mux / orientation-switch > > > devices are only registered once the driver binds to the piusb30532 i2c > > > device, so when creating the nodes we only have the piusb30532 i2c device. > > > > It's not a problem, that's why we have the software nodes. The nodes > > can be created before the device entires. The node for pi3usb30532 > > will just be the parent node for the new nodes we add for the mux and > > switch. > > > > > I've been thinking some more about this and an easy fix is to have separate > > > fwnode_match functions for typec_switch_match and typec_mux_match and have > > > them check that the dev_name ends in "-mux" resp. "-switch" that requires > > > only a very minimal change to "usb: typec: Registering real device entries for the muxes" > > > and then everything should be fine. > > > > I don't want to do anymore device name matching unless we have to, and > > here we don't have to. We can name the nodes for those virtual mux and > > switch, and then just do fwnode_find_named_child_node() in > > pi3usb30532.c for both of them. > > Thinking more about this, I have a feeling that this makes things needlessly > complicated, checking the dev_name *ends* in "-mux" resp. "-switch" should be > 100% reliable since we call: > > dev_set_name(&sw->dev, "%s-switch", dev_name(parent)); > dev_set_name(&mux->dev, "%s-mux", dev_name(parent)); > > When registering the switch / mux, so I believe doing name (suffix) comparison > here is fine and much simpler. Anyways this is just my 2 cents on this, I'm > happy with either solution, your choice. You do have a point. I'll take a look how the two options look like, but maybe your way is better after all. thanks,
On Wed, Apr 17, 2019 at 11:52:15AM +0200, Hans de Goede wrote: > Hi, > > On 17-04-19 11:32, Heikki Krogerus wrote: > > On Wed, Apr 17, 2019 at 11:19:28AM +0200, Hans de Goede wrote: > > > Hi, > > > > > > On 17-04-19 08:39, Heikki Krogerus wrote: > > > > On Tue, Apr 16, 2019 at 11:35:36PM +0200, Hans de Goede wrote: > > > > > 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? > > > > > > > > I think the correct fix is that we supply separate nodes for both > > > > device entries. > > > > > > That is not going to work since the (virtual) mux / orientation-switch > > > devices are only registered once the driver binds to the piusb30532 i2c > > > device, so when creating the nodes we only have the piusb30532 i2c device. > > > > It's not a problem, that's why we have the software nodes. The nodes > > can be created before the device entires. The node for pi3usb30532 > > will just be the parent node for the new nodes we add for the mux and > > switch. > > > > > I've been thinking some more about this and an easy fix is to have separate > > > fwnode_match functions for typec_switch_match and typec_mux_match and have > > > them check that the dev_name ends in "-mux" resp. "-switch" that requires > > > only a very minimal change to "usb: typec: Registering real device entries for the muxes" > > > and then everything should be fine. > > > > I don't want to do anymore device name matching unless we have to, and > > here we don't have to. We can name the nodes for those virtual mux and > > switch, and then just do fwnode_find_named_child_node() in > > pi3usb30532.c for both of them. > > Ok. > > > > Note that another problem with this series which I noticed while testing > > > is that the usb-role-switch is not being found at all anymore after > > > this ("Replacing the old connections with references") patch. I still need > > > start debugging that. > > > > OK, I'll wait for you results on that. > > I understand that you want to wait with doing a v4 until this is resolved, > but can you (privately) send me a v3.5 fixing the mux/switch issue so that > I can test that (and get the problem out of the way for debugging the other > stuff) ? > > I've also just replied to the following patch with a review remark: > [PATCH v3 11/13] platform/x86: intel_cht_int33fe: Provide fwnode for the USB connector > > Since you need to respin the series anyways, it would be good if you can > address that too. Sure thing. I don't have time today to prepare the patches for you to test today, nor tomorrow depending I'm a travelling or not, so this may have wait for next week (Friday's off) :-( thanks,
Hi, On 17-04-19 13:04, Heikki Krogerus wrote: > On Wed, Apr 17, 2019 at 11:52:15AM +0200, Hans de Goede wrote: >> Hi, >> >> On 17-04-19 11:32, Heikki Krogerus wrote: >>> On Wed, Apr 17, 2019 at 11:19:28AM +0200, Hans de Goede wrote: >>>> Hi, >>>> >>>> On 17-04-19 08:39, Heikki Krogerus wrote: >>>>> On Tue, Apr 16, 2019 at 11:35:36PM +0200, Hans de Goede wrote: >>>>>> 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? >>>>> >>>>> I think the correct fix is that we supply separate nodes for both >>>>> device entries. >>>> >>>> That is not going to work since the (virtual) mux / orientation-switch >>>> devices are only registered once the driver binds to the piusb30532 i2c >>>> device, so when creating the nodes we only have the piusb30532 i2c device. >>> >>> It's not a problem, that's why we have the software nodes. The nodes >>> can be created before the device entires. The node for pi3usb30532 >>> will just be the parent node for the new nodes we add for the mux and >>> switch. >>> >>>> I've been thinking some more about this and an easy fix is to have separate >>>> fwnode_match functions for typec_switch_match and typec_mux_match and have >>>> them check that the dev_name ends in "-mux" resp. "-switch" that requires >>>> only a very minimal change to "usb: typec: Registering real device entries for the muxes" >>>> and then everything should be fine. >>> >>> I don't want to do anymore device name matching unless we have to, and >>> here we don't have to. We can name the nodes for those virtual mux and >>> switch, and then just do fwnode_find_named_child_node() in >>> pi3usb30532.c for both of them. >> >> Ok. >> >>>> Note that another problem with this series which I noticed while testing >>>> is that the usb-role-switch is not being found at all anymore after >>>> this ("Replacing the old connections with references") patch. I still need >>>> start debugging that. >>> >>> OK, I'll wait for you results on that. >> >> I understand that you want to wait with doing a v4 until this is resolved, >> but can you (privately) send me a v3.5 fixing the mux/switch issue so that >> I can test that (and get the problem out of the way for debugging the other >> stuff) ? >> >> I've also just replied to the following patch with a review remark: >> [PATCH v3 11/13] platform/x86: intel_cht_int33fe: Provide fwnode for the USB connector >> >> Since you need to respin the series anyways, it would be good if you can >> address that too. > > Sure thing. I don't have time today to prepare the patches for you to > test today, nor tomorrow depending I'm a travelling or not, so this may > have wait for next week (Friday's off) :-( Note I'm on vacation myself next week, Monday I'm still reading email and I can run some tests, next week Tuesday - Friday I'm off. Regards, Hans
Hi, On 17-04-19 12:44, Heikki Krogerus wrote: > On Wed, Apr 17, 2019 at 12:15:18PM +0200, Hans de Goede wrote: >> Hi, >> >> On 17-04-19 11:32, Heikki Krogerus wrote: >>> On Wed, Apr 17, 2019 at 11:19:28AM +0200, Hans de Goede wrote: >> >> <snip> >> >>>> That is not going to work since the (virtual) mux / orientation-switch >>>> devices are only registered once the driver binds to the piusb30532 i2c >>>> device, so when creating the nodes we only have the piusb30532 i2c device. >>> >>> It's not a problem, that's why we have the software nodes. The nodes >>> can be created before the device entires. The node for pi3usb30532 >>> will just be the parent node for the new nodes we add for the mux and >>> switch. >>> >>>> I've been thinking some more about this and an easy fix is to have separate >>>> fwnode_match functions for typec_switch_match and typec_mux_match and have >>>> them check that the dev_name ends in "-mux" resp. "-switch" that requires >>>> only a very minimal change to "usb: typec: Registering real device entries for the muxes" >>>> and then everything should be fine. >>> >>> I don't want to do anymore device name matching unless we have to, and >>> here we don't have to. We can name the nodes for those virtual mux and >>> switch, and then just do fwnode_find_named_child_node() in >>> pi3usb30532.c for both of them. >> >> Thinking more about this, I have a feeling that this makes things needlessly >> complicated, checking the dev_name *ends* in "-mux" resp. "-switch" should be >> 100% reliable since we call: >> >> dev_set_name(&sw->dev, "%s-switch", dev_name(parent)); >> dev_set_name(&mux->dev, "%s-mux", dev_name(parent)); >> >> When registering the switch / mux, so I believe doing name (suffix) comparison >> here is fine and much simpler. Anyways this is just my 2 cents on this, I'm >> happy with either solution, your choice. > > You do have a point. I'll take a look how the two options look like, > but maybe your way is better after all. I whipped up a quick fix using my approach so that I can start working on debugging the usb_role_switch_get call in tcpm.c returning NULL. I've attached it, feel free to use this for v4 of the series if you decide to go with this approach. Regards, Hans > > thanks, > From 47154195c05dc7c8b3373de9603b06c2f435588a Mon Sep 17 00:00:00 2001 From: Hans de Goede <hdegoede@redhat.com> Date: Wed, 17 Apr 2019 17:58:17 +0200 Subject: [PATCH v2] FIXUP: "usb: typec: Registering real device entries for the muxes" Check the dev_name suffix so that we do not return the first registered device when a mux and switch share the same parent and fwnode. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/usb/typec/mux.c | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c index c7d4a2dd454e..a28803544301 100644 --- a/drivers/usb/typec/mux.c +++ b/drivers/usb/typec/mux.c @@ -22,9 +22,21 @@ static int name_match(struct device *dev, const void *name) return !strcmp((const char *)name, dev_name(dev)); } -static int fwnode_match(struct device *dev, const void *fwnode) +static bool dev_name_ends_with(struct device *dev, const char *suffix) { - return dev_fwnode(dev) == fwnode; + const char *name = dev_name(dev); + const int name_len = strlen(name); + const int suffix_len = strlen(suffix); + + if (suffix_len > name_len) + return false; + + return strcmp(name + (name_len - suffix_len), suffix) == 0; +} + +static int switch_fwnode_match(struct device *dev, const void *fwnode) +{ + return dev_fwnode(dev) == fwnode && dev_name_ends_with(dev, "-switch"); } static void *typec_switch_match(struct device_connection *con, int ep, @@ -37,7 +49,7 @@ static void *typec_switch_match(struct device_connection *con, int ep, return NULL; dev = class_find_device(&typec_mux_class, NULL, con->fwnode, - fwnode_match); + switch_fwnode_match); } else { dev = class_find_device(&typec_mux_class, NULL, con->endpoint[ep], name_match); @@ -167,6 +179,11 @@ EXPORT_SYMBOL_GPL(typec_switch_get_drvdata); /* ------------------------------------------------------------------------- */ +static int mux_fwnode_match(struct device *dev, const void *fwnode) +{ + return dev_fwnode(dev) == fwnode && dev_name_ends_with(dev, "-mux"); +} + static void *typec_mux_match(struct device_connection *con, int ep, void *data) { const struct typec_altmode_desc *desc = data; @@ -226,7 +243,7 @@ static void *typec_mux_match(struct device_connection *con, int ep, void *data) find_mux: dev = class_find_device(&typec_mux_class, NULL, con->fwnode, - fwnode_match); + mux_fwnode_match); return dev ? to_typec_switch(dev) : ERR_PTR(-EPROBE_DEFER); }
Hi, On 17-04-19 11:19, Hans de Goede wrote: > Note that another problem with this series which I noticed while testing > is that the usb-role-switch is not being found at all anymore after > this ("Replacing the old connections with references") patch. I still need > start debugging that. Ok, I've just finished debugging this and I'm attaching 2 FIXUP patches (to be squased) and a new patch, which those 3 small fixes added the problem of tcpm.c being unable to get the role-switch goes away. The second FIXUP patch might be a bit controversial; and we may need another solution for the problem it fixes. I've tried to explain it in more detail in the commit msg. Regards, Hans From 3a2e047608a53caaefe8364eceb7e315ec413698 Mon Sep 17 00:00:00 2001 From: Hans de Goede <hdegoede@redhat.com> Date: Wed, 17 Apr 2019 22:54:47 +0200 Subject: [PATCH v2 1/3] FIXUP: "platform/x86: intel_cht_int33fe: Link with external dependencies using fwnodes" In the else path of: if (dev->fwnode) ... else ..., we should set dev->fwnode to our own fwnode not to dev->fwnode, which is NULL as we just tested. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/platform/x86/intel_cht_int33fe.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c index e6a1ea7f33af..07bf92ece6cd 100644 --- a/drivers/platform/x86/intel_cht_int33fe.c +++ b/drivers/platform/x86/intel_cht_int33fe.c @@ -189,7 +189,7 @@ static int cht_int33fe_setup_mux(struct cht_int33fe_data *data) data->node[INT33FE_NODE_ROLE_SWITCH] = dev->fwnode; } else { /* The node can be tied to the lifetime of the device. */ - dev->fwnode = fwnode_handle_get(dev->fwnode); + dev->fwnode = fwnode_handle_get(fwnode); } put_device(dev);
On Wed, Apr 17, 2019 at 06:03:03PM +0200, Hans de Goede wrote: > Hi, > > On 17-04-19 12:44, Heikki Krogerus wrote: > > On Wed, Apr 17, 2019 at 12:15:18PM +0200, Hans de Goede wrote: > > > Hi, > > > > > > On 17-04-19 11:32, Heikki Krogerus wrote: > > > > On Wed, Apr 17, 2019 at 11:19:28AM +0200, Hans de Goede wrote: > > > > > > <snip> > > > > > > > > That is not going to work since the (virtual) mux / orientation-switch > > > > > devices are only registered once the driver binds to the piusb30532 i2c > > > > > device, so when creating the nodes we only have the piusb30532 i2c device. > > > > > > > > It's not a problem, that's why we have the software nodes. The nodes > > > > can be created before the device entires. The node for pi3usb30532 > > > > will just be the parent node for the new nodes we add for the mux and > > > > switch. > > > > > > > > > I've been thinking some more about this and an easy fix is to have separate > > > > > fwnode_match functions for typec_switch_match and typec_mux_match and have > > > > > them check that the dev_name ends in "-mux" resp. "-switch" that requires > > > > > only a very minimal change to "usb: typec: Registering real device entries for the muxes" > > > > > and then everything should be fine. > > > > > > > > I don't want to do anymore device name matching unless we have to, and > > > > here we don't have to. We can name the nodes for those virtual mux and > > > > switch, and then just do fwnode_find_named_child_node() in > > > > pi3usb30532.c for both of them. > > > > > > Thinking more about this, I have a feeling that this makes things needlessly > > > complicated, checking the dev_name *ends* in "-mux" resp. "-switch" should be > > > 100% reliable since we call: > > > > > > dev_set_name(&sw->dev, "%s-switch", dev_name(parent)); > > > dev_set_name(&mux->dev, "%s-mux", dev_name(parent)); > > > > > > When registering the switch / mux, so I believe doing name (suffix) comparison > > > here is fine and much simpler. Anyways this is just my 2 cents on this, I'm > > > happy with either solution, your choice. > > > > You do have a point. I'll take a look how the two options look like, > > but maybe your way is better after all. > > I whipped up a quick fix using my approach so that I can start working > on debugging the usb_role_switch_get call in tcpm.c returning NULL. > > I've attached it, feel free to use this for v4 of the series if you > decide to go with this approach. Thanks. I'll probable use that as is. > >From 47154195c05dc7c8b3373de9603b06c2f435588a Mon Sep 17 00:00:00 2001 > From: Hans de Goede <hdegoede@redhat.com> > Date: Wed, 17 Apr 2019 17:58:17 +0200 > Subject: [PATCH v2] FIXUP: "usb: typec: Registering real device entries for > the muxes" > > Check the dev_name suffix so that we do not return the first registered > device when a mux and switch share the same parent and fwnode. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/usb/typec/mux.c | 25 +++++++++++++++++++++---- > 1 file changed, 21 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c > index c7d4a2dd454e..a28803544301 100644 > --- a/drivers/usb/typec/mux.c > +++ b/drivers/usb/typec/mux.c > @@ -22,9 +22,21 @@ static int name_match(struct device *dev, const void *name) > return !strcmp((const char *)name, dev_name(dev)); > } > > -static int fwnode_match(struct device *dev, const void *fwnode) > +static bool dev_name_ends_with(struct device *dev, const char *suffix) > { > - return dev_fwnode(dev) == fwnode; > + const char *name = dev_name(dev); > + const int name_len = strlen(name); > + const int suffix_len = strlen(suffix); > + > + if (suffix_len > name_len) > + return false; > + > + return strcmp(name + (name_len - suffix_len), suffix) == 0; > +} > + > +static int switch_fwnode_match(struct device *dev, const void *fwnode) > +{ > + return dev_fwnode(dev) == fwnode && dev_name_ends_with(dev, "-switch"); > } > > static void *typec_switch_match(struct device_connection *con, int ep, > @@ -37,7 +49,7 @@ static void *typec_switch_match(struct device_connection *con, int ep, > return NULL; > > dev = class_find_device(&typec_mux_class, NULL, con->fwnode, > - fwnode_match); > + switch_fwnode_match); > } else { > dev = class_find_device(&typec_mux_class, NULL, > con->endpoint[ep], name_match); > @@ -167,6 +179,11 @@ EXPORT_SYMBOL_GPL(typec_switch_get_drvdata); > > /* ------------------------------------------------------------------------- */ > > +static int mux_fwnode_match(struct device *dev, const void *fwnode) > +{ > + return dev_fwnode(dev) == fwnode && dev_name_ends_with(dev, "-mux"); > +} > + > static void *typec_mux_match(struct device_connection *con, int ep, void *data) > { > const struct typec_altmode_desc *desc = data; > @@ -226,7 +243,7 @@ static void *typec_mux_match(struct device_connection *con, int ep, void *data) > > find_mux: > dev = class_find_device(&typec_mux_class, NULL, con->fwnode, > - fwnode_match); > + mux_fwnode_match); > > return dev ? to_typec_switch(dev) : ERR_PTR(-EPROBE_DEFER); > } > -- > 2.21.0 > cheers,
On Wed, Apr 17, 2019 at 11:14:19PM +0200, Hans de Goede wrote: > Hi, > > On 17-04-19 11:19, Hans de Goede wrote: > > Note that another problem with this series which I noticed while testing > > is that the usb-role-switch is not being found at all anymore after > > this ("Replacing the old connections with references") patch. I still need > > start debugging that. > > Ok, I've just finished debugging this and I'm attaching 2 FIXUP > patches (to be squased) and a new patch, which those 3 small fixes > added the problem of tcpm.c being unable to get the role-switch > goes away. > > The second FIXUP patch might be a bit controversial; and we may > need another solution for the problem it fixes. I've tried to > explain it in more detail in the commit msg. Thanks. I'll go over those, and probable squash them in. I'll think about the second patch. I'm going to first split the series in two so that I'll first introduce all the other changes, and then in a separate series the node reference stuff. I think it makes sense, since there are really to major changes here: firstly starting to use the software nodes with the connector fwnode and others, and secondly introducing the software node references. I'll send you an RFC of the first patches soon. Hope you have time to check and test it. > >From 3a2e047608a53caaefe8364eceb7e315ec413698 Mon Sep 17 00:00:00 2001 > From: Hans de Goede <hdegoede@redhat.com> > Date: Wed, 17 Apr 2019 22:54:47 +0200 > Subject: [PATCH v2 1/3] FIXUP: "platform/x86: intel_cht_int33fe: Link with > external dependencies using fwnodes" > > In the else path of: if (dev->fwnode) ... else ..., we should set > dev->fwnode to our own fwnode not to dev->fwnode, which is NULL as we > just tested. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/platform/x86/intel_cht_int33fe.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c > index e6a1ea7f33af..07bf92ece6cd 100644 > --- a/drivers/platform/x86/intel_cht_int33fe.c > +++ b/drivers/platform/x86/intel_cht_int33fe.c > @@ -189,7 +189,7 @@ static int cht_int33fe_setup_mux(struct cht_int33fe_data *data) > data->node[INT33FE_NODE_ROLE_SWITCH] = dev->fwnode; > } else { > /* The node can be tied to the lifetime of the device. */ > - dev->fwnode = fwnode_handle_get(dev->fwnode); > + dev->fwnode = fwnode_handle_get(fwnode); > } > > put_device(dev); > -- > 2.21.0 > > >From 5133467f116dff6e111d4bc0610ccbcedb397f1d Mon Sep 17 00:00:00 2001 > From: Hans de Goede <hdegoede@redhat.com> > Date: Wed, 17 Apr 2019 23:00:59 +0200 > Subject: [PATCH v2 2/3] FIXUP: "device connection: Find connections also by > checking the references" > > The reference we are looking for might be in a child node, rather then > directly in the device's own fwnode. A typical example of this is a > usb connector node with references to various muxes / switches. > > Note that we do not hit this problem for the device_connection_find_match > calls in typec_switch_get and typec_mux_get because these get called > from typec_register_port and typec_register_port creates a new device > with its fwnode pointing to the usb-connector fwnode and that new > device (rather then the parent) is passed to typec_switch/mux_get and > thus to device_connection_find_match. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/base/devcon.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/base/devcon.c b/drivers/base/devcon.c > index 4cdf95532b63..6f6f870c21eb 100644 > --- a/drivers/base/devcon.c > +++ b/drivers/base/devcon.c > @@ -76,7 +76,7 @@ fwnode_devcon_match(struct fwnode_handle *fwnode, const char *con_id, > void *device_connection_find_match(struct device *dev, const char *con_id, > void *data, devcon_match_fn_t match) > { > - struct fwnode_handle *fwnode = dev_fwnode(dev); > + struct fwnode_handle *child, *fwnode = dev_fwnode(dev); > const char *devname = dev_name(dev); > struct device_connection *con; > void *ret = NULL; > @@ -93,6 +93,12 @@ void *device_connection_find_match(struct device *dev, const char *con_id, > ret = fwnode_devcon_match(fwnode, con_id, data, match); > if (ret) > return ret; > + > + fwnode_for_each_child_node(fwnode, child) { > + ret = fwnode_devcon_match(child, con_id, data, match); > + if (ret) > + return ret; > + } > } > > mutex_lock(&devcon_lock); > -- > 2.21.0 > > >From a69f76993dfe5f43d3e6c4b2bcfbaacf2c247d6e Mon Sep 17 00:00:00 2001 > From: Hans de Goede <hdegoede@redhat.com> > Date: Wed, 17 Apr 2019 22:57:00 +0200 > Subject: [PATCH v2 3/3] usb: roles: Check for NULL con_id > > When usb_role_switch_match gets called by device_connection_find_match > because of a fwnode_reference matching the con_id passed to > device_connection_find_match, then con->id will be NULL and in this > case we do not need to check it since our caller has already checked it. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/usb/roles/class.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c > index f45d8df5cfb8..86defca6623e 100644 > --- a/drivers/usb/roles/class.c > +++ b/drivers/usb/roles/class.c > @@ -101,7 +101,7 @@ static void *usb_role_switch_match(struct device_connection *con, int ep, > struct device *dev; > > if (con->fwnode) { > - if (!fwnode_property_present(con->fwnode, con->id)) > + if (con->id && !fwnode_property_present(con->fwnode, con->id)) > return NULL; > > dev = class_find_device(role_class, NULL, con->fwnode, > -- > 2.21.0 > thanks,
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;