Hi Andy, On 10/13/23 22:05, Shevchenko, Andriy wrote: > On Thu, Oct 12, 2023 at 01:14:23PM +0200, Hans de Goede wrote: <snip> >> 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 ? > > Yes as we have a few weeks to fix corner cases. > > What I'm worrying is that opening door for _ADR that seems never used is kinda > an overkill here (resolving non-existing problem). I assume that there actually some DSDTs using the _ADR approach and that this support is not there just for fun. Wentong, can you confirm that the _ADR using codepaths are actually used on some hardware / with some DSDTs out there ? > Looking at the design of the > driver I'm not sure why ACPI HIDs are collected somewhere else than in the > respective drivers. And looking at the ID lists themselves I am not sure why > the firmware of the respective hardware platforms are not using _CID. This is a USB device which has 4 functions: 1. GPIO controller 2. I2C controller 1 3. I2C controller 2 4. SPI controller The driver for the main USB interface uses the new auxbus to create 4 child devices. The _ADR or if that fails _HID + _UID matching is done to find the correct acpi_device child of the acpi_device which is the ACPI-companion of the main USB device. After looking up the correct acpi_device child this is then set as the fwnode / ACPI-companion of the auxbus device created for that function. Having the correct fwnode is important because other parts of the DSDT reference this fwnode to specify GPIO / I2C / SPI resources and if the fwnode of the aux-device is not set correctly then the resources for other devices referencing it (typically a camera sensor) can not be found. As for why the driver for the auxbus devices / children do not use HID matching, AFAIK the auxbus has no support for using ACPI (or DT) matching for aux-devices and these drivers need to be auxiliary_driver's and bind to the auxbus device and not to a platform_device instantiated for the acpi_device since they need the auxbus device to access the USB device. Regards, Hans
