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
