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