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@xxxxxxxxx>
Signed-off-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
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
