Messages in this thread | | | From | "Wu, Wentong" <> | Subject | RE: [PATCH v20 1/4] usb: Add support for Intel LJCA device | Date | Mon, 16 Oct 2023 05:52:28 +0000 |
| |
> 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 >
| |