From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id F2E92C10F12 for ; Wed, 17 Apr 2019 09:52:20 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id C23572173C for ; Wed, 17 Apr 2019 09:52:20 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731652AbfDQJwT (ORCPT ); Wed, 17 Apr 2019 05:52:19 -0400 Received: from mail-ed1-f67.google.com ([209.85.208.67]:45958 "EHLO mail-ed1-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727282AbfDQJwS (ORCPT ); Wed, 17 Apr 2019 05:52:18 -0400 Received: by mail-ed1-f67.google.com with SMTP id k92so2296910edc.12 for ; Wed, 17 Apr 2019 02:52:17 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=0QDAFWJzTDCsMR5a8aWngWZoH/o61wzefj40665LFTI=; b=ZcErPRd4HCcMLp2r4QZTQSd/kPQFpieHXfPHzUTEN5G4Afdq1Xj8YBoO3Zo4UGq/cm AXGkszztP5iMXNJfWVOZTNbe8WLhDH/7ZbkFQlJYO7wtGDMQmZAtI0BIq/hTyJXXTDrn kFYFztjXwbU2iwBfR781X3akSS1Tun1KA1GW7nRN3Sm98nVWQfP+SlZzCEBUIF+pGw27 pAj3rThV93YI9F3dW8aMIwoWb+4Umh0d0pW6s872lO9w425tpY+dhvk/sbaZZzwd5uLk ySWlWD8t/gLtjzKViBMF/+BBafrmcRMndOOx6yX0Yj7Y8xdBzRMYDR/ak3lufGf7A1v+ VvZQ== X-Gm-Message-State: APjAAAXKmkIqijXKFfw1zmGCzJO/0GmFos7Kq4lp9oBeLO0vepd1XcIe qYszX/YexeCr9CNS0v+rrmJzjg== X-Google-Smtp-Source: APXvYqyr0dIS87DAhb3TM+g+66FGpd+o/6t8nat0fgoP2dQBsWTot3XNe2IeCrCSqD5uif/ryx7WNA== X-Received: by 2002:aa7:da19:: with SMTP id r25mr42092715eds.130.1555494736617; Wed, 17 Apr 2019 02:52:16 -0700 (PDT) Received: from shalem.localdomain (84-106-84-65.cable.dynamic.v4.ziggo.nl. [84.106.84.65]) by smtp.gmail.com with ESMTPSA id h10sm2341755ede.21.2019.04.17.02.52.15 (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128); Wed, 17 Apr 2019 02:52:15 -0700 (PDT) Subject: Re: [PATCH v3 13/13] platform/x86: intel_cht_int33fe: Replacing the old connections with references To: Heikki Krogerus Cc: "Rafael J. Wysocki" , Greg Kroah-Hartman , Darren Hart , Andy Shevchenko , linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org, platform-driver-x86@vger.kernel.org, Andy Shevchenko References: <20190412134122.82903-1-heikki.krogerus@linux.intel.com> <20190412134122.82903-14-heikki.krogerus@linux.intel.com> <20190417063918.GI1747@kuha.fi.intel.com> <76d9ab79-a1d0-f3cd-ba5d-2325740c72ff@redhat.com> <20190417093252.GO1747@kuha.fi.intel.com> From: Hans de Goede Message-ID: <9304d771-df1f-aea4-43d2-7db540953cdd@redhat.com> Date: Wed, 17 Apr 2019 11:52:15 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 MIME-Version: 1.0 In-Reply-To: <20190417093252.GO1747@kuha.fi.intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>>>> Signed-off-by: Heikki Krogerus >>>> >>>> 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