Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Message-ID: <5d2e9eba-a941-ea9a-161a-5b97d09d5d35@redhat.com>
Date:   Thu, 12 Oct 2023 13:14:23 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH v20 1/4] usb: Add support for Intel LJCA device
Content-Language: en-US, nl
To:     "Wu, Wentong" <wentong.wu@intel.com>,
        "Shevchenko, Andriy" <andriy.shevchenko@intel.com>
Cc:     "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "oneukum@suse.com" <oneukum@suse.com>,
        "wsa@kernel.org" <wsa@kernel.org>,
        "andi.shyti@linux.intel.com" <andi.shyti@linux.intel.com>,
        "broonie@kernel.org" <broonie@kernel.org>,
        "bartosz.golaszewski@linaro.org" <bartosz.golaszewski@linaro.org>,
        "linus.walleij@linaro.org" <linus.walleij@linaro.org>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "linux-i2c@vger.kernel.org" <linux-i2c@vger.kernel.org>,
        "linux-spi@vger.kernel.org" <linux-spi@vger.kernel.org>,
        "sakari.ailus@linux.intel.com" <sakari.ailus@linux.intel.com>,
        "Wang, Zhifeng" <zhifeng.wang@intel.com>,
        "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <1696833205-16716-1-git-send-email-wentong.wu@intel.com>
 <1696833205-16716-2-git-send-email-wentong.wu@intel.com>
 <ZSZ3IPgLk7uC5UGI@smile.fi.intel.com>
 <6a87b43a-0648-28d4-6c69-e0f684e44eb6@redhat.com>
 <DM6PR11MB4316BE44F53E276384FF06C88DCCA@DM6PR11MB4316.namprd11.prod.outlook.com>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <DM6PR11MB4316BE44F53E276384FF06C88DCCA@DM6PR11MB4316.namprd11.prod.outlook.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

On 10/11/23 14:50, Wu, Wentong wrote:
>> From: Hans de Goede <hdegoede>
>>
>> Hi,
>>
>> On 10/11/23 12:21, Andy Shevchenko wrote:
>>> On Mon, Oct 09, 2023 at 02:33:22PM +0800, Wentong Wu wrote:
>>>> Implements the USB part of Intel USB-I2C/GPIO/SPI adapter device
>>>> named "La Jolla Cove Adapter" (LJCA).
>>>>
>>>> The communication between the various LJCA module drivers and the
>>>> hardware will be muxed/demuxed by this driver. Three modules ( I2C,
>>>> GPIO, and SPI) are supported currently.
>>>>
>>>> Each sub-module of LJCA device is identified by type field within the
>>>> LJCA message header.
>>>>
>>>> The sub-modules of LJCA can use ljca_transfer() to issue a transfer
>>>> between host and hardware. And ljca_register_event_cb is exported to
>>>> LJCA sub-module drivers for hardware event subscription.
>>>>
>>>> The minimum code in ASL that covers this board is Scope
>>>> (\_SB.PCI0.DWC3.RHUB.HS01)
>>>>     {
>>>>         Device (GPIO)
>>>>         {
>>>>             Name (_ADR, Zero)
>>>>             Name (_STA, 0x0F)
>>>>         }
>>>>
>>>>         Device (I2C)
>>>>         {
>>>>             Name (_ADR, One)
>>>>             Name (_STA, 0x0F)
>>>>         }
>>>>
>>>>         Device (SPI)
>>>>         {
>>>>             Name (_ADR, 0x02)
>>>>             Name (_STA, 0x0F)
>>>>         }
>>>>     }
>>>
>>> This commit message is not true anymore, or misleading at bare minimum.
>>> The ACPI specification is crystal clear about usage _ADR and _HID, i.e.
>>> they must NOT be used together for the same device node. So, can you
>>> clarify how the DSDT is organized and update the commit message and it
>>> may require (quite likely) to redesign the architecture of this
>>> driver. Sorry I missed this from previous rounds as I was busy by
>>> something else.
>>
>> This part of the commit message unfortunately is not accurate.
>> _ADR is not used in either DSDTs of shipping hw; nor in the code.
> 
> We have covered the _ADR in the code like below, it first try to find the
> child device based on _ADR, if not found, it will check the _HID, and there
> is clear comment in the function.
> 
> /* bind auxiliary device to acpi device */
> static void ljca_auxdev_acpi_bind(struct ljca_adapter *adap,
> 				   struct auxiliary_device *auxdev,
> 				   u64 adr, u8 id)
> {
> 	struct ljca_match_ids_walk_data wd = { 0 };
> 	struct acpi_device *parent, *adev;
> 	struct device *dev = adap->dev;
> 	char uid[4];
> 
> 	parent = ACPI_COMPANION(dev);
> 	if (!parent)
> 		return;
> 
> 	/*
> 	 * get auxdev ACPI handle from the ACPI device directly
> 	 * under the parent that matches _ADR.
> 	 */
> 	adev = acpi_find_child_device(parent, adr, false);
> 	if (adev) {
> 		ACPI_COMPANION_SET(&auxdev->dev, adev);
> 		return;
> 	}
> 
> 	/*
> 	 * _ADR is a grey area in the ACPI specification, some
> 	 * platforms use _HID to distinguish children devices.
> 	 */
> 	switch (adr) {
> 	case LJCA_GPIO_ACPI_ADR:
> 		wd.ids = ljca_gpio_hids;
> 		break;
> 	case LJCA_I2C1_ACPI_ADR:
> 	case LJCA_I2C2_ACPI_ADR:
> 		snprintf(uid, sizeof(uid), "%d", id);
> 		wd.uid = uid;
> 		wd.ids = ljca_i2c_hids;
> 		break;
> 	case LJCA_SPI1_ACPI_ADR:
> 	case LJCA_SPI2_ACPI_ADR:
> 		wd.ids = ljca_spi_hids;
> 		break;
> 	default:
> 		dev_warn(dev, "unsupported _ADR\n");
> 		return;
> 	}
> 
> 	acpi_dev_for_each_child(parent, ljca_match_device_ids, &wd);

Ah ok, I see. So the code:

1. First tries to find the matching child acpi_device for the auxdev by ADR

2. If 1. fails then falls back to HID + UID matching

And there are DSDTs which use either:

1. Only use _ADR to identify which child device is which, like the example
   DSDT snippet from the commit msg.

2. Only use _HID + _UID like the 2 example DSDT snippets from me email

But there never is a case where both _ADR and _HID are used at
the same time (which would be an ACPI spec violation as Andy said).

So AFAICT there is no issue here since  _ADR and _HID are never
user at the same time and the commit message correctly describes
scenario 1. from above, so the commit message is fine too.

So I believe that we can continue with this patch series in
its current v20 form, which has already been staged for
going into -next by Greg.

Andy can you confirm that moving ahead with the current
version is ok ?

Regards,

Hans