Re: [PATCH v3 13/13] platform/x86: intel_cht_int33fe: Replacing the old connections with references

[Hans de Goede <hdegoede@redhat.com>]

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