Re: [PATCH v20 1/4] usb: Add support for Intel LJCA device

[Hans de Goede]

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