> From: Hans de Goede <hdegoede> > > 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. right, it's not for fun, we use _ADR here is to reduce the maintain effort because currently it defines _HID for every new platform and the drivers have to be updated accordingly, while _ADR doesn't have that problem. > Wentong, can you confirm that the _ADR using codepaths are actually used on > some hardware / with some DSDTs out there ? what I can share is that we will see. > > Looking at the design of the > > driver I'm not sure why ACPI HIDs are collected somewhere else than in > > the respective drivers. AFAIK, auxiliary bus doesn't support parsing fwnodes currently. Probably we can support it for auxiliary bus in another patch. > > And looking at the ID lists themselves I am > > not sure why the firmware of the respective hardware platforms are not using > _CID. I think firmware can select _CID as well, but the shipped hw doesn't use _CID, the driver has to make sure the shipped hw working as well. And switching to _CID for the shipped hw is not easy, and it has to change windows driver as well. > > 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. Yes, total agree. Thanks Thanks Wentong > > Regards, > > Hans >
