RE: [PATCH v20 1/4] usb: Add support for Intel LJCA device

["Wu, Wentong" <wentong.wu@xxxxxxxxx>]

> 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
>